-
Notifications
You must be signed in to change notification settings - Fork 371
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
Add support for GitHub app authentication #269
Conversation
timja
commented
Jan 25, 2020
•
edited
Loading
edited
- JENKINS issue(s):
- JENKINS-57351
- Description:
- Adds support for authenticating as a GitHub app (see the jira for more information)
- Documentation changes:
- I've added a guide to this repo internally, can easily be ported elsewhere if needed
- Users/aliases to notify:
- @bitwiseman
Great! If we wanted to try - would generating a locally built |
You’ll need to compile locally: It’s based off 1.101 but version shouldn’t matter too much as long as it’s recent |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. A few questions. Probably a few more once you've added that test coverage you want to.
src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java
Outdated
Show resolved
Hide resolved
src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java
Outdated
Show resolved
Hide resolved
@bitwiseman I might need some guidance on the tests you would like to see here, I commented out the existing code around auth and all the tests passed so i don’t think there’s any existing coverage |
private static PrivateKey getPrivateKeyFromString(final String key) throws GeneralSecurityException { | ||
if (key.contains("RSA")) { | ||
throw new InvalidPrivateKeyException( | ||
"Private key must be a PKCS#8 formatted string, to convert it from PKCS#1 use: " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could add a dependency to bouncy castle then we could handle this? or ask users to convert it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's start with asking user to do this. Smoothing can be a later release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a Bouncy Castle API Plugin that was split from Jenkins core in the past and has a wide install base that you can depend on for this kind of thing, but I agree that it seems best to save that for later.
@ojacques did you get a chance to try this? I've just pushed documentation for it |
@timja, thanks for everything, and for the doc (I will suggest some modifications about it later). It looks like this is failing in 145 is my github app ID (which is not the installation id). My test environment is: github enterprise, the github-branch-source-plugin compiled fresh from this PR and Jenkins 2.219 running in a container. Any idea? |
Do you know what version of GHE you are on? I've pushed more logging, which might help debug it |
2.19.6 |
Could you try re-build a fresh version, (or wait for CI which will give you one too if you just want to download it) I've added additional logging to help debug it |
It looks like it tries to talk to
The end point should be |
Should be fixed now, please re test |
Thanks @timja, update from my testing:
For that git phase, the credential used seems to be an ssh key ( Great progress in any case! |
@ojacques I've pushed a temporary fix, not sure how this was working for me locally, but it's not anymore. |
2875181
to
8f06734
Compare
Fetching repo / running jobs still does not work: I could be available on slack or gitter if needed. After some discussions and more tests:
|
src/main/java/org/jenkinsci/plugins/github_branch_source/Connector.java
Outdated
Show resolved
Hide resolved
We've had a chat on gitter, the GHE issue can be re-produced on Github.com by trying to checkout a private repo, for some reason the creds aren't being used on checkout |
I am fine. We can always generalize it later if needed |
FYI @sladyn98 @uhafner, the functionality created here could be reused in the GitHub Checks API integration project. https://jenkins.io/projects/gsoc/2020/project-ideas/github-checks/ |
@oleg-nenashev Yes this is definitely reusable, I used a part of this code in my POC for the Github Checks API Plugin.https://github.com/sladyn98/JenkinsChecksPOC |
Yes. Here is a PoC which posts a checks run using a valid authentication token issued from Jenkins as GitHub App, all with pipeline {
agent any
stages{
stage('Check run PoC') {
steps {
withCredentials([usernamePassword(credentialsId: 'githubapp-jenkins', usernameVariable: 'GITHUB_APP', passwordVariable: 'GITHUB_JWT_TOKEN')]) {
sh '''
curl -H "Content-Type: application/json" -H "Accept: application/vnd.github.antiope-preview+json" -H "authorization: Bearer ${GITHUB_JWT_TOKEN}" -d '{ "name": "check_run", "head_sha": "'${GIT_COMMIT}'", "status": "in_progress", "external_id": "42", "started_at": "2020-03-05T11:14:52Z", "output": { "title": "Check run from Jenkins!", "summary": "This is a check run which has been generated from Jenkins as GitHub App", "text": "...and that iss awesome"}}' https://github.xyz.com/api/v3/repos/ojacques/jenkins-with-githubapp/check-runs
'''
}
}
}
}
} |
Nice! It'll be great once it's fully integrated, tests showing up there etc |
Yes, that looks very promising! |
When do you plan to create a release with this? |
We did not yet, but were looking to start. |
@ojacques okay cool, i would love to contribute, since it is a part of my proposal for the github Checks API |
@timja I will try to facilitate the release. Just in case, would you be interested to write a blogpost about this feature or to present it at one of the next Jenkins Online Meetups? |
yes, either / both. |
Draft blog post: |
src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubAppCredentials.java
Show resolved
Hide resolved
When will this be released? |
General release in the next week. |