Merge pull request #1058 from greenqloud/pr/password_security

remibergsma · remibergsma · commit 3f7a86d8efac · 2015-11-22T12:46:32.000+01:00
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 &lt;github@remi.nl&gt;
diff --git a/utils/src/main/java/com/cloud/utils/PasswordGenerator.java b/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 static String generateRandomPassword(int num) {
                 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));
-
-            // Generate a random n-character string with only lowercase
-            // characters
-            for (int i = 0; i < num - 3; i++) {
-                password.append(generateLowercaseChar(r));
+            List<Character> passwordChars = new ArrayList<Character>();
+            passwordChars.add(generateLowercaseChar(r));
+            passwordChars.add(generateUppercaseChar(r));
+            passwordChars.add(generateDigit(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 static String generatePresharedKey(int numChars) {
         return psk.toString();
 
     }
+
 }
diff --git a/utils/src/test/java/com/cloud/utils/PasswordGeneratorTest.java b/utils/src/test/java/com/cloud/utils/PasswordGeneratorTest.java
@@ -30,13 +30,36 @@ public void generateRandomPassword() {
         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;
     }
 }
