From 042cad9e931378609bf83ac9ecb50f3a224930c8 Mon Sep 17 00:00:00 2001 From: Colm O hEigeartaigh Date: Wed, 16 Jan 2019 14:10:02 +0000 Subject: [PATCH] AMQ-7137 - Implement abort() properly in the LoginModules. Also fix a bug in LdapLoginModule relating to Logout --- .../activemq/jaas/CertificateLoginModule.java | 43 +++++++++---- .../activemq/jaas/GuestLoginModule.java | 38 ++++++++--- .../apache/activemq/jaas/LDAPLoginModule.java | 42 +++++++++++-- .../activemq/jaas/PropertiesLoginModule.java | 63 ++++++++++++------- 4 files changed, 137 insertions(+), 49 deletions(-) diff --git a/activemq-jaas/src/main/java/org/apache/activemq/jaas/CertificateLoginModule.java b/activemq-jaas/src/main/java/org/apache/activemq/jaas/CertificateLoginModule.java index f2a6528edd..dd840186ad 100644 --- a/activemq-jaas/src/main/java/org/apache/activemq/jaas/CertificateLoginModule.java +++ b/activemq-jaas/src/main/java/org/apache/activemq/jaas/CertificateLoginModule.java @@ -49,10 +49,13 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements private CallbackHandler callbackHandler; private Subject subject; - private X509Certificate certificates[]; private String username; private Set principals = new HashSet(); + /** the authentication status*/ + private boolean succeeded = false; + private boolean commitSucceeded = false; + /** * Overriding to allow for proper initialization. Standard JAAS. */ @@ -78,7 +81,7 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements } catch (UnsupportedCallbackException uce) { throw new LoginException(uce.getMessage() + " Unable to obtain client certificates."); } - certificates = ((CertificateCallback)callbacks[0]).getCertificates(); + X509Certificate[] certificates = ((CertificateCallback)callbacks[0]).getCertificates(); username = getUserNameForCertificates(certificates); if (username == null) { @@ -88,6 +91,7 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements if (debug) { LOG.debug("Certificate for user: " + username); } + succeeded = true; return true; } @@ -96,6 +100,15 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements */ @Override public boolean commit() throws LoginException { + if (debug) { + LOG.debug("commit"); + } + + if (!succeeded) { + clear(); + return false; + } + principals.add(new UserPrincipal(username)); for (String group : getUserGroups(username)) { @@ -104,11 +117,8 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements subject.getPrincipals().addAll(principals); - clear(); - - if (debug) { - LOG.debug("commit"); - } + username = null; + commitSucceeded = true; return true; } @@ -117,11 +127,19 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements */ @Override public boolean abort() throws LoginException { - clear(); - if (debug) { LOG.debug("abort"); } + if (!succeeded) { + return false; + } else if (succeeded && commitSucceeded) { + // we succeeded, but another required module failed + logout(); + } else { + // our commit failed + clear(); + succeeded = false; + } return true; } @@ -131,11 +149,14 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements @Override public boolean logout() { subject.getPrincipals().removeAll(principals); - principals.clear(); + clear(); if (debug) { LOG.debug("logout"); } + + succeeded = false; + commitSucceeded = false; return true; } @@ -143,8 +164,8 @@ public abstract class CertificateLoginModule extends PropertiesLoader implements * Helper method. */ private void clear() { - certificates = null; username = null; + principals.clear(); } /** diff --git a/activemq-jaas/src/main/java/org/apache/activemq/jaas/GuestLoginModule.java b/activemq-jaas/src/main/java/org/apache/activemq/jaas/GuestLoginModule.java index 621083de1d..724e15866c 100644 --- a/activemq-jaas/src/main/java/org/apache/activemq/jaas/GuestLoginModule.java +++ b/activemq-jaas/src/main/java/org/apache/activemq/jaas/GuestLoginModule.java @@ -54,7 +54,10 @@ public class GuestLoginModule implements LoginModule { private boolean credentialsInvalidate; private Set principals = new HashSet(); private CallbackHandler callbackHandler; - private boolean loginSucceeded; + + /** the authentication status*/ + private boolean succeeded = false; + private boolean commitSucceeded = false; @Override public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) { @@ -79,7 +82,7 @@ public class GuestLoginModule implements LoginModule { @Override public boolean login() throws LoginException { - loginSucceeded = true; + succeeded = true; if (credentialsInvalidate) { PasswordCallback passwordCallback = new PasswordCallback("Password: ", false); try { @@ -88,7 +91,7 @@ public class GuestLoginModule implements LoginModule { if (debug) { LOG.debug("Guest login failing (credentialsInvalidate=true) on presence of a password"); } - loginSucceeded = false; + succeeded = false; passwordCallback.clearPassword(); }; } catch (IOException ioe) { @@ -96,21 +99,24 @@ public class GuestLoginModule implements LoginModule { } } if (debug) { - LOG.debug("Guest login " + loginSucceeded); + LOG.debug("Guest login " + succeeded); } - return loginSucceeded; + return succeeded; } @Override public boolean commit() throws LoginException { - if (loginSucceeded) { - subject.getPrincipals().addAll(principals); - } - if (debug) { LOG.debug("commit"); } - return loginSucceeded; + + if (!succeeded) { + return false; + } + + subject.getPrincipals().addAll(principals); + commitSucceeded = true; + return true; } @Override @@ -119,6 +125,15 @@ public class GuestLoginModule implements LoginModule { if (debug) { LOG.debug("abort"); } + if (!succeeded) { + return false; + } else if (succeeded && commitSucceeded) { + // we succeeded, but another required module failed + logout(); + } else { + // our commit failed + succeeded = false; + } return true; } @@ -129,6 +144,9 @@ public class GuestLoginModule implements LoginModule { if (debug) { LOG.debug("logout"); } + + succeeded = false; + commitSucceeded = false; return true; } } diff --git a/activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java b/activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java index f0834a0660..dced7cefe9 100644 --- a/activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java +++ b/activemq-jaas/src/main/java/org/apache/activemq/jaas/LDAPLoginModule.java @@ -72,9 +72,13 @@ public class LDAPLoginModule implements LoginModule { private Subject subject; private CallbackHandler handler; private LDAPLoginProperty [] config; - private String username; + private Principal user; private Set groups = new HashSet(); + /** the authentication status*/ + private boolean succeeded = false; + private boolean commitSucceeded = false; + @Override public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) { this.subject = subject; @@ -118,7 +122,7 @@ public class LDAPLoginModule implements LoginModule { String password; - username = ((NameCallback)callbacks[0]).getName(); + String username = ((NameCallback)callbacks[0]).getName(); if (username == null) return false; @@ -130,28 +134,56 @@ public class LDAPLoginModule implements LoginModule { // authenticate will throw LoginException // in case of failed authentication authenticate(username, password); + + user = new UserPrincipal(username); + succeeded = true; return true; } @Override public boolean logout() throws LoginException { - username = null; + subject.getPrincipals().remove(user); + subject.getPrincipals().removeAll(groups); + + user = null; + groups.clear(); + + succeeded = false; + commitSucceeded = false; return true; } @Override public boolean commit() throws LoginException { + if (!succeeded) { + user = null; + groups.clear(); + return false; + } + Set principals = subject.getPrincipals(); - principals.add(new UserPrincipal(username)); + principals.add(user); for (GroupPrincipal gp : groups) { principals.add(gp); } + + commitSucceeded = true; return true; } @Override public boolean abort() throws LoginException { - username = null; + if (!succeeded) { + return false; + } else if (succeeded && commitSucceeded) { + // we succeeded, but another required module failed + logout(); + } else { + // our commit failed + user = null; + groups.clear(); + succeeded = false; + } return true; } diff --git a/activemq-jaas/src/main/java/org/apache/activemq/jaas/PropertiesLoginModule.java b/activemq-jaas/src/main/java/org/apache/activemq/jaas/PropertiesLoginModule.java index 5346bd7938..153a12534e 100644 --- a/activemq-jaas/src/main/java/org/apache/activemq/jaas/PropertiesLoginModule.java +++ b/activemq-jaas/src/main/java/org/apache/activemq/jaas/PropertiesLoginModule.java @@ -50,13 +50,16 @@ public class PropertiesLoginModule extends PropertiesLoader implements LoginModu private Map> groups; private String user; private final Set principals = new HashSet(); - private boolean loginSucceeded; + + /** the authentication status*/ + private boolean succeeded = false; + private boolean commitSucceeded = false; @Override public void initialize(Subject subject, CallbackHandler callbackHandler, Map sharedState, Map options) { this.subject = subject; this.callbackHandler = callbackHandler; - loginSucceeded = false; + succeeded = false; init(options); users = load(USER_FILE_PROP_NAME, "user", options).getProps(); groups = load(GROUP_FILE_PROP_NAME, "group", options).invertedPropertiesValuesMap(); @@ -91,63 +94,77 @@ public class PropertiesLoginModule extends PropertiesLoader implements LoginModu if (!password.equals(new String(tmpPassword))) { throw new FailedLoginException("Password does not match"); } - loginSucceeded = true; + succeeded = true; if (debug) { LOG.debug("login " + user); } - return loginSucceeded; + return succeeded; } @Override public boolean commit() throws LoginException { - boolean result = loginSucceeded; - if (result) { - principals.add(new UserPrincipal(user)); - - Set matchedGroups = groups.get(user); - if (matchedGroups != null) { - for (String entry : matchedGroups) { - principals.add(new GroupPrincipal(entry)); - } + if (!succeeded) { + clear(); + if (debug) { + LOG.debug("commit, result: false"); } - - subject.getPrincipals().addAll(principals); + return false; } - // will whack loginSucceeded - clear(); + principals.add(new UserPrincipal(user)); + + Set matchedGroups = groups.get(user); + if (matchedGroups != null) { + for (String entry : matchedGroups) { + principals.add(new GroupPrincipal(entry)); + } + } + + subject.getPrincipals().addAll(principals); if (debug) { - LOG.debug("commit, result: " + result); + LOG.debug("commit, result: true"); } - return result; + + commitSucceeded = true; + return true; } @Override public boolean abort() throws LoginException { - clear(); - if (debug) { LOG.debug("abort"); } + if (!succeeded) { + return false; + } else if (succeeded && commitSucceeded) { + // we succeeded, but another required module failed + logout(); + } else { + // our commit failed + clear(); + succeeded = false; + } return true; } @Override public boolean logout() throws LoginException { subject.getPrincipals().removeAll(principals); - principals.clear(); clear(); if (debug) { LOG.debug("logout"); } + + succeeded = false; + commitSucceeded = false; return true; } private void clear() { user = null; - loginSucceeded = false; + principals.clear(); } }