From 0c893b6175b3cdb48489f1c6be30b5bfb11dc05a Mon Sep 17 00:00:00 2001 From: mposolda Date: Wed, 25 Feb 2015 15:40:00 +0100 Subject: [PATCH] Kerberos fixes for jboss-modules refactoring. Fix environment specific test failures --- distribution/modules/build.xml | 4 +++ .../main/module.xml | 19 +++++++++++ .../keycloak-ldap-federation/main/module.xml | 1 + .../keycloak/keycloak-server/main/module.xml | 1 + .../keycloak-services/main/module.xml | 1 + .../WEB-INF/jboss-deployment-structure.xml | 1 + .../kerberos/KerberosFederationProvider.java | 33 ++++++++++++------- ...KerberosUsernamePasswordAuthenticator.java | 21 ++++++++---- .../kerberos/impl/SPNEGOAuthenticator.java | 2 +- .../ldap/LDAPFederationProvider.java | 10 ++++-- .../federation/KerberosLdapTest.java | 2 +- .../KeycloakSPNegoSchemeFactory.java | 15 ++++++++- 12 files changed, 86 insertions(+), 24 deletions(-) create mode 100644 distribution/modules/src/main/resources/modules/org/keycloak/keycloak-kerberos-federation/main/module.xml diff --git a/distribution/modules/build.xml b/distribution/modules/build.xml index 208e061b7e..0cd950a8a4 100755 --- a/distribution/modules/build.xml +++ b/distribution/modules/build.xml @@ -228,6 +228,10 @@ + + + + diff --git a/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-kerberos-federation/main/module.xml b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-kerberos-federation/main/module.xml new file mode 100644 index 0000000000..a238b9ceef --- /dev/null +++ b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-kerberos-federation/main/module.xml @@ -0,0 +1,19 @@ + + + + + + + + + + + + + + + + + + + \ No newline at end of file diff --git a/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-ldap-federation/main/module.xml b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-ldap-federation/main/module.xml index 5c81548dc9..29dfd9c7d7 100755 --- a/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-ldap-federation/main/module.xml +++ b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-ldap-federation/main/module.xml @@ -9,6 +9,7 @@ + diff --git a/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-server/main/module.xml b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-server/main/module.xml index a2efd4ac4a..c0e942c84d 100755 --- a/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-server/main/module.xml +++ b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-server/main/module.xml @@ -35,6 +35,7 @@ + diff --git a/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-services/main/module.xml b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-services/main/module.xml index e98c558b5a..4cc0d1336f 100755 --- a/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-services/main/module.xml +++ b/distribution/modules/src/main/resources/modules/org/keycloak/keycloak-services/main/module.xml @@ -35,6 +35,7 @@ + diff --git a/distribution/subsystem-war/src/main/webapp/WEB-INF/jboss-deployment-structure.xml b/distribution/subsystem-war/src/main/webapp/WEB-INF/jboss-deployment-structure.xml index 356b953564..9818d93c2b 100755 --- a/distribution/subsystem-war/src/main/webapp/WEB-INF/jboss-deployment-structure.xml +++ b/distribution/subsystem-war/src/main/webapp/WEB-INF/jboss-deployment-structure.xml @@ -26,6 +26,7 @@ + diff --git a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java index f1ea251547..603f3bd950 100644 --- a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java +++ b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/KerberosFederationProvider.java @@ -69,12 +69,13 @@ public class KerberosFederationProvider implements UserFederationProvider { @Override public UserModel getUserByUsername(RealmModel realm, String username) { - if (username.contains("@")) { - username = username.split("@")[0]; - } - KerberosUsernamePasswordAuthenticator authenticator = factory.createKerberosUsernamePasswordAuthenticator(kerberosConfig); if (authenticator.isUserAvailable(username)) { + // Case when method was called with username including kerberos realm like john@REALM.ORG . Authenticator already checked that kerberos realm was correct + if (username.contains("@")) { + username = username.split("@")[0]; + } + return findOrCreateAuthenticatedUser(realm, username); } else { return null; @@ -106,7 +107,7 @@ public class KerberosFederationProvider implements UserFederationProvider { // KerberosUsernamePasswordAuthenticator.isUserAvailable is an overhead, so avoid it for now String kerberosPrincipal = local.getUsername() + "@" + kerberosConfig.getKerberosRealm(); - return model.getId().equals(local.getFederationLink()) && kerberosPrincipal.equals(local.getAttribute(KERBEROS_PRINCIPAL)); + return kerberosPrincipal.equals(local.getAttribute(KERBEROS_PRINCIPAL)); } @Override @@ -181,8 +182,11 @@ public class KerberosFederationProvider implements UserFederationProvider { String username = spnegoAuthenticator.getAuthenticatedUsername(); UserModel user = findOrCreateAuthenticatedUser(realm, username); - - return new CredentialValidationOutput(user, CredentialValidationOutput.Status.AUTHENTICATED, state); + if (user == null) { + return CredentialValidationOutput.failed(); + } else { + return new CredentialValidationOutput(user, CredentialValidationOutput.Status.AUTHENTICATED, state); + } } else { Map state = new HashMap(); state.put(KerberosConstants.RESPONSE_TOKEN, spnegoAuthenticator.getResponseToken()); @@ -202,19 +206,24 @@ public class KerberosFederationProvider implements UserFederationProvider { /** * Called after successful authentication * - * @param realm + * @param realm realm * @param username username without realm prefix - * @return + * @return user if found or successfully created. Null if user with same username already exists, but is not linked to this provider */ protected UserModel findOrCreateAuthenticatedUser(RealmModel realm, String username) { UserModel user = session.userStorage().getUserByUsername(username, realm); if (user != null) { logger.debug("Kerberos authenticated user " + username + " found in Keycloak storage"); - if (isValid(user)) { + + if (!model.getId().equals(user.getFederationLink())) { + logger.warn("User with username " + username + " already exists, but is not linked to provider [" + model.getDisplayName() + "]"); + return null; + } else if (isValid(user)) { return proxy(user); } else { - logger.warn("User with username " + username + " already exists, but is not linked to provider [" + model.getDisplayName() + - "] or kerberos principal is not correct. Kerberos principal on user is: " + user.getAttribute(KERBEROS_PRINCIPAL)); + logger.warn("User with username " + username + " already exists and is linked to provider [" + model.getDisplayName() + + "] but kerberos principal is not correct. Kerberos principal on user is: " + user.getAttribute(KERBEROS_PRINCIPAL)); + logger.warn("Will re-create user"); session.userStorage().removeUser(realm, user); } } diff --git a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/KerberosUsernamePasswordAuthenticator.java b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/KerberosUsernamePasswordAuthenticator.java index c8e6fa040d..ddebe667bc 100644 --- a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/KerberosUsernamePasswordAuthenticator.java +++ b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/KerberosUsernamePasswordAuthenticator.java @@ -36,14 +36,13 @@ public class KerberosUsernamePasswordAuthenticator { /** * Returns true if user with given username exists in kerberos database * - * @param username username without Kerberos realm attached + * @param username username without Kerberos realm attached or with correct realm attached * @return true if user available */ public boolean isUserAvailable(String username) { - String principal = getKerberosPrincipal(username); - - logger.debug("Checking existence of principal: " + principal); + logger.debug("Checking existence of user: " + username); try { + String principal = getKerberosPrincipal(username); loginContext = new LoginContext("does-not-matter", null, createJaasCallbackHandler(principal, "fake-password-which-nobody-has"), createJaasConfiguration()); @@ -65,7 +64,7 @@ public class KerberosUsernamePasswordAuthenticator { /** * Returns true if user was successfully authenticated against Kerberos * - * @param username username without Kerberos realm attached + * @param username username without Kerberos realm attached or with correct realm attached * @param password kerberos password * @return true if user was successfully authenticated */ @@ -113,7 +112,17 @@ public class KerberosUsernamePasswordAuthenticator { - protected String getKerberosPrincipal(String username) { + protected String getKerberosPrincipal(String username) throws LoginException { + if (username.contains("@")) { + String[] tokens = username.split("@"); + username = tokens[0]; + String kerberosRealm = tokens[1]; + if (kerberosRealm.toUpperCase().equals(config.getKerberosRealm())) { + logger.warn("Invalid kerberos realm. Expected realm: " + config.getKerberosRealm() + ", username: " + username); + throw new LoginException("Invalid kerberos realm"); + } + } + return username + "@" + config.getKerberosRealm(); } diff --git a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/SPNEGOAuthenticator.java b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/SPNEGOAuthenticator.java index a320469967..1132351b17 100644 --- a/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/SPNEGOAuthenticator.java +++ b/federation/kerberos/src/main/java/org/keycloak/federation/kerberos/impl/SPNEGOAuthenticator.java @@ -47,7 +47,7 @@ public class SPNEGOAuthenticator { Subject serverSubject = kerberosSubjectAuthenticator.authenticateServerSubject(); authenticated = Subject.doAs(serverSubject, new AcceptSecContext()); } catch (Exception e) { - log.warn("SPNEGO login failed: " + e.getMessage(), e); + log.warn("SPNEGO login failed", e); } finally { kerberosSubjectAuthenticator.logoutServerSubject(); } diff --git a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java index 9f927b4aef..7aabe26811 100755 --- a/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java +++ b/federation/ldap/src/main/java/org/keycloak/federation/ldap/LDAPFederationProvider.java @@ -409,11 +409,15 @@ public class LDAPFederationProvider implements UserFederationProvider { UserModel user = session.userStorage().getUserByUsername(username, realm); if (user != null) { logger.debug("Kerberos authenticated user " + username + " found in Keycloak storage"); - if (isValid(user)) { + if (!model.getId().equals(user.getFederationLink())) { + logger.warn("User with username " + username + " already exists, but is not linked to provider [" + model.getDisplayName() + "]"); + return null; + } else if (isValid(user)) { return proxy(user); } else { - logger.warn("User with username " + username + " already exists, but is not linked to provider [" + model.getDisplayName() + - "] or LDAP_ID is not correct. Stale LDAP_ID on local user is: " + user.getAttribute(LDAP_ID)); + logger.warn("User with username " + username + " already exists and is linked to provider [" + model.getDisplayName() + + "] but is not valid. Stale LDAP_ID on local user is: " + user.getAttribute(LDAP_ID)); + logger.warn("Will re-create user"); session.userStorage().removeUser(realm, user); } } diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KerberosLdapTest.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KerberosLdapTest.java index 1f1092a86b..4123508815 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KerberosLdapTest.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KerberosLdapTest.java @@ -103,7 +103,7 @@ public class KerberosLdapTest extends AbstractKerberosTest { KeycloakRule keycloakRule = getKeycloakRule(); AssertEvents events = getAssertEvents(); - // Change editMode to READ_ONLY + // Change editMode to WRITABLE updateProviderEditMode(UserFederationProvider.EditMode.WRITABLE); // Login with username/password from kerberos diff --git a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KeycloakSPNegoSchemeFactory.java b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KeycloakSPNegoSchemeFactory.java index 34032633ca..00c9954c70 100644 --- a/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KeycloakSPNegoSchemeFactory.java +++ b/testsuite/integration/src/test/java/org/keycloak/testsuite/federation/KeycloakSPNegoSchemeFactory.java @@ -8,7 +8,10 @@ import org.apache.http.auth.AuthScheme; import org.apache.http.impl.auth.SPNegoScheme; import org.apache.http.impl.auth.SPNegoSchemeFactory; import org.apache.http.params.HttpParams; +import org.ietf.jgss.GSSContext; import org.ietf.jgss.GSSException; +import org.ietf.jgss.GSSManager; +import org.ietf.jgss.GSSName; import org.ietf.jgss.Oid; import org.keycloak.federation.kerberos.CommonKerberosConfig; import org.keycloak.federation.kerberos.impl.KerberosUsernamePasswordAuthenticator; @@ -83,7 +86,17 @@ public class KeycloakSPNegoSchemeFactory extends SPNegoSchemeFactory { @Override public ByteArrayHolder run() throws Exception { - byte[] outputToken = KeycloakSPNegoScheme.super.generateGSSToken(input, oid, authServer); + byte[] token = input; + if (token == null) { + token = new byte[0]; + } + GSSManager manager = getManager(); + GSSName serverName = manager.createName("HTTP/" + authServer + "@" + kerberosConfig.getKerberosRealm(), null); + GSSContext gssContext = manager.createContext( + serverName.canonicalize(oid), oid, null, GSSContext.DEFAULT_LIFETIME); + gssContext.requestMutualAuth(true); + gssContext.requestCredDeleg(true); + byte[] outputToken = gssContext.initSecContext(token, 0, token.length); ByteArrayHolder result = new ByteArrayHolder(); result.bytes = outputToken;