Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new password policy to validate passwords on login #1

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
import org.keycloak.models.LDAPConstants;
import org.keycloak.models.ModelDuplicateException;
import org.keycloak.models.ModelException;
import org.keycloak.models.PasswordPolicy;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RequiredActionProviderModel;
import org.keycloak.models.RoleModel;
Expand All @@ -68,6 +69,7 @@
import org.keycloak.policy.PasswordPolicyManagerProvider;
import org.keycloak.policy.PolicyError;
import org.keycloak.models.cache.UserCache;
import org.keycloak.services.managers.AuthenticationManager;
import org.keycloak.storage.DatastoreProvider;
import org.keycloak.storage.StoreManagers;
import org.keycloak.storage.ReadOnlyException;
Expand Down Expand Up @@ -886,7 +888,27 @@ public boolean isConfiguredFor(RealmModel realm, UserModel user, String credenti
public boolean isValid(RealmModel realm, UserModel user, CredentialInput input) {
if (!(input instanceof UserCredentialModel)) return false;
if (input.getType().equals(PasswordCredentialModel.TYPE) && !((UserCredentialManager) user.credentialManager()).isConfiguredLocally(PasswordCredentialModel.TYPE)) {
return validPassword(realm, user, input.getChallengeResponse());
if(getEditMode() == EditMode.READ_ONLY) {
session.getContext().getAuthenticationSession().setAuthNote(AuthenticationManager.USER_READ_ONLY, "true");
}

if(!validPassword(realm, user, input.getChallengeResponse())){
return false;
}

// After the password has been validated successfully, check that it still matches the realm's password policy.
// Only do that, if we can write the LDAP store, since only then the password can be updated in Keycloak.
// TODO: Do we want to support read-only LDAP? How to handle this then? Leave the decision to whoever evaluates the
// password policy result, so e.g. show a warning in the browser authenticators / deny authentication?
Comment on lines +901 to +902
Copy link
Collaborator Author

@sirkrypt0 sirkrypt0 May 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on this?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to enforce the policy and deny authentication in read only mode. If admins don't want that behavior they easily can disable the password policy but they cannot enforce them otherwise. However, there should be a error message to the user imho

if (ldapIdentityStore.getConfig().isValidatePasswordPolicy() && realm.getPasswordPolicy().shouldValidateOnLogin()) {
PolicyError error = session.getProvider(PasswordPolicyManagerProvider.class).validate(realm, user, input.getChallengeResponse());
if (error != null) {
logger.debug("User password no longer matches password policy");
session.getContext().getAuthenticationSession().setAuthNote(PasswordPolicy.POLICY_ERROR_AUTH_NOTE, "true");
return false;
}
}
return true;
} else {
return false; // invalid cred type
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
package org.keycloak.policy;

import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;

public class ValidateOnLoginPasswordPolicyProvider implements PasswordPolicyProvider {
@Override
public PolicyError validate(RealmModel realm, UserModel user, String password) {
return null;
}

@Override
public PolicyError validate(String user, String password) {
return null;
}

@Override
public Object parseConfig(String value) {
return null;
}

@Override
public void close() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
public void close() {
public void close() {}

I personally like this style more, but I am not sure what the Keycloak style guidelines say.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at similar implementations and mainly found the current variant. Sometimes, the empty line is removed, like this:

public void close() {
}


}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
package org.keycloak.policy;

import org.keycloak.Config;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.KeycloakSessionFactory;
import org.keycloak.models.PasswordPolicy;

public class ValidateOnLoginPasswordPolicyProviderFactory implements PasswordPolicyProviderFactory {

@Override
public String getDisplayName() {
return "Validate Policy on Login";
}

@Override
public String getConfigType() {
return null;
}

@Override
public String getDefaultConfigValue() {
return null;
}

@Override
public boolean isMultiplSupported() {
return false;
}

@Override
public PasswordPolicyProvider create(KeycloakSession session) {
return new ValidateOnLoginPasswordPolicyProvider();
}

@Override
public void init(Config.Scope config) {

}

@Override
public void postInit(KeycloakSessionFactory factory) {

}

@Override
public void close() {

}

@Override
public String getId() {
return PasswordPolicy.VALIDATE_ON_LOGIN_ID;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,5 @@ org.keycloak.policy.BlacklistPasswordPolicyProviderFactory
org.keycloak.policy.NotEmailPasswordPolicyProviderFactory
org.keycloak.policy.RecoveryCodesWarningThresholdPasswordPolicyProviderFactory
org.keycloak.policy.MaxAuthAgePasswordPolicyProviderFactory
org.keycloak.policy.AgePasswordPolicyProviderFactory
org.keycloak.policy.AgePasswordPolicyProviderFactory
org.keycloak.policy.ValidateOnLoginPasswordPolicyProviderFactory
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,10 @@ public class PasswordPolicy implements Serializable {

public static final String PASSWORD_AGE = "passwordAge";

public static final String VALIDATE_ON_LOGIN_ID = "validateOnLogin";

public static final String POLICY_ERROR_AUTH_NOTE = "POLICY_ERROR";

private Map<String, Object> policyConfig;
private Builder builder;

Expand Down Expand Up @@ -143,6 +147,10 @@ public int getMaxAuthAge() {
}
}

public boolean shouldValidateOnLogin() {
return policyConfig.containsKey(VALIDATE_ON_LOGIN_ID);
}

@Override
public String toString() {
return builder.asString();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import org.keycloak.models.AuthenticationFlowModel;
import org.keycloak.models.Constants;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.PasswordPolicy;
import org.keycloak.models.RealmModel;
import org.keycloak.models.UserModel;
import org.keycloak.services.managers.AuthenticationManager;
Expand All @@ -49,6 +50,7 @@

import static org.keycloak.services.managers.AuthenticationManager.FORCED_REAUTHENTICATION;
import static org.keycloak.services.managers.AuthenticationManager.SSO_AUTH;
import static org.keycloak.services.managers.AuthenticationManager.USER_READ_ONLY;
import static org.keycloak.services.managers.AuthenticationManager.PASSWORD_VALIDATED;

public class AuthenticatorUtil {
Expand All @@ -63,6 +65,10 @@ public static boolean isSSOAuthentication(AuthenticationSessionModel authSession
return "true".equals(authSession.getAuthNote(SSO_AUTH));
}

public static boolean isUserReadOnlyAuthentication(AuthenticationSessionModel authSession) {
return "true".equals(authSession.getAuthNote(USER_READ_ONLY));
}

public static boolean isForcedReauthentication(AuthenticationSessionModel authSession) {
return "true".equals(authSession.getAuthNote(FORCED_REAUTHENTICATION));
}
Expand All @@ -75,6 +81,10 @@ public static boolean isForkedFlow(AuthenticationSessionModel authSession) {
return authSession.getAuthNote(AuthenticationProcessor.FORKED_FROM) != null;
}

public static boolean hasPasswordPolicyError(AuthenticationSessionModel authSession) {
return "true".equals(authSession.getAuthNote(PasswordPolicy.POLICY_ERROR_AUTH_NOTE));
}

/**
* Set authentication session note for callbacks defined for {@link AuthenticationFlowCallbackFactory) factories
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.keycloak.authentication.AuthenticationFlowContext;
import org.keycloak.authentication.AuthenticationFlowError;
import org.keycloak.authentication.authenticators.util.AuthenticatorUtils;
import org.keycloak.authentication.AuthenticatorUtil;
import org.keycloak.events.Details;
import org.keycloak.events.Errors;
import org.keycloak.forms.login.LoginFormsProvider;
Expand Down Expand Up @@ -196,6 +197,29 @@ private boolean validateUser(AuthenticationFlowContext context, UserModel user,
}

public boolean validatePassword(AuthenticationFlowContext context, UserModel user, MultivaluedMap<String, String> inputData, boolean clearUser) {
// If we have validated the password before, but it didn't match the current password policy, allow to continue to
// update the password or cancel. If we don't check whether the password was validated, users could skip the authenticator simply
// by providing continueUpdate in their form data.
if (AuthenticatorUtil.isPasswordValidated(context.getAuthenticationSession())){
// Offer the option to update the password if user hit continue, and they are not read-only.
if(inputData.containsKey("continueToUpdate") && !AuthenticatorUtil.isUserReadOnlyAuthentication(context.getAuthenticationSession())) {
// Set a required action on the authentication session instead of on the user, as the password policy could be
// changed again such that the password would be compliant again. Setting an update action on the user could
// make it redundant, if their password is compliant again after a change in the policy.
context.getAuthenticationSession().addRequiredAction(UserModel.RequiredAction.UPDATE_PASSWORD);
return true;
}

// Otherwise, go back to the login field.
Response challengeResponse = challenge(context, getDefaultChallengeMessage(context), FIELD_PASSWORD);
context.forceChallenge(challengeResponse);

if (clearUser) {
context.clearUser();
}
return false;
}

String password = inputData.getFirst(CredentialRepresentation.PASSWORD);
if (password == null || password.isEmpty()) {
return badPasswordHandler(context, user, clearUser,true);
Expand All @@ -205,6 +229,22 @@ public boolean validatePassword(AuthenticationFlowContext context, UserModel use

if (password != null && !password.isEmpty() && user.credentialManager().isValid(UserCredentialModel.password(password))) {
context.getAuthenticationSession().setAuthNote(AuthenticationManager.PASSWORD_VALIDATED, "true");

// The credentialManager doesn't return policy errors for us. Instead, the individual credential providers that
// validate the password set this flag to true if there is an error.
if (AuthenticatorUtil.hasPasswordPolicyError(context.getAuthenticationSession())) {
// TODO: Pass read-only state to form.

Response challenge = context.
form().
setAttribute("userReadOnly", AuthenticatorUtil.isUserReadOnlyAuthentication(context.getAuthenticationSession())).
createForm("login-policy-error.ftl");

context.forceChallenge(challenge);

return false;
}

return true;
} else {
return badPasswordHandler(context, user, clearUser,false);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

import org.keycloak.authentication.AuthenticationFlowContext;
import org.keycloak.authentication.AuthenticationFlowError;
import org.keycloak.authentication.AuthenticatorUtil;
import org.keycloak.events.Errors;
import org.keycloak.models.AuthenticationExecutionModel;
import org.keycloak.models.KeycloakSession;
Expand Down Expand Up @@ -54,6 +55,17 @@ public void authenticate(AuthenticationFlowContext context) {
context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse);
return;
}

if (AuthenticatorUtil.hasPasswordPolicyError(context.getAuthenticationSession())) {
Response challengeResponse = errorResponse(
Response.Status.UNAUTHORIZED.getStatusCode(),
"invalid_grant",
"User password no longer matches the password policy and must be changed"
);
context.failure(AuthenticationFlowError.INVALID_USER, challengeResponse);
return;
}

context.getAuthenticationSession().setAuthNote(AuthenticationManager.PASSWORD_VALIDATED, "true");
context.success();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,15 @@ public boolean isValid(RealmModel realm, UserModel user, CredentialInput input)
return false;
}

if (realm.getPasswordPolicy().shouldValidateOnLogin()){
// After the password has been validated successfully, check that it still matches the realm's password policy.
PolicyError error = session.getProvider(PasswordPolicyManagerProvider.class).validate(realm, user, input.getChallengeResponse());
if (error != null) {
logger.debug("User password no longer matches password policy");
session.getContext().getAuthenticationSession().setAuthNote(PasswordPolicy.POLICY_ERROR_AUTH_NOTE, "true");
}
}

return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,9 @@ public class AuthenticationManager {
// clientSession note with flag that clientSession was authenticated through SSO cookie
public static final String SSO_AUTH = "SSO_AUTH";

// authSession note with flag that is true if user is authenticated through read-only user backend (e.g. read-only LDAP)
public static final String USER_READ_ONLY = "USER_READ_ONLY";

// authSession note with flag that is true if user is forced to re-authenticate by client (EG. in case of OIDC client by sending "prompt=login")
public static final String FORCED_REAUTHENTICATION = "FORCED_REAUTHENTICATION";

Expand Down
17 changes: 17 additions & 0 deletions themes/src/main/resources/theme/base/login/login-policy-error.ftl
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
<#import "template.ftl" as layout>
<@layout.registrationLayout displayMessage=false; section>
<#if section = "header">
${msg("passwordPolicyErrorTitle")}
<#elseif section = "form">
<div id="kc-terms-text">
${kcSanitize(msg("passwordPolicyErrorMessage"))?no_esc}
</div>
<form class="form-actions" action="${url.loginAction}" method="POST">
<#if !userReadOnly>
<input class="${properties.kcButtonClass!} ${properties.kcButtonPrimaryClass!} ${properties.kcButtonLargeClass!}" name="continueToUpdate" id="kc-accept" type="submit" value="${msg("doContinue")}"/>
</#if>
<input class="${properties.kcButtonClass!} ${properties.kcButtonDefaultClass!} ${properties.kcButtonLargeClass!}" name="cancelUpdate" id="kc-decline" type="submit" value="${msg("doCancel")}"/>
</form>
<div class="clearfix"></div>
</#if>
</@layout.registrationLayout>
Original file line number Diff line number Diff line change
Expand Up @@ -307,6 +307,10 @@ delegationFailedMessage=You may close this browser window and go back to your co

noAccessMessage=No access

passwordPolicyErrorTitle=New password policy
passwordPolicyErrorMessage=Your password no longer matches the password policy set by the administrator and therefore must be updated.\
Click Continue to update your password or click Cancel to go back to the login screen.

invalidPasswordMinLengthMessage=Invalid password: minimum length {0}.
invalidPasswordMaxLengthMessage=Invalid password: maximum length {0}.
invalidPasswordMinDigitsMessage=Invalid password: must contain at least {0} numerical digits.
Expand Down
Loading