-
-
Notifications
You must be signed in to change notification settings - Fork 9k
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-75370] Deprecate hudson.util.Scrambler
#10377
base: master
Are you sure you want to change the base?
Conversation
I wasn't able to add the following labels: developer removed rfe Check that the label exists and is spelt right then try again. |
I wasn't able to add the following labels: developer removed rfe Check that the label exists and is spelt right then try again. |
I wasn't able to add the following labels: developer removed rfe Check that the label exists and is spelt right then try again. |
3 similar comments
I wasn't able to add the following labels: developer removed rfe Check that the label exists and is spelt right then try again. |
I wasn't able to add the following labels: developer removed rfe Check that the label exists and is spelt right then try again. |
I wasn't able to add the following labels: developer removed rfe Check that the label exists and is spelt right then try again. |
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.
Includes a question about a change of behavior when the authorization header contains invalid base64 data
@@ -127,7 +127,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha | |||
// authenticate the user | |||
String username = null; | |||
String password = null; | |||
String uidpassword = Scrambler.descramble(authorization.substring(6)); | |||
String uidpassword = new String(Base64.getDecoder().decode(authorization.substring(6).getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8); |
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.
This seems like it will change behavior in the case where the authorization.substring(6)
is not a valid base64 encoded string. The previous Scrambler.descramble() call returns an empty string while the Base64.getDecoder() throws an illegal argument exception.
I experimented with this from the Jenkins script console with the following working code:
import java.util.Base64;
import hudson.util.Scrambler;
import java.nio.charset.StandardCharsets;
def password = 'Not a very good password';
def authorization = 'Basic:' + Scrambler.scramble(password);
println(authorization.substring(6));
println('Scrambled decodes to "' + Scrambler.descramble(authorization.substring(6)) + '"');
println('Base64 decodes to "' + new String(Base64.getDecoder().decode(authorization.substring(6).getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8) + '"');
That reports output:
Tm90IGEgdmVyeSBnb29kIHBhc3N3b3Jk
Scrambled decodes to "Not a very good password"
Base64 decodes to "Not a very good password"
The Jenkins script console with the following intentionally broken code shows the behavioral difference:
import java.util.Base64;
import hudson.util.Scrambler;
import java.nio.charset.StandardCharsets;
def password = 'Not a very good password';
def authorization = 'Basic:X' + Scrambler.scramble(password);
println(authorization.substring(6));
println('Scrambled decodes to "' + Scrambler.descramble(authorization.substring(6)) + '"');
println('Base64 decodes to "' + new String(Base64.getDecoder().decode(authorization.substring(6).getBytes(StandardCharsets.UTF_8)), StandardCharsets.UTF_8) + '"');
That results in the following output:
XTm90IGEgdmVyeSBnb29kIHBhc3N3b3Jk
Scrambled decodes to ""
java.lang.IllegalArgumentException: Last unit does not have enough valid bits
at java.base/java.util.Base64$Decoder.decode0(Unknown Source)
at java.base/java.util.Base64$Decoder.decode(Unknown Source)
Is that change of behavior OK?
See JENKINS-75370.
Scrambler
does not really have a good reason to exist: To encode/decodeBase64
, usejava.util.Base64
directly. To protect secrets, useSecret
.Removal of backward compatibility can easily be removed from this PR if a more focused PR is desired, as it's just Base64 decode. I just thought it was time and used the opportunity: 2011 and 2009 have been a while ago.
Testing done
None so far, waiting for autotests.
Proposed changelog entries
hudson.util.Scrambler
. Usejava.util.Base64
instead.Proposed changelog category
/label developer removed rfe
Proposed upgrade guidelines
N/A
Submitter checklist
@Restricted
or have@since TODO
Javadocs, as appropriate.@Deprecated(since = "TODO")
or@Deprecated(forRemoval = true, since = "TODO")
, if applicable.eval
to ease future introduction of Content Security Policy (CSP) directives (see documentation).Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist
upgrade-guide-needed
label is set and there is a Proposed upgrade guidelines section in the pull request title (see example).lts-candidate
to be considered (see query).