Recovery auth codes by rh-iatanaso · Pull Request #8730 · keycloak/keycloak

rh-iatanaso · 2021-11-08T20:19:23Z

Initial implementation for the Recovery codes base on #8518

rh-iatanaso · 2021-12-15T19:49:06Z

@mposolda @stianst Can I get approval for running the workflows . Also most of the feedback I can incorprate from the discussion is in the PR

rh-iatanaso · 2021-12-17T15:23:16Z

@stianst @mposolda Can I get anothe approval for the work flow , we had issue which we addressed.

rh-iatanaso · 2022-01-03T15:47:19Z

@stianst @mposolda Can I get another workflow approval for this PR ?

mposolda

@rh-iatanaso I think this is great work. Congratulations to you and to the team!

I've added few comments inline. Most of them are minor things.

Besides that, I have one concern: Do we want Recovery Authentication Codes required action to be registered by default in the realms? I think that it will be good similarly like Configure OTP action is registered by default. I know that WebAuthn related actions are not registered by default, but that is related to the fact that WebAuthn was not supported. However the "Recovery codes" are supported from day 1 and hence required action might be good to be registered by default from day 1?

mposolda · 2022-01-11T17:07:22Z

I think it will be better if required action is added to the authentication session rather than to the user. See OTPFormAuthenticator or WebAuthnAuthenticator classes for the inspiration. Or is there any reason why for backup codes it needs to be done differently than for OTP/WebAuthn?

It should be added to auth-session right, otherwise we're updating the user session when that's not needed, and also it would linger in a subsequent login, which isn't correct.

What priority should I give the required action for recovery codes? Should it be very low or should it be near where the Configure OTP action is in priority?

Hmm.... I am not sure what priority you have in mind? Could you please elaborate? This note was about that instead of calling user.addRequiredAction(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name()) you can call something like authenticationSession.addRequiredAction(UserModel.RequiredAction.CONFIGURE_RECOVERY_AUTHN_CODES.name()) to make sure that adding required action is scoped only to this authentication and not to user himself. There is no any priority involved unless I am missing something?

I must have commented on the wrong note. I understand what to do about removing user.addRequiredAction.

My question was about the note above that about making the required action default. There is a priority that's a number. Right now, when you register a required action it gives it the lowest priority.

See the MR at https://github.com/rh-iatanaso/keycloak/pull/7/files. In this MR I've set it to 5 but that's almost certainly wrong. @mposolda

mposolda · 2022-01-11T17:08:27Z

Minor: Star imports should be avoided in Keycloak project (It is the rule, which we try to follow in Keycloak, even if it is probably not officially documented anywhere...)

mposolda · 2022-01-11T17:09:21Z

This class is used in RecoveryAuthnCodesAuthenticatorTest, but this test does not use it for anything. So since this class is effectively not used anywhere, it can be possible removed from this PR?

mposolda · 2022-01-11T17:09:30Z

This class is used in RecoveryAuthnCodesAuthenticatorTest, but this test does not use it for anything. So since this class is effectively not used anywhere, it can be possible removed from this PR?

mposolda · 2022-01-11T17:11:03Z

Minor: This line seems to be redundant and can be removed. Class TotpGenerator likely doesn't need to be changed in this PR

stianst

Documentation and release notes need to be updated for this with a separate PR to keycloak-documentation
I would remove the label (My generated recovery authentication codes0) as it has no practical use for a user as there can only be one set of recovery codes
User can't update the codes, but have to remove and re-create. It should be made updatable and allow generating new codes when the user clicks the update button
Neither admin or account console shows how many codes are left

stianst · 2022-01-12T11:17:35Z

@mposolda does an authenticator have to deal with brute-force itself? I would have thought that would be built-into the flow somehow?

I was thinking the same, but it is not :/ The individual authenticators currently checks by itself (For example AbstractUsernameFormAuthenticator.isDisabledByBruteForce ).

