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/simplify-encoding-user-handle-in-assertion-response #120

Merged

Conversation

MasterKale
Copy link
Owner

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().

This is the opposite of how `user.id` is converted to an ArrayBuffer prior to being passed into `navigator.credentials.create()`, so when this value is returned to the RP in an assertion it will now look like the string value originally specified in attestation options, instead of the base64url-encoded version of that string “for some reason”.
@akanass
Copy link
Contributor

akanass commented Apr 16, 2021

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
Copy link
Owner Author

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
Copy link
Contributor

akanass commented Apr 16, 2021

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

No Base64URL-encoding/-decoding ever took place with this value. The server passes it to the browser in attestation options as-is, while `startAttestation()` merely converts it to an ArrayBuffer.
@MasterKale MasterKale merged commit becd141 into master Apr 18, 2021
@MasterKale MasterKale deleted the fix/simplify-encoding-user-handle-in-assertion-response branch April 18, 2021 05:06
@MasterKale MasterKale added package:browser @simplewebauthn/browser package:types @simplewebauthn/typescript-types labels Apr 18, 2021
@Ajz316
Copy link

Ajz316 commented Feb 6, 2024

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
Copy link
Owner Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:browser @simplewebauthn/browser package:types @simplewebauthn/typescript-types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants