kanidm / webauthn-rs

An implementation of webauthn components for Rustlang servers
Mozilla Public License 2.0
464 stars 79 forks source link

`name` and `displayName` validation of empty strings leads to `InvalidUsername #420

Closed smndtrl closed 3 months ago

smndtrl commented 5 months ago

I did this

let (ccr, skr) = self.inner.start_passkey_registration(
  user_unique_id, 
  "", 
  "", 
  None)
  .expect("Failed to start registration.");

I expected the following

According to webauthn-3, the displayName property SHOULD have empty string as allowed.

If no suitable or [human-palatable](https://www.w3.org/TR/webauthn-3/#human-palatability) name is available, the [Relying Party](https://www.w3.org/TR/webauthn-3/#relying-party) SHOULD set this value to an empty string.

I would argue that the same is valid for name property albeit not explicitly stated, the standard mentions The [Relying Party] MAY let the user choose this value.

What actually happened

https://github.com/kanidm/webauthn-rs/blob/master/webauthn-rs-core/src/core.rs#L195 WebauthnError::InvalidUsername

Version (and git commit)

main

Firstyear commented 5 months ago

https://www.w3.org/TR/webauthn-3/#dictionary-makecredentialoptions

""" user, of type PublicKeyCredentialUserEntity

This member contains names and an identifier for the [user account](https://www.w3.org/TR/webauthn-3/#user-account) performing the [registration](https://www.w3.org/TR/webauthn-3/#registration).

Its value’s [name](https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialentity-name), [displayName](https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialuserentity-displayname) and [id](https://www.w3.org/TR/webauthn-3/#dom-publickeycredentialuserentity-id) members are REQUIRED. 

"""

When used in a make credential, name and displayName are required per the above.

Webauthn in general is a poorly written and confusing specification.

Firstyear commented 5 months ago

Accidentally close, sorry.

Firstyear commented 3 months ago

I think per the spec we must enforce the name is set.

smndtrl commented 3 months ago

I'm with you that they should be set according to spec. Yet any string, even an empty string, SHOULD be a possible value.

Firstyear commented 3 months ago

The spec also used to let you strip and disregard UV in a lot of cases until the spec was updated to "strongly advise" people to check that. It wouldn't be the first time we've been stricter then the spec, and then the spec updates to reflect what this library does. So I'd rather be strict here, inspite of the spec. A string must have a non-empty value.

smndtrl commented 3 months ago

Fine. I'll keep a change around then. Too many UIs behave wildly different depending on the set name/display name properties and for our use case we have 0 identifiers of a user to put into these fields and don't want to add friction with an additional step.