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

WebAuthn: Add user id to PublicKeyCredentialsCreateOptions, Authenticator and WebAuthnCredentials (#580, #581) #582

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mnylen
Copy link

@mnylen mnylen commented Aug 6, 2022

Motivation:

Fixes #580 and #581

@@ -60,6 +60,10 @@ public static void fromJson(Iterable<java.util.Map.Entry<String, Object>> json,
obj.setUserName((String)member.getValue());
}
break;
case "userId":
Copy link
Author

Choose a reason for hiding this comment

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

Realized I modified these by hand... how do I run the code generator?

@mnylen mnylen force-pushed the add-user-id-to-authenticator branch from 7a8a277 to acae48b Compare August 6, 2022 07:26
@@ -20,17 +21,25 @@ public void clear() {
}

public Future<List<Authenticator>> fetch(Authenticator query) {
if (query.getUserName() == null && query.getCredID() == null && query.getUserId() == null) {
Copy link
Author

Choose a reason for hiding this comment

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

I thought it was better to explicitly fail the query in the tests if all conditions were null (which should not happen with proper usage / implementation)

@@ -43,27 +46,61 @@ public void testFIDORegister(TestContext should) {
.authenticatorFetcher(database::fetch)
.authenticatorUpdater(database::store);

database.add(
Copy link
Author

Choose a reason for hiding this comment

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

Instead of starting the register test with the authenticator already in database, I changed the test to test that the authenticator was persisted in the database after registration.

}

@Test(timeout = 1000)
public void testFIDOLoginWhenNoAuthenticatorsFoundByCredID(TestContext should) {
Copy link
Author

Choose a reason for hiding this comment

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

Just a little sanity check test to make sure that authenticate() fails if no authenticators are found by the credential id.

@mnylen
Copy link
Author

mnylen commented Aug 6, 2022

For some reason the eclipsefdn/eca action doesn't find my ECA, which I have signed. The Eclipse Foundation portal shows it as signed, the email address should match the email address on the commit, and the GitHub username should match the username configured on the Eclipse Foundation account. :/

Seemed to have resolved itself.

@mnylen mnylen force-pushed the add-user-id-to-authenticator branch from acae48b to b96e8dd Compare August 6, 2022 18:28
* Give some guidance on where to store the challenge

* Note that it's important to scope the challenge to either
  login / registration, or otherwise (as `authenticate` is
  the entrypoint for both operations) one could use a
  registration response to log in.

* Challenge should have some kind of TTL and preferably
  be invalidated after use to prevent replay attacks.
@mnylen
Copy link
Author

mnylen commented Aug 7, 2022

Updated the documentation also. Added a mention that it's up to the caller of createCredentialsOptions and getCredentialsOptions to store the challenge somewhere (with some ideas for where), so it can be retrieved in the second step.

Have to say, I don't super love that the authenticate method is the entrypoint for both registration and login. If the implementer isn't careful, it's all too easy to accept AuthenticatorAttestationResponse (webauthn.create) where AuthenticatorAssertionResponse (webauthn.get) is expected. This could open up a door to register an authenticator to another account through login.

To tackle this somehow, mentioned in the documentation that, when storing the challenge, implementer should scope the stored challenge to either login or registration, so it can only be used in that context. But maybe this is something we should tackle at the API level to prevent possibility for the user to make such mistake.

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.

WebAuthn: createCredentialOptions() uses a random UUID as user handle always
1 participant