-
-
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-74858] Added validation for Password length in FIPS mode #9995
Conversation
Yay, your first pull request towards Jenkins core was created successfully! Thank you so much! |
core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java
Outdated
Show resolved
Hide resolved
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.
Would be nice if newly added tests covered the success case (long + matching password) in addition to not relying on mocks.
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Outdated
Show resolved
Hide resolved
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.
You have spotless issues, just execute mvn clean install -DskipTests
to see and fix them
(haven't see them clearly in the CI logs, but they seem to be related to my comments about the empty line and the wildcard)
core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java
Outdated
Show resolved
Hide resolved
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Outdated
Show resolved
Hide resolved
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Outdated
Show resolved
Hide resolved
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Outdated
Show resolved
Hide resolved
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Show resolved
Hide resolved
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.
LGTM
core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java
Outdated
Show resolved
Hide resolved
@MarkEWaite @basil |
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Outdated
Show resolved
Hide resolved
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Outdated
Show resolved
Hide resolved
test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java
Outdated
Show resolved
Hide resolved
@jenkinsci/core-pr-reviewers any chance to get help to get this merged? |
core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java
Outdated
Show resolved
Hide resolved
LGTM, I've set the ready for merge label. I'll be away for a few days but someone else can feel free to merge this in 24 hours if nothing comes up. Else I'll merge this when I'm back |
Congratulations on getting your very first Jenkins core pull request merged 🎉🥳 |
See JENKINS-74858.
Made changes to ensure that an exisiting user is now unable to alter the password to less than 14 characters in length when operating in FIPS mode.
Testing done
Automated Unit Testing
Implemented unit tests to verify FIPS compliance for password changes:
Manual Testing
Did Manual testing to verify my changes to ensure that everything is working properly.
Without FIPS mode: Shorter password is being accepted.

With FIPS mode: Shorter password is not being accepted.


Proposed changelog entries
Proposed upgrade guidelines
N/A
Submitter checklist
Desired reviewers
@mention
Before the changes are marked as
ready-for-merge
:Maintainer checklist