search

duo-labs / py_webauthn

Pythonic WebAuthn 🐍
https://duo-labs.github.io/py_webauthn
BSD 3-Clause "New" or "Revised" License
856 stars 171 forks source link

Allow multiple credentials during assertion #39

Closed woodruffw closed 5 years ago

woodruffw commented 5 years ago

Updates WebAuthnAssertionOptions to take a list of WebAuthnUsers, each of which becomes the basis for a PublicKeyCredentialDescriptor.

Some additional constraints are introduced:

This last constraint isn't explicitly in the specification, but follows from our inclusion of the optional PublicKeyCredentialRequestOptions.rpId field. Our other options are to drop that field entirely (in which case the origin's effective domain will be used), or to drop the check and allow the actual verification to fail later on.

Edit: This introduces a breaking change to the API. If that's undesirable, I could refactor it to test for a single item first and listify it before continuing on to the rest of the changes. Let me know if that's what you'd like!

futureimperfect commented 5 years ago

Hey @woodruffw,

Thanks so much for the PR! This looks really good. Do you mind making the change you suggested to prevent the breaking change to the API, though? Happy to merge once that's complete!

woodruffw commented 5 years ago

Sure, I'll do that first thing tomorrow. Thanks!

woodruffw commented 5 years ago

Alright, I've made it so that the webauthn_user parameter is preserved, and correctly listified into self.webauthn_users when appropriate. This still changes the internal representation, but shouldn't cause any breakage when used to produce assertion information.

@mdedonno1337 brought up a good point in #36 about conflicting (unused) state in WebAuthnUser, although I'm personally more okay with that than with implicitly allowing a WebAuthnUser to have multiple credential IDs, as the latter breaks the correspondence between WebAuthnUser instances and the actual user state used in WebAuthn (i.e., one user, one credential ID). This changeset includes a check to ensure that each all users have the same RP ID, which should be the only relevant shared state in the context of assertion generation.

mdedonno1337 commented 5 years ago

Based upon the name of the class, I assumed that a user (in the application) will have a WebAuthnUser object (with multiple credentials if needed).

futureimperfect commented 5 years ago

This looks good to me, @woodruffw! Thanks again for the PR!

@mdedonno1337, thanks so much for your PR and other contributions! The intention of the WebAuthnUser class was to represent given WebAuthn user (credential_id), not necessarily a user of the Relying Party application, (which could represent many WebAuthnUsers). In that case, I think @woodruffw's PR fits a little better with this model.

Thanks again everyone!

woodruffw commented 5 years ago

Hey @futureimperfect, mind cutting a new release for these changes?

I'd love to use this in https://github.com/pypa/warehouse/pull/5795 😄