I don't know what was the motivation to do it this way? I guess it can be related to the fact that you still want to be able to authenticate user for example by CookieAuthenticator (or some others) even if he is temporarily disabled.

So for example flow like:

Valid user authenticates and creates session1

Attacker tries brute forcing in different laptop and this cause user to be temporarily disabled

Valid user is currently still able to SSO re-authenticate in session1 even if he is disabled by brute force.

If we check brute force at the flow level for every authenticator, it would mean that SSO re-authentication with cookie may not work and hence attacker has "power" to break the valid SSO sessions of existing users. Which is also not ideal...

Is there consensus on whether we should leave it as is, or remove the brute-force code?

We should leave it as is since there's no "global" check for it. @mposolda I think it may be worth re-visiting in the future, so it's not individual authenticators that control brute-force, but I do agree that the "cookie" is a special case though

@stianst "Neither admin or account console shows how many codes are left" the account console shows how many codes are left. Where would we show it on the admin console? Is that something we can revisit later?

@stianst +1 to revisit this in the future

stianst · 2022-01-12T11:18:52Z

It should be added to auth-session right, otherwise we're updating the user session when that's not needed, and also it would linger in a subsequent login, which isn't correct.

stianst · 2022-01-12T11:19:34Z

I don't think we should support recover codes at all with direct-grant, so I would actually suggest removing this one completely?

I was wondering about the same yesterday...

However I was thinking about the case when 2-factor is required for browser login and at the same time, you have "Direct Grant" enabled for some clients. Isn't it strange that browser login will require 2-factor, but direct-grant allows to login with simple username/password? Hence it makes sense to me to have authenticator for direct-grant as well (similarly like we have for OTP).

This is exactly what we were thinking that it would just be strange not to be able to use recovery codes since you can use it for OTP

"Direct grant" is deprecated and should not be used, so I would not even consider it for new authenticators. As a general thing a recovery code makes no sense in a direct grant scenario either, so we should not have a direct grant recovery code authenticator, although still support password+otp with direct grant until we eventually completely remove that flow.

stianst · 2022-01-12T11:22:37Z

I can't see where quite a few of these message keys are used. Like backupcodesIntroMessage and backupCodesTips?

Those where old messages from older implementation, I can remove the messages as part of this PR

Either remove them not or not update them. I suspect these where used by OTP a long time ago.

stianst · 2022-01-12T11:25:36Z

I assume all the translations are just google translate or something? We don't usually add translations, but require the community to contribute those so they can be reviewed properly

Those where actually there , At some point there was effort to add backup codes and the translations where there , I just updates the names to match the RecoveryAuthenCodes. I can remove them if needed

Hmm.. had no clue that we had ever had anything around backup codes. You sure these are not related to the old way of setting up OTP?

stianst · 2022-01-12T11:28:07Z

This should be made a bit shorter

This sounds better -> "These codes can be used to regain your access in case your other 2FA means are not available"

mposolda · 2022-01-12T14:59:51Z

@stianst @rh-iatanaso Added some inline comments:

Documentation and release notes need to be updated for this with a separate PR to keycloak-documentation

I would remove the label (My generated recovery authentication codes0) as it has no practical use for a user as there can only be one set of recovery codes

+1 . However currently account console is hardcoded to display credential label. I think that if we want to display remaining count of codes instead of label, it will require some changes at the SPI level.

I think that CredentialTypeMetadata class can have new field like accountConsoleLabel, which will by default fallback to credential user label. For Backup codes credential, it can instead show something like Remaining codes count: 10 text (localized to proper user locale).

This will require few changes though, so question is, if we want to do it as follow-up or still have it in this PR?

* User can't update the codes, but have to remove and re-create. It should be made updatable and allow generating new codes when the user clicks the update button

+1 . I think current SPI allows this if you use CredentialTypeMetadata.updateAction for recovery code credential provider?

* Neither admin or account console shows how many codes are left

