Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GH Actions: Push current ref to deploy branch after test success #5136

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

iBug
Copy link
Member

@iBug iBug commented Nov 21, 2020

So we don't rely MS on updating the deploy branch (we still rely it on notifying running Smokey instances).

The "deploy" job only runs after the "build" job succeeds (i.e. the whole build matrix succeeds). Since MS is also relying on CI success (webhook), this should make no difference in terms of software integrity.

iBug added 2 commits November 21, 2020 15:27
This relieves some pressure and dependency on MS,
     where we both [update the deploy branch]
               and [notify Smokey of updates].
Previously during MS downtime we had to update the deploy branch manually,
           moving this to CI creates a CD-like service
           that is supposed to be more automated and reliable.
@stale stale bot added the status: stale label Dec 22, 2020
@stale

This comment has been minimized.

@stale stale bot closed this Dec 24, 2020
@iBug iBug reopened this Dec 25, 2020
@stale stale bot removed the status: stale label Dec 25, 2020
@stale stale bot added the status: stale label Jan 25, 2021
@stale

This comment has been minimized.

@stale stale bot closed this Jan 26, 2021
@double-beep double-beep added the status: confirmed Confirmed as something that needs working on. label Jan 26, 2021
@double-beep double-beep reopened this Jan 26, 2021
@stale stale bot removed the status: stale label Jan 26, 2021
@Undo1
Copy link
Member

Undo1 commented Oct 27, 2021

Any reason we can't do this? I know it's been left to sit for a long time - that's my fault.

@makyen
Copy link
Contributor

makyen commented Oct 27, 2021

The last I checked, there were potential security issues. I'm unsure if those have been adequately resolved by GitHub. I'd have to go back through looking at any reported issues with their implementation.

Basically, in order to push to the repo, the process which is running the CI has to have an appropriate token, which effectively means that process could make whatever changes it wants to the code. Presumably, the write token is only passed to CI processes which are running on code which was committed directly to the main repository and not to PRs where the code is originating in forks. [IIRC, it was supposed to work that way, but there were issues, however, I could be misremembering about the issues, as I'm not remembering the details.]

In general, we already have tokens for GH access in MS and SD. It seems that it would be reasonable for SD to perform this step, if we're not wanting it to be kept with MS.

Currently, SD relies on MS to notify it about any new code being available and passing CI. If we're wanting to reduce dependencies, then it seems better to me to put the functionality in SD.

@iBug
Copy link
Member Author

iBug commented Dec 21, 2021

@makyen Following up lest this be left in the oblivion:

AFAICT, GitHub Actions never had issues with permissions of GITHUB_TOKEN from GitHub Actions. The token from Actions running in forks always has read-only access to the source repo (i.e. where it's forked from). Any token has write access only to the repo where it belongs.

So to the best of my knowledge we should be safe to proceed at any time.

@iBug
Copy link
Member Author

iBug commented Dec 28, 2021

I just stumbled upon this article. Maybe it's what @makyen was referring to?

Stealing arbitrary GitHub Actions secrets - Teddy Katz's Blog

However, as indicated in the blog post itself, the vulnerability was quickly patched on the public GitHub instance (github.com) a long time before it's disclosed to the public. So rest assured this is no longer the case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: confirmed Confirmed as something that needs working on.
Development

Successfully merging this pull request may close these issues.

4 participants