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

No implementation to check "attestation type" #80

Closed
sorah opened this issue Oct 5, 2018 · 4 comments
Closed

No implementation to check "attestation type" #80

sorah opened this issue Oct 5, 2018 · 4 comments

Comments

@sorah
Copy link
Contributor

sorah commented Oct 5, 2018

leaving this as an issue to note, this is not a requirement from me.

http://w3c.github.io/webauthn/#sctn-attestation-types

There several lines like the following in the attestation statement verification steps:

If successful, return implementation-specific values representing attestation type Basic, AttCA or uncertainty, and attestation trust path x5c.

Maybe returning attestation type from AttestationStatement::Base#valid? could be an interface?

@bdewater
Copy link
Collaborator

I believe this is useful for scenarios where you would want to allow passwordless login. By storing the attestation data during the registration ceremony, a RP could periodically check with the FIDO metadata service whether the authenticator is still considered trustworthy.

I'd like to work on this. Any preference for the API? I was thinking of an ActiveModel-like interface. Calling AuthenticatorAttestationResponse#valid?would return true and then the attributes attestation_type and attestation_trust_path would be set to values. Implementations of AttestationStatement::Base#valid? would return those two values in a simple array.

@brauliomartinezlm
Copy link
Member

Hey @bdewater, thanks for the continuous efforts 👍

I think your suggestion follows this statement in section [6.4.2]https://www.w3.org/TR/webauthn/#attestation-formats), specifically:

The procedure returns either:

An error indicating that the attestation is invalid, or

The attestation type, and the trust path. This attestation trust path is either empty (in case of self attestation), an identifier of an ECDAA-Issuer public key (in the case of ECDAA), or a set of X.509 certificates.

Given that, your suggestion makes total sense.

Later in our path we will probably have to think about better erroring to be raised when some verification procedures fail, other than just returning false when AuthenticatorAttestationResponse#valid? fails. When that happens I see the possibility of bubbling up custom errors as the result of such call. I started doing some work around that, but I haven't been able to keep up.

Maybe an intermediate option, without breaking compatibility would be to internally rescue such error in the AuthenticatorAttestationResponse#attestation_statement method for now? In the future I foresee the ability to even add configuration around this.

@bdewater
Copy link
Collaborator

I started poking at this earlier this week but I'm blocked on getting a working account for the FIDO metadata service to get an access token. I believe I need this to make a distinction between 'basic' and 'attestation CA' as described at https://w3c.github.io/webauthn/#attca

Note: Attestation statements conveying attestations of type AttCA use the same data structure as attestation statements conveying attestations of type Basic, so the two attestation types are, in general, distinguishable only with externally provided knowledge regarding the contents of the attestation certificates conveyed in the attestation statement.

An alternative would be to return 'uncertainty' for now (quoting 'packed' attestation):

  • Optionally, inspect x5c and consult externally provided knowledge to determine whether attStmt conveys a Basic or AttCA attestation.
  • If successful, return implementation-specific values representing attestation type Basic, AttCA or uncertainty, and attestation trust path x5c.

@grzuy
Copy link
Contributor

grzuy commented Nov 8, 2018

v1.7.0 is out with this included.

Kudos @bdewater 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants