KEYCLOAK-11745 Multi-factor authentication #6459

AlistairDoswald · 2019-11-08T15:58:17Z

Co-authored-by: Christophe Frattino christophe.frattino@elca.ch
Co-authored-by: Francis PEROT francis.perot@elca.ch
Co-authored-by: rpo harture414@gmail.com
Co-authored-by: mposolda mposolda@gmail.com
Co-authored-by: Jan Lieskovsky jlieskov@redhat.com
Co-authored-by: Denis drichtar@redhat.com
Co-authored-by: Tomas Kyjovsky tkyjovsk@redhat.com

This feature is based on the Managing multi-factor authentication and Step-up authentication in Keycloak design document. It only does the multi-factor and multi-token parts of this design document, the step-up will be done at a later date.

The newest JIRA this feature is for is here: https://issues.jboss.org/browse/KEYCLOAK-11745,
But it is also the answer to KEYCLOAK-9693 and KEYCLOAK-9694.

The associated documentation is currently available at https://github.com/cloudtrust/keycloak-documentation/tree/multi-token-prototype. It is complete, but as the quality review is not finished, the PR for the documentation will come a couple of days after this pull request.

hmlnarik · 2019-11-12T13:03:13Z

model/jpa/src/main/resources/META-INF/jpa-changelog-8.0.0.xml

+            <column name="PRIORITY" value="20" />
+            <column name="TYPE" value="otp" />
+            <column name="SECRET_DATA" valueComputed="'{&quot;value&quot;:&quot;' || VALUE || '&quot;}'"/>
+            <column name="CREDENTIAL_DATA" valueComputed="'{&quot;subType&quot;:&quot;totp&quot;,&quot;digits&quot;:' || DIGITS || ',&quot;period&quot;:' || PERIOD || ',&quot;algorithm&quot;:&quot;' || ALGORITHM || '&quot;}'"/>


Please verify that PERIOD does not harm MySQL / MariaDB

Verified. Works fine on both MySQL and MariaDB. Tested with MariaDB 10.3.16 , which is the version previously having issues with the PERIOD column.

hmlnarik · 2019-11-12T13:03:35Z

model/jpa/src/main/resources/META-INF/jpa-changelog-8.0.0.xml

+  ~ * limitations under the License.
+  -->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.1.xsd">
+


Please verify this changeset works well with a different database schema

Verified with custom PostgreSQL schema and it works fine.

hmlnarik · 2019-11-12T13:12:15Z

model/jpa/src/main/resources/META-INF/jpa-changelog-8.0.0.xml

+  ~ * limitations under the License.
+  -->
+<databaseChangeLog xmlns="http://www.liquibase.org/xml/ns/dbchangelog" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.1.xsd">
+


Please verify that any of the removed columns is not used in existing migrations.

