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

@@ -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'
 }

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.
- * 
+ * <p>
+ * 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.
+ *</p>
+ *
+ * <p>
+ * A few <a href=
+ * "http://bouncy-castle.1462172.n4.nabble.com/Java-Bouncy-Castle-scrypt-implementation-td4656832.html">
+ * warnings</a>:
+ * </p>
+ *
+ * <ul>
+ * <li>The currently implementation uses Bouncy castle which does not exploit
+ * parallelism/optimizations that password crackers will, so there is an
+ * unnecessary asymmetry between attacker and defender.</li>
+ * <li>Scrypt is based on Salsa20 which performs poorly in Java (on par with
+ * AES) but performs awesome (~4-5x faster) on SIMD capable platforms</li>
+ * <li>While there are some that would disagree, consider reading "<a href="
+ * http://blog.ircmaxell.com/2014/03/why-i-dont-recommend-scrypt.html
+ * ">Why I Don't Recommend Scrypt</a> (for password storage)"</li>
+ * </ul>
+ *
  * @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;
-        }
+		long params = Long.parseLong(parts[1], 16);
+		byte[] salt = decodePart(parts[2]);
+		byte[] derived = decodePart(parts[3]);
 
-        Decoder decoder = Base64.getDecoder();
-        long params = Long.parseLong(parts[1], 16);        
-        byte[] salt = decoder.decode(parts[2]);
-        byte[] derived = decoder.decode(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;
+		}
+
+		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);
 
-        if (derived.length != generated.length) {
-            return false;
-        }
+		String params = Long.toString(((int) (Math.log(cpuCost) / Math.log(2)) << 16L) | memoryCost << 8 | parallelization, 16);
 
-        int result = 0;
-        for (int i = 0; i < derived.length; i++) {
-            result |= derived[i] ^ generated[i];
-        }
-        return result == 0;
+		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 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));
-
-        return sb.toString();  
+
+	private String encodePart(byte[] part) {
+		return Utf8.decode(Base64.encode(part));
 	}
-	
-}
+}

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 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();
-
-        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);     
-    }
+	@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 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();
+
+		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);
+	}
 
 }
 

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",