Adding password salting 23/3723/4
authorAlessandro Boch <aboch@cisco.com>
Fri, 13 Dec 2013 20:58:54 +0000 (12:58 -0800)
committerAlessandro Boch <aboch@cisco.com>
Mon, 16 Dec 2013 17:05:53 +0000 (09:05 -0800)
- When A UserConfig object is created, generate its 64 bit long salt
  and use it to salt the clear text password before hashing.

Change-Id: I2ddc5ffa061cdef59d01caa6d98cb6eb4b7be014
Signed-off-by: Alessandro Boch <aboch@cisco.com>
opendaylight/usermanager/api/src/main/java/org/opendaylight/controller/usermanager/UserConfig.java
opendaylight/usermanager/api/src/test/java/org/opendaylight/controller/usermanager/AuthorizationUserConfigTest.java

index 0c14dea..83532a1 100644 (file)
@@ -9,10 +9,11 @@
 package org.opendaylight.controller.usermanager;
 
 import java.io.Serializable;
-import java.nio.charset.Charset;
 import java.security.MessageDigest;
 import java.security.NoSuchAlgorithmException;
+import java.security.SecureRandom;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.Iterator;
 import java.util.List;
 import java.util.regex.Matcher;
@@ -24,6 +25,7 @@ import javax.xml.bind.annotation.XmlElement;
 import javax.xml.bind.annotation.XmlRootElement;
 
 import org.opendaylight.controller.sal.authorization.AuthResultEnum;
+import org.opendaylight.controller.sal.packet.BitBufferHelper;
 import org.opendaylight.controller.sal.utils.HexEncode;
 import org.opendaylight.controller.sal.utils.Status;
 import org.opendaylight.controller.sal.utils.StatusCode;