Verified. It is fine and migration from 1.9.8.Final still works (however I needed to comment some failing migration test, which is failing independently of this issue - https://issues.jboss.org/browse/KEYCLOAK-11635 )

AlistairDoswald · 2019-11-12T16:41:01Z

I've done the pull request for the associated documentation

iankko · 2019-11-12T17:44:15Z

Original KEYCLOAK-11745 PR (as proposed here) with the aforementioned conflicts WRT to current Keycloak master branch fixed.

mposolda

@hmlnarik Thanks for the comments and discussion yesterday. I've verified all the points related to DB migration, but none of them was an issue in the end. I've tested with the latest branch from @iankko pointed in the comment above: https://github.com/iankko/keycloak/tree/KEYCLOAK-11745-fix-conflicts

mposolda · 2019-11-13T13:36:49Z

I've tried to run cross-dc tests on my laptop and they are fine. Failing cross-dc tests here is just travis issue.

I fixed the BrowserFlowTest failing on MariaDB and have the latest stuff in this branch: https://github.com/mposolda/keycloak/tree/KEYCLOAK-11745-fix-conflicts . It contains fixing rebase conflicts from @iankko and one additional commit for fix BrowserFlowTest on MariaDB. Feel free to update PR with that.

I am trying to run the pipeline one more time. I saw also some failures on PostgresPlus in the previous pipeline run, but I think those are unstable tests not related to this PR - I recall also seeing those failures before in some different runs.

vmuzikar

Thanks to all involved!!

As far as I can tell (it's impossible to review ~10 000 lines on such short notice :)) looks good but I've got a few questions.

Is the UI for the User Credentials tab in Admin Console final? There are some minor UI glitches (which don't affect the functionality – it just could look a bit better :)). We also need to add some Admin Console tests for this. This can be done as a follow-up.
Is the UI for the login screen final? If yes, we should add some tests to the Base UI testsuite to ensure it works with all supported browsers (incl. mobile). This could be done as a follow-up. But IIRC some discussion, there will be some polishing?
I've noticed that when you enter a wrong username on the Username form (which doesn't contain password field), "Invalid username or password." is displayed even though there was obviously no password specified. Should do this in a follow-up task?
Do we have a JIRA for redesigning the Authentication Flows in Admin Console? Couldn't find any. Or will this be reworked in the new Admin Console?

Also, I've created https://issues.jboss.org/browse/KEYCLOAK-12035

vramik · 2019-11-14T08:58:15Z

It looks good to me from Migration perspective.

iankko · 2019-11-14T09:17:48Z

@vmuzikar

Hello Vashek,

thank you for looking into this!

Thanks to all involved!!

As far as I can tell (it's impossible to review ~10 000 lines on such short notice :)) looks good but I've got a few questions.
1. Is the UI for the User Credentials tab in Admin Console final? There are some minor UI glitches (which don't affect the functionality – it just could look a bit better :)). We also need to add some Admin Console tests for this. This can be done as a follow-up.

AFAICT there's still space for changes in the Admin Console. If not even now, it can be done as a follow-up work. But could you be more specific about those glitches (or even possibly file a JIRA), so it could be evaluated, how urgent this request is? That would be helpful.

2. Is the UI for the login screen final? If yes, we should add some tests to the Base UI testsuite to ensure it works with all supported browsers (incl. mobile). This could be done as a follow-up. But IIRC some discussion, there will be some polishing?

AFAICT, there's still WIP on adding more tests happening. This (is most likely) to happen within KEYCLOAK-11913 unless something really urgent is found.

Kind request, could you go through the existing JIRAs (subtasks) of KEYCLOAK-11913 to confirm, if the use case is already covered there, and if not yet, kind report a new JIRA, so it's covered in the follow-up?

Thank you

3. I've noticed that when you enter a wrong username on the Username form (which doesn't contain password field), "Invalid username or password." is displayed even though there was obviously no password specified. Should do this in a follow-up task?

Right, good catch. Noticed the issue before too, but somehow it got lost in other work done before. Will file a JIRA for this so it's not forgotten.

4. Do we have a JIRA for redesigning the Authentication Flows in Admin Console? Couldn't find any. Or will this be reworked in the new Admin Console?

What kind of redesigning you mean? The follow-up work is tracked via subtask of KEYCLOAK-11913 one. If you miss this redesign there, please kindly report a new JIRA so it can be considered.

Thank you

Also, I've created https://issues.jboss.org/browse/KEYCLOAK-12035

Great catch! Thank you for reporting it

iankko · 2019-11-14T09:33:18Z

3. I've noticed that when you enter a wrong username on the Username form (which doesn't contain password field), "Invalid username or password." is displayed even though there was obviously no password specified. Should do this in a follow-up task?
Right, good catch. Noticed the issue before too, but somehow it got lost in other work done before. Will file a JIRA for this so it's not forgotten.

Tracked under https://issues.jboss.org/browse/KEYCLOAK-12044 now. I don't think changing the message to plain "Invalid username." would work / do the trick, since it could be used for existing users enumeration brute-force attacks. But we will look into a way, on how to implement this, simultaneously:

Telling the user, the username was wrong,
But not leaking the fact, if the user exists, or not.

Though since this might take a bit more to redesign, I would propose to implement this within KEYCLOAK-11913 follow-up work only.

@AlistairDoswald @mposolda WDYT?

AlistairDoswald · 2019-11-14T10:47:30Z

Tracked under https://issues.jboss.org/browse/KEYCLOAK-12044 now. I don't think changing the message to plain "Invalid username." would work / do the trick, since it could be used for existing users enumeration brute-force attacks. But we will look into a way, on how to implement this, simultaneously:

Telling the user, the username was wrong,

But not leaking the fact, if the user exists, or not.

Though since this might take a bit more to redesign, I would propose to implement this within KEYCLOAK-11913 follow-up work only.

@AlistairDoswald @mposolda WDYT?

@iankko During the design phase, we saw that there would be a user enumeration vulnerability when using the username-only form. The initial reason for the existence of the form is to allow password-less login (e.g. username selection into WebAuthn). We decided that it's up to the administrator to choose whether the risk is worth the convenience.

Currently, in the documentation associated with this PR, there is an example of the creation of a password-less login, and there is a warning that this leaves the system vulnerable to this attack. I'm not sure what sort of redesign would prevent this problem from happening though.

mposolda · 2019-11-14T10:51:16Z

@vmuzikar @iankko Regarding (1), there is also https://issues.jboss.org/browse/KEYCLOAK-11921 . Apart from that, I did not expect much more changes in the screen. If you have suggestions, maybe discuss here or start new topic on keycloak-dev as @iankko suggested. The JIRA for adding new admin console tests, doesn't yet exists AFAIK, so feel free to create :)

Retgardign (2), there will be some more changes in the login forms. So not sure how much to focus on "advanced" tests like mobile right now....

Regarding (3), thanks for reporting and creating for https://issues.jboss.org/browse/KEYCLOAK-12044 .

Regarding (4), there are few subtasks of https://issues.jboss.org/browse/KEYCLOAK-11913 as @iankko mentioned. Mostly based on what we discussed on KC weekly call.

vmuzikar · 2019-11-14T11:01:51Z

@iankko Thanks for creating KEYCLOAK-12044.

@iankko @mposolda Created bunch more follow-up JIRAs:
https://issues.jboss.org/browse/KEYCLOAK-12045
https://issues.jboss.org/browse/KEYCLOAK-12046
https://issues.jboss.org/browse/KEYCLOAK-12047
https://issues.jboss.org/browse/KEYCLOAK-12048

Co-authored-by: Christophe Frattino <christophe.frattino@elca.ch> Co-authored-by: Francis PEROT <francis.perot@elca.ch> Co-authored-by: rpo <harture414@gmail.com> Co-authored-by: mposolda <mposolda@gmail.com> Co-authored-by: Jan Lieskovsky <jlieskov@redhat.com> Co-authored-by: Denis <drichtar@redhat.com> Co-authored-by: Tomas Kyjovsky <tkyjovsk@redhat.com>

mposolda · 2019-11-14T13:44:30Z

Thanks everyone for comments and reviews!

lrozenblyum · 2019-11-18T08:47:17Z

A question posted regarding to methods removal from CredentialRepresentation: https://lists.jboss.org/pipermail/keycloak-user/2019-November/019687.html

abstractj assigned mposolda Nov 8, 2019

abstractj requested review from drichtarik, iankko and tkyjovsk November 8, 2019 19:44

stianst added the Prioritize label Nov 11, 2019

abstractj requested a review from vmuzikar November 11, 2019 13:40

hmlnarik reviewed Nov 12, 2019

View reviewed changes

stianst mentioned this pull request Nov 13, 2019

KEYCLOAK-11772 Fix temporary credential property to work correctly #6402

Merged

mposolda reviewed Nov 13, 2019

View reviewed changes

vmuzikar reviewed Nov 13, 2019

View reviewed changes

vramik previously approved these changes Nov 14, 2019

View reviewed changes

AlistairDoswald dismissed vramik’s stale review via 675037b November 14, 2019 09:44

AlistairDoswald force-pushed the multi-token-prototype-rebased-final branch 3 times, most recently from 2904dda to 0705607 Compare November 14, 2019 10:25

AlistairDoswald force-pushed the multi-token-prototype-rebased-final branch from 0705607 to 90f0272 Compare November 14, 2019 12:37

mposolda approved these changes Nov 14, 2019

View reviewed changes

mposolda merged commit 4553234 into keycloak:master Nov 14, 2019

ssilvert mentioned this pull request Nov 21, 2019

KEYCLOAK-11550: Single page for credentials (initial commit) #6516

Merged

tnorimat mentioned this pull request Nov 24, 2019

2FA Registration process not working webauthn4j/keycloak-webauthn-authenticator#32

Closed

k-tamura mentioned this pull request Jan 14, 2020

KEYCLOAK-11511 Improve exception handling of REST user creation #6339

Merged

fperot74 deleted the multi-token-prototype-rebased-final branch October 6, 2023 15:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KEYCLOAK-11745 Multi-factor authentication #6459

KEYCLOAK-11745 Multi-factor authentication #6459

AlistairDoswald commented Nov 8, 2019

hmlnarik Nov 12, 2019

mposolda Nov 13, 2019

hmlnarik Nov 12, 2019

mposolda Nov 13, 2019

hmlnarik Nov 12, 2019

mposolda Nov 13, 2019

AlistairDoswald commented Nov 12, 2019

iankko commented Nov 12, 2019

mposolda left a comment

mposolda commented Nov 13, 2019

vmuzikar left a comment •

edited

Loading

vramik commented Nov 14, 2019

iankko commented Nov 14, 2019 •

edited

Loading

iankko commented Nov 14, 2019

AlistairDoswald commented Nov 14, 2019

mposolda commented Nov 14, 2019

vmuzikar commented Nov 14, 2019

mposolda commented Nov 14, 2019

lrozenblyum commented Nov 18, 2019

KEYCLOAK-11745 Multi-factor authentication #6459

KEYCLOAK-11745 Multi-factor authentication #6459

Conversation

AlistairDoswald commented Nov 8, 2019

hmlnarik Nov 12, 2019

Choose a reason for hiding this comment

mposolda Nov 13, 2019

Choose a reason for hiding this comment

hmlnarik Nov 12, 2019

Choose a reason for hiding this comment

mposolda Nov 13, 2019

Choose a reason for hiding this comment

hmlnarik Nov 12, 2019

Choose a reason for hiding this comment

mposolda Nov 13, 2019

Choose a reason for hiding this comment

AlistairDoswald commented Nov 12, 2019

iankko commented Nov 12, 2019

mposolda left a comment

Choose a reason for hiding this comment

mposolda commented Nov 13, 2019

vmuzikar left a comment • edited Loading

Choose a reason for hiding this comment

vramik commented Nov 14, 2019

iankko commented Nov 14, 2019 • edited Loading

iankko commented Nov 14, 2019

AlistairDoswald commented Nov 14, 2019

mposolda commented Nov 14, 2019

vmuzikar commented Nov 14, 2019

mposolda commented Nov 14, 2019

lrozenblyum commented Nov 18, 2019

vmuzikar left a comment •

edited

Loading

iankko commented Nov 14, 2019 •

edited

Loading