syncope-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From ilgro...@apache.org
Subject [5/7] syncope git commit: [SYNCOPE-1337] Do not check password history by simple String comparison, use Encryptor#verify as authentication does
Date Fri, 13 Jul 2018 14:58:11 GMT
[SYNCOPE-1337] Do not check password history by simple String comparison, use Encryptor#verify
as authentication does


Project: http://git-wip-us.apache.org/repos/asf/syncope/repo
Commit: http://git-wip-us.apache.org/repos/asf/syncope/commit/a4710ca6
Tree: http://git-wip-us.apache.org/repos/asf/syncope/tree/a4710ca6
Diff: http://git-wip-us.apache.org/repos/asf/syncope/diff/a4710ca6

Branch: refs/heads/2_0_X
Commit: a4710ca6f8ccc176539445491ef35de4c698f303
Parents: 902d09e
Author: Francesco Chicchiriccò <ilgrosso@apache.org>
Authored: Fri Jul 13 15:57:01 2018 +0200
Committer: Francesco Chicchiriccò <ilgrosso@apache.org>
Committed: Fri Jul 13 15:57:01 2018 +0200

----------------------------------------------------------------------
 .../core/persistence/api/entity/user/User.java  |  4 --
 .../core/persistence/jpa/dao/JPAUserDAO.java    | 18 ++++++--
 .../persistence/jpa/entity/user/JPAUser.java    | 25 -----------
 .../provisioning/api/data/UserDataBinder.java   |  2 -
 .../java/data/UserDataBinderImpl.java           | 12 +-----
 .../syncope/fit/core/UserIssuesITCase.java      | 45 ++++++++++++++++++++
 6 files changed, 61 insertions(+), 45 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/syncope/blob/a4710ca6/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java
----------------------------------------------------------------------
diff --git a/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java
b/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java
index d3259e7..90805da 100644
--- a/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java
+++ b/core/persistence-api/src/main/java/org/apache/syncope/core/persistence/api/entity/user/User.java
@@ -50,8 +50,6 @@ public interface User extends
 
     CipherAlgorithm getCipherAlgorithm();
 
-    void setCipherAlgorithm(CipherAlgorithm cipherAlgorithm);
-
     boolean canDecodePassword();
 
     String getClearPassword();
@@ -66,8 +64,6 @@ public interface User extends
 
     List<String> getPasswordHistory();
 
-    boolean verifyPasswordHistory(String password, int size);
-
     SecurityQuestion getSecurityQuestion();
 
     void setSecurityQuestion(SecurityQuestion securityQuestion);

http://git-wip-us.apache.org/repos/asf/syncope/blob/a4710ca6/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAUserDAO.java
----------------------------------------------------------------------
diff --git a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAUserDAO.java
b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAUserDAO.java
index f5bbd7f..b8c5839 100644
--- a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAUserDAO.java
+++ b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/dao/JPAUserDAO.java
@@ -75,6 +75,7 @@ import org.apache.syncope.core.persistence.jpa.entity.user.JPAUser;
 import org.apache.syncope.core.provisioning.api.event.AnyCreatedUpdatedEvent;
 import org.apache.syncope.core.provisioning.api.event.AnyDeletedEvent;
 import org.apache.syncope.core.provisioning.api.utils.EntityUtils;
+import org.apache.syncope.core.spring.security.Encryptor;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.beans.factory.support.AbstractBeanDefinition;
 import org.springframework.stereotype.Repository;
@@ -87,6 +88,8 @@ public class JPAUserDAO extends AbstractAnyDAO<User> implements UserDAO
{
     private static final Pattern USERNAME_PATTERN =
             Pattern.compile("^" + SyncopeConstants.NAME_PATTERN, Pattern.CASE_INSENSITIVE
| Pattern.UNICODE_CASE);
 
+    private static final Encryptor ENCRYPTOR = Encryptor.getInstance();
+
     @Autowired
     private RoleDAO roleDAO;
 
@@ -339,7 +342,17 @@ public class JPAUserDAO extends AbstractAnyDAO<User> implements
UserDAO {
                     }
                 }
 
