Merge pull request #1058 from greenqloud/pr/password_security

Shuffling the password to avoid having a subset of characters in fixed positions.Related to CLOUDSTACK-9052.

I am shuffling the characters in the password, to avoid having a certain char type in fixed positions. I modified the tests accordingly to only check that the different character types are present.

I think it would be good to remove the hard requirement to have at least one of digits, upper-case, and  lowercase chars, as it reduces the number of possible combinations passwords can take. What do you think?

* pr/1058:
  CLOUDSTACK-9052 Shuffling the password to avoid having a subset of characters in fixed positions.

Signed-off-by: Remi Bergsma <github@remi.nl>

utils/src/main/java/com/cloud/utils/PasswordGenerator.java

@@ -20,6 +20,9 @@
 package com.cloud.utils;
 
 import java.security.SecureRandom;
+import java.util.ArrayList;
+import java.util.Collections;
+import java.util.List;
 import java.util.Random;
 
 /**
@@ -48,14 +51,19 @@ public class PasswordGenerator {
                 password.append(generateAlphaNumeric(r));
             }
         } else {
-            // Generate random 3-character string with a lowercase character,
-            // uppercase character, and a digit
-            password.append(generateLowercaseChar(r)).append(generateUppercaseChar(r)).append(generateDigit(r));
+            List<Character> passwordChars = new ArrayList<Character>();
+            passwordChars.add(generateLowercaseChar(r));
+            passwordChars.add(generateUppercaseChar(r));
+            passwordChars.add(generateDigit(r));
 
-            // Generate a random n-character string with only lowercase
-            // characters
-            for (int i = 0; i < num - 3; i++) {
-                password.append(generateLowercaseChar(r));
+            for (int i = passwordChars.size(); i < num; i++) {
+                passwordChars.add(generateAlphaNumeric(r));
+            }
+
+            Collections.shuffle(passwordChars, new SecureRandom());
+
+            for (char c : passwordChars) {
+                password.append(c);
             }
         }
 
@@ -87,4 +95,5 @@ public class PasswordGenerator {
         return psk.toString();
 
     }
+
 }

utils/src/test/java/com/cloud/utils/PasswordGeneratorTest.java

@@ -30,13 +30,36 @@ public class PasswordGeneratorTest {
         Assert.assertTrue(PasswordGenerator.generateRandomPassword(1).length() == 3);
         Assert.assertTrue(PasswordGenerator.generateRandomPassword(5).length() == 5);
         String password = PasswordGenerator.generateRandomPassword(8);
-        // TODO: this might give more help to bruteforcing than desired
-        // the actual behavior is that the first character is a random lowercase
-        // char
-        Assert.assertTrue(Character.isLowerCase(password.charAt(0)));
-        // the second character is a random upper case char
-        Assert.assertTrue(Character.isUpperCase(password.charAt(1)));
-        // and the third is a digit
-        Assert.assertTrue(Character.isDigit(password.charAt(2)));
+
+        Assert.assertTrue(containsDigit(password));
+        Assert.assertTrue(containsLowercase(password));
+        Assert.assertTrue(containsUppercase(password));
+    }
+
+    private boolean containsUppercase(String password) {
+        for (char c : password.toCharArray()) {
+            if (Character.isUpperCase(c)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private boolean containsLowercase(String password) {
+        for (char c : password.toCharArray()) {
+            if (Character.isLowerCase(c)) {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    private boolean containsDigit(String password) {
+        for (char c : password.toCharArray()) {
+            if (Character.isDigit(c)) {
+                return true;
+            }
+        }
+        return false;
     }
 }