From fc75a679d9e8a8bef4e1d029659637b62b420ce0 Mon Sep 17 00:00:00 2001 From: Rob Winch Date: Thu, 3 Mar 2016 14:05:26 -0600 Subject: [PATCH] Polish SCryptPasswordEncoder * JKD8 Base64 -> Spring Security's Base64 to continue to support older JDKs * Spaces to tabs * Javadoc cleanup * Remove of @Override to compile in Eclipse Issue gh-3702 --- crypto/crypto.gradle | 3 +- .../crypto/scrypt/SCryptPasswordEncoder.java | 229 ++++++++++-------- .../scrypt/SCryptPasswordEncoderTests.java | 170 ++++++------- gradle/javaprojects.gradle | 3 +- 4 files changed, 214 insertions(+), 191 deletions(-) diff --git a/crypto/crypto.gradle b/crypto/crypto.gradle index 8ae1cac76f..1be2d58531 100644 --- a/crypto/crypto.gradle +++ b/crypto/crypto.gradle @@ -12,6 +12,5 @@ configure(project.tasks.withType(Test)) { } dependencies { - optional 'org.bouncycastle:bcprov-jdk15on:1.54' - testCompile 'org.assertj:assertj-core:3.3.0' + optional 'org.bouncycastle:bcprov-jdk15on:1.54' } \ No newline at end of file diff --git a/crypto/src/main/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoder.java b/crypto/src/main/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoder.java index 9c33234ce8..dd505cd0fe 100644 --- a/crypto/src/main/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoder.java +++ b/crypto/src/main/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoder.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -15,136 +15,159 @@ */ package org.springframework.security.crypto.scrypt; -import java.util.Base64; -import java.util.Base64.Decoder; -import java.util.Base64.Encoder; - import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.bouncycastle.crypto.generators.SCrypt; +import org.springframework.security.crypto.codec.Base64; import org.springframework.security.crypto.codec.Utf8; import org.springframework.security.crypto.keygen.BytesKeyGenerator; import org.springframework.security.crypto.keygen.KeyGenerators; import org.springframework.security.crypto.password.PasswordEncoder; -import org.bouncycastle.crypto.generators.SCrypt; /** - * Implementation of PasswordEncoder that uses the SCrypt hashing function. Clients - * can optionally supply a cpu cost parameter, a memory cost parameter and a parallelization parameter. - * + *

+ * Implementation of PasswordEncoder that uses the SCrypt hashing function. + * Clients can optionally supply a cpu cost parameter, a memory cost parameter + * and a parallelization parameter. + *

+ * + *

+ * A few + * warnings: + *

