You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@syncope.apache.org by il...@apache.org on 2018/07/13 14:58:12 UTC
[6/7] syncope git commit: [SYNCOPE-1337] Do not check password
history by simple String comparison,
use Encryptor#verify as authentication does
[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/21c92719
Tree: http://git-wip-us.apache.org/repos/asf/syncope/tree/21c92719
Diff: http://git-wip-us.apache.org/repos/asf/syncope/diff/21c92719
Branch: refs/heads/2_1_X
Commit: 21c92719cb100bc97a601691d338ab1ca188fc73
Parents: 16458d9
Author: Francesco Chicchiriccò <il...@apache.org>
Authored: Fri Jul 13 15:57:01 2018 +0200
Committer: Francesco Chicchiriccò <il...@apache.org>
Committed: Fri Jul 13 16:57:44 2018 +0200
----------------------------------------------------------------------
.../core/persistence/api/entity/user/User.java | 4 --
.../core/persistence/jpa/dao/JPAUserDAO.java | 17 ++++++--
.../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, 60 insertions(+), 45 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/syncope/blob/21c92719/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/21c92719/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 5e97993..cbe6159 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
@@ -68,6 +68,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.spring.ImplementationManager;
+import org.apache.syncope.core.spring.security.Encryptor;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.stereotype.Repository;
import org.springframework.transaction.annotation.Propagation;
@@ -79,6 +80,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;
@@ -298,7 +301,16 @@ public class JPAUserDAO extends AbstractAnyDAO<User> implements UserDAO {
ImplementationManager.buildPasswordRule(impl).ifPresent(rule -> rule.enforce(user));
}
- if (user.verifyPasswordHistory(user.getClearPassword(), policy.getHistoryLength())) {
+ boolean matching = false;
+ if (policy.getHistoryLength() > 0) {
+ List<String> pwdHistory = user.getPasswordHistory();
+ matching = pwdHistory.subList(policy.getHistoryLength() >= pwdHistory.size()
+ ? 0
+ : pwdHistory.size() - policy.getHistoryLength(), pwdHistory.size()).stream().
+ map(old -> ENCRYPTOR.verify(user.getClearPassword(), user.getCipherAlgorithm(), old)).
+ reduce(matching, (accumulator, item) -> accumulator | item);
+ }
+ if (matching) {
throw new PasswordPolicyException("Password value was used in the past: not allowed");
}
@@ -308,8 +320,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/21c92719/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 640d250..4686aa6 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
@@ -343,11 +343,6 @@ public class JPAUser
}
@Override
- public void setCipherAlgorithm(final CipherAlgorithm cipherAlgorithm) {
- this.cipherAlgorithm = cipherAlgorithm;
- }
-
- @Override
public List<String> getPasswordHistory() {
return passwordHistory;
}
@@ -446,25 +441,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;
}
@@ -526,5 +502,4 @@ public class JPAUser
public List<? extends UMembership> getMemberships() {
return memberships;
}
-
}
http://git-wip-us.apache.org/repos/asf/syncope/blob/21c92719/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/21c92719/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 aaec195..df2f270 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
@@ -54,7 +54,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.persistence.api.dao.AnyTypeDAO;
import org.apache.syncope.core.persistence.api.dao.RoleDAO;
@@ -85,8 +84,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;
@@ -139,17 +136,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/21c92719/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 3e9024e..2ff46fc 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
@@ -39,6 +39,7 @@ import javax.naming.NamingException;
import javax.sql.DataSource;
import javax.ws.rs.core.GenericType;
import javax.ws.rs.core.Response;
+import org.apache.commons.lang3.SerializationUtils;
import org.apache.commons.lang3.tuple.Pair;
import org.apache.cxf.helpers.IOUtils;
import org.apache.syncope.client.lib.SyncopeClient;
@@ -1470,4 +1471,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);
+ }
+ }
}