From ba9e113971ac1d1554aeb1758a35ec4371bf256a Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Thu, 27 Jul 2023 16:23:34 +0530 Subject: [PATCH 01/39] Password Hashing changes in HudsonPrivateSecurityRealm --- core/src/main/java/hudson/Util.java | 11 ++ .../security/HudsonPrivateSecurityRealm.java | 163 ++++++++++++++++-- .../hudson/security/PasswordHashEncoder.java | 7 + 3 files changed, 166 insertions(+), 15 deletions(-) create mode 100644 core/src/main/java/hudson/security/PasswordHashEncoder.java diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 6adba3022f55..48ef768cb5cd 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1927,4 +1927,15 @@ public static long daysElapsedSince(@NonNull Date date) { private static PathRemover newPathRemover(@NonNull PathRemover.PathChecker pathChecker) { return PathRemover.newFilteredRobustRemover(pathChecker, DELETION_RETRIES, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); } + + private static final String GET_FIPS_MODE = ".FIPS_MODE"; + + @Restricted(NoExternalUse.class) + public static final boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + GET_FIPS_MODE); + + public static final int ITERATION_COUNT = 1000; + public static final int KEY_LENGTH = 512; + public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; + public static final String PBKDF_2 = "#pbkdf2:"; + public static final String JBCRYPT = "#jbcrypt:"; } diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index fb93f116163f..ff5a5a63a676 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -48,8 +48,12 @@ import hudson.util.XStream2; import java.io.IOException; import java.lang.reflect.Constructor; +import java.math.BigInteger; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.security.SecureRandom; +import java.security.spec.InvalidKeySpecException; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; @@ -60,6 +64,8 @@ import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; import javax.servlet.Filter; import javax.servlet.FilterChain; import javax.servlet.FilterConfig; @@ -883,11 +889,7 @@ public Category getCategory() { } } - // TODO can we instead use BCryptPasswordEncoder from Spring Security, which has its own copy of BCrypt so we could drop the special library? - /** - * {@link PasswordEncoder} that uses jBCrypt. - */ - private static class JBCryptEncoder implements PasswordEncoder { + private static class JBCryptEncoder implements PasswordHashEncoder { // in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017 // and for 18, it's "only" 20s @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") @@ -911,6 +913,7 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { * implementation defined in jBCrypt and the Wikipedia page. * */ + @Override public boolean isHashValid(String hash) { Matcher matcher = BCRYPT_PATTERN.matcher(hash); if (matcher.matches()) { @@ -925,18 +928,139 @@ public boolean isHashValid(String hash) { } } - /* package */ static final JBCryptEncoder JBCRYPT_ENCODER = new JBCryptEncoder(); + private static class PBKDF2PasswordEncoder implements PasswordHashEncoder { + + public static final int HASH_LENGTH = 8; + public static final int ZERO = 0; + public static final int HEX_TWO = 2; + public static final int RADIX = 16; + public static final String STRING_SEPARATION = ":"; + + @Override + public String encode(CharSequence rawPassword) { + try { + return generatePasswordHashWithPBKDF2(rawPassword); + } catch (NoSuchAlgorithmException e) { + //log exception message + throw new RuntimeException(e); + } catch (InvalidKeySpecException e) { + throw new RuntimeException(e); + } + } + + @Override + public boolean matches(CharSequence rawPassword, String encodedPassword) { + try { + return validatePassword(rawPassword.toString(), encodedPassword); + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException(e); + } catch (InvalidKeySpecException e) { + throw new RuntimeException(e); + } + } +//private and inside the impl class + private static String generatePasswordHashWithPBKDF2(CharSequence password) throws NoSuchAlgorithmException, InvalidKeySpecException { + + /* KeySpec spec = new PBEKeySpec(password.toString().toCharArray(), generateSalt(), Util.ITERATION_COUNT, Util.KEY_LENGTH); + SecretKeyFactory f = SecretKeyFactory.getInstance(Util.PBKDF2_ALGORITHM); + byte[] hash = f.generateSecret(spec).getEncoded(); + Base64.Encoder enc = Base64.getEncoder(); + *//* System.out.printf("salt: %s%n", enc.encodeToString(generateSalt())); + System.out.printf("hash: %s%n", enc.encodeToString(hash)); + System.out.println(toHex(hash));*//* + System.out.println(enc.encodeToString(hash)); + return enc.encodeToString(hash);*/ + int iterations = Util.ITERATION_COUNT; + byte[] salt = generateSalt(); + + PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, iterations, Util.KEY_LENGTH); + byte[] hash = generateSecretKey(spec); + return iterations + STRING_SEPARATION + toHex(salt) + STRING_SEPARATION + toHex(hash); + } + + private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmException, InvalidKeySpecException { + SecretKeyFactory secretKeyFactory = SecretKeyFactory.getInstance(Util.PBKDF2_ALGORITHM); + + byte[] hash = secretKeyFactory.generateSecret(spec).getEncoded(); + return hash; + } + + private static String toHex(byte[] array) { + + BigInteger bi = new BigInteger(1, array); + String hex = bi.toString(RADIX); + + int paddingLength = (array.length * HEX_TWO) - hex.length(); + if (paddingLength > ZERO) { + return String.format("%0" + paddingLength + "d", ZERO) + hex; + } else { + return hex; + } + } + + @SuppressFBWarnings(value = {"DMI_RANDOM_USED_ONLY_ONCE", "PREDICTABLE_RANDOM"}, justification = "https://github.com/spotbugs/spotbugs/issues/1539 and doesn't need to be secure, we're just not hardcoding a 'wrong' password") + private static byte[] generateSalt() { + SecureRandom random = new SecureRandom(); + byte[] salt = new byte[16]; + random.nextBytes(salt); + return salt; + } + + @Override + public boolean isHashValid(String hash) { + + return true; + } + + private static boolean validatePassword(String originalPassword, String storedPassword) + throws NoSuchAlgorithmException, InvalidKeySpecException + { + String[] parts = storedPassword.split(STRING_SEPARATION); + int iterations = Integer.parseInt(parts[0]); + + byte[] salt = fromHex(parts[1]); + byte[] hash = fromHex(parts[2]); + + PBEKeySpec spec = new PBEKeySpec(originalPassword.toCharArray(), + salt, iterations, hash.length * HASH_LENGTH); + + byte[] testHash = generateSecretKey(spec); + + int diff = hash.length ^ testHash.length; + for (int i = ZERO; i < hash.length && i < testHash.length; i++) + { + diff |= hash[i] ^ testHash[i]; + } + return diff == 0; + } + + private static byte[] fromHex(String hex) { + byte[] bytes = new byte[hex.length() / HEX_TWO]; + for (int i = ZERO; i < bytes.length; i++) + { + bytes[i] = (byte) Integer.parseInt(hex.substring(HEX_TWO * i, HEX_TWO * i + HEX_TWO), RADIX); + } + return bytes; + } + + } + + /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.FIPS_MODE ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); + + + /** + * Magic header used to detect if a password is hashed. + */ + private static final String PASSWORD_HASH_HEADER = Util.FIPS_MODE ? Util.PBKDF_2 : Util.JBCRYPT; + + /* package */ // TODO check if DelegatingPasswordEncoder can be used /** - * Wraps {@link #JBCRYPT_ENCODER}. + * Wraps {@link #PASSWORD_HASH_ENCODER}. * There used to be a SHA-256-based encoder but this is long deprecated, and insecure anyway. */ /* package */ static class MultiPasswordEncoder implements PasswordEncoder { - /** - * Magic header used to detect if a password is bcrypt hashed. - */ - private static final String JBCRYPT_HEADER = "#jbcrypt:"; /* CLASSIC encoder outputs "salt:hash" where salt is [a-z]+, so we use unique prefix '#jbcyrpt" @@ -944,28 +1068,37 @@ public boolean isHashValid(String hash) { '#' is neither in base64 nor hex, which makes it a good choice. */ + @Override public String encode(CharSequence rawPassword) { - return JBCRYPT_HEADER + JBCRYPT_ENCODER.encode(rawPassword); + LOGGER.info(String.valueOf(Util.FIPS_MODE)); + return PASSWORD_HASH_HEADER + PASSWORD_HASH_ENCODER.encode(rawPassword); + } @Override public boolean matches(CharSequence rawPassword, String encPass) { if (isPasswordHashed(encPass)) { - return JBCRYPT_ENCODER.matches(rawPassword, encPass.substring(JBCRYPT_HEADER.length())); + return PASSWORD_HASH_ENCODER.matches(rawPassword, encPass.substring(PASSWORD_HASH_HEADER.length())); } else { return false; } } /** - * Returns true if the supplied password starts with a prefix indicating it is already hashed. + * Returns true if the supplied password starts with a prefix indicating i + * t is already hashed. */ public boolean isPasswordHashed(String password) { if (password == null) { return false; } - return password.startsWith(JBCRYPT_HEADER) && JBCRYPT_ENCODER.isHashValid(password.substring(JBCRYPT_HEADER.length())); + if (Util.FIPS_MODE) { + return password.startsWith(PASSWORD_HASH_HEADER); + } + else { + return password.startsWith(PASSWORD_HASH_HEADER) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(PASSWORD_HASH_HEADER.length())); + } } } diff --git a/core/src/main/java/hudson/security/PasswordHashEncoder.java b/core/src/main/java/hudson/security/PasswordHashEncoder.java new file mode 100644 index 000000000000..c49e9350b1fd --- /dev/null +++ b/core/src/main/java/hudson/security/PasswordHashEncoder.java @@ -0,0 +1,7 @@ +package hudson.security; + +import org.springframework.security.crypto.password.PasswordEncoder; + +public interface PasswordHashEncoder extends PasswordEncoder { + boolean isHashValid(String hash); +} From 1e640c1a6bcca8525b3863ec3a6561989f4e34a6 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Fri, 4 Aug 2023 15:43:57 +0530 Subject: [PATCH 02/39] Password Hashing -PBKDF2 Implementation --- core/src/main/java/hudson/Util.java | 11 +- .../security/HudsonPrivateSecurityRealm.java | 101 +++++++----------- .../HudsonPrivateSecurityRealmTest.java | 32 ++++-- 3 files changed, 70 insertions(+), 74 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 48ef768cb5cd..759eb08f601a 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1930,12 +1930,7 @@ private static PathRemover newPathRemover(@NonNull PathRemover.PathChecker pathC private static final String GET_FIPS_MODE = ".FIPS_MODE"; - @Restricted(NoExternalUse.class) - public static final boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + GET_FIPS_MODE); - - public static final int ITERATION_COUNT = 1000; - public static final int KEY_LENGTH = 512; - public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; - public static final String PBKDF_2 = "#pbkdf2:"; - public static final String JBCRYPT = "#jbcrypt:"; + public static boolean isFipsMode() { + return SystemProperties.getBoolean(Util.class.getName() + GET_FIPS_MODE); + } } diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index ff5a5a63a676..bb74432abb50 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -24,6 +24,7 @@ package hudson.security; +import static java.util.logging.Level.WARNING; import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import com.thoughtworks.xstream.converters.UnmarshallingContext; @@ -933,68 +934,55 @@ private static class PBKDF2PasswordEncoder implements PasswordHashEncoder { public static final int HASH_LENGTH = 8; public static final int ZERO = 0; public static final int HEX_TWO = 2; - public static final int RADIX = 16; + + public static final int RADIX_LENGTH = 16; public static final String STRING_SEPARATION = ":"; + public static final int ITERATION_COUNT = 1000; + public static final int KEY_LENGTH = 512; + public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; @Override public String encode(CharSequence rawPassword) { - try { - return generatePasswordHashWithPBKDF2(rawPassword); - } catch (NoSuchAlgorithmException e) { - //log exception message - throw new RuntimeException(e); - } catch (InvalidKeySpecException e) { - throw new RuntimeException(e); - } + return generatePasswordHashWithPBKDF2(rawPassword); } @Override public boolean matches(CharSequence rawPassword, String encodedPassword) { - try { return validatePassword(rawPassword.toString(), encodedPassword); - } catch (NoSuchAlgorithmException e) { - throw new RuntimeException(e); - } catch (InvalidKeySpecException e) { - throw new RuntimeException(e); - } } -//private and inside the impl class - private static String generatePasswordHashWithPBKDF2(CharSequence password) throws NoSuchAlgorithmException, InvalidKeySpecException { - - /* KeySpec spec = new PBEKeySpec(password.toString().toCharArray(), generateSalt(), Util.ITERATION_COUNT, Util.KEY_LENGTH); - SecretKeyFactory f = SecretKeyFactory.getInstance(Util.PBKDF2_ALGORITHM); - byte[] hash = f.generateSecret(spec).getEncoded(); - Base64.Encoder enc = Base64.getEncoder(); - *//* System.out.printf("salt: %s%n", enc.encodeToString(generateSalt())); - System.out.printf("hash: %s%n", enc.encodeToString(hash)); - System.out.println(toHex(hash));*//* - System.out.println(enc.encodeToString(hash)); - return enc.encodeToString(hash);*/ - int iterations = Util.ITERATION_COUNT; - byte[] salt = generateSalt(); - PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, iterations, Util.KEY_LENGTH); + private static String generatePasswordHashWithPBKDF2(CharSequence password) { + int iterations = ITERATION_COUNT; + byte[] salt = generateSalt(); + PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, iterations, KEY_LENGTH); byte[] hash = generateSecretKey(spec); return iterations + STRING_SEPARATION + toHex(salt) + STRING_SEPARATION + toHex(hash); } - private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmException, InvalidKeySpecException { - SecretKeyFactory secretKeyFactory = SecretKeyFactory.getInstance(Util.PBKDF2_ALGORITHM); - - byte[] hash = secretKeyFactory.generateSecret(spec).getEncoded(); + private static byte[] generateSecretKey(PBEKeySpec spec) { + SecretKeyFactory secretKeyFactory = null; + byte[] hash = new byte[0]; + try { + secretKeyFactory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); + hash = secretKeyFactory.generateSecret(spec).getEncoded(); + } catch (NoSuchAlgorithmException e) { + LOGGER.log(WARNING, "No such algorithm found for encode", e); + } catch (InvalidKeySpecException e) { + LOGGER.log(WARNING, "Invalid key spec exception", e); + } return hash; } - private static String toHex(byte[] array) { + private static String toHex(byte[] secretKey) { - BigInteger bi = new BigInteger(1, array); - String hex = bi.toString(RADIX); + BigInteger bi = new BigInteger(1, secretKey); + String saltHashValue = bi.toString(RADIX_LENGTH); - int paddingLength = (array.length * HEX_TWO) - hex.length(); + int paddingLength = (secretKey.length * HEX_TWO) - saltHashValue.length(); if (paddingLength > ZERO) { - return String.format("%0" + paddingLength + "d", ZERO) + hex; + return String.format("%0" + paddingLength + "d", ZERO) + saltHashValue; } else { - return hex; + return saltHashValue; } } @@ -1008,13 +996,10 @@ private static byte[] generateSalt() { @Override public boolean isHashValid(String hash) { - return true; } - private static boolean validatePassword(String originalPassword, String storedPassword) - throws NoSuchAlgorithmException, InvalidKeySpecException - { + private static boolean validatePassword(String originalPassword, String storedPassword) { String[] parts = storedPassword.split(STRING_SEPARATION); int iterations = Integer.parseInt(parts[0]); @@ -1024,34 +1009,36 @@ private static boolean validatePassword(String originalPassword, String storedPa PBEKeySpec spec = new PBEKeySpec(originalPassword.toCharArray(), salt, iterations, hash.length * HASH_LENGTH); - byte[] testHash = generateSecretKey(spec); + byte[] generatedHashValue = generateSecretKey(spec); - int diff = hash.length ^ testHash.length; - for (int i = ZERO; i < hash.length && i < testHash.length; i++) + int diff = hash.length ^ generatedHashValue.length; + for (int i = ZERO; i < hash.length && i < generatedHashValue.length; i++) { - diff |= hash[i] ^ testHash[i]; + diff |= hash[i] ^ generatedHashValue[i]; } return diff == 0; } - private static byte[] fromHex(String hex) { - byte[] bytes = new byte[hex.length() / HEX_TWO]; + private static byte[] fromHex(String storedPassword) { + byte[] bytes = new byte[storedPassword.length() / HEX_TWO]; for (int i = ZERO; i < bytes.length; i++) { - bytes[i] = (byte) Integer.parseInt(hex.substring(HEX_TWO * i, HEX_TWO * i + HEX_TWO), RADIX); + bytes[i] = (byte) Integer.parseInt(storedPassword.substring(HEX_TWO * i, HEX_TWO * i + HEX_TWO), RADIX_LENGTH); } return bytes; } } - /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.FIPS_MODE ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); + /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.isFipsMode() ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); + public static final String PBKDF2 = "#pbkdf2:"; + public static final String JBCRYPT = "#jbcrypt:"; /** * Magic header used to detect if a password is hashed. */ - private static final String PASSWORD_HASH_HEADER = Util.FIPS_MODE ? Util.PBKDF_2 : Util.JBCRYPT; + private static final String PASSWORD_HASH_HEADER = Util.isFipsMode() ? PBKDF2 : JBCRYPT; /* package */ @@ -1071,9 +1058,7 @@ private static byte[] fromHex(String hex) { @Override public String encode(CharSequence rawPassword) { - LOGGER.info(String.valueOf(Util.FIPS_MODE)); return PASSWORD_HASH_HEADER + PASSWORD_HASH_ENCODER.encode(rawPassword); - } @Override @@ -1093,12 +1078,7 @@ public boolean isPasswordHashed(String password) { if (password == null) { return false; } - if (Util.FIPS_MODE) { - return password.startsWith(PASSWORD_HASH_HEADER); - } - else { return password.startsWith(PASSWORD_HASH_HEADER) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(PASSWORD_HASH_HEADER.length())); - } } } @@ -1166,4 +1146,5 @@ public void destroy() { }; private static final Logger LOGGER = Logger.getLogger(HudsonPrivateSecurityRealm.class.getName()); + } diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 9883b0680843..d59aac55608b 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -25,6 +25,7 @@ package hudson.security; import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER; +import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_HASH_ENCODER; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -37,9 +38,12 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; +import hudson.Util; import hudson.model.User; import hudson.security.pages.SignupPage; import java.lang.reflect.Field; @@ -72,6 +76,7 @@ import org.jvnet.hudson.test.JenkinsRule.WebClient; import org.jvnet.hudson.test.TestExtension; import org.mindrot.jbcrypt.BCrypt; +import org.mockito.Mock; @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) public class HudsonPrivateSecurityRealmTest { @@ -81,17 +86,14 @@ public class HudsonPrivateSecurityRealmTest { private SpySecurityListenerImpl spySecurityListener; + @Mock + private Util util; + @Before public void linkExtension() { spySecurityListener = ExtensionList.lookup(SecurityListener.class).get(SpySecurityListenerImpl.class); } - @Before - public void setup() throws Exception { - Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); - field.setAccessible(true); - field.set(null, null); - } @Issue("SECURITY-243") @Test @@ -559,6 +561,24 @@ public void ensureHashingVersion_2y_isNotSupported() { assertThrows(IllegalArgumentException.class, () -> BCrypt.checkpw("a", "$2y$08$cfcvVd2aQ8CMvoMpP2EBfeodLEkkFJ9umNEfPD18.hUF62qqlC/V.")); } + @Test + public void hashedPasswordTestPBKDF2() { + mockStatic(Util.class); + when(Util.isFipsMode()).thenReturn(true); + PASSWORD_ENCODER.isPasswordHashed("#pbkdf2:" + PASSWORD_HASH_ENCODER.encode("password")); + assertNotNull(PASSWORD_HASH_ENCODER.encode("password")); + } + + @Test + public void hashedPasswordTestMatchesPBKDF2() { + PASSWORD_ENCODER.matches("password", "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b"); + } + + @Test + public void hashedPasswordTestMatchesPBKDF2False() { + assertFalse(PASSWORD_ENCODER.matches(null, "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b")); + } + private void checkUserCanBeCreatedWith(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email) throws Exception { JenkinsRule.WebClient wc = j.createWebClient(); SignupPage signup = new SignupPage(wc.goTo("signup")); From a1b676bb5b4114238e1b76704b442ed3b47a53ec Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Fri, 4 Aug 2023 15:58:23 +0530 Subject: [PATCH 03/39] reverted docs --- .../main/java/hudson/security/HudsonPrivateSecurityRealm.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index bb74432abb50..fa57a7c777c1 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -890,6 +890,10 @@ public Category getCategory() { } } + // TODO can we instead use BCryptPasswordEncoder from Spring Security, which has its own copy of BCrypt so we could drop the special library? + /** + * {@link PasswordHashEncoder} that uses jBCrypt. + */ private static class JBCryptEncoder implements PasswordHashEncoder { // in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017 // and for 18, it's "only" 20s From 26164765e0119c528fb1ec11b916026dec5b0e2b Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Thu, 10 Aug 2023 14:52:14 +0530 Subject: [PATCH 04/39] isHashValid Changes --- core/src/main/java/hudson/Util.java | 8 +-- .../security/HudsonPrivateSecurityRealm.java | 65 ++++++++++++++----- 2 files changed, 53 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 759eb08f601a..ac63497d5fd0 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1928,9 +1928,7 @@ private static PathRemover newPathRemover(@NonNull PathRemover.PathChecker pathC return PathRemover.newFilteredRobustRemover(pathChecker, DELETION_RETRIES, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); } - private static final String GET_FIPS_MODE = ".FIPS_MODE"; - - public static boolean isFipsMode() { - return SystemProperties.getBoolean(Util.class.getName() + GET_FIPS_MODE); - } + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL") + @Restricted(NoExternalUse.class) + public static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); } diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index fa57a7c777c1..be3bcbb8183b 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -894,7 +894,7 @@ public Category getCategory() { /** * {@link PasswordHashEncoder} that uses jBCrypt. */ - private static class JBCryptEncoder implements PasswordHashEncoder { + public static class JBCryptEncoder implements PasswordHashEncoder { // in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017 // and for 18, it's "only" 20s @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") @@ -933,7 +933,7 @@ public boolean isHashValid(String hash) { } } - private static class PBKDF2PasswordEncoder implements PasswordHashEncoder { + public static class PBKDF2PasswordEncoder implements PasswordHashEncoder { public static final int HASH_LENGTH = 8; public static final int ZERO = 0; @@ -941,7 +941,7 @@ private static class PBKDF2PasswordEncoder implements PasswordHashEncoder { public static final int RADIX_LENGTH = 16; public static final String STRING_SEPARATION = ":"; - public static final int ITERATION_COUNT = 1000; + public static final int KEY_LENGTH = 512; public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; @@ -955,12 +955,16 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { return validatePassword(rawPassword.toString(), encodedPassword); } + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") + @Restricted(NoExternalUse.class) + private static int ITERATION_COUNT = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".ITERATION_COUNT", 10); + private static String generatePasswordHashWithPBKDF2(CharSequence password) { - int iterations = ITERATION_COUNT; + int iterations = (int) Math.pow(2, ITERATION_COUNT); byte[] salt = generateSalt(); PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, iterations, KEY_LENGTH); byte[] hash = generateSecretKey(spec); - return iterations + STRING_SEPARATION + toHex(salt) + STRING_SEPARATION + toHex(hash); + return "$PBKDF2$HMACSHA512:" + iterations + STRING_SEPARATION + toHex(salt) + "$" + toHex(hash); } private static byte[] generateSecretKey(PBEKeySpec spec) { @@ -998,17 +1002,33 @@ private static byte[] generateSalt() { return salt; } + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") + @Restricted(NoExternalUse.class) + private static int MAXIMUM_PBKDF2_LOG_ROUND = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".maximumPBKDF2LogRound", (int) Math.pow(2, 10)); + + private static final Pattern PBKDF2_PATTERN = Pattern.compile("^\\$PBKDF2\\$HMACSHA512\\:([0-9]{4})\\:[a-zA-Z0-9=]+\\$[a-zA-Z0-9=]+"); + @Override public boolean isHashValid(String hash) { - return true; + Matcher matcher = PBKDF2_PATTERN.matcher(hash); + if (matcher.matches()) { + String logNumOfRound = matcher.group(1); + // no number format exception due to the expression + int logNumOfRoundInt = Integer.parseInt(logNumOfRound); + if (logNumOfRoundInt > 0 && logNumOfRoundInt <= MAXIMUM_PBKDF2_LOG_ROUND) { + return true; + } + } + return false; } + private static boolean validatePassword(String originalPassword, String storedPassword) { - String[] parts = storedPassword.split(STRING_SEPARATION); - int iterations = Integer.parseInt(parts[0]); + String[] parts = storedPassword.split("[:$]"); + int iterations = Integer.parseInt(parts[3]); - byte[] salt = fromHex(parts[1]); - byte[] hash = fromHex(parts[2]); + byte[] salt = fromHex(parts[4]); + byte[] hash = fromHex(parts[5]); PBEKeySpec spec = new PBEKeySpec(originalPassword.toCharArray(), salt, iterations, hash.length * HASH_LENGTH); @@ -1034,7 +1054,13 @@ private static byte[] fromHex(String storedPassword) { } - /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.isFipsMode() ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); + + + /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = getPasswordHashEncoder(); + + public static PasswordHashEncoder getPasswordHashEncoder() { + return Util.FIPS_MODE ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); + } public static final String PBKDF2 = "#pbkdf2:"; @@ -1042,7 +1068,13 @@ private static byte[] fromHex(String storedPassword) { /** * Magic header used to detect if a password is hashed. */ - private static final String PASSWORD_HASH_HEADER = Util.isFipsMode() ? PBKDF2 : JBCRYPT; + + // private static String PASSWORD_HASH_HEADER = getPasswordHeader(); + + + public static String getPasswordHeader() { + return Util.FIPS_MODE ? PBKDF2 : JBCRYPT; + } /* package */ @@ -1062,13 +1094,15 @@ private static byte[] fromHex(String storedPassword) { @Override public String encode(CharSequence rawPassword) { - return PASSWORD_HASH_HEADER + PASSWORD_HASH_ENCODER.encode(rawPassword); + //LOGGER.info("FIPS MODE ENABLED :"); + // LOGGER.info("Implementation :" + PASSWORD_HASH_ENCODER); + return getPasswordHeader() + PASSWORD_HASH_ENCODER.encode(rawPassword); } @Override public boolean matches(CharSequence rawPassword, String encPass) { if (isPasswordHashed(encPass)) { - return PASSWORD_HASH_ENCODER.matches(rawPassword, encPass.substring(PASSWORD_HASH_HEADER.length())); + return PASSWORD_HASH_ENCODER.matches(rawPassword, encPass.substring(getPasswordHeader().length())); } else { return false; } @@ -1082,11 +1116,12 @@ public boolean isPasswordHashed(String password) { if (password == null) { return false; } - return password.startsWith(PASSWORD_HASH_HEADER) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(PASSWORD_HASH_HEADER.length())); + return password.startsWith(getPasswordHeader()) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(getPasswordHeader().length())); } } + public static final MultiPasswordEncoder PASSWORD_ENCODER = new MultiPasswordEncoder(); /** From 195db22575ddf8a4593ac3fd5e4e18801681d43c Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Mon, 14 Aug 2023 12:29:13 +0530 Subject: [PATCH 05/39] isHashValid Changes --- .../security/HudsonPrivateSecurityRealm.java | 47 ++++------- .../HudsonPrivateSecurityRealmTest.java | 77 +++++++++++++------ 2 files changed, 67 insertions(+), 57 deletions(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index be3bcbb8183b..9afe41f1900c 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -905,6 +905,7 @@ public static class JBCryptEncoder implements PasswordHashEncoder { @Override public String encode(CharSequence rawPassword) { + LOGGER.info("BCRYPT Implementation"); return BCrypt.hashpw(rawPassword.toString(), BCrypt.gensalt()); } @@ -938,15 +939,20 @@ public static class PBKDF2PasswordEncoder implements PasswordHashEncoder { public static final int HASH_LENGTH = 8; public static final int ZERO = 0; public static final int HEX_TWO = 2; - public static final int RADIX_LENGTH = 16; public static final String STRING_SEPARATION = ":"; - public static final int KEY_LENGTH = 512; public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") + @Restricted(NoExternalUse.class) + private static int MAXIMUM_PBKDF2_LOG_ROUND = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".maximumPBKDF2LogRound", (int) Math.pow(2, 10)); + + private static final Pattern PBKDF2_PATTERN = Pattern.compile("^\\$PBKDF2\\$HMACSHA512\\:([0-9]{4})\\:[a-zA-Z0-9=]+\\$[a-zA-Z0-9=]+"); + @Override public String encode(CharSequence rawPassword) { + LOGGER.info("PBKDF2 Implementation"); return generatePasswordHashWithPBKDF2(rawPassword); } @@ -955,16 +961,11 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { return validatePassword(rawPassword.toString(), encodedPassword); } - @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") - @Restricted(NoExternalUse.class) - private static int ITERATION_COUNT = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".ITERATION_COUNT", 10); - private static String generatePasswordHashWithPBKDF2(CharSequence password) { - int iterations = (int) Math.pow(2, ITERATION_COUNT); byte[] salt = generateSalt(); - PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, iterations, KEY_LENGTH); + PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, 1000, KEY_LENGTH); byte[] hash = generateSecretKey(spec); - return "$PBKDF2$HMACSHA512:" + iterations + STRING_SEPARATION + toHex(salt) + "$" + toHex(hash); + return PBKDF2 + "$HMACSHA512:" + 1000 + STRING_SEPARATION + toHex(salt) + "$" + toHex(hash); } private static byte[] generateSecretKey(PBEKeySpec spec) { @@ -1002,12 +1003,6 @@ private static byte[] generateSalt() { return salt; } - @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") - @Restricted(NoExternalUse.class) - private static int MAXIMUM_PBKDF2_LOG_ROUND = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".maximumPBKDF2LogRound", (int) Math.pow(2, 10)); - - private static final Pattern PBKDF2_PATTERN = Pattern.compile("^\\$PBKDF2\\$HMACSHA512\\:([0-9]{4})\\:[a-zA-Z0-9=]+\\$[a-zA-Z0-9=]+"); - @Override public boolean isHashValid(String hash) { Matcher matcher = PBKDF2_PATTERN.matcher(hash); @@ -1022,7 +1017,6 @@ public boolean isHashValid(String hash) { return false; } - private static boolean validatePassword(String originalPassword, String storedPassword) { String[] parts = storedPassword.split("[:$]"); int iterations = Integer.parseInt(parts[3]); @@ -1054,29 +1048,19 @@ private static byte[] fromHex(String storedPassword) { } + /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.FIPS_MODE ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); - /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = getPasswordHashEncoder(); - - public static PasswordHashEncoder getPasswordHashEncoder() { - return Util.FIPS_MODE ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); - } - - - public static final String PBKDF2 = "#pbkdf2:"; + public static final String PBKDF2 = "$PBKDF2"; public static final String JBCRYPT = "#jbcrypt:"; /** * Magic header used to detect if a password is hashed. */ - // private static String PASSWORD_HASH_HEADER = getPasswordHeader(); - - public static String getPasswordHeader() { return Util.FIPS_MODE ? PBKDF2 : JBCRYPT; } - /* package */ // TODO check if DelegatingPasswordEncoder can be used /** @@ -1087,15 +1071,14 @@ public static String getPasswordHeader() { /* CLASSIC encoder outputs "salt:hash" where salt is [a-z]+, so we use unique prefix '#jbcyrpt" - to designate JBCRYPT-format hash. + to designate JBCRYPT-format hash and $PBKDF2 to designate PBKDF2 format hash. '#' is neither in base64 nor hex, which makes it a good choice. */ @Override public String encode(CharSequence rawPassword) { - //LOGGER.info("FIPS MODE ENABLED :"); - // LOGGER.info("Implementation :" + PASSWORD_HASH_ENCODER); + LOGGER.info("FIPS MODE ENABLED :"+ Util.FIPS_MODE); return getPasswordHeader() + PASSWORD_HASH_ENCODER.encode(rawPassword); } @@ -1118,10 +1101,8 @@ public boolean isPasswordHashed(String password) { } return password.startsWith(getPasswordHeader()) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(getPasswordHeader().length())); } - } - public static final MultiPasswordEncoder PASSWORD_ENCODER = new MultiPasswordEncoder(); /** diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index d59aac55608b..e7f0f52e0954 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -26,6 +26,7 @@ import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER; import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_HASH_ENCODER; +import static hudson.security.HudsonPrivateSecurityRealm.getPasswordHeader; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -76,7 +77,7 @@ import org.jvnet.hudson.test.JenkinsRule.WebClient; import org.jvnet.hudson.test.TestExtension; import org.mindrot.jbcrypt.BCrypt; -import org.mockito.Mock; + @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) public class HudsonPrivateSecurityRealmTest { @@ -86,15 +87,11 @@ public class HudsonPrivateSecurityRealmTest { private SpySecurityListenerImpl spySecurityListener; - @Mock - private Util util; - @Before public void linkExtension() { spySecurityListener = ExtensionList.lookup(SecurityListener.class).get(SpySecurityListenerImpl.class); } - @Issue("SECURITY-243") @Test public void fullNameCollisionPassword() throws Exception { @@ -271,6 +268,10 @@ public void selfRegistrationTriggerLoggedIn() throws Exception { j.jenkins.setSecurityRealm(securityRealm); j.jenkins.setCrumbIssuer(null); + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, null); + assertTrue(spySecurityListener.loggedInUsernames.isEmpty()); createFirstAccount("admin"); @@ -326,7 +327,9 @@ public void userCreationFromRealm() throws Exception { public void userCreationWithHashedPasswords() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); j.jenkins.setSecurityRealm(securityRealm); - + Field field1 = Util.class.getDeclaredField("FIPS_MODE"); + field1.setAccessible(false); + field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); spySecurityListener.createdUsers.clear(); assertTrue(spySecurityListener.createdUsers.isEmpty()); @@ -428,6 +431,10 @@ public void controlCharacterAreNoMoreValid() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); j.jenkins.setSecurityRealm(securityRealm); + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, null); + String password = "testPwd"; String email = "test@test.com"; int i = 0; @@ -500,11 +507,47 @@ public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { } } + @Test + public void passwordHashWithPBKDF2() throws IllegalAccessException, NoSuchFieldException { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + Field field1 = Util.class.getDeclaredField("FIPS_MODE"); + field1.setAccessible(true); + field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); + + mockStatic(HudsonPrivateSecurityRealm.class); + when(getPasswordHeader()).thenReturn("$PBKDF2"); + assertNotNull(pbkdf2PasswordEncoder.encode("password")); + assertTrue(PASSWORD_ENCODER.isPasswordHashed("$PBKDF2" + PASSWORD_HASH_ENCODER.encode("password"))); + } + + @Test + public void passwordHashMatchesPBKDF2() throws IllegalAccessException, NoSuchFieldException { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + Field field = Util.class.getDeclaredField("FIPS_MODE"); + field.setAccessible(true); + field.setBoolean(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); + + mockStatic(HudsonPrivateSecurityRealm.class); + when(getPasswordHeader()).thenReturn("$PBKDF2"); + + assertTrue(pbkdf2PasswordEncoder.matches("3a6f9ee5a3af41ef844cb291c63b40f4", + "$PBKDF2$HMACSHA512:1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215ff7bf64b2af5792581039faa62b52")); + } + + @Test + public void passwordHashNotMatches() { + assertFalse(PASSWORD_ENCODER.matches(null, "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b")); + } + @Test public void createAccountSupportsHashedPasswords() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); j.jenkins.setSecurityRealm(securityRealm); + Field field1 = Util.class.getDeclaredField("FIPS_MODE"); + field1.setAccessible(false); + field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); + securityRealm.createAccountWithHashedPassword("user_hashed", "#jbcrypt:" + BCrypt.hashpw("password", BCrypt.gensalt())); WebClient wc = j.createWebClient(); @@ -561,24 +604,6 @@ public void ensureHashingVersion_2y_isNotSupported() { assertThrows(IllegalArgumentException.class, () -> BCrypt.checkpw("a", "$2y$08$cfcvVd2aQ8CMvoMpP2EBfeodLEkkFJ9umNEfPD18.hUF62qqlC/V.")); } - @Test - public void hashedPasswordTestPBKDF2() { - mockStatic(Util.class); - when(Util.isFipsMode()).thenReturn(true); - PASSWORD_ENCODER.isPasswordHashed("#pbkdf2:" + PASSWORD_HASH_ENCODER.encode("password")); - assertNotNull(PASSWORD_HASH_ENCODER.encode("password")); - } - - @Test - public void hashedPasswordTestMatchesPBKDF2() { - PASSWORD_ENCODER.matches("password", "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b"); - } - - @Test - public void hashedPasswordTestMatchesPBKDF2False() { - assertFalse(PASSWORD_ENCODER.matches(null, "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b")); - } - private void checkUserCanBeCreatedWith(HudsonPrivateSecurityRealm securityRealm, String id, String password, String fullName, String email) throws Exception { JenkinsRule.WebClient wc = j.createWebClient(); SignupPage signup = new SignupPage(wc.goTo("signup")); @@ -617,10 +642,14 @@ private void checkUserCannotBeCreatedWith_custom(HudsonPrivateSecurityRealm secu @Test @Issue("SECURITY-1158") public void singupNoLongerVulnerableToSessionFixation() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); j.jenkins.setSecurityRealm(securityRealm); JenkinsRule.WebClient wc = j.createWebClient(); + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, null); // to trigger the creation of a session wc.goTo(""); Cookie sessionBefore = wc.getCookieManager().getCookie("JSESSIONID"); From c0cc0ae6dd62f8e2acb6b66f02855ac36b643fa8 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Thu, 17 Aug 2023 12:12:26 +0530 Subject: [PATCH 06/39] commented logger --- .../java/hudson/security/HudsonPrivateSecurityRealm.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 9afe41f1900c..adf7972c4cf2 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -905,7 +905,7 @@ public static class JBCryptEncoder implements PasswordHashEncoder { @Override public String encode(CharSequence rawPassword) { - LOGGER.info("BCRYPT Implementation"); + // LOGGER.info("BCRYPT Implementation"); return BCrypt.hashpw(rawPassword.toString(), BCrypt.gensalt()); } @@ -952,7 +952,7 @@ public static class PBKDF2PasswordEncoder implements PasswordHashEncoder { @Override public String encode(CharSequence rawPassword) { - LOGGER.info("PBKDF2 Implementation"); + //LOGGER.info("PBKDF2 Implementation"); return generatePasswordHashWithPBKDF2(rawPassword); } @@ -1078,7 +1078,7 @@ public static String getPasswordHeader() { @Override public String encode(CharSequence rawPassword) { - LOGGER.info("FIPS MODE ENABLED :"+ Util.FIPS_MODE); + // LOGGER.info("FIPS MODE ENABLED :" + Util.FIPS_MODE); return getPasswordHeader() + PASSWORD_HASH_ENCODER.encode(rawPassword); } From 2cb8dbbd0869614e90f1b39f0ef7c3b53a3c548c Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Thu, 17 Aug 2023 13:53:27 +0530 Subject: [PATCH 07/39] commented logger --- .../HudsonPrivateSecurityRealmTest.java | 38 ++++++++++++++++++- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index e7f0f52e0954..b0e73117453c 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -39,8 +39,7 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mockStatic; -import static org.mockito.Mockito.when; +import static org.mockito.Mockito.*; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; @@ -50,6 +49,8 @@ import java.lang.reflect.Field; import java.net.URL; import java.nio.charset.StandardCharsets; +import java.security.NoSuchAlgorithmException; +import java.security.spec.InvalidKeySpecException; import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; @@ -60,6 +61,7 @@ import jenkins.security.apitoken.ApiTokenPropertyConfiguration; import jenkins.security.seed.UserSeedProperty; import org.apache.commons.lang.StringUtils; +import org.apache.commons.lang3.builder.ToStringExclude; import org.htmlunit.HttpMethod; import org.htmlunit.WebRequest; import org.htmlunit.html.HtmlForm; @@ -78,6 +80,9 @@ import org.jvnet.hudson.test.TestExtension; import org.mindrot.jbcrypt.BCrypt; +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; + @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) public class HudsonPrivateSecurityRealmTest { @@ -520,6 +525,23 @@ public void passwordHashWithPBKDF2() throws IllegalAccessException, NoSuchFieldE assertTrue(PASSWORD_ENCODER.isPasswordHashed("$PBKDF2" + PASSWORD_HASH_ENCODER.encode("password"))); } + @Test + public void passwordHashWithPBKDF2InvalidKey() throws IllegalAccessException, NoSuchFieldException, NoSuchAlgorithmException, InvalidKeySpecException { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + Field field1 = Util.class.getDeclaredField("FIPS_MODE"); + field1.setAccessible(true); + field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); + + mockStatic(HudsonPrivateSecurityRealm.class); + PBEKeySpec spec = mock(PBEKeySpec.class); + SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); + assertThrows(NoSuchAlgorithmException.class,()->when(SecretKeyFactory.getInstance("")).thenThrow(NoSuchAlgorithmException.class)); + when(secretKeyFactory.generateSecret(any())).thenThrow(InvalidKeySpecException.class); + when(spec.getPassword()).thenReturn("".toCharArray()); + + when(getPasswordHeader()).thenReturn("$PBKDF2"); + assertNotNull(pbkdf2PasswordEncoder.encode("password")); + } @Test public void passwordHashMatchesPBKDF2() throws IllegalAccessException, NoSuchFieldException { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); @@ -534,6 +556,18 @@ public void passwordHashMatchesPBKDF2() throws IllegalAccessException, NoSuchFie "$PBKDF2$HMACSHA512:1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215ff7bf64b2af5792581039faa62b52")); } + @Test + public void hashPatternMatches(){ + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + assertTrue(pbkdf2PasswordEncoder.isHashValid("$PBKDF2$HMACSHA512:1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); + } + + @Test + public void hashPatternNotMatches(){ + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + assertFalse(pbkdf2PasswordEncoder.isHashValid("1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); + } + @Test public void passwordHashNotMatches() { assertFalse(PASSWORD_ENCODER.matches(null, "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b")); From 43d758bf879ebad9dbb73ef4c6dfbcd4e5bbe7f1 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Thu, 17 Aug 2023 19:46:42 +0530 Subject: [PATCH 08/39] modified test cases --- core/src/main/java/hudson/Util.java | 5 ++ .../security/HudsonPrivateSecurityRealm.java | 12 ++- .../HudsonPrivateSecurityRealmTest.java | 80 ++++++++++--------- 3 files changed, 51 insertions(+), 46 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index ac63497d5fd0..0b09002abb3d 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1931,4 +1931,9 @@ private static PathRemover newPathRemover(@NonNull PathRemover.PathChecker pathC @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL") @Restricted(NoExternalUse.class) public static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); + + public static boolean isFipsMode() { + LOGGER.info("FIPS mode : " + FIPS_MODE); + return FIPS_MODE; + } } diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index adf7972c4cf2..4566c39dc259 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -905,7 +905,6 @@ public static class JBCryptEncoder implements PasswordHashEncoder { @Override public String encode(CharSequence rawPassword) { - // LOGGER.info("BCRYPT Implementation"); return BCrypt.hashpw(rawPassword.toString(), BCrypt.gensalt()); } @@ -946,13 +945,12 @@ public static class PBKDF2PasswordEncoder implements PasswordHashEncoder { @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") @Restricted(NoExternalUse.class) - private static int MAXIMUM_PBKDF2_LOG_ROUND = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".maximumPBKDF2LogRound", (int) Math.pow(2, 10)); + private static int MAXIMUM_PBKDF2_LOG_ROUND = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".maximumPBKDF2LogRound", 1000); private static final Pattern PBKDF2_PATTERN = Pattern.compile("^\\$PBKDF2\\$HMACSHA512\\:([0-9]{4})\\:[a-zA-Z0-9=]+\\$[a-zA-Z0-9=]+"); @Override public String encode(CharSequence rawPassword) { - //LOGGER.info("PBKDF2 Implementation"); return generatePasswordHashWithPBKDF2(rawPassword); } @@ -1048,17 +1046,18 @@ private static byte[] fromHex(String storedPassword) { } - /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.FIPS_MODE ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); + /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.isFipsMode() ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); public static final String PBKDF2 = "$PBKDF2"; public static final String JBCRYPT = "#jbcrypt:"; + /** * Magic header used to detect if a password is hashed. */ public static String getPasswordHeader() { - return Util.FIPS_MODE ? PBKDF2 : JBCRYPT; + return Util.isFipsMode() ? PBKDF2 : JBCRYPT; } @@ -1078,7 +1077,6 @@ public static String getPasswordHeader() { @Override public String encode(CharSequence rawPassword) { - // LOGGER.info("FIPS MODE ENABLED :" + Util.FIPS_MODE); return getPasswordHeader() + PASSWORD_HASH_ENCODER.encode(rawPassword); } @@ -1165,6 +1163,6 @@ public void destroy() { } }; - private static final Logger LOGGER = Logger.getLogger(HudsonPrivateSecurityRealm.class.getName()); + static final Logger LOGGER = Logger.getLogger(HudsonPrivateSecurityRealm.class.getName()); } diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index b0e73117453c..a32c557e0ea3 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -39,7 +39,10 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; @@ -56,12 +59,13 @@ import java.util.Base64; import java.util.Collections; import java.util.List; +import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; import jenkins.security.ApiTokenProperty; import jenkins.security.SecurityListener; import jenkins.security.apitoken.ApiTokenPropertyConfiguration; import jenkins.security.seed.UserSeedProperty; import org.apache.commons.lang.StringUtils; -import org.apache.commons.lang3.builder.ToStringExclude; import org.htmlunit.HttpMethod; import org.htmlunit.WebRequest; import org.htmlunit.html.HtmlForm; @@ -80,8 +84,6 @@ import org.jvnet.hudson.test.TestExtension; import org.mindrot.jbcrypt.BCrypt; -import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.PBEKeySpec; @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) @@ -91,12 +93,26 @@ public class HudsonPrivateSecurityRealmTest { public JenkinsRule j = new JenkinsRule(); private SpySecurityListenerImpl spySecurityListener; + private Field field; + @Before public void linkExtension() { spySecurityListener = ExtensionList.lookup(SecurityListener.class).get(SpySecurityListenerImpl.class); } + @Before + public void setup() throws Exception { + + Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); + field.setAccessible(true); + field.set(null, null); + + Field field1 = Util.class.getDeclaredField("FIPS_MODE"); + field1.setAccessible(true); + field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); + } + @Issue("SECURITY-243") @Test public void fullNameCollisionPassword() throws Exception { @@ -273,10 +289,6 @@ public void selfRegistrationTriggerLoggedIn() throws Exception { j.jenkins.setSecurityRealm(securityRealm); j.jenkins.setCrumbIssuer(null); - Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); - field.setAccessible(true); - field.set(null, null); - assertTrue(spySecurityListener.loggedInUsernames.isEmpty()); createFirstAccount("admin"); @@ -332,9 +344,9 @@ public void userCreationFromRealm() throws Exception { public void userCreationWithHashedPasswords() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); j.jenkins.setSecurityRealm(securityRealm); - Field field1 = Util.class.getDeclaredField("FIPS_MODE"); - field1.setAccessible(false); - field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); + Field field = Util.class.getDeclaredField("FIPS_MODE"); + field.setAccessible(false); + field.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); spySecurityListener.createdUsers.clear(); assertTrue(spySecurityListener.createdUsers.isEmpty()); @@ -436,10 +448,6 @@ public void controlCharacterAreNoMoreValid() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); j.jenkins.setSecurityRealm(securityRealm); - Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); - field.setAccessible(true); - field.set(null, null); - String password = "testPwd"; String email = "test@test.com"; int i = 0; @@ -513,11 +521,8 @@ public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { } @Test - public void passwordHashWithPBKDF2() throws IllegalAccessException, NoSuchFieldException { + public void passwordHashWithPBKDF2() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - Field field1 = Util.class.getDeclaredField("FIPS_MODE"); - field1.setAccessible(true); - field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); mockStatic(HudsonPrivateSecurityRealm.class); when(getPasswordHeader()).thenReturn("$PBKDF2"); @@ -526,28 +531,23 @@ public void passwordHashWithPBKDF2() throws IllegalAccessException, NoSuchFieldE } @Test - public void passwordHashWithPBKDF2InvalidKey() throws IllegalAccessException, NoSuchFieldException, NoSuchAlgorithmException, InvalidKeySpecException { + public void passwordHashWithPBKDF2InvalidKey() throws InvalidKeySpecException { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - Field field1 = Util.class.getDeclaredField("FIPS_MODE"); - field1.setAccessible(true); - field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); mockStatic(HudsonPrivateSecurityRealm.class); PBEKeySpec spec = mock(PBEKeySpec.class); SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); - assertThrows(NoSuchAlgorithmException.class,()->when(SecretKeyFactory.getInstance("")).thenThrow(NoSuchAlgorithmException.class)); + assertThrows(NoSuchAlgorithmException.class, () -> when(SecretKeyFactory.getInstance("")).thenThrow(NoSuchAlgorithmException.class)); when(secretKeyFactory.generateSecret(any())).thenThrow(InvalidKeySpecException.class); when(spec.getPassword()).thenReturn("".toCharArray()); when(getPasswordHeader()).thenReturn("$PBKDF2"); assertNotNull(pbkdf2PasswordEncoder.encode("password")); } + @Test - public void passwordHashMatchesPBKDF2() throws IllegalAccessException, NoSuchFieldException { + public void passwordHashMatchesPBKDF2() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - Field field = Util.class.getDeclaredField("FIPS_MODE"); - field.setAccessible(true); - field.setBoolean(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); mockStatic(HudsonPrivateSecurityRealm.class); when(getPasswordHeader()).thenReturn("$PBKDF2"); @@ -557,15 +557,15 @@ public void passwordHashMatchesPBKDF2() throws IllegalAccessException, NoSuchFie } @Test - public void hashPatternMatches(){ + public void hashPatternMatches() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - assertTrue(pbkdf2PasswordEncoder.isHashValid("$PBKDF2$HMACSHA512:1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); + assertTrue(pbkdf2PasswordEncoder.isHashValid("$PBKDF2$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); } @Test - public void hashPatternNotMatches(){ + public void hashPatternNotMatches() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - assertFalse(pbkdf2PasswordEncoder.isHashValid("1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); + assertFalse(pbkdf2PasswordEncoder.isHashValid("1020:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); } @Test @@ -578,9 +578,9 @@ public void createAccountSupportsHashedPasswords() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); j.jenkins.setSecurityRealm(securityRealm); - Field field1 = Util.class.getDeclaredField("FIPS_MODE"); - field1.setAccessible(false); - field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); + Field field = Util.class.getDeclaredField("FIPS_MODE"); + field.setAccessible(false); + field.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); securityRealm.createAccountWithHashedPassword("user_hashed", "#jbcrypt:" + BCrypt.hashpw("password", BCrypt.gensalt())); @@ -600,7 +600,12 @@ public void createAccountWithHashedPasswordRequiresPrefix() { } @Test - public void hashedPasswordTest() { + public void hashedPasswordTest() throws NoSuchFieldException, IllegalAccessException { + + Field field = Util.class.getDeclaredField("FIPS_MODE"); + field.setAccessible(false); + field.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); + assertTrue("password is hashed", PASSWORD_ENCODER.isPasswordHashed("#jbcrypt:" + BCrypt.hashpw("password", BCrypt.gensalt()))); assertFalse("password is not hashed", PASSWORD_ENCODER.isPasswordHashed("password")); assertFalse("only valid hashed passwords allowed", PASSWORD_ENCODER.isPasswordHashed("#jbcrypt:$2a$blah")); @@ -681,9 +686,6 @@ public void singupNoLongerVulnerableToSessionFixation() throws Exception { j.jenkins.setSecurityRealm(securityRealm); JenkinsRule.WebClient wc = j.createWebClient(); - Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); - field.setAccessible(true); - field.set(null, null); // to trigger the creation of a session wc.goTo(""); Cookie sessionBefore = wc.getCookieManager().getCookie("JSESSIONID"); From c0c932fe4ad1a924ca7560a8fa0d783674a03ee8 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Fri, 18 Aug 2023 11:56:54 +0530 Subject: [PATCH 09/39] modified test cases --- .../security/HudsonPrivateSecurityRealm.java | 4 ++-- .../HudsonPrivateSecurityRealmTest.java | 23 ++++++++++--------- 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 4566c39dc259..4e5eb6f77b45 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -894,7 +894,7 @@ public Category getCategory() { /** * {@link PasswordHashEncoder} that uses jBCrypt. */ - public static class JBCryptEncoder implements PasswordHashEncoder { + static class JBCryptEncoder implements PasswordHashEncoder { // in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017 // and for 18, it's "only" 20s @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") @@ -933,7 +933,7 @@ public boolean isHashValid(String hash) { } } - public static class PBKDF2PasswordEncoder implements PasswordHashEncoder { + static class PBKDF2PasswordEncoder implements PasswordHashEncoder { public static final int HASH_LENGTH = 8; public static final int ZERO = 0; diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index a32c557e0ea3..2e3f17286075 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -523,7 +523,6 @@ public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { @Test public void passwordHashWithPBKDF2() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - mockStatic(HudsonPrivateSecurityRealm.class); when(getPasswordHeader()).thenReturn("$PBKDF2"); assertNotNull(pbkdf2PasswordEncoder.encode("password")); @@ -531,28 +530,30 @@ public void passwordHashWithPBKDF2() { } @Test - public void passwordHashWithPBKDF2InvalidKey() throws InvalidKeySpecException { + public void passwordHashWithInvalidAgorithm() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + mockStatic(HudsonPrivateSecurityRealm.class); + SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); + assertThrows(NoSuchAlgorithmException.class, () -> when(SecretKeyFactory.getInstance("")).thenThrow(NoSuchAlgorithmException.class)); + pbkdf2PasswordEncoder.encode("password"); + } + @Test + public void passwordHashWithInvalidKeySpec() throws InvalidKeySpecException { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); mockStatic(HudsonPrivateSecurityRealm.class); - PBEKeySpec spec = mock(PBEKeySpec.class); SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); - assertThrows(NoSuchAlgorithmException.class, () -> when(SecretKeyFactory.getInstance("")).thenThrow(NoSuchAlgorithmException.class)); when(secretKeyFactory.generateSecret(any())).thenThrow(InvalidKeySpecException.class); - when(spec.getPassword()).thenReturn("".toCharArray()); - - when(getPasswordHeader()).thenReturn("$PBKDF2"); - assertNotNull(pbkdf2PasswordEncoder.encode("password")); + pbkdf2PasswordEncoder.encode("password"); } + @Test public void passwordHashMatchesPBKDF2() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - mockStatic(HudsonPrivateSecurityRealm.class); when(getPasswordHeader()).thenReturn("$PBKDF2"); - - assertTrue(pbkdf2PasswordEncoder.matches("3a6f9ee5a3af41ef844cb291c63b40f4", + assertTrue(pbkdf2PasswordEncoder.matches("3a6f9ee5a3af41ef844cb291c63b40f4", "$PBKDF2$HMACSHA512:1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215ff7bf64b2af5792581039faa62b52")); } From 033c8aac4059d6bd201fece967111ff36edc1b8e Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Fri, 18 Aug 2023 12:29:40 +0530 Subject: [PATCH 10/39] modified test cases --- .../java/hudson/security/HudsonPrivateSecurityRealmTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 2e3f17286075..fda9eb29153f 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -60,7 +60,6 @@ import java.util.Collections; import java.util.List; import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.PBEKeySpec; import jenkins.security.ApiTokenProperty; import jenkins.security.SecurityListener; import jenkins.security.apitoken.ApiTokenPropertyConfiguration; From 20beb4876f861eb107a0d73eef326028b16c310e Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Tue, 22 Aug 2023 15:18:29 +0530 Subject: [PATCH 11/39] resolved Util conflicts --- core/src/main/java/hudson/Util.java | 71 +++++++++---------- .../HudsonPrivateSecurityRealmTest.java | 1 + 2 files changed, 36 insertions(+), 36 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index a1172e4ac0b1..714cebdbcd8e 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -161,7 +161,7 @@ public static List filter(@NonNull List base, @NonNull Class type) /** * Pattern for capturing variables. Either $xyz, ${xyz} or ${a.b} but not $a.b, while ignoring "$$" - */ + */ private static final Pattern VARIABLE = Pattern.compile("\\$([A-Za-z0-9_]+|\\{[A-Za-z0-9_.]+\\}|\\$)"); /** @@ -198,10 +198,10 @@ public static String replaceMacro(@CheckForNull String s, @NonNull VariableResol // escape the dollar sign or get the key to resolve String value; if (key.charAt(0) == '$') { - value = "$"; + value = "$"; } else { - if (key.charAt(0) == '{') key = key.substring(1, key.length() - 1); - value = resolver.resolve(key); + if (key.charAt(0) == '{') key = key.substring(1, key.length() - 1); + value = resolver.resolve(key); } if (value == null) @@ -261,8 +261,8 @@ public static String loadFile(@NonNull File logfile, @NonNull Charset charset) t .onMalformedInput(CodingErrorAction.REPLACE) .onUnmappableCharacter(CodingErrorAction.REPLACE); try (InputStream is = Files.newInputStream(Util.fileToPath(logfile)); - Reader isr = new InputStreamReader(is, decoder); - Reader br = new BufferedReader(isr)) { + Reader isr = new InputStreamReader(is, decoder); + Reader br = new BufferedReader(isr)) { return IOUtils.toString(br); } catch (NoSuchFileException e) { return ""; @@ -886,9 +886,9 @@ public static String encode(@NonNull String s) { static { String raw = - "! $ &'()*+,-. 0123456789 = @ABCDEFGHIJKLMNOPQRSTUVWXYZ _ abcdefghijklmnopqrstuvwxyz"; - // "# % / :;< >? [\]^ ` {|}~ - // ^--so these are encoded + "! $ &'()*+,-. 0123456789 = @ABCDEFGHIJKLMNOPQRSTUVWXYZ _ abcdefghijklmnopqrstuvwxyz"; + // "# % / :;< >? [\]^ ` {|}~ + // ^--so these are encoded int i; // Encode control chars and space for (i = 0; i < 33; i++) uriMap[i] = true; @@ -1321,19 +1321,19 @@ private static boolean createSymlinkAtomic(@NonNull Path pathForSymlink, @NonNul Files.move(tempSymlinkPath, pathForSymlink, StandardCopyOption.ATOMIC_MOVE); return true; } catch ( - UnsupportedOperationException | - SecurityException | - IOException ex) { + UnsupportedOperationException | + SecurityException | + IOException ex) { // If we couldn't perform an atomic move or the setup, we fall through to another approach reportAtomicFailure(pathForSymlink, ex); } // If we didn't return after our atomic move, then we want to clean up our symlink tryToDeleteSymlink(symlink); } catch ( - SecurityException | - InvalidPathException | - UnsupportedOperationException | - IOException ex) { + SecurityException | + InvalidPathException | + UnsupportedOperationException | + IOException ex) { // We couldn't perform an atomic move or the setup. reportAtomicFailure(pathForSymlink, ex); } @@ -1353,7 +1353,7 @@ private static boolean createSymlinkAtomic(@NonNull Path pathForSymlink, @NonNul * Where to create a symlink in (relative to {@code baseDir}) */ public static void createSymlink(@NonNull File baseDir, @NonNull String targetPath, - @NonNull String symlinkPath, @NonNull TaskListener listener) throws InterruptedException { + @NonNull String symlinkPath, @NonNull TaskListener listener) throws InterruptedException { File fileForSymlink = new File(baseDir, symlinkPath); try { Path pathForSymlink = fileToPath(fileForSymlink); @@ -1689,7 +1689,7 @@ public static Properties loadProperties(@NonNull String properties) throws IOExc */ @Restricted(NoExternalUse.class) public static void closeAndLogFailures(@CheckForNull Closeable toClose, @NonNull Logger logger, - @NonNull String closeableName, @NonNull String closeableOwner) { + @NonNull String closeableName, @NonNull String closeableOwner) { if (toClose == null) { return; } @@ -1716,7 +1716,7 @@ public static int permissionsToMode(Set permissions) { @Restricted(NoExternalUse.class) public static Set modeToPermissions(int mode) throws IOException { - // Anything larger is a file type, not a permission. + // Anything larger is a file type, not a permission. int PERMISSIONS_MASK = 07777; // setgid/setuid/sticky are not supported. int MAX_SUPPORTED_MODE = 0777; @@ -1927,19 +1927,6 @@ public static long daysElapsedSince(@NonNull Date date) { private static PathRemover newPathRemover(@NonNull PathRemover.PathChecker pathChecker) { return PathRemover.newFilteredRobustRemover(pathChecker, DELETION_RETRIES, GC_AFTER_FAILED_DELETE, WAIT_BETWEEN_DELETION_RETRIES); } - - /** - * Setting this flag to true enables FIPS mode - */ - - @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL") - @Restricted(NoExternalUse.class) - public static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); - - public static boolean isFipsMode() { - LOGGER.info("FIPS mode : " + FIPS_MODE); - return FIPS_MODE; - } /** * Returns SHA-256 Digest of input bytes @@ -1947,9 +1934,9 @@ public static boolean isFipsMode() { @Restricted(NoExternalUse.class) public static byte[] getSHA256DigestOf(@NonNull byte[] input) { try { - MessageDigest messageDigest = MessageDigest.getInstance("SHA-256"); - messageDigest.update(input); - return messageDigest.digest(); + MessageDigest messageDigest = MessageDigest.getInstance("SHA-256"); + messageDigest.update(input); + return messageDigest.digest(); } catch (NoSuchAlgorithmException noSuchAlgorithmException) { throw new IllegalStateException("SHA-256 could not be instantiated, but is required to" + " be implemented by the language specification", noSuchAlgorithmException); @@ -1965,5 +1952,17 @@ public static String getHexOfSHA256DigestOf(byte[] input) throws IOException { byte[] payloadDigest = Util.getSHA256DigestOf(input); return (payloadDigest != null) ? Util.toHexString(payloadDigest) : null; } -} + /** + * Setting this flag to true enables FIPS mode + */ + @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL") + @Restricted(NoExternalUse.class) + public static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); + + public static boolean isFipsMode() { + LOGGER.info("FIPS mode : " + FIPS_MODE); + return FIPS_MODE; + } + +} diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index fda9eb29153f..78fedd0c1107 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -570,6 +570,7 @@ public void hashPatternNotMatches() { @Test public void passwordHashNotMatches() { + assertFalse(PASSWORD_ENCODER.matches(null, "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b")); } From b8defa4aad92a66fd4392e2e370b12012edc7b61 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Tue, 22 Aug 2023 15:49:53 +0530 Subject: [PATCH 12/39] resolved Util conflicts --- core/src/main/java/hudson/Util.java | 44 ++++++++++++++--------------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 714cebdbcd8e..6d524737a6fa 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -161,7 +161,7 @@ public static List filter(@NonNull List base, @NonNull Class type) /** * Pattern for capturing variables. Either $xyz, ${xyz} or ${a.b} but not $a.b, while ignoring "$$" - */ + */ private static final Pattern VARIABLE = Pattern.compile("\\$([A-Za-z0-9_]+|\\{[A-Za-z0-9_.]+\\}|\\$)"); /** @@ -198,10 +198,10 @@ public static String replaceMacro(@CheckForNull String s, @NonNull VariableResol // escape the dollar sign or get the key to resolve String value; if (key.charAt(0) == '$') { - value = "$"; + value = "$"; } else { - if (key.charAt(0) == '{') key = key.substring(1, key.length() - 1); - value = resolver.resolve(key); + if (key.charAt(0) == '{') key = key.substring(1, key.length() - 1); + value = resolver.resolve(key); } if (value == null) @@ -261,8 +261,8 @@ public static String loadFile(@NonNull File logfile, @NonNull Charset charset) t .onMalformedInput(CodingErrorAction.REPLACE) .onUnmappableCharacter(CodingErrorAction.REPLACE); try (InputStream is = Files.newInputStream(Util.fileToPath(logfile)); - Reader isr = new InputStreamReader(is, decoder); - Reader br = new BufferedReader(isr)) { + Reader isr = new InputStreamReader(is, decoder); + Reader br = new BufferedReader(isr)) { return IOUtils.toString(br); } catch (NoSuchFileException e) { return ""; @@ -886,9 +886,9 @@ public static String encode(@NonNull String s) { static { String raw = - "! $ &'()*+,-. 0123456789 = @ABCDEFGHIJKLMNOPQRSTUVWXYZ _ abcdefghijklmnopqrstuvwxyz"; - // "# % / :;< >? [\]^ ` {|}~ - // ^--so these are encoded + "! $ &'()*+,-. 0123456789 = @ABCDEFGHIJKLMNOPQRSTUVWXYZ _ abcdefghijklmnopqrstuvwxyz"; + // "# % / :;< >? [\]^ ` {|}~ + // ^--so these are encoded int i; // Encode control chars and space for (i = 0; i < 33; i++) uriMap[i] = true; @@ -1321,19 +1321,19 @@ private static boolean createSymlinkAtomic(@NonNull Path pathForSymlink, @NonNul Files.move(tempSymlinkPath, pathForSymlink, StandardCopyOption.ATOMIC_MOVE); return true; } catch ( - UnsupportedOperationException | - SecurityException | - IOException ex) { + UnsupportedOperationException | + SecurityException | + IOException ex) { // If we couldn't perform an atomic move or the setup, we fall through to another approach reportAtomicFailure(pathForSymlink, ex); } // If we didn't return after our atomic move, then we want to clean up our symlink tryToDeleteSymlink(symlink); } catch ( - SecurityException | - InvalidPathException | - UnsupportedOperationException | - IOException ex) { + SecurityException | + InvalidPathException | + UnsupportedOperationException | + IOException ex) { // We couldn't perform an atomic move or the setup. reportAtomicFailure(pathForSymlink, ex); } @@ -1353,7 +1353,7 @@ private static boolean createSymlinkAtomic(@NonNull Path pathForSymlink, @NonNul * Where to create a symlink in (relative to {@code baseDir}) */ public static void createSymlink(@NonNull File baseDir, @NonNull String targetPath, - @NonNull String symlinkPath, @NonNull TaskListener listener) throws InterruptedException { + @NonNull String symlinkPath, @NonNull TaskListener listener) throws InterruptedException { File fileForSymlink = new File(baseDir, symlinkPath); try { Path pathForSymlink = fileToPath(fileForSymlink); @@ -1689,7 +1689,7 @@ public static Properties loadProperties(@NonNull String properties) throws IOExc */ @Restricted(NoExternalUse.class) public static void closeAndLogFailures(@CheckForNull Closeable toClose, @NonNull Logger logger, - @NonNull String closeableName, @NonNull String closeableOwner) { + @NonNull String closeableName, @NonNull String closeableOwner) { if (toClose == null) { return; } @@ -1716,7 +1716,7 @@ public static int permissionsToMode(Set permissions) { @Restricted(NoExternalUse.class) public static Set modeToPermissions(int mode) throws IOException { - // Anything larger is a file type, not a permission. + // Anything larger is a file type, not a permission. int PERMISSIONS_MASK = 07777; // setgid/setuid/sticky are not supported. int MAX_SUPPORTED_MODE = 0777; @@ -1934,9 +1934,9 @@ private static PathRemover newPathRemover(@NonNull PathRemover.PathChecker pathC @Restricted(NoExternalUse.class) public static byte[] getSHA256DigestOf(@NonNull byte[] input) { try { - MessageDigest messageDigest = MessageDigest.getInstance("SHA-256"); - messageDigest.update(input); - return messageDigest.digest(); + MessageDigest messageDigest = MessageDigest.getInstance("SHA-256"); + messageDigest.update(input); + return messageDigest.digest(); } catch (NoSuchAlgorithmException noSuchAlgorithmException) { throw new IllegalStateException("SHA-256 could not be instantiated, but is required to" + " be implemented by the language specification", noSuchAlgorithmException); From 1164b8b599ba60deb809d8d0f17daf1b0929047c Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Tue, 22 Aug 2023 17:40:06 +0530 Subject: [PATCH 13/39] resolved Util conflicts --- .../java/hudson/security/HudsonPrivateSecurityRealmTest.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 78fedd0c1107..7d9f1601b4c1 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -546,7 +546,6 @@ public void passwordHashWithInvalidKeySpec() throws InvalidKeySpecException { pbkdf2PasswordEncoder.encode("password"); } - @Test public void passwordHashMatchesPBKDF2() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); @@ -570,7 +569,6 @@ public void hashPatternNotMatches() { @Test public void passwordHashNotMatches() { - assertFalse(PASSWORD_ENCODER.matches(null, "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b")); } From 4b0eabec25c3d1dfcdd3e0f6b1b83282c6c891f5 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Tue, 22 Aug 2023 17:41:22 +0530 Subject: [PATCH 14/39] modified test cases --- .../java/hudson/security/HudsonPrivateSecurityRealmTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 7d9f1601b4c1..4e4976ef71fd 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -521,6 +521,7 @@ public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { @Test public void passwordHashWithPBKDF2() { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); mockStatic(HudsonPrivateSecurityRealm.class); when(getPasswordHeader()).thenReturn("$PBKDF2"); From 5bb32278c09f1bad793894cc0c0931ab048a2424 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Tue, 29 Aug 2023 07:29:20 +0530 Subject: [PATCH 15/39] fixed review comments --- .../hudson/security/HudsonPrivateSecurityRealm.java | 12 +++++------- .../security/HudsonPrivateSecurityRealmTest.java | 2 ++ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 4e5eb6f77b45..8005d87b85ae 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -963,7 +963,7 @@ private static String generatePasswordHashWithPBKDF2(CharSequence password) { byte[] salt = generateSalt(); PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, 1000, KEY_LENGTH); byte[] hash = generateSecretKey(spec); - return PBKDF2 + "$HMACSHA512:" + 1000 + STRING_SEPARATION + toHex(salt) + "$" + toHex(hash); + return PBKDF2 + "$HMACSHA512:" + 1000 + STRING_SEPARATION + Util.toHexString(salt) + "$" + Util.toHexString(hash); } private static byte[] generateSecretKey(PBEKeySpec spec) { @@ -1019,8 +1019,8 @@ private static boolean validatePassword(String originalPassword, String storedPa String[] parts = storedPassword.split("[:$]"); int iterations = Integer.parseInt(parts[3]); - byte[] salt = fromHex(parts[4]); - byte[] hash = fromHex(parts[5]); + byte[] salt = Util.fromHexString(parts[4]); + byte[] hash = Util.fromHexString(parts[5]); PBEKeySpec spec = new PBEKeySpec(originalPassword.toCharArray(), salt, iterations, hash.length * HASH_LENGTH); @@ -1090,8 +1090,7 @@ public boolean matches(CharSequence rawPassword, String encPass) { } /** - * Returns true if the supplied password starts with a prefix indicating i - * t is already hashed. + * Returns true if the supplied password starts with a prefix indicating it is already hashed. */ public boolean isPasswordHashed(String password) { if (password == null) { @@ -1163,6 +1162,5 @@ public void destroy() { } }; - static final Logger LOGGER = Logger.getLogger(HudsonPrivateSecurityRealm.class.getName()); - + private static final Logger LOGGER = Logger.getLogger(HudsonPrivateSecurityRealm.class.getName()); } diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 4e4976ef71fd..fdbb2cd79db8 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -80,6 +80,7 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsRule.WebClient; +import org.jvnet.hudson.test.WithoutJenkins; import org.jvnet.hudson.test.TestExtension; import org.mindrot.jbcrypt.BCrypt; @@ -520,6 +521,7 @@ public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { } @Test + @WithoutJenkins public void passwordHashWithPBKDF2() { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); From 3b4bafb2ba24081d711f47b06a345a7e42b58b8b Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Tue, 29 Aug 2023 12:52:16 +0530 Subject: [PATCH 16/39] fixed review comments --- .../java/hudson/security/HudsonPrivateSecurityRealmTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index fdbb2cd79db8..aef6dc378fc6 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -80,8 +80,8 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsRule.WebClient; -import org.jvnet.hudson.test.WithoutJenkins; import org.jvnet.hudson.test.TestExtension; +import org.jvnet.hudson.test.WithoutJenkins; import org.mindrot.jbcrypt.BCrypt; From 4f329b2de6031e6dcd73d309513943bedb7dd9c6 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Tue, 29 Aug 2023 15:48:50 +0530 Subject: [PATCH 17/39] fixed review comments --- .../java/hudson/security/HudsonPrivateSecurityRealmTest.java | 1 + 1 file changed, 1 insertion(+) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index aef6dc378fc6..7ce01b059ace 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -551,6 +551,7 @@ public void passwordHashWithInvalidKeySpec() throws InvalidKeySpecException { @Test public void passwordHashMatchesPBKDF2() { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); mockStatic(HudsonPrivateSecurityRealm.class); when(getPasswordHeader()).thenReturn("$PBKDF2"); From f3175cb352eadd8bbca3858eeac4df3a839db755 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Wed, 6 Sep 2023 15:29:37 +0530 Subject: [PATCH 18/39] fixed review comments --- core/src/main/java/hudson/Util.java | 2 +- .../security/HudsonPrivateSecurityRealm.java | 61 ++++++------------- .../HudsonPrivateSecurityRealmTest.java | 17 ++++-- 3 files changed, 33 insertions(+), 47 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 6d524737a6fa..ac1a04d583ea 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1961,7 +1961,7 @@ public static String getHexOfSHA256DigestOf(byte[] input) throws IOException { public static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); public static boolean isFipsMode() { - LOGGER.info("FIPS mode : " + FIPS_MODE); + LOGGER.fine("FIPS mode : " + FIPS_MODE); return FIPS_MODE; } diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 8005d87b85ae..355ad487a127 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -24,7 +24,7 @@ package hudson.security; -import static java.util.logging.Level.WARNING; +import static java.util.logging.Level.SEVERE; import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import com.thoughtworks.xstream.converters.UnmarshallingContext; @@ -49,7 +49,6 @@ import hudson.util.XStream2; import java.io.IOException; import java.lang.reflect.Constructor; -import java.math.BigInteger; import java.nio.charset.StandardCharsets; import java.security.MessageDigest; import java.security.NoSuchAlgorithmException; @@ -936,9 +935,6 @@ public boolean isHashValid(String hash) { static class PBKDF2PasswordEncoder implements PasswordHashEncoder { public static final int HASH_LENGTH = 8; - public static final int ZERO = 0; - public static final int HEX_TWO = 2; - public static final int RADIX_LENGTH = 16; public static final String STRING_SEPARATION = ":"; public static final int KEY_LENGTH = 512; public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; @@ -951,48 +947,45 @@ static class PBKDF2PasswordEncoder implements PasswordHashEncoder { @Override public String encode(CharSequence rawPassword) { - return generatePasswordHashWithPBKDF2(rawPassword); + try { + return generatePasswordHashWithPBKDF2(rawPassword); + } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { + throw new RuntimeException(e); + } } @Override public boolean matches(CharSequence rawPassword, String encodedPassword) { + try { return validatePassword(rawPassword.toString(), encodedPassword); + } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { + throw new RuntimeException(e); + } } - private static String generatePasswordHashWithPBKDF2(CharSequence password) { + private static String generatePasswordHashWithPBKDF2(CharSequence password) throws NoSuchAlgorithmException, InvalidKeySpecException { byte[] salt = generateSalt(); PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, 1000, KEY_LENGTH); byte[] hash = generateSecretKey(spec); return PBKDF2 + "$HMACSHA512:" + 1000 + STRING_SEPARATION + Util.toHexString(salt) + "$" + Util.toHexString(hash); } - private static byte[] generateSecretKey(PBEKeySpec spec) { + private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmException, InvalidKeySpecException { SecretKeyFactory secretKeyFactory = null; - byte[] hash = new byte[0]; + byte[] hash; try { secretKeyFactory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); hash = secretKeyFactory.generateSecret(spec).getEncoded(); } catch (NoSuchAlgorithmException e) { - LOGGER.log(WARNING, "No such algorithm found for encode", e); + LOGGER.log(SEVERE, "No such algorithm found for encode", e); + throw new NoSuchAlgorithmException("No such algorithm found for encode", e); } catch (InvalidKeySpecException e) { - LOGGER.log(WARNING, "Invalid key spec exception", e); + LOGGER.log(SEVERE, "Invalid key spec exception", e); + throw new InvalidKeySpecException("Invalid key spec exception", e); } return hash; } - private static String toHex(byte[] secretKey) { - - BigInteger bi = new BigInteger(1, secretKey); - String saltHashValue = bi.toString(RADIX_LENGTH); - - int paddingLength = (secretKey.length * HEX_TWO) - saltHashValue.length(); - if (paddingLength > ZERO) { - return String.format("%0" + paddingLength + "d", ZERO) + saltHashValue; - } else { - return saltHashValue; - } - } - @SuppressFBWarnings(value = {"DMI_RANDOM_USED_ONLY_ONCE", "PREDICTABLE_RANDOM"}, justification = "https://github.com/spotbugs/spotbugs/issues/1539 and doesn't need to be secure, we're just not hardcoding a 'wrong' password") private static byte[] generateSalt() { SecureRandom random = new SecureRandom(); @@ -1015,7 +1008,7 @@ public boolean isHashValid(String hash) { return false; } - private static boolean validatePassword(String originalPassword, String storedPassword) { + private static boolean validatePassword(String originalPassword, String storedPassword) throws NoSuchAlgorithmException, InvalidKeySpecException { String[] parts = storedPassword.split("[:$]"); int iterations = Integer.parseInt(parts[3]); @@ -1027,21 +1020,7 @@ private static boolean validatePassword(String originalPassword, String storedPa byte[] generatedHashValue = generateSecretKey(spec); - int diff = hash.length ^ generatedHashValue.length; - for (int i = ZERO; i < hash.length && i < generatedHashValue.length; i++) - { - diff |= hash[i] ^ generatedHashValue[i]; - } - return diff == 0; - } - - private static byte[] fromHex(String storedPassword) { - byte[] bytes = new byte[storedPassword.length() / HEX_TWO]; - for (int i = ZERO; i < bytes.length; i++) - { - bytes[i] = (byte) Integer.parseInt(storedPassword.substring(HEX_TWO * i, HEX_TWO * i + HEX_TWO), RADIX_LENGTH); - } - return bytes; + return MessageDigest.isEqual(hash, generatedHashValue); } } @@ -1096,7 +1075,7 @@ public boolean isPasswordHashed(String password) { if (password == null) { return false; } - return password.startsWith(getPasswordHeader()) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(getPasswordHeader().length())); + return password.startsWith(getPasswordHeader()) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(getPasswordHeader().length())); } } diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 7ce01b059ace..b19218ad27b3 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -39,8 +39,8 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.any; import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockConstruction; import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.when; @@ -60,6 +60,7 @@ import java.util.Collections; import java.util.List; import javax.crypto.SecretKeyFactory; +import javax.crypto.spec.PBEKeySpec; import jenkins.security.ApiTokenProperty; import jenkins.security.SecurityListener; import jenkins.security.apitoken.ApiTokenPropertyConfiguration; @@ -83,7 +84,7 @@ import org.jvnet.hudson.test.TestExtension; import org.jvnet.hudson.test.WithoutJenkins; import org.mindrot.jbcrypt.BCrypt; - +import org.mockito.MockedConstruction; @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) @@ -544,9 +545,15 @@ public void passwordHashWithInvalidAgorithm() { public void passwordHashWithInvalidKeySpec() throws InvalidKeySpecException { HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); mockStatic(HudsonPrivateSecurityRealm.class); - SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); - when(secretKeyFactory.generateSecret(any())).thenThrow(InvalidKeySpecException.class); - pbkdf2PasswordEncoder.encode("password"); + SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); + PBEKeySpec spec = mock(PBEKeySpec.class); + + try (MockedConstruction mocked = mockConstruction(PBEKeySpec.class)) { + PBEKeySpec pbeKeySpec = new PBEKeySpec("".toCharArray()); + when(pbeKeySpec.getSalt()).thenReturn(null); + when(secretKeyFactory.generateSecret(pbeKeySpec)).thenThrow(InvalidKeySpecException.class); + assertThrows(RuntimeException.class, () -> pbkdf2PasswordEncoder.encode("password")); + } } @Test From 4d27f84e4c49d6a227d04de2510ab35d758c5140 Mon Sep 17 00:00:00 2001 From: c_dsivasamy Date: Wed, 6 Sep 2023 15:30:06 +0530 Subject: [PATCH 19/39] fixed review comments --- core/src/main/java/hudson/security/PasswordHashEncoder.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/PasswordHashEncoder.java b/core/src/main/java/hudson/security/PasswordHashEncoder.java index c49e9350b1fd..a89c59643d72 100644 --- a/core/src/main/java/hudson/security/PasswordHashEncoder.java +++ b/core/src/main/java/hudson/security/PasswordHashEncoder.java @@ -2,6 +2,6 @@ import org.springframework.security.crypto.password.PasswordEncoder; -public interface PasswordHashEncoder extends PasswordEncoder { +interface PasswordHashEncoder extends PasswordEncoder { boolean isHashValid(String hash); } From 769a5943dd3d43d45605d10b543df1625425f6a7 Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 7 Sep 2023 13:12:07 +0100 Subject: [PATCH 20/39] Apply suggestions from code review --- core/src/main/java/hudson/Util.java | 8 +++++++- .../security/HudsonPrivateSecurityRealm.java | 17 ++++------------- .../HudsonPrivateSecurityRealmTest.java | 2 -- 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index ac1a04d583ea..ae61a9f5cf9c 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1958,8 +1958,14 @@ public static String getHexOfSHA256DigestOf(byte[] input) throws IOException { */ @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL") @Restricted(NoExternalUse.class) - public static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); + static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); + /** + * Provide a hint to plugins that the system should be FIPS-140-2 compliant if a plugin or code needs to make some choices as to if some optional behaviour that is not compliant with FIPS-140 should not be allowed. + * If this is {@code true} it does not mean that the instance is compliant, it merely acts as a hint. + * @return {@code true} iff the system should prefer compliance with FIPS-140-2 over compatibility. + * @since TODO + */ public static boolean isFipsMode() { LOGGER.fine("FIPS mode : " + FIPS_MODE); return FIPS_MODE; diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 355ad487a127..f0c743705030 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -893,7 +893,7 @@ public Category getCategory() { /** * {@link PasswordHashEncoder} that uses jBCrypt. */ - static class JBCryptEncoder implements PasswordHashEncoder { + static class JBCryptEncoder implements PasswordHashEncoder { // in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017 // and for 18, it's "only" 20s @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") @@ -959,7 +959,7 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { try { return validatePassword(rawPassword.toString(), encodedPassword); } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { - throw new RuntimeException(e); + throw new RuntimeException("Unable to check password for PBKDF2WithHmacSHA512", e); } } @@ -973,17 +973,8 @@ private static String generatePasswordHashWithPBKDF2(CharSequence password) thro private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmException, InvalidKeySpecException { SecretKeyFactory secretKeyFactory = null; byte[] hash; - try { - secretKeyFactory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); - hash = secretKeyFactory.generateSecret(spec).getEncoded(); - } catch (NoSuchAlgorithmException e) { - LOGGER.log(SEVERE, "No such algorithm found for encode", e); - throw new NoSuchAlgorithmException("No such algorithm found for encode", e); - } catch (InvalidKeySpecException e) { - LOGGER.log(SEVERE, "Invalid key spec exception", e); - throw new InvalidKeySpecException("Invalid key spec exception", e); - } - return hash; + secretKeyFactory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); + return secretKeyFactory.generateSecret(spec).getEncoded(); } @SuppressFBWarnings(value = {"DMI_RANDOM_USED_ONLY_ONCE", "PREDICTABLE_RANDOM"}, justification = "https://github.com/spotbugs/spotbugs/issues/1539 and doesn't need to be secure, we're just not hardcoding a 'wrong' password") diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index b19218ad27b3..531193f46998 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -94,8 +94,6 @@ public class HudsonPrivateSecurityRealmTest { public JenkinsRule j = new JenkinsRule(); private SpySecurityListenerImpl spySecurityListener; - private Field field; - @Before public void linkExtension() { From 4877cb18b3650113d4f98c7d474532a6d06bcfd4 Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 7 Sep 2023 13:53:23 +0100 Subject: [PATCH 21/39] checkstyle --- .../main/java/hudson/security/HudsonPrivateSecurityRealm.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index f0c743705030..c1b2d966787c 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -24,7 +24,6 @@ package hudson.security; -import static java.util.logging.Level.SEVERE; import static javax.servlet.http.HttpServletResponse.SC_UNAUTHORIZED; import com.thoughtworks.xstream.converters.UnmarshallingContext; From 13887f5a9869fd66b64cabb328fc0234dd6f736f Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 7 Sep 2023 14:45:12 +0100 Subject: [PATCH 22/39] introduce specific class for FIPS --- core/src/main/java/hudson/Util.java | 18 ------ .../security/HudsonPrivateSecurityRealm.java | 5 +- .../main/java/jenkins/security/FIPS140.java | 59 +++++++++++++++++++ 3 files changed, 62 insertions(+), 20 deletions(-) create mode 100644 core/src/main/java/jenkins/security/FIPS140.java diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index ae61a9f5cf9c..8065b0312426 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1953,22 +1953,4 @@ public static String getHexOfSHA256DigestOf(byte[] input) throws IOException { return (payloadDigest != null) ? Util.toHexString(payloadDigest) : null; } - /** - * Setting this flag to true enables FIPS mode - */ - @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL") - @Restricted(NoExternalUse.class) - static boolean FIPS_MODE = SystemProperties.getBoolean(Util.class.getName() + ".FIPS_MODE"); - - /** - * Provide a hint to plugins that the system should be FIPS-140-2 compliant if a plugin or code needs to make some choices as to if some optional behaviour that is not compliant with FIPS-140 should not be allowed. - * If this is {@code true} it does not mean that the instance is compliant, it merely acts as a hint. - * @return {@code true} iff the system should prefer compliance with FIPS-140-2 over compatibility. - * @since TODO - */ - public static boolean isFipsMode() { - LOGGER.fine("FIPS mode : " + FIPS_MODE); - return FIPS_MODE; - } - } diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index c1b2d966787c..c3459dd8542d 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -75,6 +75,7 @@ import javax.servlet.http.HttpServletResponse; import javax.servlet.http.HttpSession; import jenkins.model.Jenkins; +import jenkins.security.FIPS140; import jenkins.security.SecurityListener; import jenkins.security.seed.UserSeedProperty; import jenkins.util.SystemProperties; @@ -1015,7 +1016,7 @@ private static boolean validatePassword(String originalPassword, String storedPa } - /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = Util.isFipsMode() ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); + /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = FIPS140.useCompliantAlgorithms() ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); public static final String PBKDF2 = "$PBKDF2"; @@ -1026,7 +1027,7 @@ private static boolean validatePassword(String originalPassword, String storedPa */ public static String getPasswordHeader() { - return Util.isFipsMode() ? PBKDF2 : JBCRYPT; + return FIPS140.useCompliantAlgorithms() ? PBKDF2 : JBCRYPT; } diff --git a/core/src/main/java/jenkins/security/FIPS140.java b/core/src/main/java/jenkins/security/FIPS140.java new file mode 100644 index 000000000000..534eff936200 --- /dev/null +++ b/core/src/main/java/jenkins/security/FIPS140.java @@ -0,0 +1,59 @@ +/* + * The MIT License + * + * Copyright (c) 2023, Cloudbees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ +package jenkins.security; + +import java.util.logging.Level; +import java.util.logging.Logger; +import jenkins.util.SystemProperties; + +/** + * Utilities to help code change behaviour when it is desired to run in a FIPS-140 enabled environment. + * The environment (host, JVM and servlet container), must be suitably configured which is outside the scope of the Jenkins project. + * @see FIPS-140-2 + * @see JEP-XXXX + * @since TODO + */ +public class FIPS140 { + + private final static Logger LOGGER = Logger.getLogger(FIPS140.class.getName()); + + private /* almost final */ static boolean FIPS_MODE = SystemProperties.getBoolean(FIPS140.class.getName() + ".COMPLIANCE"); + + static { + if (useCompliantAlgorithms()) { + LOGGER.log(Level.CONFIG, "System has been asked to run in FIPS-140 compliant mode"); + } + } + + /** + * Provide a hint that the system should strive to be compliant with FIPS-140-2. + * This can be used by code that needs to make choices at runtime whether to disable some optional behaviour that is not compliant with FIPS-140, + * or to switch to a compliant (yet less secure) alternative. + * If this returns {@code true} it does not mean that the instance is compliant, it merely acts as a hint. + * @return {@code true} iff the system should prefer compliance with FIPS-140-2 over compatibility with existing data or alternative non approved algorithms. + */ + public static boolean useCompliantAlgorithms() { + return FIPS_MODE; + } + +} From cfafad8c4d82fe3fe0e8caeeb906b5f0db73f855 Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 7 Sep 2023 15:48:28 +0100 Subject: [PATCH 23/39] misc code cleanups --- core/src/main/java/hudson/Util.java | 1 - .../security/HudsonPrivateSecurityRealm.java | 46 +++++++++---------- .../main/java/jenkins/security/FIPS140.java | 7 +-- 3 files changed, 25 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/hudson/Util.java b/core/src/main/java/hudson/Util.java index 8065b0312426..c4d1e7088336 100644 --- a/core/src/main/java/hudson/Util.java +++ b/core/src/main/java/hudson/Util.java @@ -1952,5 +1952,4 @@ public static String getHexOfSHA256DigestOf(byte[] input) throws IOException { byte[] payloadDigest = Util.getSHA256DigestOf(input); return (payloadDigest != null) ? Util.toHexString(payloadDigest) : null; } - } diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index c3459dd8542d..eca8a7c7f288 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -934,16 +934,17 @@ public boolean isHashValid(String hash) { static class PBKDF2PasswordEncoder implements PasswordHashEncoder { - public static final int HASH_LENGTH = 8; - public static final String STRING_SEPARATION = ":"; - public static final int KEY_LENGTH = 512; - public static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; + private static final String STRING_SEPARATION = ":"; + private static final int KEY_LENGTH_BITS = 512; + private static final int SALT_LENGTH_BYTES = 16; + private static final int ITTERATIONS = 1000; + private static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; - @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") - @Restricted(NoExternalUse.class) - private static int MAXIMUM_PBKDF2_LOG_ROUND = SystemProperties.getInteger(HudsonPrivateSecurityRealm.class.getName() + ".maximumPBKDF2LogRound", 1000); + private SecureRandom random; // defer construction until we need to use it to not delay startup in the case of lack of entropy. - private static final Pattern PBKDF2_PATTERN = Pattern.compile("^\\$PBKDF2\\$HMACSHA512\\:([0-9]{4})\\:[a-zA-Z0-9=]+\\$[a-zA-Z0-9=]+"); + // Identifier($PBKDF2) $ algorithm(HMACSHA512) : rounds : salt_in_hex $ mac_in_hex + private static final Pattern PBKDF2_PATTERN = + Pattern.compile("^\\$PBKDF2\\$HMACSHA512\\:1000\\:[A-F0-9]{" + (SALT_LENGTH_BYTES * 2) + "}\\$[A-F0-9]{" + ((KEY_LENGTH_BITS / 8) * 2) + "}$"); @Override public String encode(CharSequence rawPassword) { @@ -963,9 +964,9 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { } } - private static String generatePasswordHashWithPBKDF2(CharSequence password) throws NoSuchAlgorithmException, InvalidKeySpecException { + private String generatePasswordHashWithPBKDF2(CharSequence password) throws NoSuchAlgorithmException, InvalidKeySpecException { byte[] salt = generateSalt(); - PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, 1000, KEY_LENGTH); + PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, ITTERATIONS, KEY_LENGTH_BITS); byte[] hash = generateSecretKey(spec); return PBKDF2 + "$HMACSHA512:" + 1000 + STRING_SEPARATION + Util.toHexString(salt) + "$" + Util.toHexString(hash); } @@ -977,26 +978,21 @@ private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmE return secretKeyFactory.generateSecret(spec).getEncoded(); } - @SuppressFBWarnings(value = {"DMI_RANDOM_USED_ONLY_ONCE", "PREDICTABLE_RANDOM"}, justification = "https://github.com/spotbugs/spotbugs/issues/1539 and doesn't need to be secure, we're just not hardcoding a 'wrong' password") - private static byte[] generateSalt() { - SecureRandom random = new SecureRandom(); - byte[] salt = new byte[16]; + @SuppressFBWarnings(value = "DMI_RANDOM_USED_ONLY_ONCE", justification = "SpotBugs you are drunk! https://github.com/spotbugs/spotbugs/issues/2128") + private synchronized byte[] generateSalt() { + // synchronized for lazy initialization but also not all SecureRandom s are thread safe. + if (random == null) { + random = new SecureRandom(); + } + byte[] salt = new byte[SALT_LENGTH_BYTES]; random.nextBytes(salt); return salt; } @Override - public boolean isHashValid(String hash) { + public boolean isHashValid(String hash) { Matcher matcher = PBKDF2_PATTERN.matcher(hash); - if (matcher.matches()) { - String logNumOfRound = matcher.group(1); - // no number format exception due to the expression - int logNumOfRoundInt = Integer.parseInt(logNumOfRound); - if (logNumOfRoundInt > 0 && logNumOfRoundInt <= MAXIMUM_PBKDF2_LOG_ROUND) { - return true; - } - } - return false; + return matcher.matches(); } private static boolean validatePassword(String originalPassword, String storedPassword) throws NoSuchAlgorithmException, InvalidKeySpecException { @@ -1007,7 +1003,7 @@ private static boolean validatePassword(String originalPassword, String storedPa byte[] hash = Util.fromHexString(parts[5]); PBEKeySpec spec = new PBEKeySpec(originalPassword.toCharArray(), - salt, iterations, hash.length * HASH_LENGTH); + salt, iterations, hash.length * 8 /* bits in a byte */); byte[] generatedHashValue = generateSecretKey(spec); diff --git a/core/src/main/java/jenkins/security/FIPS140.java b/core/src/main/java/jenkins/security/FIPS140.java index 534eff936200..049d32efa825 100644 --- a/core/src/main/java/jenkins/security/FIPS140.java +++ b/core/src/main/java/jenkins/security/FIPS140.java @@ -20,6 +20,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN * THE SOFTWARE. */ + package jenkins.security; import java.util.logging.Level; @@ -35,9 +36,9 @@ */ public class FIPS140 { - private final static Logger LOGGER = Logger.getLogger(FIPS140.class.getName()); + private static final Logger LOGGER = Logger.getLogger(FIPS140.class.getName()); - private /* almost final */ static boolean FIPS_MODE = SystemProperties.getBoolean(FIPS140.class.getName() + ".COMPLIANCE"); + private static /* almost final */ boolean FIPS_COMPLIANCE_MODE = SystemProperties.getBoolean(FIPS140.class.getName() + ".COMPLIANCE"); static { if (useCompliantAlgorithms()) { @@ -53,7 +54,7 @@ public class FIPS140 { * @return {@code true} iff the system should prefer compliance with FIPS-140-2 over compatibility with existing data or alternative non approved algorithms. */ public static boolean useCompliantAlgorithms() { - return FIPS_MODE; + return FIPS_COMPLIANCE_MODE; } } From 83e6d32e1b8b2a71b99fabda5388d57aa43f6fbd Mon Sep 17 00:00:00 2001 From: James Nord Date: Thu, 7 Sep 2023 17:14:03 +0100 Subject: [PATCH 24/39] revert changes to HudsonPrivateSecurityRealmTest --- .../HudsonPrivateSecurityRealmTest.java | 97 +------------------ 1 file changed, 2 insertions(+), 95 deletions(-) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 531193f46998..9883b0680843 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -25,8 +25,6 @@ package hudson.security; import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_ENCODER; -import static hudson.security.HudsonPrivateSecurityRealm.PASSWORD_HASH_ENCODER; -import static hudson.security.HudsonPrivateSecurityRealm.getPasswordHeader; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.is; @@ -39,28 +37,19 @@ import static org.junit.Assert.assertNull; import static org.junit.Assert.assertThrows; import static org.junit.Assert.assertTrue; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.mockConstruction; -import static org.mockito.Mockito.mockStatic; -import static org.mockito.Mockito.when; import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; -import hudson.Util; import hudson.model.User; import hudson.security.pages.SignupPage; import java.lang.reflect.Field; import java.net.URL; import java.nio.charset.StandardCharsets; -import java.security.NoSuchAlgorithmException; -import java.security.spec.InvalidKeySpecException; import java.util.ArrayList; import java.util.Arrays; import java.util.Base64; import java.util.Collections; import java.util.List; -import javax.crypto.SecretKeyFactory; -import javax.crypto.spec.PBEKeySpec; import jenkins.security.ApiTokenProperty; import jenkins.security.SecurityListener; import jenkins.security.apitoken.ApiTokenPropertyConfiguration; @@ -82,10 +71,7 @@ import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsRule.WebClient; import org.jvnet.hudson.test.TestExtension; -import org.jvnet.hudson.test.WithoutJenkins; import org.mindrot.jbcrypt.BCrypt; -import org.mockito.MockedConstruction; - @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) public class HudsonPrivateSecurityRealmTest { @@ -102,14 +88,9 @@ public void linkExtension() { @Before public void setup() throws Exception { - Field field = HudsonPrivateSecurityRealm.class.getDeclaredField("ID_REGEX"); field.setAccessible(true); field.set(null, null); - - Field field1 = Util.class.getDeclaredField("FIPS_MODE"); - field1.setAccessible(true); - field1.set(System.setProperty("hudson.security.Util.FIPS_MODE", "true"), true); } @Issue("SECURITY-243") @@ -343,9 +324,7 @@ public void userCreationFromRealm() throws Exception { public void userCreationWithHashedPasswords() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); j.jenkins.setSecurityRealm(securityRealm); - Field field = Util.class.getDeclaredField("FIPS_MODE"); - field.setAccessible(false); - field.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); + spySecurityListener.createdUsers.clear(); assertTrue(spySecurityListener.createdUsers.isEmpty()); @@ -519,77 +498,11 @@ public void controlCharacterAreNoMoreValid_CustomRegex() throws Exception { } } - @Test - @WithoutJenkins - public void passwordHashWithPBKDF2() { - - HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - mockStatic(HudsonPrivateSecurityRealm.class); - when(getPasswordHeader()).thenReturn("$PBKDF2"); - assertNotNull(pbkdf2PasswordEncoder.encode("password")); - assertTrue(PASSWORD_ENCODER.isPasswordHashed("$PBKDF2" + PASSWORD_HASH_ENCODER.encode("password"))); - } - - @Test - public void passwordHashWithInvalidAgorithm() { - HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - mockStatic(HudsonPrivateSecurityRealm.class); - SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); - assertThrows(NoSuchAlgorithmException.class, () -> when(SecretKeyFactory.getInstance("")).thenThrow(NoSuchAlgorithmException.class)); - pbkdf2PasswordEncoder.encode("password"); - } - - @Test - public void passwordHashWithInvalidKeySpec() throws InvalidKeySpecException { - HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - mockStatic(HudsonPrivateSecurityRealm.class); - SecretKeyFactory secretKeyFactory = mock(SecretKeyFactory.class); - PBEKeySpec spec = mock(PBEKeySpec.class); - - try (MockedConstruction mocked = mockConstruction(PBEKeySpec.class)) { - PBEKeySpec pbeKeySpec = new PBEKeySpec("".toCharArray()); - when(pbeKeySpec.getSalt()).thenReturn(null); - when(secretKeyFactory.generateSecret(pbeKeySpec)).thenThrow(InvalidKeySpecException.class); - assertThrows(RuntimeException.class, () -> pbkdf2PasswordEncoder.encode("password")); - } - } - - @Test - public void passwordHashMatchesPBKDF2() { - - HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - mockStatic(HudsonPrivateSecurityRealm.class); - when(getPasswordHeader()).thenReturn("$PBKDF2"); - assertTrue(pbkdf2PasswordEncoder.matches("3a6f9ee5a3af41ef844cb291c63b40f4", - "$PBKDF2$HMACSHA512:1024:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215ff7bf64b2af5792581039faa62b52")); - } - - @Test - public void hashPatternMatches() { - HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - assertTrue(pbkdf2PasswordEncoder.isHashValid("$PBKDF2$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); - } - - @Test - public void hashPatternNotMatches() { - HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); - assertFalse(pbkdf2PasswordEncoder.isHashValid("1020:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851")); - } - - @Test - public void passwordHashNotMatches() { - assertFalse(PASSWORD_ENCODER.matches(null, "1000:137287e0ae3e24ae15df2f6caf068d5a:7bcdd7d6788bf20747812fd39b3ff5451235b12dfa62f6b")); - } - @Test public void createAccountSupportsHashedPasswords() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); j.jenkins.setSecurityRealm(securityRealm); - Field field = Util.class.getDeclaredField("FIPS_MODE"); - field.setAccessible(false); - field.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); - securityRealm.createAccountWithHashedPassword("user_hashed", "#jbcrypt:" + BCrypt.hashpw("password", BCrypt.gensalt())); WebClient wc = j.createWebClient(); @@ -608,12 +521,7 @@ public void createAccountWithHashedPasswordRequiresPrefix() { } @Test - public void hashedPasswordTest() throws NoSuchFieldException, IllegalAccessException { - - Field field = Util.class.getDeclaredField("FIPS_MODE"); - field.setAccessible(false); - field.set(System.setProperty("hudson.security.Util.FIPS_MODE", "false"), false); - + public void hashedPasswordTest() { assertTrue("password is hashed", PASSWORD_ENCODER.isPasswordHashed("#jbcrypt:" + BCrypt.hashpw("password", BCrypt.gensalt()))); assertFalse("password is not hashed", PASSWORD_ENCODER.isPasswordHashed("password")); assertFalse("only valid hashed passwords allowed", PASSWORD_ENCODER.isPasswordHashed("#jbcrypt:$2a$blah")); @@ -689,7 +597,6 @@ private void checkUserCannotBeCreatedWith_custom(HudsonPrivateSecurityRealm secu @Test @Issue("SECURITY-1158") public void singupNoLongerVulnerableToSessionFixation() throws Exception { - HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(true, false, null); j.jenkins.setSecurityRealm(securityRealm); JenkinsRule.WebClient wc = j.createWebClient(); From d6853a0d3b60c732495260de0dae0b0299b62fdf Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 8 Sep 2023 13:48:50 +0100 Subject: [PATCH 25/39] code fixups and moving tests --- .../security/HudsonPrivateSecurityRealm.java | 20 ++-- .../HudsonPrivateSecurityRealmTest.java | 92 +++++++++++++++++++ .../HudsonPrivateSecurityRealmFIPSTest.java | 87 ++++++++++++++++++ 3 files changed, 190 insertions(+), 9 deletions(-) create mode 100644 core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java create mode 100644 test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index eca8a7c7f288..ba02a65369b8 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -530,10 +530,11 @@ public User createAccount(String userName, String password) throws IOException { } /** - * Creates a new user account by registering a JBCrypt Hashed password with the user. + * Creates a new user account by registering a Hashed password with the user. * * @param userName The user's name - * @param hashedPassword A hashed password, must begin with {@code #jbcrypt:} + * @param hashedPassword A hashed password, must begin with {@code getPasswordHeader()} + * @see #getPasswordHeader() */ public User createAccountWithHashedPassword(String userName, String hashedPassword) throws IOException { if (!PASSWORD_ENCODER.isPasswordHashed(hashedPassword)) { @@ -893,7 +894,7 @@ public Category getCategory() { /** * {@link PasswordHashEncoder} that uses jBCrypt. */ - static class JBCryptEncoder implements PasswordHashEncoder { + private static class JBCryptEncoder implements PasswordHashEncoder { // in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017 // and for 18, it's "only" 20s @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") @@ -942,9 +943,10 @@ static class PBKDF2PasswordEncoder implements PasswordHashEncoder { private SecureRandom random; // defer construction until we need to use it to not delay startup in the case of lack of entropy. - // Identifier($PBKDF2) $ algorithm(HMACSHA512) : rounds : salt_in_hex $ mac_in_hex + // $PBDKF2 is already checked before we get here. + // $algorithm(HMACSHA512) : rounds : salt_in_hex $ mac_in_hex private static final Pattern PBKDF2_PATTERN = - Pattern.compile("^\\$PBKDF2\\$HMACSHA512\\:1000\\:[A-F0-9]{" + (SALT_LENGTH_BYTES * 2) + "}\\$[A-F0-9]{" + ((KEY_LENGTH_BITS / 8) * 2) + "}$"); + Pattern.compile("^\\$HMACSHA512\\:1000\\:[a-f0-9]{" + (SALT_LENGTH_BYTES * 2) + "}\\$[a-f0-9]{" + ((KEY_LENGTH_BITS / 8) * 2) + "}$"); @Override public String encode(CharSequence rawPassword) { @@ -968,7 +970,7 @@ private String generatePasswordHashWithPBKDF2(CharSequence password) throws NoSu byte[] salt = generateSalt(); PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, ITTERATIONS, KEY_LENGTH_BITS); byte[] hash = generateSecretKey(spec); - return PBKDF2 + "$HMACSHA512:" + 1000 + STRING_SEPARATION + Util.toHexString(salt) + "$" + Util.toHexString(hash); + return "$HMACSHA512:" + 1000 + STRING_SEPARATION + Util.toHexString(salt) + "$" + Util.toHexString(hash); } private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmException, InvalidKeySpecException { @@ -997,10 +999,10 @@ public boolean isHashValid(String hash) { private static boolean validatePassword(String originalPassword, String storedPassword) throws NoSuchAlgorithmException, InvalidKeySpecException { String[] parts = storedPassword.split("[:$]"); - int iterations = Integer.parseInt(parts[3]); + int iterations = Integer.parseInt(parts[2]); - byte[] salt = Util.fromHexString(parts[4]); - byte[] hash = Util.fromHexString(parts[5]); + byte[] salt = Util.fromHexString(parts[3]); + byte[] hash = Util.fromHexString(parts[4]); PBEKeySpec spec = new PBEKeySpec(originalPassword.toCharArray(), salt, iterations, hash.length * 8 /* bits in a byte */); diff --git a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java new file mode 100644 index 000000000000..7bb6142f60fd --- /dev/null +++ b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -0,0 +1,92 @@ +package hudson.security; + +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.mockStatic; +import static org.mockito.Mockito.when; + +import hudson.security.HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder; +import java.security.NoSuchAlgorithmException; +import java.security.spec.InvalidKeySpecException; +import javax.crypto.SecretKeyFactory; +import org.junit.jupiter.api.Test; +import org.mockito.Mockito; + +public class HudsonPrivateSecurityRealmTest { + + // MySecurePassword + private static final String PBKDF2_HMAC_SHA512_ENCODED_PASSWORD = + "$HMACSHA512:1000:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"; + + @Test + public void testPBKDF2RegExp() { + PBKDF2PasswordEncoder encoder = new PBKDF2PasswordEncoder(); + String encoded = encoder.encode("thisIsMyPassword"); + assertTrue(encoder.isHashValid(encoded)); + + assertTrue(encoder.isHashValid( + "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215ff7bf64b2af5792581039faa62b52")); + + assertFalse(encoder.isHashValid( + "$HMACSHA512:1:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "not enough iterations"); + assertFalse(encoder.isHashValid( + "$HMACSHA512:9999:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "too many iterations"); + assertFalse(encoder.isHashValid( + "$HMACSHA512:1000:fe899fcfcef4302ec3f0d36164efef$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "salt too short"); + assertFalse(encoder.isHashValid( + "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "salt too long"); + assertFalse(encoder.isHashValid( + "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e094$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215f"), + "hash result too short"); + assertFalse(encoder.isHashValid( + "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b42662"), + "hash result too long"); + assertFalse(encoder.isHashValid( + "$HMACSHA256:1000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b42662"), + "wrong format"); + assertFalse(encoder.isHashValid( + "::$sfdfssdf"), + "wrong format"); + + } + + @Test + public void testPBKDF2PasswordMatching() { + PBKDF2PasswordEncoder encoder = new PBKDF2PasswordEncoder(); + String encoded = encoder.encode("thisIsMyPassword"); + assertTrue(encoder.matches("thisIsMyPassword", encoded)); + assertFalse(encoder.matches("thisIsNotMyPassword", encoded)); + } + + @Test + public void passwordPBKDF2WithMissingAgorithm() throws Exception { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + try (var ignored = mockStatic(SecretKeyFactory.class)) { + when(SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512")).thenThrow(NoSuchAlgorithmException.class); + assertThrows(RuntimeException.class, () -> pbkdf2PasswordEncoder.encode("password")); + + assertTrue(pbkdf2PasswordEncoder.isHashValid(PBKDF2_HMAC_SHA512_ENCODED_PASSWORD)); + assertThrows(RuntimeException.class, () -> pbkdf2PasswordEncoder.matches("MySecurePassword", PBKDF2_HMAC_SHA512_ENCODED_PASSWORD)); + } + } + + @Test + public void passwordPBKDF2HashWithInvalidKeySpec() throws Exception { + HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder pbkdf2PasswordEncoder = new HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder(); + try (var ignored = mockStatic(SecretKeyFactory.class)) { + SecretKeyFactory skf = mock(SecretKeyFactory.class); + when(SecretKeyFactory.getInstance("PBKDF2WithHmacSHA512")).thenReturn(skf); + when(skf.generateSecret(Mockito.any())).thenThrow(InvalidKeySpecException.class); + assertThrows(RuntimeException.class, () -> pbkdf2PasswordEncoder.encode("password")); + + assertTrue(pbkdf2PasswordEncoder.isHashValid(PBKDF2_HMAC_SHA512_ENCODED_PASSWORD)); + assertThrows(RuntimeException.class, () -> pbkdf2PasswordEncoder.matches("MySecurePassword", PBKDF2_HMAC_SHA512_ENCODED_PASSWORD)); + } + } +} diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java new file mode 100644 index 000000000000..0936ca22fdbd --- /dev/null +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java @@ -0,0 +1,87 @@ +/* + * The MIT License + * + * Copyright (c) 2015, CloudBees, Inc. and others + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * copies of the Software, and to permit persons to whom the Software is + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ + +package hudson.security; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.startsWith; +import static org.junit.Assert.assertThrows; + +import hudson.model.User; +import hudson.security.HudsonPrivateSecurityRealm.Details; +import org.htmlunit.FailingHttpStatusCodeException; +import org.junit.ClassRule; +import org.junit.Rule; +import org.junit.Test; +import org.junit.rules.TestRule; +import org.jvnet.hudson.test.FlagRule; +import org.jvnet.hudson.test.For; +import org.jvnet.hudson.test.JenkinsRule; +import org.jvnet.hudson.test.JenkinsRule.WebClient; + + +@For(HudsonPrivateSecurityRealm.class) +public class HudsonPrivateSecurityRealmFIPSTest { + + @ClassRule + // do not use the FIPS140 class here as that initializes the field before we set the property! + public static TestRule flagRule = FlagRule.systemProperty("jenkins.security.FIPS140.COMPLIANCE", "true"); + + @Rule + public JenkinsRule j = new JenkinsRule(); + + @Test + public void generalLogin() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); + j.jenkins.setSecurityRealm(securityRealm); + + User u1 = securityRealm.createAccount("user", "password"); + u1.setFullName("A User"); + u1.save(); + + // we should be using PBKDF2 hasher + String hashedPassword = u1.getProperty(Details.class).getPassword(); + assertThat(hashedPassword, startsWith("$PBKDF2$HMACSHA512:1000:")); + + WebClient wc = j.createWebClient(); + wc.login("user", "password"); + + assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user", "wrongPass")); + } + + @Test + public void userCreationWithHashedPasswords() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); + j.jenkins.setSecurityRealm(securityRealm); + // "password" after it has gone through the KDF + securityRealm.createAccountWithHashedPassword("user_hashed", + "$PBKDF2$HMACSHA512:1000:89901f42cb107a8dcb2e1ab5e6891aaa$2983a219a5f35115b35d2eb8d752498414628ea8428d97f707bdf5aaa9e485d16e4a3b9e36e55115735924f4be4cb48f560b3da333e11fd250fd15362bef315d"); + WebClient wc = j.createWebClient(); + + // login should succeed + wc.login("user_hashed", "password"); + + assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user_hashed", "password2")); + } +} From 46a1c5a3fd771d903ebb7b4dfcd308c9b9ca2bdc Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 8 Sep 2023 15:41:07 +0100 Subject: [PATCH 26/39] code fixups and moving tests --- .../security/HudsonPrivateSecurityRealm.java | 18 +++--- .../HudsonPrivateSecurityRealmTest.java | 62 ++++++++++++++++--- .../HudsonPrivateSecurityRealmFIPSTest.java | 2 +- 3 files changed, 62 insertions(+), 20 deletions(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index ba02a65369b8..d1c3dd292535 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -894,7 +894,7 @@ public Category getCategory() { /** * {@link PasswordHashEncoder} that uses jBCrypt. */ - private static class JBCryptEncoder implements PasswordHashEncoder { + static class JBCryptEncoder implements PasswordHashEncoder { // in jBCrypt the maximum is 30, which takes ~22h with laptop late-2017 // and for 18, it's "only" 20s @SuppressFBWarnings(value = "MS_SHOULD_BE_FINAL", justification = "Accessible via System Groovy Scripts") @@ -938,7 +938,9 @@ static class PBKDF2PasswordEncoder implements PasswordHashEncoder { private static final String STRING_SEPARATION = ":"; private static final int KEY_LENGTH_BITS = 512; private static final int SALT_LENGTH_BYTES = 16; - private static final int ITTERATIONS = 1000; + // https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#pbkdf2 + // ~230ms on an Intel i7-10875H CPU (JBCryptEncoder is ~57ms) + private static final int ITTERATIONS = 210_000; private static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; private SecureRandom random; // defer construction until we need to use it to not delay startup in the case of lack of entropy. @@ -946,7 +948,7 @@ static class PBKDF2PasswordEncoder implements PasswordHashEncoder { // $PBDKF2 is already checked before we get here. // $algorithm(HMACSHA512) : rounds : salt_in_hex $ mac_in_hex private static final Pattern PBKDF2_PATTERN = - Pattern.compile("^\\$HMACSHA512\\:1000\\:[a-f0-9]{" + (SALT_LENGTH_BYTES * 2) + "}\\$[a-f0-9]{" + ((KEY_LENGTH_BITS / 8) * 2) + "}$"); + Pattern.compile("^\\$HMACSHA512\\:" + ITTERATIONS + "\\:[a-f0-9]{" + (SALT_LENGTH_BYTES * 2) + "}\\$[a-f0-9]{" + ((KEY_LENGTH_BITS / 8) * 2) + "}$"); @Override public String encode(CharSequence rawPassword) { @@ -970,13 +972,11 @@ private String generatePasswordHashWithPBKDF2(CharSequence password) throws NoSu byte[] salt = generateSalt(); PBEKeySpec spec = new PBEKeySpec(password.toString().toCharArray(), salt, ITTERATIONS, KEY_LENGTH_BITS); byte[] hash = generateSecretKey(spec); - return "$HMACSHA512:" + 1000 + STRING_SEPARATION + Util.toHexString(salt) + "$" + Util.toHexString(hash); + return "$HMACSHA512:" + ITTERATIONS + STRING_SEPARATION + Util.toHexString(salt) + "$" + Util.toHexString(hash); } private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmException, InvalidKeySpecException { - SecretKeyFactory secretKeyFactory = null; - byte[] hash; - secretKeyFactory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); + SecretKeyFactory secretKeyFactory = SecretKeyFactory.getInstance(PBKDF2_ALGORITHM); return secretKeyFactory.generateSecret(spec).getEncoded(); } @@ -997,14 +997,14 @@ public boolean isHashValid(String hash) { return matcher.matches(); } - private static boolean validatePassword(String originalPassword, String storedPassword) throws NoSuchAlgorithmException, InvalidKeySpecException { + private static boolean validatePassword(String password, String storedPassword) throws NoSuchAlgorithmException, InvalidKeySpecException { String[] parts = storedPassword.split("[:$]"); int iterations = Integer.parseInt(parts[2]); byte[] salt = Util.fromHexString(parts[3]); byte[] hash = Util.fromHexString(parts[4]); - PBEKeySpec spec = new PBEKeySpec(originalPassword.toCharArray(), + PBEKeySpec spec = new PBEKeySpec(password.toCharArray(), salt, iterations, hash.length * 8 /* bits in a byte */); byte[] generatedHashValue = generateSecretKey(spec); diff --git a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 7bb6142f60fd..a1396c4b0bf6 100644 --- a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -7,9 +7,11 @@ import static org.mockito.Mockito.mockStatic; import static org.mockito.Mockito.when; +import hudson.security.HudsonPrivateSecurityRealm.JBCryptEncoder; import hudson.security.HudsonPrivateSecurityRealm.PBKDF2PasswordEncoder; import java.security.NoSuchAlgorithmException; import java.security.spec.InvalidKeySpecException; +import java.time.Duration; import javax.crypto.SecretKeyFactory; import org.junit.jupiter.api.Test; import org.mockito.Mockito; @@ -18,7 +20,47 @@ public class HudsonPrivateSecurityRealmTest { // MySecurePassword private static final String PBKDF2_HMAC_SHA512_ENCODED_PASSWORD = - "$HMACSHA512:1000:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"; + "$HMACSHA512:210000:30f9e0a5470a8bc67f128ca1aae25dd4$88abaca4f442caeff0096ec0f75df2d77cc31a956c564133232f4d2532a72c8d4380a718d5b2a3dccab9e752027eeadd8f9f2c0c624505531bf3a57ec7d08aad"; + + //@Test + public void timingPBKDF2() { + // ignore the salt generation - check just matching.... + PBKDF2PasswordEncoder encoder = new PBKDF2PasswordEncoder(); + String encoded = encoder.encode("thisIsMyPassword1"); + + long start = System.nanoTime(); + for (int i = 0; i < 10; i++) { + System.out.println(encoder.matches("thisIsMyPassword" + i, encoded)); + } + long end = System.nanoTime(); + long duration = end - start; + long duration_per_iteration = duration / 10; + + Duration d = Duration.ofNanos(duration_per_iteration); + System.out.println("PBKDF2 took " + d.toNanos() + "ns"); + System.out.println("PBKDF2 took " + d.toMillis() + "ms"); + System.out.println("PBKDF2 took " + d.toSeconds() + "s"); + } + + //@Test + public void timingBcrypt() { + // ignore the salt generation - check just matching.... + JBCryptEncoder encoder = new JBCryptEncoder(); + String encoded = encoder.encode("thisIsMyPassword1"); + + long start = System.nanoTime(); + for (int i = 0; i < 10; i++) { + System.out.println(encoder.matches("thisIsMyPassword" + i, encoded)); + } + long end = System.nanoTime(); + long duration = end - start; + long duration_per_iteration = duration / 10; + + Duration d = Duration.ofNanos(duration_per_iteration); + System.out.println("BCrypt took " + d.toNanos() + "ns"); + System.out.println("BCrypt took " + d.toMillis() + "ms"); + System.out.println("BCrypt took " + d.toSeconds() + "s"); + } @Test public void testPBKDF2RegExp() { @@ -26,29 +68,29 @@ public void testPBKDF2RegExp() { String encoded = encoder.encode("thisIsMyPassword"); assertTrue(encoder.isHashValid(encoded)); - assertTrue(encoder.isHashValid( - "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e0$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215ff7bf64b2af5792581039faa62b52")); + // and a static one for other tests... + assertTrue(encoder.isHashValid(PBKDF2_HMAC_SHA512_ENCODED_PASSWORD)); assertFalse(encoder.isHashValid( - "$HMACSHA512:1:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "$HMACSHA512:1000:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), "not enough iterations"); assertFalse(encoder.isHashValid( - "$HMACSHA512:9999:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "$HMACSHA512:500000:fe899fcfcef4302ec3f0d36164efefdc$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), "too many iterations"); assertFalse(encoder.isHashValid( - "$HMACSHA512:1000:fe899fcfcef4302ec3f0d36164efef$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "$HMACSHA512:210000:fe899fcfcef4302ec3f0d36164efef$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), "salt too short"); assertFalse(encoder.isHashValid( - "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), + "$HMACSHA512:210000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b"), "salt too long"); assertFalse(encoder.isHashValid( - "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e094$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215f"), + "$HMACSHA512:210000:f6865c02cc759fd061db0f3121a093e094$079bd3a0c2851248343584a9a4625360e9ebb13c36be49542268d2ebdbd1fb71f004db9ce7335a61885985e32e08cb20215f"), "hash result too short"); assertFalse(encoder.isHashValid( - "$HMACSHA512:1000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b42662"), + "$HMACSHA512:210000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b42662"), "hash result too long"); assertFalse(encoder.isHashValid( - "$HMACSHA256:1000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b42662"), + "$HMACSHA256:210000:f6865c02cc759fd061db0f3121a093e094$0781364eae9dac4ef1c4c3bf34c28e13965b46105fec0b6fcf4bae78e246fb5e51a1694fff19acac2dfb37b16055092644f3682c25beea9a7a286bf94e52f63b42662"), "wrong format"); assertFalse(encoder.isHashValid( "::$sfdfssdf"), diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java index 0936ca22fdbd..4aa30c68d20e 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java @@ -62,7 +62,7 @@ public void generalLogin() throws Exception { // we should be using PBKDF2 hasher String hashedPassword = u1.getProperty(Details.class).getPassword(); - assertThat(hashedPassword, startsWith("$PBKDF2$HMACSHA512:1000:")); + assertThat(hashedPassword, startsWith("$PBKDF2$HMACSHA512:210000:")); WebClient wc = j.createWebClient(); wc.login("user", "password"); From 1b4104438ca6522ab4e61a4928e16c5a5fa0fad5 Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 8 Sep 2023 15:55:52 +0100 Subject: [PATCH 27/39] make methods and fields private, and add message to exception --- .../hudson/security/HudsonPrivateSecurityRealm.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index d1c3dd292535..3672237b2c62 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -955,7 +955,7 @@ public String encode(CharSequence rawPassword) { try { return generatePasswordHashWithPBKDF2(rawPassword); } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { - throw new RuntimeException(e); + throw new RuntimeException("Unable to generate password with PBKDF2WithHmacSHA512", e); } } @@ -964,7 +964,7 @@ public boolean matches(CharSequence rawPassword, String encodedPassword) { try { return validatePassword(rawPassword.toString(), encodedPassword); } catch (NoSuchAlgorithmException | InvalidKeySpecException e) { - throw new RuntimeException("Unable to check password for PBKDF2WithHmacSHA512", e); + throw new RuntimeException("Unable to check password with PBKDF2WithHmacSHA512", e); } } @@ -1017,14 +1017,13 @@ private static boolean validatePassword(String password, String storedPassword) /* package */ static final PasswordHashEncoder PASSWORD_HASH_ENCODER = FIPS140.useCompliantAlgorithms() ? new PBKDF2PasswordEncoder() : new JBCryptEncoder(); - public static final String PBKDF2 = "$PBKDF2"; - public static final String JBCRYPT = "#jbcrypt:"; + private static final String PBKDF2 = "$PBKDF2"; + private static final String JBCRYPT = "#jbcrypt:"; /** * Magic header used to detect if a password is hashed. */ - - public static String getPasswordHeader() { + private static String getPasswordHeader() { return FIPS140.useCompliantAlgorithms() ? PBKDF2 : JBCRYPT; } From 0d73df1fad0d0f055131ffa4c44fbeef6b6f2ae1 Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 8 Sep 2023 15:59:29 +0100 Subject: [PATCH 28/39] SecureRandom s -> SecureRandoms --- .../main/java/hudson/security/HudsonPrivateSecurityRealm.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 3672237b2c62..f1a1d59d4364 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -982,7 +982,7 @@ private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmE @SuppressFBWarnings(value = "DMI_RANDOM_USED_ONLY_ONCE", justification = "SpotBugs you are drunk! https://github.com/spotbugs/spotbugs/issues/2128") private synchronized byte[] generateSalt() { - // synchronized for lazy initialization but also not all SecureRandom s are thread safe. + // synchronized for lazy initialization but also not all SecureRandoms are thread safe. if (random == null) { random = new SecureRandom(); } From 0f7acfd6569ad15b1d3f180860d87e81ab8eb47a Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 8 Sep 2023 16:08:49 +0100 Subject: [PATCH 29/39] fixup IT after increasing the number of hasing rounds --- .../hudson/security/HudsonPrivateSecurityRealmFIPSTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java index 4aa30c68d20e..6f60a9ff9ac1 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java @@ -76,7 +76,7 @@ public void userCreationWithHashedPasswords() throws Exception { j.jenkins.setSecurityRealm(securityRealm); // "password" after it has gone through the KDF securityRealm.createAccountWithHashedPassword("user_hashed", - "$PBKDF2$HMACSHA512:1000:89901f42cb107a8dcb2e1ab5e6891aaa$2983a219a5f35115b35d2eb8d752498414628ea8428d97f707bdf5aaa9e485d16e4a3b9e36e55115735924f4be4cb48f560b3da333e11fd250fd15362bef315d"); + "$PBKDF2$HMACSHA512:210000:ffbb207b847010af98cdd2b09c79392c$f67c3b985daf60db83a9088bc2439f7b77016d26c1439a9877c4f863c377272283ce346edda4578a5607ea620a4beb662d853b800f373297e6f596af797743a6"); WebClient wc = j.createWebClient(); // login should succeed From 67c7a240984c27710aa79418176effbcc5e2372b Mon Sep 17 00:00:00 2001 From: James Nord Date: Mon, 11 Sep 2023 10:49:04 +0100 Subject: [PATCH 30/39] move SecureRandom to double-checked-locking as SecureRandom itself is thread safe --- .../security/HudsonPrivateSecurityRealm.java | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index f1a1d59d4364..2df8259cd1b5 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -943,7 +943,7 @@ static class PBKDF2PasswordEncoder implements PasswordHashEncoder { private static final int ITTERATIONS = 210_000; private static final String PBKDF2_ALGORITHM = "PBKDF2WithHmacSHA512"; - private SecureRandom random; // defer construction until we need to use it to not delay startup in the case of lack of entropy. + private volatile SecureRandom random; // defer construction until we need to use it to not delay startup in the case of lack of entropy. // $PBDKF2 is already checked before we get here. // $algorithm(HMACSHA512) : rounds : salt_in_hex $ mac_in_hex @@ -980,14 +980,21 @@ private static byte[] generateSecretKey(PBEKeySpec spec) throws NoSuchAlgorithmE return secretKeyFactory.generateSecret(spec).getEncoded(); } - @SuppressFBWarnings(value = "DMI_RANDOM_USED_ONLY_ONCE", justification = "SpotBugs you are drunk! https://github.com/spotbugs/spotbugs/issues/2128") - private synchronized byte[] generateSalt() { - // synchronized for lazy initialization but also not all SecureRandoms are thread safe. + private SecureRandom secureRandom() { + // lazy initialisation so that we do not block startup due to entropy if (random == null) { - random = new SecureRandom(); + synchronized (this) { + if (random == null) { + random = new SecureRandom(); + } + } } + return random; + } + + private byte[] generateSalt() { byte[] salt = new byte[SALT_LENGTH_BYTES]; - random.nextBytes(salt); + secureRandom().nextBytes(salt); return salt; } From 1cf01ddf73e0b7850bbc87665a8b3a93cb089b5b Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 12 Sep 2023 13:28:35 +0100 Subject: [PATCH 31/39] make FIPS_COMPLIANCE_MODE final changing this at run time makes no sense as decisions will already have been made to configure things differently. --- core/src/main/java/jenkins/security/FIPS140.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/jenkins/security/FIPS140.java b/core/src/main/java/jenkins/security/FIPS140.java index 049d32efa825..00c28b231363 100644 --- a/core/src/main/java/jenkins/security/FIPS140.java +++ b/core/src/main/java/jenkins/security/FIPS140.java @@ -38,7 +38,7 @@ public class FIPS140 { private static final Logger LOGGER = Logger.getLogger(FIPS140.class.getName()); - private static /* almost final */ boolean FIPS_COMPLIANCE_MODE = SystemProperties.getBoolean(FIPS140.class.getName() + ".COMPLIANCE"); + private static final boolean FIPS_COMPLIANCE_MODE = SystemProperties.getBoolean(FIPS140.class.getName() + ".COMPLIANCE"); static { if (useCompliantAlgorithms()) { From 5c6070997f1cd59cc691727ddfa90f43bd264bcc Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 22 Sep 2023 16:09:30 +0100 Subject: [PATCH 32/39] Add (c) header --- .../hudson/security/PasswordHashEncoder.java | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/core/src/main/java/hudson/security/PasswordHashEncoder.java b/core/src/main/java/hudson/security/PasswordHashEncoder.java index a89c59643d72..682619eac200 100644 --- a/core/src/main/java/hudson/security/PasswordHashEncoder.java +++ b/core/src/main/java/hudson/security/PasswordHashEncoder.java @@ -1,3 +1,25 @@ +/* + * The MIT License + * + * Copyright (c) 2023, Cloudbees, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a copy + * of this software and associated documentation files (the "Software"), to deal + * in the Software without restriction, including without limitation the rights + * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + * furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice shall be included in + * all copies or substantial portions of the Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + * THE SOFTWARE. + */ package hudson.security; import org.springframework.security.crypto.password.PasswordEncoder; From e8fb99902421381df036904c2c0342eb19927b6f Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 22 Sep 2023 16:20:22 +0100 Subject: [PATCH 33/39] keep the checkstyle overlord happy --- core/src/main/java/hudson/security/PasswordHashEncoder.java | 1 + 1 file changed, 1 insertion(+) diff --git a/core/src/main/java/hudson/security/PasswordHashEncoder.java b/core/src/main/java/hudson/security/PasswordHashEncoder.java index 682619eac200..14c0da93ba91 100644 --- a/core/src/main/java/hudson/security/PasswordHashEncoder.java +++ b/core/src/main/java/hudson/security/PasswordHashEncoder.java @@ -20,6 +20,7 @@ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN * THE SOFTWARE. */ + package hudson.security; import org.springframework.security.crypto.password.PasswordEncoder; From f37bcc31de0a041a2f1782fa3bf253701d04ebcd Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 22 Sep 2023 17:28:13 +0100 Subject: [PATCH 34/39] update exception message depending on the specific error issue --- .../hudson/security/HudsonPrivateSecurityRealm.java | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 2df8259cd1b5..565fb0348113 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -538,7 +538,15 @@ public User createAccount(String userName, String password) throws IOException { */ public User createAccountWithHashedPassword(String userName, String hashedPassword) throws IOException { if (!PASSWORD_ENCODER.isPasswordHashed(hashedPassword)) { - throw new IllegalArgumentException("this method should only be called with a pre-hashed password"); + final String message; + if (hashedPassword == null) { + message = "The hashed password cannot be null"; + } else if (hashedPassword.startsWith(getPasswordHeader())) { + message = "The hashed password was hashed with the correct algorithm, but the format was not correct"; + } else { + message = "The hashed password was hashed with an incorrect algorithm. Jenkins is expecting " + getPasswordHeader(); + } + throw new IllegalArgumentException(message); } User user = User.getById(userName, true); user.addProperty(Details.fromHashedPassword(hashedPassword)); From 15e6c88be791c73f1e4a4b4afeabac0473ec7ce9 Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 26 Sep 2023 09:11:24 +0100 Subject: [PATCH 35/39] Leave comment about commented out tests --- .../security/HudsonPrivateSecurityRealmTest.java | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index a1396c4b0bf6..1cc4464be552 100644 --- a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -22,6 +22,11 @@ public class HudsonPrivateSecurityRealmTest { private static final String PBKDF2_HMAC_SHA512_ENCODED_PASSWORD = "$HMACSHA512:210000:30f9e0a5470a8bc67f128ca1aae25dd4$88abaca4f442caeff0096ec0f75df2d77cc31a956c564133232f4d2532a72c8d4380a718d5b2a3dccab9e752027eeadd8f9f2c0c624505531bf3a57ec7d08aad"; + /* This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we + * use correct for the state of CPUs). + * We do not want to assert that the range < x and > y as that would make the test flaky on overloaded + * or slow hardware, so this is commented out but left for ease of running locally when desired. + */ //@Test public void timingPBKDF2() { // ignore the salt generation - check just matching.... @@ -42,6 +47,11 @@ public void timingPBKDF2() { System.out.println("PBKDF2 took " + d.toSeconds() + "s"); } + /* This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we + * use correct for the state of CPUs). + * We do not want to assert that the range < x and > y as that would make the test flaky on overloaded + * or slow hardware, so this is commented out but left for ease of running locally when desired. + */ //@Test public void timingBcrypt() { // ignore the salt generation - check just matching.... From d76ff1706369a7f78a9f426f41e7631b4c3bd575 Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 26 Sep 2023 09:13:03 +0100 Subject: [PATCH 36/39] remove extra space in comment --- .../java/hudson/security/HudsonPrivateSecurityRealmTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 1cc4464be552..79c446a5695a 100644 --- a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -25,7 +25,7 @@ public class HudsonPrivateSecurityRealmTest { /* This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we * use correct for the state of CPUs). * We do not want to assert that the range < x and > y as that would make the test flaky on overloaded - * or slow hardware, so this is commented out but left for ease of running locally when desired. + * or slow hardware, so this is commented out but left for ease of running locally when desired. */ //@Test public void timingPBKDF2() { @@ -50,7 +50,7 @@ public void timingPBKDF2() { /* This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we * use correct for the state of CPUs). * We do not want to assert that the range < x and > y as that would make the test flaky on overloaded - * or slow hardware, so this is commented out but left for ease of running locally when desired. + * or slow hardware, so this is commented out but left for ease of running locally when desired. */ //@Test public void timingBcrypt() { From 3fbbf1da6465d5cb00b1a61e2f6fd3952505aa33 Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 26 Sep 2023 11:34:55 +0100 Subject: [PATCH 37/39] remove trailing line ends to keep spotless happy --- .../security/HudsonPrivateSecurityRealmTest.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 79c446a5695a..9d7e2b7b5980 100644 --- a/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/core/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -22,8 +22,9 @@ public class HudsonPrivateSecurityRealmTest { private static final String PBKDF2_HMAC_SHA512_ENCODED_PASSWORD = "$HMACSHA512:210000:30f9e0a5470a8bc67f128ca1aae25dd4$88abaca4f442caeff0096ec0f75df2d77cc31a956c564133232f4d2532a72c8d4380a718d5b2a3dccab9e752027eeadd8f9f2c0c624505531bf3a57ec7d08aad"; - /* This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we - * use correct for the state of CPUs). + /* + * This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we + * use correct for the state of CPUs). * We do not want to assert that the range < x and > y as that would make the test flaky on overloaded * or slow hardware, so this is commented out but left for ease of running locally when desired. */ @@ -47,8 +48,9 @@ public void timingPBKDF2() { System.out.println("PBKDF2 took " + d.toSeconds() + "s"); } - /* This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we - * use correct for the state of CPUs). + /* + * This exists so that we can easily check the complexity of how long this takes (ie is the number of iterations we + * use correct for the state of CPUs). * We do not want to assert that the range < x and > y as that would make the test flaky on overloaded * or slow hardware, so this is commented out but left for ease of running locally when desired. */ From 9946596378ad74c7623e6eea96cd1d6e95fd131b Mon Sep 17 00:00:00 2001 From: James Nord Date: Tue, 3 Oct 2023 16:00:36 +0100 Subject: [PATCH 38/39] warn when password hashed with incorrect algorithm --- .../security/HudsonPrivateSecurityRealm.java | 13 ++++- .../HudsonPrivateSecurityRealmFIPSTest.java | 46 ++++++++++++++++++ .../HudsonPrivateSecurityRealmTest.java | 47 +++++++++++++++++++ 3 files changed, 105 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java index 565fb0348113..5d78c8887c8f 100644 --- a/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java +++ b/core/src/main/java/hudson/security/HudsonPrivateSecurityRealm.java @@ -60,6 +60,7 @@ import java.util.List; import java.util.Random; import java.util.Set; +import java.util.logging.Level; import java.util.logging.Logger; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -1078,7 +1079,17 @@ public boolean isPasswordHashed(String password) { if (password == null) { return false; } - return password.startsWith(getPasswordHeader()) && PASSWORD_HASH_ENCODER.isHashValid(password.substring(getPasswordHeader().length())); + if (password.startsWith(getPasswordHeader())) { + return PASSWORD_HASH_ENCODER.isHashValid(password.substring(getPasswordHeader().length())); + } + if (password.startsWith(FIPS140.useCompliantAlgorithms() ? JBCRYPT : PBKDF2)) { + // switch the header to see if this is using a different encryption + LOGGER.log(Level.WARNING, "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"); + } else { + LOGGER.log(Level.FINE, "A password appears to be stored (or is attempting to be stored) that is not hashed/encrypted."); + } + return false; } } diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java index 6f60a9ff9ac1..155e9cab1adc 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmFIPSTest.java @@ -25,11 +25,15 @@ package hudson.security; import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.startsWith; import static org.junit.Assert.assertThrows; import hudson.model.User; import hudson.security.HudsonPrivateSecurityRealm.Details; +import java.util.logging.Level; +import org.hamcrest.Matcher; import org.htmlunit.FailingHttpStatusCodeException; import org.junit.ClassRule; import org.junit.Rule; @@ -39,15 +43,22 @@ import org.jvnet.hudson.test.For; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsRule.WebClient; +import org.jvnet.hudson.test.LoggerRule; @For(HudsonPrivateSecurityRealm.class) public class HudsonPrivateSecurityRealmFIPSTest { + // the jbcrypt encoded for of "a" without the quotes + private static final String JBCRYPT_ENCODED_PASSWORD = "#jbcrypt:$2a$06$m0CrhHm10qJ3lXRY.5zDGO3rS2KdeeWLuGmsfGlMfOxih58VYVfxe"; + @ClassRule // do not use the FIPS140 class here as that initializes the field before we set the property! public static TestRule flagRule = FlagRule.systemProperty("jenkins.security.FIPS140.COMPLIANCE", "true"); + @Rule + public LoggerRule lr = new LoggerRule().record(HudsonPrivateSecurityRealm.class, Level.WARNING).capture(5); + @Rule public JenkinsRule j = new JenkinsRule(); @@ -83,5 +94,40 @@ public void userCreationWithHashedPasswords() throws Exception { wc.login("user_hashed", "password"); assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user_hashed", "password2")); + assertThat(lr, not(hasIncorrectHashingLogEntry())); + } + + @Test + public void userLoginAfterEnablingFIPS() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); + j.jenkins.setSecurityRealm(securityRealm); + + User u1 = securityRealm.createAccount("user", "a"); + u1.setFullName("A User"); + // overwrite the password property using an password created using an incorrect algorithm + u1.addProperty(Details.fromHashedPassword(JBCRYPT_ENCODED_PASSWORD)); + + u1.save(); + assertThat(u1.getProperty(Details.class).getPassword(), is(JBCRYPT_ENCODED_PASSWORD)); + + try (WebClient wc = j.createWebClient()) { + assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user", "a")); + } + assertThat(lr, hasIncorrectHashingLogEntry()); + } + + @Test + public void userCreationWithJBCryptPasswords() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); + + IllegalArgumentException illegalArgumentException = assertThrows(IllegalArgumentException.class, + () -> securityRealm.createAccountWithHashedPassword("user_hashed_incorrect_algorithm", JBCRYPT_ENCODED_PASSWORD)); + assertThat(illegalArgumentException.getMessage(), + is("The hashed password was hashed with an incorrect algorithm. Jenkins is expecting $PBKDF2")); + } + + private static Matcher hasIncorrectHashingLogEntry() { + return LoggerRule.recorded(is( + "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")); } } diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 9883b0680843..8f98fde31d30 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -41,6 +41,7 @@ import edu.umd.cs.findbugs.annotations.NonNull; import hudson.ExtensionList; import hudson.model.User; +import hudson.security.HudsonPrivateSecurityRealm.Details; import hudson.security.pages.SignupPage; import java.lang.reflect.Field; import java.net.URL; @@ -50,11 +51,14 @@ import java.util.Base64; import java.util.Collections; import java.util.List; +import java.util.logging.Level; import jenkins.security.ApiTokenProperty; import jenkins.security.SecurityListener; import jenkins.security.apitoken.ApiTokenPropertyConfiguration; import jenkins.security.seed.UserSeedProperty; import org.apache.commons.lang.StringUtils; +import org.hamcrest.Matcher; +import org.htmlunit.FailingHttpStatusCodeException; import org.htmlunit.HttpMethod; import org.htmlunit.WebRequest; import org.htmlunit.html.HtmlForm; @@ -70,15 +74,23 @@ import org.jvnet.hudson.test.Issue; import org.jvnet.hudson.test.JenkinsRule; import org.jvnet.hudson.test.JenkinsRule.WebClient; +import org.jvnet.hudson.test.LoggerRule; import org.jvnet.hudson.test.TestExtension; import org.mindrot.jbcrypt.BCrypt; @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) public class HudsonPrivateSecurityRealmTest { + // the PBKDF encoded for of "password" without the quotes + private static final String PBKDF_ENDOCED_PASSWORD = + "$PBKDF2$HMACSHA512:210000:ffbb207b847010af98cdd2b09c79392c$f67c3b985daf60db83a9088bc2439f7b77016d26c1439a9877c4f863c377272283ce346edda4578a5607ea620a4beb662d853b800f373297e6f596af797743a6"; + @Rule public JenkinsRule j = new JenkinsRule(); + @Rule + public LoggerRule lr = new LoggerRule().record(HudsonPrivateSecurityRealm.class, Level.WARNING).capture(5); + private SpySecurityListenerImpl spySecurityListener; @Before @@ -511,6 +523,7 @@ public void createAccountSupportsHashedPasswords() throws Exception { XmlPage w2 = (XmlPage) wc.goTo("whoAmI/api/xml", "application/xml"); assertThat(w2, hasXPath("//name", is("user_hashed"))); + assertThat(lr, not(hasIncorrectHashingLogEntry())); } @Test @@ -717,6 +730,40 @@ public void changingPassword_withSeedDisable_hasNoImpact() throws Exception { } } + @Test + public void userLoginAfterDisalbingFIPS() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); + j.jenkins.setSecurityRealm(securityRealm); + + User u1 = securityRealm.createAccount("user", "password"); + u1.setFullName("A User"); + // overwrite the password property using an password created using an incorrect algorithm + u1.addProperty(Details.fromHashedPassword(PBKDF_ENDOCED_PASSWORD)); + + u1.save(); + assertThat(u1.getProperty(Details.class).getPassword(), is(PBKDF_ENDOCED_PASSWORD)); + + try (WebClient wc = j.createWebClient()) { + assertThrows(FailingHttpStatusCodeException.class, () -> wc.login("user", "password")); + } + assertThat(lr, hasIncorrectHashingLogEntry()); + } + + @Test + public void userCreationWithPBKDFPasswords() throws Exception { + HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); + + IllegalArgumentException illegalArgumentException = assertThrows(IllegalArgumentException.class, + () -> securityRealm.createAccountWithHashedPassword("user_hashed_incorrect_algorithm", PBKDF_ENDOCED_PASSWORD)); + assertThat(illegalArgumentException.getMessage(), + is("The hashed password was hashed with an incorrect algorithm. Jenkins is expecting #jbcrypt:")); + } + + private static Matcher hasIncorrectHashingLogEntry() { + return LoggerRule.recorded(is( + "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")); + } + private User prepareRealmAndAlice() throws Exception { j.jenkins.setDisableRememberMe(false); HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); From 526118a7bd4f605bbabbc6d751bc5e7226ea4f84 Mon Sep 17 00:00:00 2001 From: James Nord Date: Wed, 4 Oct 2023 11:11:35 +0100 Subject: [PATCH 39/39] fix typos Co-authored-by: Daniel Beck <1831569+daniel-beck@users.noreply.github.com> --- .../java/hudson/security/HudsonPrivateSecurityRealmTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java index 8f98fde31d30..544715d2d2f1 100644 --- a/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java +++ b/test/src/test/java/hudson/security/HudsonPrivateSecurityRealmTest.java @@ -81,7 +81,7 @@ @For({UserSeedProperty.class, HudsonPrivateSecurityRealm.class}) public class HudsonPrivateSecurityRealmTest { - // the PBKDF encoded for of "password" without the quotes + // the PBKDF encoded form of "password" without the quotes private static final String PBKDF_ENDOCED_PASSWORD = "$PBKDF2$HMACSHA512:210000:ffbb207b847010af98cdd2b09c79392c$f67c3b985daf60db83a9088bc2439f7b77016d26c1439a9877c4f863c377272283ce346edda4578a5607ea620a4beb662d853b800f373297e6f596af797743a6"; @@ -731,7 +731,7 @@ public void changingPassword_withSeedDisable_hasNoImpact() throws Exception { } @Test - public void userLoginAfterDisalbingFIPS() throws Exception { + public void userLoginAfterDisablingFIPS() throws Exception { HudsonPrivateSecurityRealm securityRealm = new HudsonPrivateSecurityRealm(false, false, null); j.jenkins.setSecurityRealm(securityRealm);