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

Conversation

sirkrypt0
Copy link
Collaborator

Previously, Keycloak would only validate the password policy for new users and password changes. However, it may be desired to force all existing users to update their passwords when the password policy has changed.

To accomplish this, this adds a new ValidateOnLogin password policy that can be configured per realm much like the existing password policies. When this policy is present, the password of the user will be validated against the current password policy on each login. This can be done for both, local users and users in the LDAP.

Closes keycloak#14150

@sirkrypt0 sirkrypt0 requested a review from lmm-git May 14, 2024 14:33
@sirkrypt0 sirkrypt0 self-assigned this May 14, 2024
Comment on lines +896 to +902
// 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?
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

Copy link
Member

@lmm-git lmm-git left a comment

Choose a reason for hiding this comment

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

Overall, it looks good although I did not test it yet.

Comment on lines +896 to +902
// 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?
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

}

@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() {
}

return true;
}
if(inputData.containsKey("cancelUpdate")) {
return badPasswordHandler(context, user, clearUser, false);
Copy link
Member

Choose a reason for hiding this comment

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

Is this proper use? The password itself was ok, wasn't it?

This would also send a password error mail, doesn't it?. Is this intended?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right, I tried to reuse the existing functionality, but we I agree that might not want the invalid credentials event (which might trigger an email, depending on the settings of course).

@sirkrypt0 sirkrypt0 force-pushed the feature/validate-on-login branch 2 times, most recently from 7e00fa7 to 640404e Compare July 30, 2024 11:40
Previously, Keycloak would only validate the password policy for new
users and password changes. However, it may be desired to force all
existing users to update their passwords when the password policy has
changed.

To accomplish this, this adds a new ValidateOnLogin password policy
that can be configured per realm much like the existing password
policies. When this policy is present, the password of the user will
be validated against the current password policy on each login. This
can be done for both, local users and users in the LDAP.

When the LDAP is in read-only mode and the password no longer matches
the policy, an error is shown, but the user is not given the option to
update their password, as that doesn't work with read-only LDAP.
Administrators with a read-only LDAP are free to disable the policy on
login to avoid this.

Currently, users are only shown a generic error message that their
password no longer matches the policy, but not the exact error.
This is because I didn't find a way to properly pass the PolicyError up
to the authenticator which handles the password validation, as the
policy errors contain parameters (like minimum lower case chars) and
their error messages are localized based on the users locale.

Closes keycloak#14150

Signed-off-by: Tobias Kantusch <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password policy change not forcing users to update password
2 participants