Skip to content

Commit 5e36565

Browse files
daniel-beckjtnord
daniel-beck
and
jtnord
authored
[JENKINS-75533] Remove jbcrypt mindrot, use Spring Security instead (#10604)
Co-authored-by: Daniel Beck <daniel-beck@users.noreply.github.com> Co-authored-by: James Nord <jtnord@users.noreply.github.com>
1 parent 60df855 commit 5e36565

File tree

9 files changed

+126
-53
lines changed

9 files changed

+126
-53
lines changed

‎bom/pom.xml‎

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -206,11 +206,6 @@ THE SOFTWARE.
206206
<artifactId>groovy-all</artifactId>
207207
<version>${groovy.version}</version>
208208
</dependency>
209-
<dependency>
210-
<groupId>org.connectbot</groupId>
211-
<artifactId>jbcrypt</artifactId>
212-
<version>1.0.2</version>
213-
</dependency>
214209
<dependency>
215210
<!-- Groovy shell uses this, but it doesn't declare the dependency -->
216211
<groupId>org.fusesource.jansi</groupId>

‎core/pom.xml‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -246,10 +246,6 @@ THE SOFTWARE.
246246
<groupId>org.codehaus.groovy</groupId>
247247
<artifactId>groovy-all</artifactId>
248248
</dependency>
249-
<dependency>
250-
<groupId>org.connectbot</groupId>
251-
<artifactId>jbcrypt</artifactId>
252-
</dependency>
253249
<dependency>
254250
<!-- Groovy shell uses this, but it doesn't declare the dependency -->
255251
<groupId>org.fusesource.jansi</groupId>

‎core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java‎

Lines changed: 26 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@
9191
import org.kohsuke.stapler.StaplerRequest2;
9292
import org.kohsuke.stapler.StaplerResponse2;
9393
import org.kohsuke.stapler.interceptor.RequirePOST;
94-
import org.mindrot.jbcrypt.BCrypt;
9594
import org.springframework.security.authentication.BadCredentialsException;
9695
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
9796
import org.springframework.security.core.Authentication;
@@ -100,6 +99,7 @@
10099
import org.springframework.security.core.context.SecurityContextHolder;
101100
import org.springframework.security.core.userdetails.UserDetails;
102101
import org.springframework.security.core.userdetails.UsernameNotFoundException;
102+
import org.springframework.security.crypto.bcrypt.BCryptPasswordEncoder;
103103
import org.springframework.security.crypto.password.PasswordEncoder;
104104

105105
/**
@@ -451,11 +451,12 @@ private SignupInfo validateAccountCreationForm(StaplerRequest2 req, boolean vali
451451
si.errors.put("password1", Messages.HudsonPrivateSecurityRealm_CreateAccount_PasswordRequired());
452452
}
453453

454-
if (FIPS140.useCompliantAlgorithms()) {
455-
if (si.password1.length() < FIPS_PASSWORD_LENGTH) {
456-
si.errors.put("password1", Messages.HudsonPrivateSecurityRealm_CreateAccount_FIPS_PasswordLengthInvalid());
457-
}
454+
try {
455+
PASSWORD_HASH_ENCODER.encode(si.password1);
456+
} catch (RuntimeException ex) {
457+
si.errors.put("password1", ex.getMessage());
458458
}
459+
459460
if (si.fullname == null || si.fullname.isEmpty()) {
460461
si.fullname = si.username;
461462
}
@@ -823,10 +824,6 @@ public Details newInstance(StaplerRequest2 req, JSONObject formData) throws Form
823824
throw new FormException("Please confirm the password by typing it twice", "user.password2");
824825
}
825826

826-
if (FIPS140.useCompliantAlgorithms() && pwd.length() < FIPS_PASSWORD_LENGTH) {
827-
throw new FormException(Messages.HudsonPrivateSecurityRealm_CreateAccount_FIPS_PasswordLengthInvalid(), "user.password");
828-
}
829-
830827
// will be null if it wasn't encrypted
831828
String data = Protector.unprotect(pwd);
832829
String data2 = Protector.unprotect(pwd2);
@@ -849,10 +846,18 @@ public Details newInstance(StaplerRequest2 req, JSONObject formData) throws Form
849846
if (data != null) {
850847
String prefix = Stapler.getCurrentRequest2().getSession().getId() + ':';
851848
if (data.startsWith(prefix)) {
849+
// The password is not being changed
852850
return Details.fromHashedPassword(data.substring(prefix.length()));
853851
}
854852
}
855853

854+
// The password is being changed
855+
try {
856+
PASSWORD_HASH_ENCODER.encode(pwd);
857+
} catch (RuntimeException ex) {
858+
throw new FormException(ex.getMessage(), "user.password");
859+
}
860+
856861
User user = Util.getNearestAncestorOfTypeOrThrow(req, User.class);
857862
// the UserSeedProperty is not touched by the configure page
858863
UserSeedProperty userSeedProperty = user.getProperty(UserSeedProperty.class);
@@ -917,11 +922,10 @@ public Category getCategory() {
917922
}
918923
}
919924

920-
// TODO can we instead use BCryptPasswordEncoder from Spring Security, which has its own copy of BCrypt so we could drop the special library?
921925
/**
922926
* {@link PasswordHashEncoder} that uses jBCrypt.
923927
*/
924-
static class JBCryptEncoder implements PasswordHashEncoder {
928+
static class JBCryptEncoder extends BCryptPasswordEncoder implements PasswordHashEncoder {
925929
// in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017
926930
// and for 18, it's "only" 20s
927931
@Restricted(NoExternalUse.class)
@@ -931,12 +935,14 @@ static class JBCryptEncoder implements PasswordHashEncoder {
931935

932936
@Override
933937
public String encode(CharSequence rawPassword) {
934-
return BCrypt.hashpw(rawPassword.toString(), BCrypt.gensalt());
935-
}
936-
937-
@Override
938-
public boolean matches(CharSequence rawPassword, String encodedPassword) {
939-
return BCrypt.checkpw(rawPassword.toString(), encodedPassword);
938+
try {
939+
return super.encode(rawPassword);
940+
} catch (IllegalArgumentException ex) {
941+
if (ex.getMessage().equals("password cannot be more than 72 bytes")) {
942+
throw new IllegalArgumentException(Messages.HudsonPrivateSecurityRealm_CreateAccount_BCrypt_PasswordTooLong());
943+
}
944+
throw ex;
945+
}
940946
}
941947

942948
/**
@@ -978,6 +984,9 @@ static class PBKDF2PasswordEncoder implements PasswordHashEncoder {
978984

979985
@Override
980986
public String encode(CharSequence rawPassword) {
987+
if (rawPassword.length() < FIPS_PASSWORD_LENGTH) {
988+
throw new IllegalArgumentException(Messages.HudsonPrivateSecurityRealm_CreateAccount_FIPS_PasswordLengthInvalid());
989+
}
981990
try {
982991
return generatePasswordHashWithPBKDF2(rawPassword);
983992
} catch (NoSuchAlgorithmException | InvalidKeySpecException e) {

‎core/src/main/resources/hudson/security/Messages.properties‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ HudsonPrivateSecurityRealm.ManageUserLinks.Description=Create/delete/modify user
3737
HudsonPrivateSecurityRealm.CreateAccount.TextNotMatchWordInImage=Text didn''t match the word shown in the image
3838
HudsonPrivateSecurityRealm.CreateAccount.PasswordNotMatch=Password didn''t match
3939
HudsonPrivateSecurityRealm.CreateAccount.FIPS.PasswordLengthInvalid=Password must be at least 14 characters long
40+
HudsonPrivateSecurityRealm.CreateAccount.BCrypt.PasswordTooLong=Jenkins’ own user database currently only supports passwords of up to 72 bytes UTF-8 (72 basic ASCII characters, 24-36 CJK characters, or 18 emoji). Please use a shorter password.
4041
HudsonPrivateSecurityRealm.CreateAccount.PasswordRequired=Password is required
4142
HudsonPrivateSecurityRealm.CreateAccount.UserNameRequired=User name is required
4243
HudsonPrivateSecurityRealm.CreateAccount.UserNameInvalidCharacters=User name must only contain alphanumeric characters, underscore and dash

‎core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
package hudson.security;
22

3+
import static org.hamcrest.MatcherAssert.assertThat;
4+
import static org.hamcrest.Matchers.is;
35
import static org.junit.jupiter.api.Assertions.assertFalse;
46
import static org.junit.jupiter.api.Assertions.assertThrows;
57
import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -13,7 +15,9 @@
1315
import java.security.spec.InvalidKeySpecException;
1416
import java.time.Duration;
1517
import javax.crypto.SecretKeyFactory;
18+
import org.junit.Assert;
1619
import org.junit.jupiter.api.Test;
20+
import org.jvnet.hudson.test.Issue;
1721
import org.mockito.Mockito;
1822

1923
public class HudsonPrivateSecurityRealmTest {
@@ -151,4 +155,10 @@ public void testJBCryptPasswordMatching() {
151155
assertTrue(encoder.matches("thisIsMyPassword", encoded));
152156
assertFalse(encoder.matches("thisIsNotMyPassword", encoded));
153157
}
158+
159+
@Issue("JENKINS-75533")
160+
public void ensureExpectedMessage() {
161+
final IllegalArgumentException ex = Assert.assertThrows(IllegalArgumentException.class, () -> HudsonPrivateSecurityRealm.PASSWORD_HASH_ENCODER.encode("1234567890123456789012345678901234567890123456789012345678901234567890123"));
162+
assertThat(ex.getMessage(), is(Messages.HudsonPrivateSecurityRealm_CreateAccount_BCrypt_PasswordTooLong()));
163+
}
154164
}

‎test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java‎

Lines changed: 13 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,6 @@
3535
import hudson.logging.LogRecorderManager;
3636
import hudson.model.User;
3737
import hudson.security.HudsonPrivateSecurityRealm.Details;
38-
import java.lang.reflect.Method;
3938
import java.util.List;
4039
import java.util.logging.Level;
4140
import java.util.logging.LogRecord;
@@ -52,13 +51,14 @@
5251
import org.jvnet.hudson.test.JenkinsRule;
5352
import org.jvnet.hudson.test.JenkinsRule.WebClient;
5453
import org.jvnet.hudson.test.RealJenkinsRule;
54+
import org.jvnet.hudson.test.recipes.LocalData;
5555

5656

5757
@For(HudsonPrivateSecurityRealm.class)
5858
public class HudsonPrivateSecurityRealmFIPSTest {
5959

60-
// the jbcrypt encoded for of "a" without the quotes
61-
private static final String JBCRYPT_ENCODED_PASSWORD = "#jbcrypt:$2a$06$m0CrhHm10qJ3lXRY.5zDGO3rS2KdeeWLuGmsfGlMfOxih58VYVfxe";
60+
// the bcrypt encoded form of "passwordpassword" without the quotes
61+
private static final String JBCRYPT_ENCODED_PASSWORD = "#jbcrypt:$2a$10$Nm37vwdZwJ5T2QTBwYuBYONHD3qKilgd5UO7wuDXI83z5dAdrgi4i";
6262

6363
private static final String LOG_RECORDER_NAME = "HPSR_LOG_RECORDER";
6464

@@ -75,7 +75,7 @@ private static void generalLoginStep(JenkinsRule j) throws Exception {
7575
HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null);
7676
j.jenkins.setSecurityRealm(securityRealm);
7777

78-
User u1 = securityRealm.createAccount("user", "password");
78+
User u1 = securityRealm.createAccount("user", "passwordpassword");
7979
u1.setFullName("A User");
8080
u1.save();
8181

@@ -84,8 +84,8 @@ private static void generalLoginStep(JenkinsRule j) throws Exception {
8484
assertThat(hashedPassword, startsWith("$PBKDF2$HMACSHA512:210000:"));
8585

8686
try (WebClient wc = j.createWebClient()) {
87-
wc.login("user", "password");
88-
assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user", "wrongPass"));
87+
wc.login("user", "passwordpassword");
88+
assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user", "wrongPass123456"));
8989
}
9090
}
9191

@@ -98,19 +98,20 @@ private static void userCreationWithHashedPasswordsStep(JenkinsRule j) throws Ex
9898
setupLogRecorder();
9999
HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null);
100100
j.jenkins.setSecurityRealm(securityRealm);
101-
// "password" after it has gone through the KDF
101+
// "passwordpassword" after it has gone through the KDF
102102
securityRealm.createAccountWithHashedPassword("user_hashed",
103-
"$PBKDF2$HMACSHA512:210000:ffbb207b847010af98cdd2b09c79392c$f67c3b985daf60db83a9088bc2439f7b77016d26c1439a9877c4f863c377272283ce346edda4578a5607ea620a4beb662d853b800f373297e6f596af797743a6");
103+
"$PBKDF2$HMACSHA512:210000:92857a190ac711436e8a9cb56595d642$0d66e61da0b04283148b4a574422a17762c96c46bcd3aa587b6d447a908367fe8030bd2083dc54313639561d36cc9ac707bed72fc3400465e7dc1d6805cffb66");
104104

105105
try (WebClient wc = j.createWebClient()) {
106106
// login should succeed
107-
wc.login("user_hashed", "password");
108-
assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user_hashed", "password2"));
107+
wc.login("user_hashed", "passwordpassword");
108+
assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user_hashed", "passwordpassword2"));
109109
}
110110
assertThat(getLogRecords(), not(hasItem(incorrectHashingLogEntry())));
111111
}
112112

113113
@Test
114+
@LocalData
114115
public void userLoginAfterEnablingFIPS() throws Throwable {
115116
rjr.then(HudsonPrivateSecurityRealmFIPSTest::userLoginAfterEnablingFIPSStep);
116117
}
@@ -120,19 +121,10 @@ private static void userLoginAfterEnablingFIPSStep(JenkinsRule j) throws Excepti
120121
HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null);
121122
j.jenkins.setSecurityRealm(securityRealm);
122123

123-
User u1 = securityRealm.createAccount("user", "a");
124-
u1.setFullName("A User");
125-
// overwrite the password property using an password created using an incorrect algorithm
126-
Method m = Details.class.getDeclaredMethod("fromHashedPassword", String.class);
127-
m.setAccessible(true);
128-
Details d = (Details) m.invoke(null, JBCRYPT_ENCODED_PASSWORD);
129-
u1.addProperty(d);
130-
131-
u1.save();
132-
assertThat(u1.getProperty(Details.class).getPassword(), is(JBCRYPT_ENCODED_PASSWORD));
124+
assertThat(securityRealm.getUser("user").getProperty(Details.class).getPassword(), is(JBCRYPT_ENCODED_PASSWORD));
133125

134126
try (WebClient wc = j.createWebClient()) {
135-
assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user", "a"));
127+
assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user", "passwordpassword"));
136128
}
137129
assertThat(getLogRecords(), hasItem(incorrectHashingLogEntry()));
138130
}

‎test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@
7676
import org.jvnet.hudson.test.JenkinsRule.WebClient;
7777
import org.jvnet.hudson.test.LoggerRule;
7878
import org.jvnet.hudson.test.TestExtension;
79-
import org.mindrot.jbcrypt.BCrypt;
79+
import org.springframework.security.crypto.bcrypt.BCrypt;
8080

8181
@For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class})
8282
public class HudsonPrivateSecurityRealmTest {
@@ -549,7 +549,7 @@ public void hashedPasswordTest() {
549549
// password = password
550550
assertFalse("too big number of iterations", PASSWORD_ENCODER.isPasswordHashed("#jbcrypt:$2a208$aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"));
551551

552-
// until https://github.com/jeremyh/jBCrypt/pull/16 is merged, the lib released and the dep updated, only the version 2a is supported
552+
// Supported by Spring Security's BCrypt, but not by the Jenkins wrapper
553553
assertFalse("unsupported version", PASSWORD_ENCODER.isPasswordHashed("#jbcrypt:$2x$08$Ro0CUfOqk6cXEKf3dyaM7OhSCvnwM9s4wIX9JeLapehKK5YdLxKcm"));
554554
assertFalse("unsupported version", PASSWORD_ENCODER.isPasswordHashed("#jbcrypt:$2y$06$m0CrhHm10qJ3lXRY.5zDGO3rS2KdeeWLuGmsfGlMfOxih58VYVfxe"));
555555

@@ -563,13 +563,15 @@ public void ensureHashingVersion_2a_isSupported() {
563563
}
564564

565565
@Test
566-
public void ensureHashingVersion_2x_isNotSupported() {
567-
assertThrows(IllegalArgumentException.class, () -> BCrypt.checkpw("abc", "$2x$08$Ro0CUfOqk6cXEKf3dyaM7OhSCvnwM9s4wIX9JeLapehKK5YdLxKcm"));
566+
public void ensureHashingVersion_2x_isSupported() {
567+
// See #hashedPasswordTest for the corresponding test going through the Jenkins core wrapper class rejecting 2x
568+
assertTrue("version 2x is supported", BCrypt.checkpw("abc", "$2x$08$Ro0CUfOqk6cXEKf3dyaM7OhSCvnwM9s4wIX9JeLapehKK5YdLxKcm"));
568569
}
569570

570571
@Test
571-
public void ensureHashingVersion_2y_isNotSupported() {
572-
assertThrows(IllegalArgumentException.class, () -> BCrypt.checkpw("a", "$2y$08$cfcvVd2aQ8CMvoMpP2EBfeodLEkkFJ9umNEfPD18.hUF62qqlC/V."));
572+
public void ensureHashingVersion_2y_isSupported() {
573+
// See #hashedPasswordTest for the corresponding test going through the Jenkins core wrapper class rejecting 2y
574+
assertTrue("version 2y is supported", BCrypt.checkpw("a", "$2y$08$cfcvVd2aQ8CMvoMpP2EBfeodLEkkFJ9umNEfPD18.hUF62qqlC/V."));
573575
}
574576

575577
private void checkUserCanBeCreatedWith(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email) throws Exception {
@@ -759,6 +761,23 @@ public void userCreationWithPBKDFPasswords() throws Exception {
759761
is("The hashed password was hashed with an incorrect algorithm. Jenkins is expecting #jbcrypt:"));
760762
}
761763

764+
@Issue("JENKINS-75533")
765+
public void supportLongerPasswordToLogIn() throws Exception {
766+
HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null);
767+
j.jenkins.setSecurityRealm(securityRealm);
768+
final String _72CharPass = "123456789012345678901234567890123456789012345678901234567890123456789012";
769+
final String username = "user";
770+
securityRealm.createAccount(username, _72CharPass);
771+
try (WebClient wc = j.createWebClient()) {
772+
// can log in with the real 72 byte password
773+
wc.login(username, _72CharPass);
774+
}
775+
try (WebClient wc = j.createWebClient()) {
776+
// can log in with even longer password for this edge case
777+
wc.login(username, _72CharPass + "345");
778+
}
779+
}
780+
762781
private static Matcher<LoggerRule> hasIncorrectHashingLogEntry() {
763782
return LoggerRule.recorded(is(
764783
"A password appears to be stored (or is attempting to be stored) that was created with a different hashing/encryption algorithm, check the FIPS-140 state of the system has not changed inadvertently"));

‎test/src/test/resources/hudson/security/HudsonPrivateSecurityRealmFIPSTest/userLoginAfterEnablingFIPS/users/user_1072549610582667998/config.xml‎

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,41 @@
1+
<?xml version='1.1' encoding='UTF-8'?>
2+
<user>
3+
<version>10</version>
4+
<id>user</id>
5+
<fullName>The User</fullName>
6+
<properties>
7+
<jenkins.console.ConsoleUrlProviderUserProperty/>
8+
<hudson.model.MyViewsProperty>
9+
<views>
10+
<hudson.model.AllView>
11+
<owner class="hudson.model.MyViewsProperty" reference="../../.."/>
12+
<name>all</name>
13+
<filterExecutors>false</filterExecutors>
14+
<filterQueue>false</filterQueue>
15+
<properties class="hudson.model.View$PropertyList"/>
16+
</hudson.model.AllView>
17+
</views>
18+
</hudson.model.MyViewsProperty>
19+
<hudson.model.PaneStatusProperties>
20+
<collapsed/>
21+
</hudson.model.PaneStatusProperties>
22+
<hudson.search.UserSearchProperty>
23+
<insensitiveSearch>true</insensitiveSearch>
24+
</hudson.search.UserSearchProperty>
25+
<hudson.model.TimeZoneProperty/>
26+
<jenkins.model.experimentalflags.UserExperimentalFlagsProperty>
27+
<flags/>
28+
</jenkins.model.experimentalflags.UserExperimentalFlagsProperty>
29+
<jenkins.security.ApiTokenProperty>
30+
<tokenStore>
31+
<tokenList/>
32+
</tokenStore>
33+
</jenkins.security.ApiTokenProperty>
34+
<hudson.security.HudsonPrivateSecurityRealm_-Details>
35+
<passwordHash>#jbcrypt:$2a$10$Nm37vwdZwJ5T2QTBwYuBYONHD3qKilgd5UO7wuDXI83z5dAdrgi4i</passwordHash>
36+
</hudson.security.HudsonPrivateSecurityRealm_-Details>
37+
<jenkins.security.seed.UserSeedProperty>
38+
<seed>ec31fd374346de1e</seed>
39+
</jenkins.security.seed.UserSeedProperty>
40+
</properties>
41+
</user>

‎test/src/test/resources/hudson/security/HudsonPrivateSecurityRealmFIPSTest/userLoginAfterEnablingFIPS/users/users.xml‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
<?xml version='1.1' encoding='UTF-8'?>
2+
<hudson.model.UserIdMapper>
3+
<version>1</version>
4+
<idToDirectoryNameMap class="concurrent-hash-map">
5+
<entry>
6+
<string>user</string>
7+
<string>user_1072549610582667998</string>
8+
</entry>
9+
</idToDirectoryNameMap>
10+
</hudson.model.UserIdMapper>

0 commit comments

Comments
 (0)