w3c / webauthn

Web Authentication: An API for accessing Public Key Credentials
https://w3c.github.io/webauthn/
Other
1.18k stars 171 forks source link

AttestationResponse vs AssertionResponse #854

Closed apowers313 closed 6 years ago

apowers313 commented 6 years ago

There seems to be a weird asymmetry between AuthenticatorAttestationResponse and AuthenticatorAssertionResponse:

There is no functional reason to return a CBOR map for attestations and an object for assertions, especially when they are containing similar fields. My understanding is that the APIs are this way because the attestation interface was converted to CBOR (with the argument that "servers need to understand CBOR anyway") but the same editorial conversion never happened for assertions.

Perhaps the only reason to find a solution to this issue is that it impacts the users of the API that may be confused by the two different representations. Also for architectural cleanliness and preventing OCD people from losing their minds...

emlun commented 6 years ago

I don't think the attestation interface was converted to CBOR, instead it's just that the attestationObject and the unwrapped authenticatorData are the raw results returned from the authenticator. The argument "servers need to understand CBOR anyway" was against duplicating information (the public key, mainly) from the attestation object in an easier to use format, because the RP will need to parse the binary structures anyway in order to verify the signatures.

Returning them raw also meant that the client didn't have to parse the CBOR. That premise may be out of date now, though, as clients are now expected to blind attestations (i.e., modify the attestationObject) unless the RP asks for direct attestation. But you're right that the only things that need to be passed through untouched are the authenticatorData, clientDataJSON and signature, so it would be functionally equivalent to transform the attestationObject to a JavaScript object structure.

Just off the top of my head, though: One argument against doing that is that new attestation statement formats can't be used if the client doesn't know how to transform them. If that means the raw attestationObject needs to be passed along as well as a fallback, then we're back at duplicating information, which I think would be more confusing than slightly different response formats.

arnar commented 6 years ago

Even without blinding, clients must parse and rewrite attestation objects because authenticators return CBOR maps with integer keys, which are defined in CTAP but not allowed by WebAuthn, and thus would not be understood by an RP.

https://github.com/fido-alliance/fido-2-specs/issues/501

equalsJeffH commented 6 years ago

@apowers313 wrote:

There is no functional reason to return a CBOR map for attestations and an object for assertions...

actually, IIUC, AuthenticatorAttestationResponse will be a javascript object containing two byte arrays, one of which (clientDataJSON) is JSON-serialized data, the other (attestationObject) is as you note a CBOR-encoded binary object. I do not see how this is markedly different from the AuthenticatorAssertionResponse which will be a JS object containing three byte arrays.

We adopted the CBOR-encoded attestationObject as part of coalescing the authenticator data with the attestation statement, while accommodating variable-length authenticator data and multiple attestation statement formats and various other nuances, and with the goal of having the client not need to understand or parse the attestationObject. See omnibus PR #321 from early 2017.

@emlun is correct that now with the advent of AttestationConveyancePreference, the client is obliged by default to parse and alter portions of the attestationObject. Oh well.

At this late stage, I am generally not in favor of making any further changes to these structures/objects. They're implemented and queued to ship and we've demonstrated interoperability.

WRT @arnar's comment regarding authnrs returning CBOR maps with integer keys -- I'm not sure that's correct, see https://github.com/fido-alliance/fido-2-specs/issues/501#issuecomment-377045760

update 2018-Apr-11: fwiw - @arnar's comment is correct, see issue #501

selfissued commented 6 years ago

Declining to make this breaking change at this late date, per the 11-Apr-18 call.

arnar commented 6 years ago

(Note that issue 501 at the end of @equalsJeffH's comment above refers to FIDO repo fido-alliance/fido-2-specs#501 - ignore the automatically linked issue)