[JENKINS-63631] Add support for GitHub App credentials #50
Conversation
...enkins/plugins/kubernetes_credentials_provider/convertors/GitHubAppCredentialsConvertor.java
Outdated
Show resolved
Hide resolved
|
|
||
| hudson.util.Secret privateKeySecret = null; | ||
| try { | ||
| privateKeySecret = hudson.util.Secret.fromString(new String(privateKey, "UTF-8")); |
There was a problem hiding this comment.
This seems wrong.
The convention in the plugin is that the secrets are shared between Jenkins and possibly something else (possibly even multiple Jenkins). As such this should not be a secret. Also seems weird we have a String constructed from utf8 byte array here when that can be done above with a change to call a different method which is already done in the preceding line.
There was a problem hiding this comment.
I've simplified this to call SecretUtils.base64DecodeToString as recommended. The only constructor I seem to have available takes in a Secret so i'm not sure of another way to do this.
|
|
||
| String privateKeyBase64 = SecretUtils.getNonNullSecretData(secret, "privateKey", "gitHubApp credential is missing the privateKey"); | ||
|
|
||
| String appID = SecretUtils.requireNonNull(SecretUtils.base64DecodeToString(appIDBase64), "gitHubApp credential has an invalid appID (must be base64 encoded UTF-8)"); |
There was a problem hiding this comment.
Iiuc appId is an integer numerical type so should be safe to store directly in the secret without encoding?
@bitwiseman ?
There was a problem hiding this comment.
It could go as an annotation? but i think everything underneath the data element will be base64 encoded
There was a problem hiding this comment.
no - you are right. this is how a native app would expect it. all k8s secrets are expected to be base64 encoded strings.
jtnord
left a comment
jtnord
left a comment
There was a problem hiding this comment.
Thanks for the PR Gareth.
I've left a few comments inline, if you could let me have your feedback.
jtnord
changed the title
jtnord
changed the title
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>github-branch-source</artifactId> | ||
| <version>2.7.1</version> |
There was a problem hiding this comment.
Really? Have you considered updating to version that isn't almost a year old? There's been a ton of work in this area in that past year.
There was a problem hiding this comment.
Upgrading to the latest introduces a lot of enforcer issues, but i'll give it a go.
There was a problem hiding this comment.
I'll try and introduce the plugin bom in a separate PR and update some of the dependencies
|
#51 introduces the bom for 2.249-x |
|
@jtnord anything else I need to do to get this merged and a release created? |
|
0.17 released. Thanks Gareth. |
3 participants
This PR adds support for "gitHubApp" credentials.
The GitHub app credential is mapped from
"jenkins.io/credentials-type": "gitHubApp", with the parametersappID&privateKey.Fixes JENKINS-63631