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
GitHub access not possible without an oAuth token #225
Comments
Just thought I would update this issue with a note that we are still working on it. I have tested a few alternatives, but haven't gotten anything working to a satisfactory degree yet. I am considering dropping back to strategy of cloning a shallow clone of the repo into a temp dir so that we can auth with a deploy key. |
Related to #407 |
#407 switched the default GH gateway implementation to use the new GH App model, which can be used without a token - I'm closing this, but if you're still using the old gateway and still need to use it without a token, feel free to re-open. Thanks! |
Background
While testing out a proof of concept with a private GitHub repo, I noticed that my calls to the cr-gateway were failing with this message:
The culprit causing this message is here:
https://github.com/Azure/brigade/blob/51ebbe1b177e9c8d67a71c0eb951556c3a3471fd/pkg/webhook/client.go#L90-L92
The webhook tries to confirm that the
brigade.js
file is present before responding to the webhook.https://github.com/Azure/brigade/blob/7b2f3836620ac13862e5cee4793a86cfac6b8a2e/pkg/webhook/dockerhub.go#L57-L59
Note that the comment there isn't exactly right - it does not clone the repo (which would work for my use-case because I provide a GitHub deploy key) that allows read-only ssh access to only this repo), it uses the GitHub API to check for the presence of that file.
Security Concerns
I definitely understand why it's not desirable to hold the webhook while you clone the entire repo to check for the presence of the
brigade.js
file. However, the OAuth scope required for the current API call requiresrepo
scope (docs) which, in my opinion, provides way too much access to all of my private repos, and additional permissions far beyond the read access intended.Leaking that single key (which is visible to others in my organization by viewing the helm chart for the
brigade-project
) is undesirable.The other option is a GitHub deploy key, which seems much more suited to this task. It provides per-repo, read-only (by default) access to just the one repo in question. It also feels like brigade fits really nicely in with the rest of the kinds of apps configured in this section.
Options
These are a few options I've thought about for this:
Skip the
brigade.js
file checkIt seems like the lack of the
brigade.js
file will pretty much only happen once during the initial setup, or very rarely otherwise. The webhook could skip the check entirely by default or, if the user has defined an SSH key, it's documented that we don't actually check for the file. It could also be a configuration you set in helm when you deploy.I'm not sure how the worker would handle the lack of a brigade.js file on it's end, but that could be fleshed out.
Clone the repo and keep it stored in a persistent volume
I haven't thought through this all the way, and I don't love the idea, but I thought I'd document it at least. We could clone the private repo using the SSH key and keep it in some kind of cache or persistent volume. That way, the initial clone / response to the webhook might be slow, but subsequent check would be decently fast.
Again, I don't love this, there are weird race conditions, potential security concerns storing all the private code in one place, etc.
Either way...
Either way, the documentation and the sample
values.yaml
file is not very clear that you must provide a github oauth token for private repos. If we're not sure what to do here, a documentation update is probably in order so people can get set up. I'm happy to do a documentation PR if requested.Initial Slack Conversation
This issue was originally a conversation between myself and @technosophos in the #brigade slack channel (link).
The text was updated successfully, but these errors were encountered: