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

PublicKeyCredentialUserEntityRepository saves anonymousUser #16385

Open
justincranford opened this issue Jan 9, 2025 · 5 comments
Open

PublicKeyCredentialUserEntityRepository saves anonymousUser #16385

justincranford opened this issue Jan 9, 2025 · 5 comments
Assignees
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Milestone

Comments

@justincranford
Copy link

justincranford commented Jan 9, 2025

Asked in Stack Overflow a week ago.

https://stackoverflow.com/questions/79322876/why-does-spring-security-webauthn-authentication-save-anonymoususer-in-publick

Minimum Viable Example with Steps to Reproduce

I included a Minimum Reproducible Example with Steps in the Stack Overflow post.

App in GitHub: https://github.com/justincranford/spring-security-webauthn-demo

Spring versions used:

  • Spring Boot 3.4.1
  • Spring Security 6.4.1

Expected behavior

Expected behavior is anonymousUser should not be persisted in PublicKeyCredentialUserEntityRepository.java.

Said another way, expectation is WebAuthn functionality should only ever persist UserEntity and Credential, for authenticated users.

However, I see anonymousUser is persisted during WebAuthn Authentication. That seems like a bug.

Or, if there is a valid reason for persisting anonymousUser, I would like to understand the design intent, so I can handle it securely and correctly.

Logs

Logs from my wrapper of MapPublicKeyCredentialUserEntityRepository.java, to highlight what I see during WebAuthn Register and WebAuthn Authenticate.

WebAuthn Register

Notice user u was not found, then saved, then it was looked up again and found.

findByUsername failed, name: u

save, id: Bytes[nF5bm4qc-cztclmzyi-vbvz7ruzS7VOULT8aS9C0kWw], name: u, displayName: u

findByUsername succeeded, name: u, id: Bytes[nF5bm4qc-cztclmzyi-vbvz7ruzS7VOULT8aS9C0kWw], displayName: u

Assumes user logged in at https://localhost:8443/ of my sample app with username=u and password=p, before attempting WebAuthn Register.

WebAuthn Authenticate

Notice user anonymousUser was not found, then saved, and then user u was looked up and found. User u is the correct user saved during WebAuthn Register. It think the saving of anonymousUser is likely a bug.

findByUsername failed, name: anonymousUser

save, id: Bytes[fL8lr_HE0Yfe5DgPYAXOJfcj4OQdWRT8GhNwjHYvnQA], name: anonymousUser, displayName: anonymousUser

findById succeeded, id: Bytes[nF5bm4qc-cztclmzyi-vbvz7ruzS7VOULT8aS9C0kWw], name: u, displayName: u

Assumes user logged out before attempting WebAuthn Authenticate at https://localhost:8443/ of my sample app.

@justincranford justincranford added status: waiting-for-triage An issue we've not yet triaged type: bug A general bug labels Jan 9, 2025
@justincranford justincranford changed the title Why does Spring Security WebAuthn authentication save anonymousUser in PublicKeyCredentialUserEntityRepository? Bug? Spring Security WebAuthn authentication saves anonymousUser in PublicKeyCredentialUserEntityRepository Jan 9, 2025
@Kehrlann
Copy link
Contributor

Kehrlann commented Jan 9, 2025

Hey @justincranford, thanks for reaching out!

This is a known issue. We have merged the first milestone of passkey support, but there are still some rough edges.

In this specific case, see

// FIXME: do not load credentialRecords if anonymous
PublicKeyCredentialUserEntity userEntity = findUserEntityOrCreateAndSave(authentication.getName());

If you'd like to submit a PR, please let me know. Otherwise, it's on my todo list and I'll eventually get to it.

@justincranford
Copy link
Author

Thank you for the update.

Could you clarify "first milestone of passkey support"? I don't know where to find that information.

I am considering submitting a PR, but I have two other open WebAuthn issues that I would consider doing before this one.

https://github.com/spring-projects/spring-security/issues?q=is%3Aissue%20state%3Aopen%20author%3Ajustincranford

I would like to understand what are the known issues from the "first milestone of passkey support", and what work is planned for second milestone. It would be good to avoid duplicate work if it is underway for the next milestone.

@Kehrlann
Copy link
Contributor

I mentioned "first milestone" loosely, I meant that we delivered the core configuration classes for webauthn, as well as support for the basic registration and authentication flows. However, there are some rough edges left. As you pointed out here and in the other two issues, there are also outstanding problems we need to fix, as well as feature requests we need to prioritize (account creation flow).

Some of the rough edges we know about have a FIXME mention in the code (see this search: https://github.com/search?q=repo%3Aspring-projects%2Fspring-security%20fixme&type=code) but do not have associated Github issues.

I'll come back to you about the roadmap. For information, at a minimum, JDBC implementations of the WebAuthn-related repositories will ship with 6.5 (gh-16224).

Re: PRs you'd like to submit, I think both this issue and the serialization PR (gh-16328) do not require a lot of discussion or design work, and will be easier to get merged. I'd hold on the registration flow for now.

@rwinch rwinch changed the title Bug? Spring Security WebAuthn authentication saves anonymousUser in PublicKeyCredentialUserEntityRepository PublicKeyCredentialUserEntityRepository saves anonymousUser Jan 14, 2025
@rwinch rwinch self-assigned this Jan 14, 2025
@rwinch
Copy link
Member

rwinch commented Jan 15, 2025

Thanks for the detailed report & sample application @justincranford!

You are correct that saving of anonymousUser is a bug. The location is where @Kehrlann highlighted (thanks for the reply!). Here we should make a few changes:

  • If the authentication is anonymous, then credentialRecords should be an empty list (do not look up the credentialRecords at all
  • findUserEntityOrCreateAndSave should be this.userEntities.findByUsername(username) so no user is ever saved (that should only happen on registering a credential
  • If the result of this.userEntities.findByUsername(username) is null, then credentialRecords should be an empty list

NOTE: All of this logic might be best in a new method findCredentialRecords(Authentication).

@justincranford If you are interested in creating a Pull Request, we'd love to get one! Please be sure create tests and to target the 6.4.x branch since this is a bug.

@rwinch rwinch added in: web An issue in web modules (web, webmvc) and removed status: waiting-for-triage An issue we've not yet triaged labels Jan 15, 2025
@rwinch rwinch added this to the 6.4.x milestone Jan 15, 2025
@rwinch
Copy link
Member

rwinch commented Jan 16, 2025

@justincranford / others - if you would like to work on this issue please mention it on this issue first

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web An issue in web modules (web, webmvc) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

3 participants