From 9c8b57e635d5b2ff58ad1ebaa24c046b1c552e9f Mon Sep 17 00:00:00 2001
From: Chris Ridd <chris.ridd@forgerock.com>
Date: Mon, 23 Jun 2014 16:19:20 +0000
Subject: [PATCH] Fix OPENDJ-1497: pwdHistory stops updating when multiple storage schemes are used
---
opendj-sdk/opends/src/server/org/opends/server/core/PasswordPolicyState.java | 145 +++++++++++++++--------
opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/core/PasswordPolicyTestCase.java | 201 ++++++++++++++++++++++++++++-----
2 files changed, 263 insertions(+), 83 deletions(-)
diff --git a/opendj-sdk/opends/src/server/org/opends/server/core/PasswordPolicyState.java b/opendj-sdk/opends/src/server/org/opends/server/core/PasswordPolicyState.java
index b42b52f..3592c94 100644
--- a/opendj-sdk/opends/src/server/org/opends/server/core/PasswordPolicyState.java
+++ b/opendj-sdk/opends/src/server/org/opends/server/core/PasswordPolicyState.java
@@ -22,7 +22,7 @@
*
*
* Copyright 2006-2010 Sun Microsystems, Inc.
- * Portions Copyright 2011-2013 ForgeRock AS
+ * Portions Copyright 2011-2014 ForgeRock AS
*/
package org.opends.server.core;
@@ -271,6 +271,58 @@
}
+ /**
+ * Get the broken-down components of the given password value.
+ *
+ * @param v The encoded password value to break down.
+ *
+ * @return An array of components.
+ */
+ private StringBuilder[] getPasswordComponents(AttributeValue v)
+ throws DirectoryException
+ {
+ if (passwordPolicy.isAuthPasswordSyntax())
+ {
+ return AuthPasswordSyntax.decodeAuthPassword(v.toString());
+ }
+ else
+ {
+ String[] userPwComponents = UserPasswordSyntax.decodeUserPassword(v.toString());
+ StringBuilder[] pwComponents = new StringBuilder[userPwComponents.length];
+ for (int i = 0; i < userPwComponents.length; ++i)
+ {
+ pwComponents[i] = new StringBuilder(userPwComponents[i]);
+ }
+ return pwComponents;
+ }
+ }
+
+
+
+ /**
+ * Get the password storage scheme used by a given password value.
+ *
+ * @param v The encoded password value to check.
+ *
+ * @return The scheme used by the password.
+ *
+ * @throws DirectoryException If the password could not be decoded.
+ */
+ private PasswordStorageScheme<?> getPasswordStorageScheme(AttributeValue v)
+ throws DirectoryException
+ {
+ if (passwordPolicy.isAuthPasswordSyntax())
+ {
+ StringBuilder[] pwComps = AuthPasswordSyntax.decodeAuthPassword(v.toString());
+ return DirectoryServer.getAuthPasswordStorageScheme(pwComps[0].toString());
+ }
+ else
+ {
+ String[] pwComps = UserPasswordSyntax.decodeUserPassword(v.toString());
+ return DirectoryServer.getPasswordStorageScheme(pwComps[0]);
+ }
+ }
+
/**
* {@inheritDoc}
@@ -2554,22 +2606,7 @@
{
try
{
- StringBuilder[] pwComponents;
- if (usesAuthPasswordSyntax)
- {
- pwComponents =
- AuthPasswordSyntax.decodeAuthPassword(v.getValue().toString());
- }
- else
- {
- String[] userPwComponents =
- UserPasswordSyntax.decodeUserPassword(v.getValue().toString());
- pwComponents = new StringBuilder[userPwComponents.length];
- for (int i = 0; i < userPwComponents.length; ++i)
- {
- pwComponents[i] = new StringBuilder(userPwComponents[i]);
- }
- }
+ StringBuilder[] pwComponents = getPasswordComponents(v);
String schemeName = pwComponents[0].toString();
PasswordStorageScheme<?> scheme = (usesAuthPasswordSyntax)
@@ -2647,22 +2684,7 @@
{
try
{
- StringBuilder[] pwComponents;
- if (usesAuthPasswordSyntax)
- {
- pwComponents =
- AuthPasswordSyntax.decodeAuthPassword(v.getValue().toString());
- }
- else
- {
- String[] userPwComponents =
- UserPasswordSyntax.decodeUserPassword(v.getValue().toString());
- pwComponents = new StringBuilder[userPwComponents.length];
- for (int i = 0; i < userPwComponents.length; ++i)
- {
- pwComponents[i] = new StringBuilder(userPwComponents[i]);
- }
- }
+ StringBuilder[] pwComponents = getPasswordComponents(v);
String schemeName = pwComponents[0].toString();
PasswordStorageScheme<?> scheme = (usesAuthPasswordSyntax)
@@ -2881,22 +2903,7 @@
try
{
- StringBuilder[] pwComponents;
- if (usesAuthPasswordSyntax)
- {
- pwComponents =
- AuthPasswordSyntax.decodeAuthPassword(v.getValue().toString());
- }
- else
- {
- String[] userPwComponents =
- UserPasswordSyntax.decodeUserPassword(v.getValue().toString());
- pwComponents = new StringBuilder[userPwComponents.length];
- for (int i = 0; i < userPwComponents.length; ++i)
- {
- pwComponents[i] = new StringBuilder(userPwComponents[i]);
- }
- }
+ StringBuilder[] pwComponents = getPasswordComponents(v);
String schemeName = pwComponents[0].toString();
PasswordStorageScheme<?> scheme = (usesAuthPasswordSyntax)
@@ -3081,7 +3088,6 @@
return false;
}
-
// Check to see if the provided password is equal to any of the current
// passwords. If so, then we'll consider it to be in the history.
if (passwordMatches(password))
@@ -3370,8 +3376,10 @@
/**
- * Updates the password history information for this user by adding all
- * current passwords to it.
+ * Updates the password history information for this user by adding one of
+ * the passwords to it. It will choose the first password encoded using a
+ * secure storage scheme, and will fall back to a password encoded using an
+ * insecure storage scheme if necessary.
*/
public void updatePasswordHistory()
{
@@ -3381,9 +3389,40 @@
{
for (Attribute a : attrList)
{
+ boolean usesAuthPasswordSyntax = passwordPolicy.isAuthPasswordSyntax();
+ String insecurePassword = null;
for (AttributeValue v : a)
{
- addPasswordToHistory(v.getValue().toString());
+ try
+ {
+ PasswordStorageScheme<?> scheme = getPasswordStorageScheme(v);
+
+ if (scheme.isStorageSchemeSecure())
+ {
+ addPasswordToHistory(v.getValue().toString());
+ insecurePassword = null;
+ // no need to check any more values for this attribute
+ break;
+ }
+ else if (insecurePassword == null)
+ {
+ insecurePassword = v.getValue().toString();
+ }
+ }
+ catch (DirectoryException e)
+ {
+ if (debugEnabled())
+ {
+ TRACER.debugInfo("Encoded password " + v.getValue().toString() +
+ " cannot be decoded and cannot be added to history.");
+ }
+ }
+ }
+ // If we get here we haven't found a password encoded securely, so we
+ // have to use one of the other values.
+ if (insecurePassword != null)
+ {
+ addPasswordToHistory(insecurePassword);
}
}
}
diff --git a/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/core/PasswordPolicyTestCase.java b/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/core/PasswordPolicyTestCase.java
index 08ef0c1..fbf3ecb 100644
--- a/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/core/PasswordPolicyTestCase.java
+++ b/opendj-sdk/opends/tests/unit-tests-testng/src/server/org/opends/server/core/PasswordPolicyTestCase.java
@@ -22,7 +22,7 @@
*
*
* Copyright 2006-2008 Sun Microsystems, Inc.
- * Portions Copyright 2011 ForgeRock AS
+ * Portions Copyright 2011-2014 ForgeRock AS
*/
package org.opends.server.core;
@@ -42,13 +42,12 @@
import org.opends.server.api.PasswordStorageScheme;
import org.opends.server.config.ConfigEntry;
import org.opends.server.config.ConfigException;
+import org.opends.server.schema.UserPasswordSyntax;
import org.opends.server.tools.LDAPModify;
-import org.opends.server.types.AttributeType;
-import org.opends.server.types.DN;
-import org.opends.server.types.Entry;
-import org.opends.server.types.InitializationException;
+import org.opends.server.types.*;
import org.opends.server.util.TimeThread;
+import static org.assertj.core.api.Assertions.*;
import static org.testng.Assert.*;
@@ -4277,6 +4276,170 @@
/**
+ * Sleep until we can be sure that the time thread has been updated.
+ *
+ * Otherwise, password changes can all appear to be in the same
+ * millisecond and really weird things start to happen because of the way
+ * that we handle conflicts in password history timestamps. In short, if
+ * the history is already at the maximum count and all the previous
+ * changes occurred in the same millisecond as the new change, then it's
+ * possible for a new change to come with a timestamp that is before the
+ * timestamps of all the existing values.
+ *
+ * @throws InterruptedException
+ */
+ private void advanceTimeThread() throws InterruptedException
+ {
+ long timeThreadCurrentTime = TimeThread.getTime();
+ while (timeThreadCurrentTime == TimeThread.getTime())
+ {
+ Thread.sleep(10);
+ }
+ }
+
+
+
+ /**
+ * Check there are 3 securely stored passwords in the entry's password history.
+ *
+ * @param dn The entry to check.
+ *
+ * @return The password history.
+ */
+ private Attribute checkPasswordHistory(String dn)
+ throws DirectoryException
+ {
+ Entry entry = DirectoryServer.getEntry(DN.decode(dn));
+ assertNotNull(entry);
+ AttributeType pwdHistory = DirectoryServer.getAttributeType("pwdhistory");
+ assertNotNull(pwdHistory);
+ Attribute historyAttr = entry.getExactAttribute(pwdHistory, null);
+ assertNotNull(historyAttr);
+ assertThat(historyAttr).hasSize(3);
+
+ for (AttributeValue v : historyAttr)
+ {
+ String[] history = v.getValue().toString().split("#");
+ assertEquals(history.length, 3);
+ String[] pwComps = UserPasswordSyntax.decodeUserPassword(history[2]);
+ PasswordStorageScheme<?> scheme = DirectoryServer.getPasswordStorageScheme(pwComps[0]);
+ assertTrue(scheme.isStorageSchemeSecure());
+ }
+
+ return historyAttr;
+ }
+
+
+
+ /**
+ * Use multiple storage schemes in the policy. Verify that history only
+ * contains one secure version of each password, is truncated correctly,
+ * and continues to change after reaching maximum.
+ *
+ * @throws Exception If an unexpected problem occurs.
+ */
+ @Test()
+ public void testHistoryWithMultipleSchemes()
+ throws Exception
+ {
+ TestCaseUtils.initializeTestBackend(true);
+ TestCaseUtils.dsconfig(
+ "set-password-policy-prop",
+ "--policy-name", "Default Password Policy",
+ "--set", "password-history-count:3",
+ "--set", "default-password-storage-scheme:Base64",
+ "--set", "default-password-storage-scheme:Salted SHA-256");
+
+ TestCaseUtils.addEntry(
+ "dn: uid=test.user,o=test",
+ "objectClass: top",
+ "objectClass: person",
+ "objectClass: organizationalPerson",
+ "objectClass: inetOrgPerson",
+ "uid: test.user",
+ "givenName: Test",
+ "sn: User",
+ "cn: Test User",
+ "userPassword: originalPassword",
+ "ds-privilege-name: bypass-acl");
+
+ try
+ {
+ // Change the password three times.
+ String newPWsPath = TestCaseUtils.createTempFile(
+ "dn: uid=test.user,o=test",
+ "changetype: modify",
+ "replace: userPassword",
+ "userPassword: newPassword1",
+ "",
+ "dn: uid=test.user,o=test",
+ "changetype: modify",
+ "replace: userPassword",
+ "userPassword: newPassword2",
+ "",
+ "dn: uid=test.user,o=test",
+ "changetype: modify",
+ "replace: userPassword",
+ "userPassword: newPassword3");
+
+ String[] args = new String[]
+ {
+ "-h", "127.0.0.1",
+ "-p", String.valueOf(TestCaseUtils.getServerLdapPort()),
+ "-D", "uid=test.user,o=test",
+ "-w", "originalPassword",
+ "-f", newPWsPath
+ };
+
+ assertEquals(LDAPModify.mainModify(args, false, System.out, System.err),
+ 0);
+
+
+ advanceTimeThread();
+
+
+ Attribute historyAfterThreeMods = checkPasswordHistory("uid=test.user,o=test");
+
+ newPWsPath = TestCaseUtils.createTempFile(
+ "dn: uid=test.user,o=test",
+ "changetype: modify",
+ "replace: userPassword",
+ "userPassword: newPassword4");
+
+ args = new String[]
+ {
+ "-h", "127.0.0.1",
+ "-p", String.valueOf(TestCaseUtils.getServerLdapPort()),
+ "-D", "uid=test.user,o=test",
+ "-w", "newPassword3",
+ "-f", newPWsPath
+ };
+
+ assertEquals(LDAPModify.mainModify(args, false, System.out, System.err),
+ 0);
+
+
+ advanceTimeThread();
+
+
+ Attribute historyAfterFourMods = checkPasswordHistory("uid=test.user,o=test");
+
+ // check history is now different (OPENDJ-1497)
+ assertFalse(historyAfterThreeMods.equals(historyAfterFourMods));
+
+ }
+ finally
+ {
+ TestCaseUtils.dsconfig(
+ "set-password-policy-prop",
+ "--policy-name", "Default Password Policy",
+ "--set", "default-password-storage-scheme:Salted SHA-1",
+ "--set", "password-history-count:0");
+ }
+ }
+
+
+ /**
* Tests the Directory Server's password history maintenance capabilities
* using only the password history count configuration option.
*
@@ -4362,19 +4525,7 @@
0);
- // Sleep until we can be sure that the time thread has been updated.
- // Otherwise, the password changes can all appear to be in the same
- // millisecond and really weird things start to happen because of the way
- // that we handle conflicts in password history timestamps. In short, if
- // the history is already at the maximum count and all the previous
- // changes occurred in the same millisecond as the new change, then it's
- // possible for a new change to come with a timestamp that is before the
- // timestamps of all the existing values.
- long timeThreadCurrentTime = TimeThread.getTime();
- while (timeThreadCurrentTime == TimeThread.getTime())
- {
- Thread.sleep(10);
- }
+ advanceTimeThread();
// Make sure that we still can't use the original password.
@@ -4417,12 +4568,7 @@
0);
- // Sleep again to ensure that the time thread is updated.
- timeThreadCurrentTime = TimeThread.getTime();
- while (timeThreadCurrentTime == TimeThread.getTime())
- {
- Thread.sleep(10);
- }
+ advanceTimeThread();
// Make sure that we can't use the second new password.
@@ -4445,12 +4591,7 @@
0);
- // Sleep again to ensure that the time thread is updated.
- timeThreadCurrentTime = TimeThread.getTime();
- while (timeThreadCurrentTime == TimeThread.getTime())
- {
- Thread.sleep(10);
- }
+ advanceTimeThread();
// Reduce the password history count from 3 to 2 and verify that we can
--
Gitblit v1.10.0