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

Escape special characters for GitUsernamePasswordBinding in withCrede… #1443

Merged

Conversation

Seros
Copy link
Contributor

@Seros Seros commented Apr 26, 2023

JENKINS-47514 - Escape special characters in GitUsernamePasswordBinding in withCredentials

As described in the issue using git usernames or password with special chars in the GitUsernamePasswordBinding for withCredentials lead to several errors making the step unusable in such cases. This PR introduces the same credential passing technique as is used in the git client plugin. It avoids almost all cases of special characters in passwords causing issues by using a separate file to store the password then using the operating system appropriate command to pass the contents of the file to command line git.

Checklist

  • I have read the CONTRIBUTING doc
  • I have referenced the Jira issue related to my changes in one or more commit messages
  • I have added tests that verify my changes
  • Unit tests pass locally with my changes
  • I have added documentation as necessary
  • No Javadoc warnings were introduced with my changes
  • No spotbugs warnings were introduced with my changes
  • Documentation in README has been updated as necessary
  • Online help has been added and reviewed for any new or modified fields
  • I have interactively tested my changes - tested with BitBucket, GitHub, GitLab, and Gitea using Pipeline jobs
  • Any dependent changes have been merged and published in upstream modules (like git-client-plugin)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

@MarkEWaite
Copy link
Contributor

Thanks for the pull request. It is similar to the technique offered in #1310 and #1314. Unfortunately, all three implementations fail in interesting ways with special characters and special conditions.

Would you be willing to adapt your pull request to use the technique that is used in the git client plugin? Instead of echo and expanding a value, it writes the value to a file and uses cat to deliver the contents of the file. See CliGitAPIImpl in git client plugin for the details.

@Seros Seros force-pushed the escape-special-characters-with-credentials branch from 6e4d10a to 54cfe20 Compare July 11, 2023 08:01
Seros and others added 8 commits July 11, 2023 12:16
In the future, the repository will be formatted with spotless and that
ugliness will be banished forever.  Helps my code review to make the
diffs small now.
No need to make them visible outside the class where they are used.

Also adds the same comment on these methods as is used on the methods
in the git client plugin so that future consumers will know to not use
them for any other purpose than their current very limited use.
@MarkEWaite MarkEWaite added the bugfix Fixes a bug - used by Release Drafter label Jul 11, 2023
@MarkEWaite MarkEWaite self-assigned this Jul 11, 2023
@MarkEWaite MarkEWaite merged commit 7968e3f into jenkinsci:master Jul 12, 2023
14 checks passed
@gabyx
Copy link

gabyx commented Jul 15, 2023

@MarkEWaite : No offence, but I am unsure if that fix went in the right direction?

Whats wrong with

#!/bin/bash 
if [[ "$1" =~ Password ]]; then 
  echo "$GIT_PASSWORD" 
else 
  echo "$GIT_USERNAME" 
fi

This relies on env variables which can contain the special characters, we hotfixed exactly this in our JNLP containers in our entrerprise Jenkins.
This is better than producing another Java code which escapes stuff which also again writes passwords onto disk, IMO worse in all senses (???)

@MarkEWaite
Copy link
Contributor

@MarkEWaite : No offence, but I am unsure if that fix went in the right direction?

Whats wrong with

#!/bin/bash 
if [[ "$1" =~ Password ]]; then 
  echo "$GIT_PASSWORD" 
else 
  echo "$GIT_USERNAME" 
fi

I'll need some time to consider that as an alternative. The technique used in this pull request is the same technique that has been used in the git client plugin for many years. I preferred consistency with the git client plugin because that is known to be working reliably in the existing installations of the git plugin.

This relies on env variables which can contain the special characters, we hotfixed exactly this in our JNLP containers in our entrerprise Jenkins. This is better than producing another Java code which escapes stuff which also again writes passwords onto disk, IMO worse in all senses (???)

I disagree that it is worse to write passwords to disk. We're already writing private keys to disc for ssh authentication of git repositories. We're already writing passwords to disk for for username / password authentication when cloning a git repository and when performing other username / password authenticated operations. Writing a username and password to disc for the withCredentials step should allow it to work as reliably as the other authenticated git operations already work from the git plugin.

If others want to implement replacement authentication techniques, I'm open to consider them, though those techniques need to continue to work correctly with command line git 1.8 through 2.41 and with both username / password and private key authentication. After the November 2023 end of support for Red Hat Enterprise Linux 7, we can narrow the required ranges to command line git 2.11 through 2.41.

@gabyx
Copy link

gabyx commented Jul 16, 2023

I understand that the PR builds on the existing solution. But when I looked at the code, what I did not understand is, that the withCredentials(gitUsernamePassword) already exposes env variables as said in the documentation, namely GIT_USERNAME etc... and from there I did not understand why writing to disk is even needed, when the bash script anyway has access to the same modified environment, it can directly use the GIT_... variables. Basically this file is to inject the credentials into GIT, which it still does, it just uses the Jenkins described/documented env variables directly...

Less code, less bugs, less things to manage. Especially when I think about the escaping code...

@Seros
Copy link
Contributor Author

Seros commented Jul 17, 2023

From what I know about git and the use case most Jenkins users face is that you want configure git to use specific basic auth credentials for specific commands but git does not support reading credentials from the environment variables. Instead it does automatically search for the GIT_ASKPASS variable and once it's available and git fails authenticating against the git server it takes the script which is declared through the GIT_ASKPASS and reads credentials provided by the script. It's a bit complicated but better than rewriting remote urls IMO. Using ssh would also be possible but in this particular use case it's about avoiding as it is not possible. And the solution you described in #1443 (comment) was how it was implemented before but did definitely not work in our case unfortunately.

@gabyx
Copy link

gabyx commented Jul 17, 2023

How can the script #1443 (comment)
not have worked, if Jenkins already defines the environment variables they will be visible to the above scripts as well (GIT_ASKPASS yes) ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a bug - used by Release Drafter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants