[JENKINS-73806] Do not allows users to authenticate with short passwords in FIPS mode #296
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
BorisYaoA
changed the title
BorisYaoA
changed the title
BorisYaoA
commented
Sep 23, 2024
| @@ -411,6 +413,9 @@ public FormValidation doCheckServer(@QueryParameter String value, @QueryParamete | |||
| if(!Jenkins.get().hasPermission(Jenkins.ADMINISTER)) | |||
| return FormValidation.ok(); | |||
|
|
|||
| if(FIPS140.useCompliantAlgorithms() && managerPassword.length() < 14) | |||
There was a problem hiding this comment.
Checking that we are running Jenkins in FIPS mode and that the password is FIPS compliant (see https://github.com/jenkinsci/jep/tree/master/jep/237 for Jenkins FIPS support)
alecharp
reviewed
Sep 24, 2024
| public class LDAPConfigurationWithFIPSTest { | ||
|
|
||
| @Rule | ||
| public JenkinsRule r = new JenkinsRule(); |
There was a problem hiding this comment.
Could you use a RealJenkinsRule so that we could benefit from jenkinsci/jenkins-test-harness#829?
fcojfernandez
requested changes
Sep 24, 2024
There was a problem hiding this comment.
While the FormValidation is the way to inform the user, it won't prevent the user to click on Save button and store a non-compliant configuration. Also, with CasC you could save a unsecure configuration, so the proper fix would be:
- Add the FormValidation as you did so in the UI you can see there's an error
- Add a check to prevent the non-compliant configuration to be saved. You can do that either in the DataBoundConstructor, in the DataBoundSetter or in a
PostConstructmethod. The usual way is throwing an IllegalArgumentException like in https://github.com/jenkinsci/active-directory-plugin/blob/ff7b23e0c59f21c35486a5f8fb533097e43a7e81/src/main/java/hudson/plugins/active_directory/ActiveDirectorySecurityRealm.java#L283-L286, or a FormException and take advantage of TolerateHttpResponsethrown fromDescriptor.newInstancejenkins#9495
rsandell
requested changes
Sep 24, 2024
|
Bobby is right, this PR is fixing another different issue. The fix for this ticket should be in |
rsandell
reviewed
Sep 27, 2024
| @@ -753,6 +754,9 @@ public SecurityComponents createSecurityComponents() { | |||
| */ | |||
| @Override | |||
| protected UserDetails authenticate2(String username, String password) throws AuthenticationException { | |||
| if(FIPS140.useCompliantAlgorithms() && (StringUtils.isBlank(password) || password.length() < 14)) { | |||
| throw new org.springframework.ldap.AuthenticationException(new javax.naming.AuthenticationException("When running in FIPS compliance mode, the password must be at least 14 characters long")); | |||
There was a problem hiding this comment.
out of curiosity, why not
Suggested change
| throw new org.springframework.ldap.AuthenticationException(new javax.naming.AuthenticationException("When running in FIPS compliance mode, the password must be at least 14 characters long")); | |
| throw new AuthenticationException("When running in FIPS compliance mode, the password must be at least 14 characters long")); |
?
There was a problem hiding this comment.
I'm having this message 'AuthenticationException' is abstract; cannot be instantiated
| @@ -6,6 +6,7 @@ LDAPSecurityRealm.LoginHeader=Login | |||
| LDAPSecurityRealm.AuthenticationSuccessful=Authentication: successful | |||
| LDAPSecurityRealm.AuthenticationFailed=Authentication: failed for user "{0}" | |||
| LDAPSecurityRealm.AuthenticationFailedEmptyPass=Authentication: failed for user "{0}" with empty password | |||
| LDAPSecurityRealm.AuthenticationFailedNotFipsCompliantPass=When running in FIPS compliance mode, the password must be at least 14 characters long | |||
There was a problem hiding this comment.
No I will remove it
…s_in_fips_mode' into unblock-JENKINS-73806
Move FIPS check to AuthenticationManager
fcojfernandez
approved these changes
Sep 27, 2024
olamy
approved these changes
Sep 28, 2024
PereBueno
approved these changes
Sep 30, 2024
rsandell
approved these changes
Sep 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
see https://issues.jenkins.io/browse/JENKINS-73806
The goal of this PR is to check that the ldap user's password is FIPS compliant (at least 14 characters).
By running a local jenkins in FIPS mode (
mvn clean hpi:run -P core-oc -Djenkins.security.FIPS140.COMPLIANCE=true) I can see an angry Jenkins when the password is shorter than 14 characters in FIPS mode and the message in the logs.Testing done
Submitter checklist