@@ -46,6 +48,7 @@ public class UserConfig implements Serializable {
     protected static final String PASSWORD_REGEX = "(?=.*[^a-zA-Z0-9])(?=.*\\d)(?=.*[a-z])(?=.*[A-Z]).{8,256}$";
     private static final Pattern INVALID_USERNAME_CHARACTERS = Pattern.compile("([/\\s\\.\\?#%;\\\\]+)");
     private static MessageDigest oneWayFunction;
+    private static SecureRandom randomGenerator;
 
     static {
         try {
@@ -54,6 +57,7 @@ public class UserConfig implements Serializable {
             log.error(String.format("Implementation of %s digest algorithm not found: %s", DIGEST_ALGORITHM,
                     e.getMessage()));
         }
+        UserConfig.randomGenerator = new SecureRandom(BitBufferHelper.toByteArray(System.currentTimeMillis()));
     }
 
     /**
@@ -81,6 +85,8 @@ public class UserConfig implements Serializable {
     @XmlElement
     private String password;
 
+    private byte[] salt;
+
 
 
     public UserConfig() {
@@ -102,11 +108,19 @@ public class UserConfig implements Serializable {
         /*
          * Password validation to be done on clear text password. If fails, mark
          * the password with a well known label, so that object validation can
-         * report the proper error. Only if password is a valid one, hash it.
+         * report the proper error. Only if password is a valid one, generate
+         * salt, concatenate it with clear text password and hash the
+         * resulting string. Hash result is going to be our stored password.
          */
-        this.password = (validatePassword(password).isSuccess()) ? hash(password) : BAD_PASSWORD;
+        if (validateClearTextPassword(password).isSuccess()) {
+            this.salt = BitBufferHelper.toByteArray(randomGenerator.nextLong());
+            this.password = hash(salt, password);
+        } else {
+            this.salt = null;
+            this.password = BAD_PASSWORD;
+        }
 
-        this.roles = (roles == null) ? new ArrayList<String>() : new ArrayList<String>(roles);
+        this.roles = (roles == null) ? Collections.<String>emptyList() : new ArrayList<String>(roles);
     }
 
     public String getUser() {
@@ -176,6 +190,7 @@ public class UserConfig implements Serializable {
     public Status validate() {
         Status validCheck = validateUsername();
         if (validCheck.isSuccess()) {
+            // Password validation was run at object construction time
             validCheck = (!password.equals(BAD_PASSWORD)) ? new Status(StatusCode.SUCCESS) : new Status(
                     StatusCode.BADREQUEST,
                     "Password should be 8 to 256 characters long, contain both upper and lower case letters, "
@@ -203,7 +218,7 @@ public class UserConfig implements Serializable {
         return new Status(StatusCode.SUCCESS);
     }
 
-    private Status validatePassword(String password) {
+    private Status validateClearTextPassword(String password) {
         if (password == null || password.isEmpty()) {
             return new Status(StatusCode.BADREQUEST, "Password cannot be empty");
         }
@@ -227,14 +242,14 @@ public class UserConfig implements Serializable {
 
         // To make any changes to a user configured profile, current password
         // must always be provided
-        if (!this.password.equals(hash(currentPassword))) {
+        if (!this.password.equals(hash(this.salt, currentPassword))) {
             return new Status(StatusCode.BADREQUEST, "Current password is incorrect");
         }
 
         // Create a new object with the proposed modifications
         UserConfig proposed = new UserConfig();
         proposed.user = this.user;
-        proposed.password = (newPassword == null)? this.password : hash(newPassword);
+        proposed.password = (newPassword == null)? this.password : hash(this.salt, newPassword);
         proposed.roles = (newRoles == null)? this.roles : newRoles;
 
         // Validate it
@@ -251,9 +266,9 @@ public class UserConfig implements Serializable {
         return status;
     }
 
-    public AuthResponse authenticate(String clearTextPass) {
+    public AuthResponse authenticate(String clearTextPassword) {
         AuthResponse locResponse = new AuthResponse();
-        if (password.equals(hash(clearTextPass))) {
+        if (password.equals(hash(this.salt, clearTextPassword))) {
             locResponse.setStatus(AuthResultEnum.AUTH_ACCEPT_LOC);
             locResponse.addData(getRolesString());
         } else {
@@ -275,12 +290,32 @@ public class UserConfig implements Serializable {
         return buffer.toString();
     }
 
-    public static String hash(String message) {
+    private static byte[] concatenate(byte[] salt, String password) {
+        byte[] messageArray = password.getBytes();
+        byte[] concatenation = new byte[salt.length + password.length()];
+        System.arraycopy(salt, 0, concatenation, 0, salt.length);
+        System.arraycopy(messageArray, 0, concatenation, salt.length, messageArray.length);
+        return concatenation;
+    }
+
+    private static String hash(byte[] salt, String message) {
         if (message == null) {
+            log.warn("Password hash requested but empty or no password provided");
             return message;
         }
+        if (salt == null || salt.length == 0) {
+            log.warn("Password hash requested but empty or no salt provided");
+            return message;
+        }
+
+        // Concatenate salt and password
+        byte[] messageArray = message.getBytes();
+        byte[] concatenation = new byte[salt.length + message.length()];
+        System.arraycopy(salt, 0, concatenation, 0, salt.length);
+        System.arraycopy(messageArray, 0, concatenation, salt.length, messageArray.length);
+
         UserConfig.oneWayFunction.reset();
-        return HexEncode.bytesToHexString(UserConfig.oneWayFunction.digest(message.getBytes(Charset.defaultCharset())));
+        return HexEncode.bytesToHexString(UserConfig.oneWayFunction.digest(concatenate(salt, message)));
     }
 
     /**
@@ -299,7 +334,8 @@ public class UserConfig implements Serializable {
     public static UserConfig getUncheckedUserConfig(String userName, String password, List<String> roles) {
         UserConfig config = new UserConfig();
         config.user = userName;
-        config.password = hash(password);
+        config.salt = BitBufferHelper.toByteArray(randomGenerator.nextLong());
+        config.password = hash(config.salt, password);
         config.roles = roles;
         return config;
     }
index d225e5c..8fc7f07 100644 (file)
@@ -7,7 +7,6 @@
  */
 package org.opendaylight.controller.usermanager;
 
-import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
@@ -71,12 +70,12 @@ public class AuthorizationUserConfigTest {
                 .isSuccess());
 
         // New Password = null, No change in password
-        assertTrue(userConfig.getPassword().equals(UserConfig.hash("ciscocisco")));
+        assertTrue(userConfig.authenticate("ciscocisco").getStatus().equals(AuthResultEnum.AUTH_ACCEPT_LOC));
 
         // Password changed successfully, no change in user role
         assertTrue(userConfig.update("ciscocisco", "cisco123", roles)
                 .isSuccess());
-        assertTrue(userConfig.getPassword().equals(UserConfig.hash("cisco123")));
+        assertTrue(userConfig.authenticate("cisco123").getStatus().equals(AuthResultEnum.AUTH_ACCEPT_LOC));
         assertTrue(userConfig.getRoles().get(0).equals(
                 UserLevel.NETWORKOPERATOR.toString()));
 
@@ -85,14 +84,14 @@ public class AuthorizationUserConfigTest {
         roles.add(UserLevel.SYSTEMADMIN.toString());
         assertTrue(userConfig.update("cisco123", "cisco123", roles)
                 .isSuccess());
-        assertTrue(userConfig.getPassword().equals(UserConfig.hash("cisco123")));
+        assertTrue(userConfig.authenticate("cisco123").getStatus().equals(AuthResultEnum.AUTH_ACCEPT_LOC));
         assertTrue(userConfig.getRoles().get(0)
                 .equals(UserLevel.SYSTEMADMIN.toString()));
 
         // Password and role changed successfully
         assertTrue(userConfig.update("cisco123", "ciscocisco", roles)
                 .isSuccess());
-        assertTrue(userConfig.getPassword().equals(UserConfig.hash("ciscocisco")));
+        assertTrue(userConfig.authenticate("ciscocisco").getStatus().equals(AuthResultEnum.AUTH_ACCEPT_LOC));
         assertTrue(userConfig.getRoles().get(0)
                 .equals(UserLevel.SYSTEMADMIN.toString()));
 
@@ -104,14 +103,6 @@ public class AuthorizationUserConfigTest {
         assertTrue(authresp.getStatus().equals(AuthResultEnum.AUTH_ACCEPT_LOC));
         authresp = userConfig.authenticate("wrongPassword");
         assertTrue(authresp.getStatus().equals(AuthResultEnum.AUTH_REJECT_LOC));
-
-        // test equals()
-        roles.clear();
-        roles.add(UserLevel.NETWORKOPERATOR.toString());
-        userConfig = new UserConfig("uname", "ciscocisco", roles);
-        assertEquals(userConfig, userConfig);
-        UserConfig userConfig2 = new UserConfig("uname", "ciscocisco", roles);
-        assertEquals(userConfig, userConfig2);
     }
 
     @Test