+ * + * + * * @author Shazin Sadakath + * @author Rob Winch * */ public class SCryptPasswordEncoder implements PasswordEncoder { - - private final Log logger = LogFactory.getLog(getClass()); - - private final int cpuCost; - - private final int memoryCost; - - private final int parallelization; - - private final int keyLength; - - private final BytesKeyGenerator saltGenerator; - - public SCryptPasswordEncoder() { - this(16384, 8, 1, 32, 64); - } - - /** - * @param cpu cost of the algorithm. must be power of 2 greater than 1 - * @param memory cost of the algorithm - * @param parallelization of the algorithm - * @param key length for the algorithm - * @param salt length - */ - public SCryptPasswordEncoder(int cpuCost, int memoryCost, int parallelization, int keyLength, int saltLength) { - if (cpuCost <= 1) { - throw new IllegalArgumentException("Cpu cost parameter must be > 1."); - } - if (memoryCost == 1 && cpuCost > 65536) { - throw new IllegalArgumentException("Cpu cost parameter must be > 1 and < 65536."); - } - if (memoryCost < 1) { - throw new IllegalArgumentException("Memory cost must be >= 1."); - } - int maxParallel = Integer.MAX_VALUE / (128 * memoryCost * 8); - if (parallelization < 1 || parallelization > maxParallel) { - throw new IllegalArgumentException("Parallelisation parameter p must be >= 1 and <= " + maxParallel - + " (based on block size r of " + memoryCost + ")"); - } - if (keyLength < 1 || keyLength > Integer.MAX_VALUE) { - throw new IllegalArgumentException("Key length must be >= 1 and <= "+Integer.MAX_VALUE); - } - if (saltLength < 1 || saltLength > Integer.MAX_VALUE) { - throw new IllegalArgumentException("Salt length must be >= 1 and <= "+Integer.MAX_VALUE); - } - - this.cpuCost = cpuCost; - this.memoryCost = memoryCost; - this.parallelization = parallelization; - this.keyLength = keyLength; - this.saltGenerator = KeyGenerators.secureRandom(saltLength); - } - @Override - public String encode(CharSequence rawPassword) { - return digest(rawPassword, saltGenerator.generateKey()); + private final Log logger = LogFactory.getLog(getClass()); + + private final int cpuCost; + + private final int memoryCost; + + private final int parallelization; + + private final int keyLength; + + private final BytesKeyGenerator saltGenerator; + + public SCryptPasswordEncoder() { + this(16384, 8, 1, 32, 64); + } + + /** + * Creates a new instance + * + * @param cpuCost cpu cost of the algorithm (as defined in scrypt this is N). must be power of 2 greater than 1. Default is currently 16,348 or 2^14) + * @param memoryCost memory cost of the algorithm (as defined in scrypt this is r) Default is currently 8. + * @param parallelization the parallelization of the algorithm (as defined in scrypt this is p) Default is currently 1. Note that the implementation does not currently take advantage of parallelization. + * @param key length for the algorithm (as defined in scrypt this is dkLen). The default is currently 32. + * @param salt length (as defined in scrypt this is the length of S). The default is currently 64. + */ + public SCryptPasswordEncoder(int cpuCost, int memoryCost, int parallelization, int keyLength, int saltLength) { + if (cpuCost <= 1) { + throw new IllegalArgumentException("Cpu cost parameter must be > 1."); + } + if (memoryCost == 1 && cpuCost > 65536) { + throw new IllegalArgumentException("Cpu cost parameter must be > 1 and < 65536."); + } + if (memoryCost < 1) { + throw new IllegalArgumentException("Memory cost must be >= 1."); + } + int maxParallel = Integer.MAX_VALUE / (128 * memoryCost * 8); + if (parallelization < 1 || parallelization > maxParallel) { + throw new IllegalArgumentException("Parallelisation parameter p must be >= 1 and <= " + maxParallel + + " (based on block size r of " + memoryCost + ")"); + } + if (keyLength < 1 || keyLength > Integer.MAX_VALUE) { + throw new IllegalArgumentException("Key length must be >= 1 and <= "+Integer.MAX_VALUE); + } + if (saltLength < 1 || saltLength > Integer.MAX_VALUE) { + throw new IllegalArgumentException("Salt length must be >= 1 and <= "+Integer.MAX_VALUE); + } + + this.cpuCost = cpuCost; + this.memoryCost = memoryCost; + this.parallelization = parallelization; + this.keyLength = keyLength; + this.saltGenerator = KeyGenerators.secureRandom(saltLength); + } + + public String encode(CharSequence rawPassword) { + return digest(rawPassword, saltGenerator.generateKey()); } - @Override public boolean matches(CharSequence rawPassword, String encodedPassword) { if(encodedPassword == null || encodedPassword.length() < keyLength) { - logger.warn("Empty encoded password"); - return false; + logger.warn("Empty encoded password"); + return false; } - return decodeAndCheckMatches(rawPassword, encodedPassword); - } - + return decodeAndCheckMatches(rawPassword, encodedPassword); + } + private boolean decodeAndCheckMatches(CharSequence rawPassword, String encodedPassword) { - String[] parts = encodedPassword.split("\\$"); + String[] parts = encodedPassword.split("\\$"); - if (parts.length != 4) { - return false; - } + if (parts.length != 4) { + return false; + } - Decoder decoder = Base64.getDecoder(); - long params = Long.parseLong(parts[1], 16); - byte[] salt = decoder.decode(parts[2]); - byte[] derived = decoder.decode(parts[3]); + long params = Long.parseLong(parts[1], 16); + byte[] salt = decodePart(parts[2]); + byte[] derived = decodePart(parts[3]); - int cpuCost = (int) Math.pow(2, params >> 16 & 0xffff); - int memoryCost = (int) params >> 8 & 0xff; - int parallelization = (int) params & 0xff; + int cpuCost = (int) Math.pow(2, params >> 16 & 0xffff); + int memoryCost = (int) params >> 8 & 0xff; + int parallelization = (int) params & 0xff; - byte[] generated = SCrypt.generate(Utf8.encode(rawPassword), salt, cpuCost, memoryCost, parallelization, keyLength); + byte[] generated = SCrypt.generate(Utf8.encode(rawPassword), salt, cpuCost, memoryCost, parallelization, keyLength); - if (derived.length != generated.length) { - return false; - } + if (derived.length != generated.length) { + return false; + } - int result = 0; - for (int i = 0; i < derived.length; i++) { - result |= derived[i] ^ generated[i]; - } - return result == 0; + int result = 0; + for (int i = 0; i < derived.length; i++) { + result |= derived[i] ^ generated[i]; + } + return result == 0; } - - private String digest(CharSequence rawPassword, byte[] salt) { - byte[] derived = SCrypt.generate(Utf8.encode(rawPassword), salt, cpuCost, memoryCost, parallelization, 32); - String params = Long.toString(((int) (Math.log(cpuCost) / Math.log(2)) << 16L) | memoryCost << 8 | parallelization, 16); - Encoder encoder = Base64.getEncoder(); - - StringBuilder sb = new StringBuilder((salt.length + derived.length) * 2); - sb.append("$").append(params).append('$'); - sb.append(encoder.encodeToString(salt)).append('$'); - sb.append(encoder.encodeToString(derived)); + private String digest(CharSequence rawPassword, byte[] salt) { + byte[] derived = SCrypt.generate(Utf8.encode(rawPassword), salt, cpuCost, memoryCost, parallelization, 32); - return sb.toString(); + String params = Long.toString(((int) (Math.log(cpuCost) / Math.log(2)) << 16L) | memoryCost << 8 | parallelization, 16); + + StringBuilder sb = new StringBuilder((salt.length + derived.length) * 2); + sb.append("$").append(params).append('$'); + sb.append(encodePart(salt)).append('$'); + sb.append(encodePart(derived)); + + return sb.toString(); } - -} + + private byte[] decodePart(String part) { + return Base64.decode(Utf8.encode(part)); + } + + private String encodePart(byte[] part) { + return Utf8.decode(Base64.encode(part)); + } +} \ No newline at end of file diff --git a/crypto/src/test/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoderTests.java b/crypto/src/test/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoderTests.java index 8e24afc455..e725175041 100644 --- a/crypto/src/test/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoderTests.java +++ b/crypto/src/test/java/org/springframework/security/crypto/scrypt/SCryptPasswordEncoderTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2016 the original author or authors. * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. @@ -25,96 +25,96 @@ import org.junit.Test; */ public class SCryptPasswordEncoderTests { - @Test - public void matches() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); - String result = encoder.encode("password"); - assertThat(result).isNotEqualTo("password"); - assertThat(encoder.matches("password", result)).isTrue(); - } + @Test + public void matches() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); + String result = encoder.encode("password"); + assertThat(result).isNotEqualTo("password"); + assertThat(encoder.matches("password", result)).isTrue(); + } - @Test - public void unicode() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); - String result = encoder.encode("passw\u9292rd"); - assertThat(encoder.matches("pass\u9292\u9292rd", result)).isFalse(); - assertThat(encoder.matches("passw\u9292rd", result)).isTrue(); - } + @Test + public void unicode() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); + String result = encoder.encode("passw\u9292rd"); + assertThat(encoder.matches("pass\u9292\u9292rd", result)).isFalse(); + assertThat(encoder.matches("passw\u9292rd", result)).isTrue(); + } - @Test - public void notMatches() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); - String result = encoder.encode("password"); - assertThat(encoder.matches("bogus", result)).isFalse(); - } - - @Test - public void customParameters() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(512, 8, 4, 32, 16); - String result = encoder.encode("password"); - assertThat(result).isNotEqualTo("password"); - assertThat(encoder.matches("password", result)).isTrue(); - } - - @Test - public void differentPasswordHashes() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); - String password = "secret"; - assertThat(encoder.encode(password)).isNotEqualTo(encoder.encode(password)); - } - - @Test - public void samePasswordWithDifferentParams() { - SCryptPasswordEncoder oldEncoder = new SCryptPasswordEncoder(512, 8, 4, 64, 16); - SCryptPasswordEncoder newEncoder = new SCryptPasswordEncoder(); + @Test + public void notMatches() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); + String result = encoder.encode("password"); + assertThat(encoder.matches("bogus", result)).isFalse(); + } - String password = "secret"; - String oldEncodedPassword = oldEncoder.encode(password); - assertThat(newEncoder.matches(password, oldEncodedPassword)).isTrue(); - } + @Test + public void customParameters() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(512, 8, 4, 32, 16); + String result = encoder.encode("password"); + assertThat(result).isNotEqualTo("password"); + assertThat(encoder.matches("password", result)).isTrue(); + } - @Test - public void doesntMatchNullEncodedValue() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); - assertThat(encoder.matches("password", null)).isFalse(); - } + @Test + public void differentPasswordHashes() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); + String password = "secret"; + assertThat(encoder.encode(password)).isNotEqualTo(encoder.encode(password)); + } - @Test - public void doesntMatchEmptyEncodedValue() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); - assertThat(encoder.matches("password", "")).isFalse(); - } + @Test + public void samePasswordWithDifferentParams() { + SCryptPasswordEncoder oldEncoder = new SCryptPasswordEncoder(512, 8, 4, 64, 16); + SCryptPasswordEncoder newEncoder = new SCryptPasswordEncoder(); - @Test - public void doesntMatchBogusEncodedValue() { - SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); - assertThat(encoder.matches("password", "012345678901234567890123456789")).isFalse(); - } - - @Test(expected = IllegalArgumentException.class) - public void invalidCpuCostParameter() { - new SCryptPasswordEncoder(Integer.MIN_VALUE, 16, 2, 32, 16); - } - - @Test(expected = IllegalArgumentException.class) - public void invalidMemoryCostParameter() { - new SCryptPasswordEncoder(2, Integer.MAX_VALUE, 2, 32, 16); - } - - @Test(expected = IllegalArgumentException.class) - public void invalidParallelizationParameter() { - new SCryptPasswordEncoder(2, 8, Integer.MAX_VALUE, 32, 16); - } - - @Test(expected = IllegalArgumentException.class) - public void invalidSaltLengthParameter() { - new SCryptPasswordEncoder(2, 8, 1, 16, -1); - } - - @Test(expected = IllegalArgumentException.class) - public void invalidKeyLengthParameter() { - new SCryptPasswordEncoder(2, 8, 1, -1, 16); - } + String password = "secret"; + String oldEncodedPassword = oldEncoder.encode(password); + assertThat(newEncoder.matches(password, oldEncodedPassword)).isTrue(); + } + + @Test + public void doesntMatchNullEncodedValue() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); + assertThat(encoder.matches("password", null)).isFalse(); + } + + @Test + public void doesntMatchEmptyEncodedValue() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); + assertThat(encoder.matches("password", "")).isFalse(); + } + + @Test + public void doesntMatchBogusEncodedValue() { + SCryptPasswordEncoder encoder = new SCryptPasswordEncoder(); + assertThat(encoder.matches("password", "012345678901234567890123456789")).isFalse(); + } + + @Test(expected = IllegalArgumentException.class) + public void invalidCpuCostParameter() { + new SCryptPasswordEncoder(Integer.MIN_VALUE, 16, 2, 32, 16); + } + + @Test(expected = IllegalArgumentException.class) + public void invalidMemoryCostParameter() { + new SCryptPasswordEncoder(2, Integer.MAX_VALUE, 2, 32, 16); + } + + @Test(expected = IllegalArgumentException.class) + public void invalidParallelizationParameter() { + new SCryptPasswordEncoder(2, 8, Integer.MAX_VALUE, 32, 16); + } + + @Test(expected = IllegalArgumentException.class) + public void invalidSaltLengthParameter() { + new SCryptPasswordEncoder(2, 8, 1, 16, -1); + } + + @Test(expected = IllegalArgumentException.class) + public void invalidKeyLengthParameter() { + new SCryptPasswordEncoder(2, 8, 1, -1, 16); + } } diff --git a/gradle/javaprojects.gradle b/gradle/javaprojects.gradle index d667d87cd9..6445c28fbc 100644 --- a/gradle/javaprojects.gradle +++ b/gradle/javaprojects.gradle @@ -151,7 +151,8 @@ dependencies { testCompile "junit:junit:$junitVersion", 'org.mockito:mockito-core:1.10.19', "org.springframework:spring-test:$springVersion", - 'org.easytesting:fest-assert:1.4' + 'org.easytesting:fest-assert:1.4', + "org.assertj:assertj-core:2.3.0" // Use slf4j/logback for logging testRuntime "org.slf4j:jcl-over-slf4j:$slf4jVersion",