Admin console displays it when you click on the credential and click "Show data", it shows remaining codes count. Now I figured there is bug with "total codes", which displays incorrect count (I've commented in the inline comment for this).

For account console, I commented above.

mposolda · 2022-01-12T15:18:15Z

It will be ideal if we can avoid the account-console code specific for "recovery-code" credential type.

The account console is supposed to be used with custom credential type implementations provided by administrator. Having the code specific for each credential does not scale IMO. Will be good if we can handle all of this with credential metadata.

I can move the userLabel generation inside the code. I am not sure how to call the alert in agnostic way.

Yes, it can be a bit tricky to do it properly and might require some small changes in the SPI/infrastructure itself.

One possible path I am thinking about is along the way of:

Introduce method CredentialProvider.getCredentialMetadata . This will have likely credential as one of the arguments and CredentialTypeMetadata as another argument. I suppose this method will be called from the method AccountCredentialResource.credentialTypes and likely will be called for each credential of the user

Credential metadata can have fields like the credential itself, but also some others like:

infoMessage (This can be credential.userLabel by default, but for recovery codes, this may be some different text like "Remaining codes: 8")

warningMessage (This can be null by default, but it can be the message for recovery codes displayed here in case there is small amount of remaining codes)
Both infoMessage and warningMessage likely should have support for localization including parametes, so theme can translate them into proper localized message

I can imagine something like this:

class CredentialMetadata { CredentialModel credential; LocalizedMessage infoMessage; LocalizedMessage warningMessage; class LocalizedMessage { String key; Object parameters; // Parameters of localized message. Something similar to class `FormMessage` from `keycloak-services` module } }

Hopefully these metadata can be consumed from the account console credential screen.

WDYT?

Right now, CredentialModel contains credentialData. Are you saying we should add credentialMetadata to CredentialModel? Or add credentialModel to CredentialMetadata and then return instances of CredentialMetadata via JSON? @mposolda

@stelewis-redhat Yes, the second thing you mentioned. I am thinking about CredentialMetadata as separate object, which will contain "CredentialModel" as one of the fields. See my comment above.

The main motivation is that SigningInPage.tsx doesn't need to contain any if/else blocks or other hacks specific to Recovery-code credential. As it will be ideal that customers, who potentially implement their own credentials/authenticators don't need to change UI classes, but SPI is good enough so they can add their own info/warning labels shown alongside their credential just by implementing this method on CredentialProvider interface.

OK. I can switch that around. I have a working solution for the first, but it shouldn't be too hard to swap that structure.

@mposolda Is it possible to revisit making CredentialMetadata contain CredentialModel, after we do the account console redesign? We're a bit worried about the merge/rebasing issues with these two initiatives going on at once.

Here's what I currently have working, for reference: rh-iatanaso#14 not final, work in progress.

Here's the discussion for the console redesign: #9923

I vote to avoid doing changes in CredentialModel and CredentialRepresentation classes and rather do it the other way around.
The problem with changing CredentialModel is, that the class is part of public SPI and part of the model. This mean that if we change anything in it, it will become part of "public SPI" and it is a pain to remove/change it later. So ideal would be if the initial commit already is done in a way that CredentialModel does not need to be changed.

OK. Thank you @mposolda

stelewis-redhat · 2022-02-14T13:55:54Z

For the required action as default, what priority should it have? Same as OTP? Or something else?

stelewis-redhat · 2022-03-04T14:21:49Z

@stianst @thomasdarimont @stelewis-redhat I missed some discussions related to password policy, but IMO password policies should be used to configure only things related to passwords. Using password policy for configure stuff related to "recovery codes" seems to me like a workaround and abusing password policies for something, which is not related to "password" at all.
I think it will be better to just hardcode the number to 4 to specify when warning for remaining codes should be displayed. And possibly expand this with "Recovery Code Policies" (or something like that) in some future release based on the feedback from the community.
So my vote is definitely to remove RecoveryCodesWarningThresholdPasswordPolicyProviderFactory and related stuff from this PR and just have the "4" as hardcoded number of remaining counts when warning is displayed.

Removing.

Removed @mposolda

mposolda · 2022-03-07T09:12:02Z

@stelewis-redhat The PR looks good to me! Thanks for all the updates to it!

I hope we can make this merged soon. I see the remaining steps are:

Check the tests are ok (will wait until GH actions are finished in this PR. I hope there is not any issue here)
Documentation - the PR should be send to it to https://github.com/keycloak/keycloak-documentation . I think it will be good to update the sections:
-- Profiles section in the installation guide. New column for recovery codes https://www.keycloak.org/docs/latest/server_installation/#profiles should be added here
-- Release notes for Keycloak 18 - this worth to be mentioned in the release notes (Please mention here that it is "preview" feature)
-- New section under https://www.keycloak.org/docs/latest/server_admin/#configuring-authentication . I suggest to add only relatively small section. Please don't add any screenshot or example of how to configure authentication flows. There is already bunch of examples of how to configure authentication flows in the other sections (For example in the WebAuthn section). And adding another example of that means more refactoring in the future (For example when we add authentication policies. So regarding configuration, please just add something like "You can configure REcovery codes as two-factor authenticator. You need to add authenticator 'Recovery Authentication Code Form' as two-factor authenticator to your authentication flow. See the WebAuthn section above (link to something like https://www.keycloak.org/docs/latest/server_admin/#configuring-authentication ) to see an example of how to configure authenticator as two-factor authenticator" ). Also please add that this is available just if the feature is enabled.

stelewis-redhat · 2022-03-07T14:10:05Z

@stelewis-redhat The PR looks good to me! Thanks for all the updates to it!

I hope we can make this merged soon. I see the remaining steps are:

Check the tests are ok (will wait until GH actions are finished in this PR. I hope there is not any issue here)

Is there anything I need to do on this? I fixed the tests that seemed to be directly related, but I'm unsure about the things that are failing. They seemed to be related to timing issues, last week. Things were timing out.

I can see tests are ok in GH actions, so you are fine :-) The "Keycloak Operator CI" failed test is not related to this PR as you pointed.

So the docs seems to be only remaining task though from my point of view.

stelewis-redhat · 2022-03-07T20:20:38Z

Documentation - the PR should be send to it to https://github.com/keycloak/keycloak-documentation . I think it will be good to update the sections:

Also, what do I need to do to send it to the keycloak-documentation project? Is there someone in particular I need to notify? Open an issue in that project?

mposolda · 2022-03-08T07:40:49Z

Documentation - the PR should be send to it to https://github.com/keycloak/keycloak-documentation . I think it will be good to update the sections:

Also, what do I need to do to send it to the keycloak-documentation project? Is there someone in particular I need to notify? Open an issue in that project?

Feel free to just send PR to the keycloak-documentation project and then comment here with the link to that PR. Also in the comment of the PR to keycloak-documentation, please add comment (or add it directly to the description of that PR) with the link to this "main codebase" PR.

stelewis-redhat · 2022-03-08T13:29:38Z

Documentation - the PR should be send to it to https://github.com/keycloak/keycloak-documentation . I think it will be good to update the sections:

Also, what do I need to do to send it to the keycloak-documentation project? Is there someone in particular I need to notify? Open an issue in that project?

Feel free to just send PR to the keycloak-documentation project and then comment here with the link to that PR. Also in the comment of the PR to keycloak-documentation, please add comment (or add it directly to the description of that PR) with the link to this "main codebase" PR.

OK, so I'm updating the documentation as well?

mposolda · 2022-03-08T13:36:16Z

OK, so I'm updating the documentation as well?

Yes, it will be nice if you can do it, so we can merge this PR together with the documentation.

stelewis-redhat · 2022-03-08T13:50:38Z

OK, so I'm updating the documentation as well?

Yes, it will be nice if you can do it, so we can merge this PR together with the documentation.

No problem. Starting now.

stelewis-redhat · 2022-03-08T15:07:50Z

OK, so I'm updating the documentation as well?

Yes, it will be nice if you can do it, so we can merge this PR together with the documentation.

keycloak/keycloak-documentation#1429 created. I've linked this PR from that PR. @mposolda

mposolda · 2022-03-09T13:33:32Z

@stelewis-redhat Sorry, but there is conflict as some other features were added into the "Profile" and "ProfileTest" in the meantime :/ Do you please have a chance to rebase this to latest "main" and fix the conflict?

stelewis-redhat · 2022-03-09T13:56:13Z

@stelewis-redhat Sorry, but there is conflict as some other features were added into the "Profile" and "ProfileTest" in the meantime :/ Do you please have a chance to rebase this to latest "main" and fix the conflict?

I'll look at it.

Recovery auth codes modify after review (#17) * CIAM-2154: Make required action default. (#7) * CIAM-2162: shorten help text message. (#9) * CIAM-2197: Remove direct grant. (#11) * CIAM-2197: Missed removing direct grant stuff from services. (#13) * cleared out the label for remaining recovery codes (#12) Co-authored-by: zwitter <zwitter@redhat.com> * CIAM-2154: Change recovery code required action default priority. (#16) * CIAM-2155: set updateAction to make codes updatable. (#8) * CIAM-2153: Tie required action to session instead of user. (#10) * Ciam 2165 make generic metadata (#14) * CIAM-2165: work in progress. * CIAM-2165: add missing import. * CIAM-2165: minor cleanup. * CIAM-2165: first attempt at getting signing page to use new data structure. * CIAM-2165: Add missing java file. * CIAM-2165: default to null instead of null string. * CIAM-2165: fix messages. * CIAM-2165: Clean up exception handling. * CIAM-2165: Stop adding metadata to credentialRepresentation. * CIAM-2165: Fix tabbing. * CIAM-2165: Fix tabbing again. * CIAM-2161: remove unused messages. (#15) * CIAM-2161: remove unused messages. * CIAM-2161: Remove old messages from previous version of recovery codes. * CIAM-2161: Remove old messages from previous version of backup codes. * CIAM-2161: Remove backupcodesIntroMessage from properties. * Ciam 2165 make generic metadata (#18) * CIAM-2165: work in progress. * CIAM-2165: minor cleanup. * CIAM-2165: first attempt at getting signing page to use new data structure. * CIAM-2165: default to null instead of null string. * CIAM-2165: fix messages. * CIAM-2165: Clean up exception handling. * CIAM-2165: Stop adding metadata to credentialRepresentation. * CIAM-2165: Fix tabbing. * CIAM-2165: Fix tabbing again. * CIAM-2165: Fix failing tests. * CIAM-2165: Improve log message. * CIMA-2165: Remove duplicate method. * CIAM-2165: Remove unused interface. * CIAM-2165: skip secretdata readValue if it's null. Co-authored-by: Zachary Witter <torquekma@gmail.com> Co-authored-by: zwitter <zwitter@redhat.com> CIAM-2165: Fix npe that could happen. (#19) CIAM-2295: Make suggested changes from reviewers. (#21) * CIAM-2295: Make suggest changes from Thomas. * CIAM-2295: Make threshold configurable via password policy. * CIAM-2295: Make other changes requested. * CIAM-2295: More cleanup. * CIAM-2295: Clarify the message for recovery codes used/total. * CIAM-2295: Remove comment. * CIAM-2295: Get feature flag working. * CIAM-2295: add feature flag for recovery codes. * CIAM-2295: restore whitespace.

mposolda · 2022-03-10T12:18:00Z

@stelewis-redhat Can I request one last update to the documentation inside this PR? See the comment keycloak/keycloak-documentation#1429 (comment) . If you can just update the section with preview features inside the file "features.adoc" and make sure that this feature for recovery codes is listed here?

stelewis-redhat · 2022-03-10T14:28:06Z

@stelewis-redhat Can I request one last update to the documentation inside this PR? See the comment keycloak/keycloak-documentation#1429 (comment) . If you can just update the section with preview features inside the file "features.adoc" and make sure that this feature for recovery codes is listed here?

@mposolda it looks like features.adoc is driven by the java in Profile.java, so I think there's no more changes necessary?

mposolda · 2022-03-10T14:44:18Z

@stelewis-redhat Good point :-) I am not very familiar with this stuff yet and didn't check it. Thanks again to you and everyone from your team for the patience with this task!

stelewis-redhat · 2022-03-10T14:45:49Z

@stelewis-redhat Good point :-) I am not very familiar with this stuff yet and didn't check it. Thanks again to you and everyone from your team for the patience with this task!

Thank you @mposolda @stianst and @thomasdarimont !

Closes keycloak#9540 Co-authored-by: Zachary Witter <torquekma@gmail.com> Co-authored-by: stelewis-redhat <91681638+stelewis-redhat@users.noreply.github.com>

heddn · 2022-03-14T13:45:41Z

I know this is a bit off-topic, but when is v18.0.0 planned for release so this feature will be more generally available?

stianst reviewed Nov 9, 2021

View reviewed changes

Comment thread server-spi-private/src/main/java/org/keycloak/forms/login/LoginFormsPages.java Resolved

rh-iatanaso force-pushed the recovery_auth_codes branch from 34952db to 18e0e96 Compare November 18, 2021 14:52

mposolda self-requested a review November 19, 2021 09:37

rh-iatanaso force-pushed the recovery_auth_codes branch from 18e0e96 to 9c7a618 Compare November 19, 2021 13:12

rh-iatanaso marked this pull request as ready for review November 19, 2021 13:13

mposolda mentioned this pull request Nov 24, 2021

Add ability to set default authenticator #8397

Closed

rh-iatanaso force-pushed the recovery_auth_codes branch 3 times, most recently from 6622483 to cdaac9b Compare December 15, 2021 19:47

rh-iatanaso force-pushed the recovery_auth_codes branch 2 times, most recently from f1122e1 to 4bc6818 Compare December 17, 2021 15:19

rh-iatanaso force-pushed the recovery_auth_codes branch 4 times, most recently from bdd03f4 to a579f26 Compare January 6, 2022 14:59

This was referenced Jan 7, 2022

otp backupcodes incompatible with 16.x thomasdarimont/keycloak-extension-playground#37

Closed

OTP base32 decode improvements #9434

Closed

mposolda requested changes Jan 11, 2022

View reviewed changes

stianst requested changes Jan 12, 2022

View reviewed changes

mposolda reviewed Jan 12, 2022

View reviewed changes

Comment thread ...r-spi/src/main/java/org/keycloak/models/credential/dto/RecoveryAuthnCodesCredentialData.java Resolved

mposolda reviewed Jan 12, 2022

View reviewed changes

rh-iatanaso mentioned this pull request Jan 12, 2022

Recovery authentication codes #9540

Closed

mposolda self-assigned this Jan 21, 2022

rh-iatanaso force-pushed the recovery_auth_codes branch 2 times, most recently from 4f2a32a to cd1e0bf Compare February 7, 2022 20:27

mposolda added this to the 18.0.0 milestone Mar 7, 2022

mposolda added missing/docs priority/important Must be worked on very soon area/authentication Auth flows, authenticators, required actions, OTP, password policies, brute force labels Mar 7, 2022

stianst modified the milestones: 18.0.0, 17.1.0 Mar 8, 2022

stelewis-redhat mentioned this pull request Mar 8, 2022

Add documentation for recovery codes preview feature. keycloak/keycloak-documentation#1429

Merged

stelewis-redhat force-pushed the recovery_auth_codes branch from d13e0c9 to 8226ee7 Compare March 9, 2022 14:12

mposolda approved these changes Mar 10, 2022

View reviewed changes

mposolda merged commit 5c6b123 into keycloak:main Mar 10, 2022

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!