MasterKale / SimpleWebAuthn

WebAuthn, Simplified. A collection of TypeScript-first libraries for simpler WebAuthn integration. Supports modern browsers, Node, Deno, and more.
https://simplewebauthn.dev
MIT License
1.63k stars 137 forks source link

fix/simplify-encoding-user-handle-in-assertion-response #120

Closed MasterKale closed 3 years ago

MasterKale commented 3 years ago

While dogfooding my own library it confused me why I'd pass in '5c240dc4-bfdf-474b-929c-c4484008a302' as user.id in attestation options, but get back 'NWMyNDBkYzQtYmZkZi00NzRiLTkyOWMtYzQ0ODQwMDhhMzAy' as userHandle in the assertion.

I realized that userHandle, returned in an assertion response, is Base64URL-encoded before coming out of startAssertion() for transmission back to the RP. It doesn't make any sense to do this, though, as the value is never Base64URL-encoded coming out of generateAttestationOptions() or within startAttestation(). This PR fixes this behavior so that the value of userHandle in an assertion response is the same as the value of user.id passed into navigator.credentials.create().

akanass commented 3 years ago

This change makes sense but it will have to be indicated in the documentation because the userHandle is stored in Buffer in the DB, following Yubico's recommendations on the data structure and as we have already discussed in the past, which means that the search for information for usernameless / passwordless, which is based on this field, will have to be adapted because the transformation into Buffer will no longer have the base64 encoding

And of course the types will also change at the typescript level between server and client communication because a dedicated base64string existed

MasterKale commented 3 years ago

And of course the types will also change at the typescript level between server and client communication because a dedicated base64string existed

This is a good callout, I forgot about updating the types (because base64urlString is just an alias for string, I really wish we had more nuance in typing...) 😬

I think I'm terms of usability this'll be a better way to go. Developers can more easily pass in, say, a UUID string for a user and get it back later in a recognizable format. It'll still be up to the RP to choose the encoding of the userHandle within its DB, whether that's raw bytes, Base64URL-encoded, UTF8-encoded...this change just makes sure that whatever you pass in during attestation you get back during assertion.

akanass commented 3 years ago

I will be in that case that's why I told you 😀 and when I will finish to update my project with the latest version I will give you the link like this you will be able to see the implementation

And maybe you have to check the server part as well if typings are changing

Ajz316 commented 9 months ago

While dogfooding my own library it confused me why I'd pass in '5c240dc4-bfdf-474b-929c-c4484008a302' as user.id in attestation options, but get back 'NWMyNDBkYzQtYmZkZi00NzRiLTkyOWMtYzQ0ODQwMDhhMzAy' as userHandle in the assertion.

I realized that userHandle, returned in an assertion response, is Base64URL-encoded before coming out of startAssertion() for transmission back to the RP. It doesn't make any sense to do this, though, as the value is never Base64URL-encoded coming out of generateAttestationOptions() or within startAttestation(). This PR fixes this behaviour so that the value of userHandle in an assertion response is the same as the value of user.id passed into navigator.credentials.create().

Good morning, I came across this conversation while debugging an issue in reusing a passkey that was created by another client layer on the same machine/laptop. It seems the other client layer (another JavaScript library that persisted or created the passkey) is doing what this library was doing prior to this pull request i.e. converting the user.id from Base64Encoding to its original value and using that as passkey ID. So, my understanding is that user.id is generally a RP generated unique code, could be a HEX string as well. But for correct transmission and consistent interpretation we use Base64encoded version for communication purposes. It is up to the client to then decode it and store the correct ID or user device. Now, with this pull request my understanding is we have kept it simple and no longer decoding Base64Encoded string but storing it on client device straightaway, as that is what the RP needs anyway in all communications. Seems simple, well contained and should work in majority of the cases, but I think with synced passkeys it might fail? Just seeing if it will be a possibility to restore the original implementation may be controlled by a flag so that it is backward compatible with existing consumers of this library?

MasterKale commented 9 months ago

Hello @Ajz316 your comment will get lost here in a three-year-old PR. Can you instead please take a look at https://github.com/MasterKale/SimpleWebAuthn/discussions/515 and let me know your thoughts over in there? I might end up going full circle...