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

Fix: passkey login not working anymore #32623

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

hiifong
Copy link
Contributor

@hiifong hiifong commented Nov 23, 2024

Quick fix #32595, use authenticator auth flags to login

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 23, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Nov 23, 2024
@hiifong hiifong changed the title Fix #32595 Fix: passkey login not working anymore Nov 23, 2024
@hiifong hiifong marked this pull request as draft November 23, 2024 15:18
@github-actions github-actions bot added modifies/go Pull requests that update Go code and removed modifies/dependencies labels Nov 23, 2024
@hiifong hiifong marked this pull request as ready for review November 23, 2024 16:38
@james-d-elliott
Copy link

james-d-elliott commented Nov 23, 2024

Yes, you will need to store the flags. This will break for everyone who doesn't have backup eligible authenticators.

See here for more info: https://www.w3.org/TR/webauthn-3/#sctn-credential-backup

@hiifong hiifong marked this pull request as draft November 24, 2024 02:52
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 24, 2024
@hiifong hiifong marked this pull request as ready for review November 24, 2024 05:00
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Nov 24, 2024
@james-d-elliott
Copy link

james-d-elliott commented Nov 24, 2024

Looking at the way you're using this we can probably simplify this with tooling from the go-webauthn side, the binary value of the flags is already available so you don't need to convert it, but we can add a convenience method to derive the flags when reconstructing the credential. Let me know if this is preferable?

@wxiaoguang
Copy link
Contributor

we can probably simplify this with tooling from the go-webauthn side, the binary value of the flags is already available so you don't need to convert it, but we can add a convenience method to derive the flags when reconstructing the credential. Let me know if this is preferable?

If there could be a simple and compatible (backward&forward) solution, it definitely helps a lot. Indeed as a library downstream, I think the fewer details the downstream knows, the more robust the system is. Thank you very much!

@james-d-elliott
Copy link

james-d-elliott commented Nov 25, 2024

Inspect the change in go-webauthn/webauthn#337 and see what you think. I still need to update the docs and will do that time permitting. It's worthwhile to note that I plan on releasing soon but have one other fix I'd like to merge first. It adds an additional field which you should just backup Flags.Raw which can restore the whole of the Flags field with webauthn.NewCredentialFlags()

Also it's worth noting I personally recommend storing the credential attestation (field named Attestation), using json encoding to a TEXT/BYTEA column. I personally also encrypt it in all of my implementations as well as the pub key to avoid database tampering, but depends on your tooling if this is practical.

You don't need to restore it each time, but it will allow you to validate the attestation at a later date if you decide to leverage metadata. I can also provide some fairly decent recommendations on how to handle this, though you can also expect that the recommendations will make their way into the library as well.

@wxiaoguang
Copy link
Contributor

Inspect the change in go-webauthn/webauthn#337 and see what you think. I still need to update the docs and will do that time permitting. It's worthwhile to note that I plan on releasing soon but have one other fix I'd like to merge first. It adds an additional field which you should just backup Flags.Raw which can restore the whole of the Flags field with webauthn.NewCredentialFlags()

Also it's worth noting I personally recommend storing the credential attestation (field named Attestation), using json encoding to a TEXT/BYTEA column. I personally also encrypt it in all of my implementations as well as the pub key to avoid database tampering, but depends on your tooling if this is practical.

You don't need to restore it each time, but it will allow you to validate the attestation at a later date if you decide to leverage metadata. I can also provide some fairly decent recommendations on how to handle this, though you can also expect that the recommendations will make their way into the library as well.

Thank you very much. After reading the code, I think maybe the current struct types are enough. We could just json encode/decode the flags.

Still a question in my mind, how could we recover the existing users without flags correctly? Should "setting BackupEligible=true" for them be enough?

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 25, 2024
@james-d-elliott
Copy link

james-d-elliott commented Nov 25, 2024

Still a question in my mind, how could we recover the existing users without flags correctly? Should "setting BackupEligible=true" for them be enough?

If I understand the issue correctly you're wondering how to deal with previously stored credentials that may be in a state that is not serviceable. In theory the best option here would be to use a default value for the relevant column of NULL, and if it's NULL you add intermediary logic to automatically add the values during the login phase.

If my recollection is correct before you validate the login you can see the challenge response from the authenticator where you can copy those values. If you need help finding that let me know and I'll take a look in about 12 hours.

Effective steps:

  1. Add the column(s) with default values of NULL.
  2. If the columns are NULL:
    1. During a login copy the values from the challenge.
    2. Store the values in the database with an UPDATE.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 26, 2024

In theory the best option here would be to use a default value for the relevant column of NULL, and if it's NULL you add intermediary logic to automatically add the values during the login phase.

Then what this PR did is not right .......

During a login copy the values from the challenge.

Maybe the last question: how to get the flags value from the challenge? I am not familiar with the code base and haven't figure out the key point to get the flags from the login request.


Hmm, get it from protocol.ParseCredentialRequestResponse => parsedResponse.Response.AuthenticatorData.Flags, is it right?

@wxiaoguang wxiaoguang marked this pull request as draft November 26, 2024 00:47
@pull-request-size pull-request-size bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Nov 26, 2024
@james-d-elliott
Copy link

Hmm, get it from protocol.ParseCredentialRequestResponse => parsedResponse.Response.AuthenticatorData.Flags, is it right?

That's correct.

@pull-request-size pull-request-size bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 26, 2024
@wxiaoguang
Copy link
Contributor

wxiaoguang commented Nov 26, 2024

Thank you for your help, finally I think I somewhat could understand the details now.

Now we have 2 commits:

  • c5a32f4 revert unnecessary changes and make some necessary preparations, not that important
  • 5eeac46 !!the key change!! use the default authenticator flags when login, also added some comments there

I think we could resolve the problem now (the same behavior as webauthn 0.10), and leave the database migration to another PR (or if it doesn't really cause security problem, maybe we could just keep using the authenticator auth flags without really storing them?)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 1 This PR needs approval from one additional maintainer to be merged. modifies/frontend modifies/go Pull requests that update Go code modifies/migrations size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Passkey login not working anymore
5 participants