revert application secret generation, encryption uses first bytes of …

…the secret
ninjaframework · Mar 4, 2015 · 92f4824 · 92f4824
1 parent 33b4861
commit 92f4824
Show file tree

Hide file tree

Showing 7 changed files with 105 additions and 54 deletions.
diff --git a/ninja-core/src/main/java/ninja/utils/CookieEncryption.java b/ninja-core/src/main/java/ninja/utils/CookieEncryption.java
@@ -17,6 +17,7 @@
 
 import java.nio.charset.StandardCharsets;
 import java.security.GeneralSecurityException;
+import java.security.InvalidKeyException;
 import java.util.Objects;
 
 import javax.crypto.Cipher;
@@ -37,6 +38,8 @@
 @Singleton
 public class CookieEncryption {
 
+    public static final String ALGORITHM = "AES";
+
     private static final Logger LOGGER = LoggerFactory.getLogger(CookieEncryption.class);
 
     private SecretKey secretKey;
@@ -47,7 +50,17 @@ public CookieEncryption(NinjaProperties properties) {
         if (properties.getBooleanWithDefault(NinjaConstant.applicationCookieEncrypted, false)) {
 
             String secret = properties.getOrDie(NinjaConstant.applicationSecret);
-            this.secretKey = new SecretKeySpec(Base64.decodeBase64(secret), SecretGenerator.ALGORITHM);
+            try {
+                int maxKeyLenghtBits = Cipher.getMaxAllowedKeyLength(ALGORITHM);
+                if (maxKeyLenghtBits == Integer.MAX_VALUE) {
+                    maxKeyLenghtBits = 256;
+                }
+
+                this.secretKey = new SecretKeySpec(secret.getBytes(), 0, maxKeyLenghtBits / Byte.SIZE, ALGORITHM);
+
+            } catch (Exception ex) {
+                LOGGER.warn("Encryption secret not created. Sessions cookies will not be encrypted.", ex);
+            }
 
         }
 
@@ -70,17 +83,21 @@ public String encrypt(String data) {
 
         try {
             // encrypt data
-            Cipher cipher = Cipher.getInstance(SecretGenerator.ALGORITHM);
+            Cipher cipher = Cipher.getInstance(ALGORITHM);
             cipher.init(Cipher.ENCRYPT_MODE, secretKey);
             byte[] encrypted = cipher.doFinal(data.getBytes(StandardCharsets.UTF_8));
 
             // convert encrypted bytes to string in base64
             return Base64.encodeBase64URLSafeString(encrypted);
 
+        } catch (InvalidKeyException ex) {
+            LOGGER.error(getHelperLogMessage());
+            LOGGER.debug("", ex);
         } catch (GeneralSecurityException ex) {
-            LOGGER.error("Failed to encrypt data. {}", getHelperLogMessage(), ex);
+            LOGGER.error("Failed to encrypt data. {}", ex.getMessage());
+            LOGGER.debug("", ex);
         }
-        return data;
+        return "";
     }
 
     /**
@@ -102,23 +119,26 @@ public String decrypt(String data) {
         byte[] decoded = Base64.decodeBase64(data);
         try {
             // decrypt bytes
-            Cipher cipher = Cipher.getInstance(SecretGenerator.ALGORITHM);
+            Cipher cipher = Cipher.getInstance(ALGORITHM);
             cipher.init(Cipher.DECRYPT_MODE, secretKey);
             byte[] decrypted = cipher.doFinal(decoded);
 
             // convert bytes to string
             return new String(decrypted, StandardCharsets.UTF_8);
 
+        } catch (InvalidKeyException ex) {
+            LOGGER.error(getHelperLogMessage());
+            LOGGER.debug("", ex);
         } catch (GeneralSecurityException ex) {
-            LOGGER.error("Failed to decrypt data. {}", getHelperLogMessage(), ex);
+            LOGGER.error("Failed to decrypt data. {}", ex.getMessage());
+            LOGGER.debug("", ex);
         }
-        return data;
+        return "";
     }
 
     private String getHelperLogMessage() {
         StringBuilder sb = new StringBuilder();
-        sb.append("Check if application secret is a valid base64 encoded ");
+        sb.append("Invalid key provided. Check if application secret is properly set.").append(System.lineSeparator());
-        sb.append(SecretGenerator.ALGORITHM).append(" key.").append(System.lineSeparator());
         sb.append("You can remove '").append(NinjaConstant.applicationSecret).append("' key in configuration file ");
         sb.append("and restart application. Ninja will generate new key for you.");
         return sb.toString();

diff --git a/ninja-core/src/main/java/ninja/utils/NinjaPropertiesImplTool.java b/ninja-core/src/main/java/ninja/utils/NinjaPropertiesImplTool.java
@@ -19,7 +19,6 @@
 import java.io.File;
 import java.io.IOException;
 
-import org.apache.commons.codec.binary.Base64;
 import org.apache.commons.configuration.Configuration;
 import org.apache.commons.configuration.ConfigurationException;
 import org.apache.commons.configuration.PropertiesConfiguration;
@@ -52,18 +51,18 @@ public static void checkThatApplicationSecretIsSet(
         String applicationSecret = compositeConfiguration.getString(NinjaConstant.applicationSecret);
 
         if (applicationSecret == null
-                || applicationSecret.isEmpty() || !Base64.isBase64(applicationSecret)) {
+                || applicationSecret.isEmpty()) {
 
             // If in production we stop the startup process. It simply does not make
             // sense to run in production if the secret is not set.
             if (isProd) {
-                String errorMessage = "Fatal error. Key application.secret not set or invalid. Please fix that.";
+                String errorMessage = "Fatal error. Key application.secret not set. Please fix that.";
                 logger.error(errorMessage);
                 throw new RuntimeException(errorMessage);
             }
-
 
-            logger.info("Key application.secret not set or invalid. Generating new one and setting in conf/application.conf.");
+
+            logger.info("Key application.secret not set. Generating new one and setting in conf/application.conf.");
 
             // generate new secret
             String secret = SecretGenerator.generateSecret();
@@ -97,6 +96,6 @@ public static void checkThatApplicationSecretIsSet(
         }
 
     }
-
+    
 
 }
diff --git a/ninja-core/src/main/java/ninja/utils/SecretGenerator.java b/ninja-core/src/main/java/ninja/utils/SecretGenerator.java
@@ -16,35 +16,50 @@
 
 package ninja.utils;
 
-import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
-
+import java.util.Random;
-import javax.crypto.KeyGenerator;
-import javax.crypto.SecretKey;
-
-import org.apache.commons.codec.binary.Base64;
 
 
 public class SecretGenerator {
-
+
-    public static final String ALGORITHM = "AES";
-
     /**
-     * Generates secret key encoded in base64. This string is suitable as secret for your application (key
+     * Generates a random String of length 64. This string is suitable
-     * "application.secret" in conf/application.conf).
+     * as secret for your application (key "application.secret" in conf/application.conf).
-     *
+     * 
      * @return A string that can be used as "application.secret".
      */
     public static String generateSecret() {
-        try {
+
-            KeyGenerator keyGenerator = KeyGenerator.getInstance(ALGORITHM);
+        return generateSecret(new SecureRandom());
-            keyGenerator.init(128);
-            SecretKey key = keyGenerator.generateKey();
-
-            return Base64.encodeBase64String(key.getEncoded());
 
-        } catch (NoSuchAlgorithmException ex) {
+    }
-            throw new RuntimeException("Failed to generate application secret", ex);
+
+    /**
+     * !!!! Only for testing purposes !!!!
+     * 
+     * Usually you want to use {@link SecretGenerator#generateSecret()}
+     * 
+     * @param random the random generator to use. Usually new Random(), but for testing you can
+     *          use a predefined seed.
+     * @return A String suitable as random secret for eg signing a session.
+     */
+    protected static String generateSecret(Random random) {
+
+        String charsetForSecret = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";
+
+        StringBuilder stringBuilder = new StringBuilder(64);
+
+        for (int i = 0; i < 64; i++) {
+
+            int charToPoPickFromCharset = random.nextInt(charsetForSecret.length());            
+            stringBuilder.append(charsetForSecret.charAt(charToPoPickFromCharset));
+
         }
-    }    
+
+        return stringBuilder.toString(); 
+
+    }
+
+
 
 }
diff --git a/ninja-core/src/test/java/ninja/utils/NinjaPropertiesImplTest.java b/ninja-core/src/test/java/ninja/utils/NinjaPropertiesImplTest.java
@@ -17,18 +17,30 @@
 package ninja.utils;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertTrue;
+import static org.junit.Assert.fail;
 
+import java.io.File;
+
+import com.google.inject.Inject;
 import javax.inject.Named;
 
-import org.apache.commons.codec.binary.Base64;
+import org.apache.commons.configuration.PropertiesConfiguration;
 import org.junit.After;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
+import com.google.common.base.Charsets;
+import com.google.common.io.Files;
 import com.google.inject.AbstractModule;
 import com.google.inject.Guice;
-import com.google.inject.Inject;
+import org.hamcrest.CoreMatchers;
+import static org.hamcrest.CoreMatchers.equalTo;
+import org.junit.Assert;
+import static org.junit.Assert.assertThat;
 
 public class NinjaPropertiesImplTest {
 
@@ -51,16 +63,16 @@ public void testSkippingThroughModesWorks() {
         ninjaPropertiesImpl = new NinjaPropertiesImpl(NinjaMode.dev);
         assertEquals("dev_testproperty",
                 ninjaPropertiesImpl.get("testproperty"));
-        assertTrue(Base64.isBase64(
+        assertEquals("secret",
-                ninjaPropertiesImpl.get(NinjaConstant.applicationSecret)));
+                ninjaPropertiesImpl.get(NinjaConstant.applicationSecret));
 
         // and in a completely different mode with no "%"-prefixed key the
         // default value use used
         ninjaPropertiesImpl = new NinjaPropertiesImpl(NinjaMode.prod);
         assertEquals("testproperty_without_prefix",
                 ninjaPropertiesImpl.get("testproperty"));
-        assertTrue(Base64.isBase64(
+        assertEquals("secret",
-                ninjaPropertiesImpl.get(NinjaConstant.applicationSecret)));
+                ninjaPropertiesImpl.get(NinjaConstant.applicationSecret));
 
         // tear down
         System.clearProperty(NinjaConstant.MODE_KEY_NAME);
@@ -138,7 +150,7 @@ protected void configure() {
         }).getInstance(MockService.class);
         assertNotNull("Application secret not set by Guice",
                 service.applicationSecret);
-        assertTrue(Base64.isBase64(service.applicationSecret));
+        assertEquals("secret", service.applicationSecret);
     }
 
     public static class MockService {

diff --git a/ninja-core/src/test/java/ninja/utils/NinjaPropertiesImplToolTest.java b/ninja-core/src/test/java/ninja/utils/NinjaPropertiesImplToolTest.java
@@ -22,7 +22,6 @@
 import java.io.File;
 import java.util.UUID;
 
-import org.apache.commons.codec.binary.Base64;
 import org.apache.commons.configuration.Configuration;
 import org.apache.commons.configuration.PropertiesConfiguration;
 import org.apache.commons.io.FileUtils;
@@ -73,8 +72,10 @@ public void testMissingSecretCreatesNewOneInDevMode() throws Exception {
                 baseDirWithoutTrailingSlash, defaultConfiguration,
                 compositeConfiguration);
 
-        assertTrue(Base64.isBase64(compositeConfiguration.getString(NinjaConstant.applicationSecret)));
+        assertTrue(compositeConfiguration.getString(
-        assertTrue(Base64.isBase64(defaultConfiguration.getString(NinjaConstant.applicationSecret)));
+                NinjaConstant.applicationSecret).length() == 64);
+        assertTrue(defaultConfiguration.getString(
+                NinjaConstant.applicationSecret).length() == 64);
 
         assertTrue(Files.toString(new File(devConf), Charsets.UTF_8).contains(
                 NinjaConstant.applicationSecret));

diff --git a/ninja-core/src/test/java/ninja/utils/SecretGeneratorTest.java b/ninja-core/src/test/java/ninja/utils/SecretGeneratorTest.java
@@ -16,21 +16,25 @@
 
 package ninja.utils;
 
-import org.apache.commons.codec.binary.Base64;
+import static org.junit.Assert.assertEquals;
-import org.junit.Assert;
+
+import java.util.Random;
+
 import org.junit.Test;
 
 public class SecretGeneratorTest {
 
     @Test
     public void testGenerateSecret() {
 
-        String secret = SecretGenerator.generateSecret();
+        assertEquals("5EJYQbXUb81LhuSoNO5l4eh2ZrNPoUBzZaGNixcPOFUsKzRkpTOeu9sm8CGUKaXZ",
-
+                SecretGenerator.generateSecret(new Random(323232L)));
-        // ensure we have value and it is in base64
-        Assert.assertNotNull(secret);
-        Assert.assertTrue(Base64.isBase64(secret));
 
+        assertEquals("oC8rHI6rDAiYSgMKHP6b4NlWG8UDdo5ALy66t3h2A5mhwWIBGjdyeFDBCoUn8Cov",
+                SecretGenerator.generateSecret(new Random(2L)));
+
+        assertEquals("0C27oI94jXZkXyB0ID8ZPq1zinxNmrenSwItFwRXphCKOC6ZwGTFX3nYZsYKafxw",
+                SecretGenerator.generateSecret(new Random(3L)));
     }
 
 }
diff --git a/ninja-core/src/test/resources/conf/application.conf b/ninja-core/src/test/resources/conf/application.conf
@@ -44,4 +44,4 @@ getMultipleElementStringArrayWithoutSpaces=one,me
 
 # make sure that the constants are okay in NinjaConstant.*
 application.session.transferred_over_https_only=false
-application.secret = 
+application.secret = secret