-                if (user.verifyPasswordHistory(user.getClearPassword(), policy.getHistoryLength()))
{
+                boolean matching = false;
+                if (policy.getHistoryLength() > 0) {
+                    List<String> pwdHistory = user.getPasswordHistory();
+                    for (String oldPwdValue
+                            : pwdHistory.subList(policy.getHistoryLength() >= pwdHistory.size()
+                                    ? 0
+                                    : pwdHistory.size() - policy.getHistoryLength(), pwdHistory.size()))
{
+                        matching |= ENCRYPTOR.verify(user.getClearPassword(), user.getCipherAlgorithm(),
oldPwdValue);
+                    }
+                }
+                if (matching) {
                     throw new PasswordPolicyException("Password value was used in the past:
not allowed");
                 }
 
@@ -349,8 +362,7 @@ public class JPAUserDAO extends AbstractAnyDAO<User> implements
UserDAO {
             }
 
             // update user's password history with encrypted password
-            if (maxPPSpecHistory > 0 && user.getPassword() != null
-                    && !user.getPasswordHistory().contains(user.getPassword())) {
+            if (maxPPSpecHistory > 0 && user.getPassword() != null) {
                 user.getPasswordHistory().add(user.getPassword());
             }
             // keep only the last maxPPSpecHistory items in user's password history

http://git-wip-us.apache.org/repos/asf/syncope/blob/a4710ca6/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java
----------------------------------------------------------------------
diff --git a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java
b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java
index b3dfac1..6ccf3c7 100644
--- a/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java
+++ b/core/persistence-jpa/src/main/java/org/apache/syncope/core/persistence/jpa/entity/user/JPAUser.java
@@ -353,11 +353,6 @@ public class JPAUser
     }
 
     @Override
-    public void setCipherAlgorithm(final CipherAlgorithm cipherAlgorithm) {
-        this.cipherAlgorithm = cipherAlgorithm;
-    }
-
-    @Override
     public List<String> getPasswordHistory() {
         return passwordHistory;
     }
@@ -458,25 +453,6 @@ public class JPAUser
     }
 
     @Override
-    public boolean verifyPasswordHistory(final String password, final int size) {
-        boolean res = false;
-
-        if (size > 0) {
-            try {
-                res = passwordHistory.subList(size >= passwordHistory.size()
-                        ? 0
-                        : passwordHistory.size() - size, passwordHistory.size()).contains(cipherAlgorithm
== null
-                        ? password
-                        : ENCRYPTOR.encode(password, cipherAlgorithm));
-            } catch (Exception e) {
-                LOG.error("Error evaluating password history", e);
-            }
-        }
-
-        return res;
-    }
-
-    @Override
     public SecurityQuestion getSecurityQuestion() {
         return securityQuestion;
     }
@@ -541,5 +517,4 @@ public class JPAUser
     public List<? extends UMembership> getMemberships() {
         return memberships;
     }
-
 }

http://git-wip-us.apache.org/repos/asf/syncope/blob/a4710ca6/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java
----------------------------------------------------------------------
diff --git a/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java
b/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java
index 3f55a23..09efe46 100644
--- a/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java
+++ b/core/provisioning-api/src/main/java/org/apache/syncope/core/provisioning/api/data/UserDataBinder.java
@@ -44,6 +44,4 @@ public interface UserDataBinder {
      * @see PropagationByResource
      */
     PropagationByResource update(User toBeUpdated, UserPatch userPatch);
-
-    boolean verifyPassword(User user, String password);
 }

http://git-wip-us.apache.org/repos/asf/syncope/blob/a4710ca6/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
----------------------------------------------------------------------
diff --git a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
index 7fd39c5..04246bd 100644
--- a/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
+++ b/core/provisioning-java/src/main/java/org/apache/syncope/core/provisioning/java/data/UserDataBinderImpl.java
@@ -61,7 +61,6 @@ import org.apache.syncope.core.persistence.api.entity.user.User;
 import org.apache.syncope.core.provisioning.api.PropagationByResource;
 import org.apache.syncope.core.provisioning.api.data.UserDataBinder;
 import org.apache.syncope.core.spring.security.AuthContextUtils;
-import org.apache.syncope.core.spring.security.Encryptor;
 import org.apache.syncope.core.spring.BeanUtils;
 import org.apache.syncope.core.provisioning.api.utils.EntityUtils;
 import org.apache.syncope.core.persistence.api.dao.AnyTypeDAO;
