-
Notifications
You must be signed in to change notification settings - Fork 352
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
[JENKINS-73471] Restore passing credentialsId to the GitSCM #867
base: master
Are you sure you want to change the base?
Conversation
I don't know if Yaroslav is still active in the Jenkins project. It is unfortunate that ad359b3 did not add any tests. Here are the reproduction steps from the security ticket though to check if it regresses the fix manually:
|
@Dohbedoh Yaroslav asked me to pass on some info from when he worked on the security ticket initially. The reason he made the one-line change that you are proposing to revert in this PR has to do with unusual behavior he saw while testing:
Log
After that comment he changed the line in question to stop passing He doesn't think that your change would regress the security fix, the question is just whether the error that he saw when the code was like you have it in the PR now is representative of a real bug that would affect users or is just some kind of environmental or configuration issue. |
Thanks!
|
Allright, so actually this cannot work .. In I wonder why we don't decorate the client at the end of the If unit tests are required, I would need more time to provide them.. |
This solution has limitation I think. The extension is not used when polling:
Maybe it's best to wait for jenkinsci/git-plugin#1649 .. That would complement what we are doing here. |
26ccca2
to
0847bd9
Compare
git-plugin 5.5.0 has been released. cc @jenkinsci/bitbucket-branch-source-plugin-developers |
6a23a48
to
fb52504
Compare
Alternately, revert ad359b3 and just use a https://javadoc.jenkins.io/plugin/workflow-api/org/jenkinsci/plugins/workflow/log/TaskListenerDecorator.Factory.html which is triggered by the use of this SCM source and which masks any |
@jglick The decorator would solve the leakage in the console output but I recall another source.. I think the "Changes" page that of a job with GitSCM that displays the remote URL. Using secrets in the GitSCM remote URL is a problem. |
I was thinking of splitting this into 2 PRs.
We need to make sure the We would coordinate the fix in the following order::
WDYT @jenkinsci/bitbucket-branch-source-plugin-developers ? |
Make https://github.com/jenkinsci/git-plugin/blob/f7b32740f2abfc148692fe0d4628c094eed31798/src/main/java/hudson/plugins/git/util/BuildData.java#L92 strip any authentication fields out of URLs before storing them. |
@Dohbedoh the changes merged in git plugin has been reverted. In our production environment we use a OAuth 2 credential to discover branches and SSH trait to perform git operation (clone, pull, ...). I had update plugin to latest version, now I will install build from this branch and I will have a look to the console to see if secret appear in some logs. |
Well, discover branches and clone using OAuth2 credential fail:
Without changes of this PR the checkout using OAuth2 credentials works:
Discover branches using OAuth2 credential and clone using SSH trait works:
|
Now I understand the issue. If the BitbucketGitSCMBuilder is built with credentialsId, it will build a GitClient performing a lookup a StandardUsernameCredentials for the given credentialsId. This override what the extension GitClientAuthenticatorExtension contribute, because it decorate the GitClient in GitSCM before than it configure all UserRemoteConfigs. And that why you make a PR to the git-plugin to move extensions after UserRemoteConfigs configurations. In case of OAuth2 the authorization header is enough, and that why with credentilsId = null no StandardUsernameCredentials is applied for the URL to clone and it works. |
@Dohbedoh as suggested the first part of this fix has been merged (set to null credentials in case of SSHTrait). |
@nfalco79 Thanks for the help. Can we get a release of the first part ? The automated release did not kick in because it misses required labels. |
6a8f063
to
a4f0832
Compare
@nfalco79 turns out git-plugin already moved to 2.479.1 baseline so I had to bump this requirement here too. |
Could you point me to some help page about this required labels? Other plugins I'm the mantainer I perform manual release |
Unfortunately in jenkins-infra exclusive JEP-229 is enabled, so deny upload permission to the mantainers |
Do you mean https://github.com/jenkinsci/.github/blob/f1a34987f2919f039282a13b8741e3c463d18bc6/.github/release-drafter.yml#L9-L55 ? |
From https://www.jenkins.io/doc/developer/publishing/releasing-cd/#releasing:
|
I mean is there a specific label on a pull request that trigger a release? |
I saw that documentation and I had already try but Release stage is disable and there is not way (I found) to trigger a deploy |
Yes, any of the interesting ones, here I guess |
Proposal to fix #862. Reinstore the
credentialsId
in theGitSCM
configuration.It would also guarantee that credentials usage is still tracked. Checking down the line, GitClient still uses the authenticator credentials reference.
@yaroslavafenkin Per my understanding, the issue that
SECURITY-3363
fixes was the clone link of the OAuth Authenticator at https://github.com/jenkinsci/bitbucket-branch-source-plugin/blob/886.v44cf5e4ecec5/src/main/java/com/cloudbees/jenkins/plugins/bitbucket/api/credentials/BitbucketOAuthAuthenticator.java#L48-L57 ? In which case instantiating the GitSCM with thecredentialsId
is fine ? I am not sure what is the scenario to validate that this does not bring back this security problem ?Your checklist for this pull request