@@ -93,8 +92,6 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements
UserDat
         "plainAttrs", "derAttrs", "virAttrs", "resources", "securityQuestion", "securityAnswer"
     };
 
-    private static final Encryptor ENCRYPTOR = Encryptor.getInstance();
-
     @Autowired
     private RoleDAO roleDAO;
 
@@ -147,17 +144,10 @@ public class UserDataBinderImpl extends AbstractAnyDataBinder implements
UserDat
         return authUserTO;
     }
 
-    @Transactional(readOnly = true)
-    @Override
-    public boolean verifyPassword(final User user, final String password) {
-        return ENCRYPTOR.verify(password, user.getCipherAlgorithm(), user.getPassword());
-    }
-
     private void setPassword(final User user, final String password, final SyncopeClientCompositeException
scce) {
         try {
             String algorithm = confDAO.find("password.cipher.algorithm", CipherAlgorithm.AES.name());
-            CipherAlgorithm predefined = CipherAlgorithm.valueOf(algorithm);
-            user.setPassword(password, predefined);
+            user.setPassword(password, CipherAlgorithm.valueOf(algorithm));
         } catch (IllegalArgumentException e) {
             SyncopeClientException invalidCiperAlgorithm = SyncopeClientException.build(ClientExceptionType.NotFound);
             invalidCiperAlgorithm.getElements().add(e.getMessage());

http://git-wip-us.apache.org/repos/asf/syncope/blob/a4710ca6/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java
----------------------------------------------------------------------
diff --git a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java
b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java
index f785884..6a6da2c 100644
--- a/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java
+++ b/fit/core-reference/src/test/java/org/apache/syncope/fit/core/UserIssuesITCase.java
@@ -37,6 +37,7 @@ import javax.sql.DataSource;
 import javax.ws.rs.core.GenericType;
 import org.apache.commons.collections4.CollectionUtils;
 import org.apache.commons.collections4.Predicate;
+import org.apache.commons.lang3.SerializationUtils;
 import org.apache.commons.lang3.tuple.Pair;
 import org.apache.cxf.common.util.Base64Utility;
 import org.apache.cxf.helpers.IOUtils;
@@ -1439,4 +1440,48 @@ public class UserIssuesITCase extends AbstractITCase {
         assertEquals(1, result.getPropagationStatuses().size());
         assertEquals(RESOURCE_NAME_LDAP, result.getPropagationStatuses().get(0).getResource());
     }
+
+    @Test
+    public void issueSYNCOPE1337() {
+        // 1. save current cipher algorithm and set it to something salted
+        AttrTO original = configurationService.get("password.cipher.algorithm");
+
+        AttrTO salted = SerializationUtils.clone(original);
+        salted.getValues().set(0, CipherAlgorithm.SSHA512.name());
+        configurationService.set(salted);
+
+        try {
+            // 2. create user under /even/two to get password policy with history length
1
+            UserTO userTO = UserITCase.getUniqueSampleTO("syncope1337@apache.org");
+            userTO.setPassword("Password123");
+            userTO.setRealm("/even/two");
+            userTO = createUser(userTO).getEntity();
+            assertNotNull(userTO);
+
+            // 3. attempt to set the same password value: fails
+            UserPatch patch = new UserPatch();
+            patch.setKey(userTO.getKey());
+            patch.setPassword(new PasswordPatch.Builder().onSyncope(true).value("Password123").build());
+            try {
+                updateUser(patch);
+                fail("Password update should not work");
+            } catch (SyncopeClientException e) {
+                assertEquals(ClientExceptionType.InvalidUser, e.getType());
+                assertTrue(e.getMessage().contains("InvalidPassword"));
+            }
+
+            // 4. set another password value: works
+            patch.setPassword(new PasswordPatch.Builder().onSyncope(true).value("Password124").build());
+            userTO = updateUser(patch).getEntity();
+            assertNotNull(userTO);
+
+            // 5. set the original password value: works (history length is 1)
+            patch.setPassword(new PasswordPatch.Builder().onSyncope(true).value("Password123").build());
+            userTO = updateUser(patch).getEntity();
+            assertNotNull(userTO);
+        } finally {
+            // finally revert the cipher algorithm
+            configurationService.set(original);
+        }
+    }
 }


Mime
View raw message