w3c / webauthn

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

detail-level issues in signature format, attestation format(s), attestation statement #233

Closed equalsJeffH closed 6 years ago

equalsJeffH commented 7 years ago

There's various detail-level issues in the signature format, attestation format(s), attestation statement sections. Here's at least some of them..

1) the term DAA root key is not defined.

2) authenticatorData aka "authenticator data" -- is inconsistently named in both latter fashions. Plus, its data structure fields are not formally named.

3) attestation data -- this data structure is not formally named (ie no <dfn> for it). Plus its fields are not formally named.

4) daaKey is not defined/described in the Syntax subsections of {#packed-attestation} nor {#tpm-attestation}

5) Verification procedure for {#packed-attestation} does not stipulate behavior if both x5c and daaKey are present (throw error?)

6) Verification procedure for {#tpm-attestation} does not stipulate behavior if both x5c and daaKey are present, or if neither are present (throw error?)

Issues (2) & (3) lead to trying to denote items in the data structure with imprecise names such as: ScopedCredentialInfo.attestation.authenticatorData."Attestation data"."public key"

One way of addressing (2) & (3) is to employ ABNF to formally define both authenticatorData and "Attestation data", like so..

;--------  Authenticator Data ----------

authenticatorData = rpIdHash flags signCount [attData] [extData]

rpIdHash    = 32OCTET   ; 32 octets

flagByte    = 1OCTET    ; 1 octet
flags       =  flagByte
flags       =/ ED AT RFU TUP

    ED  = BIT   ; Extension Data present (most significant bit)

    AT  = BIT   ; ATtestation data present

    RFU = 5BIT  ; Reserved for Future Use

    TUP = BIT   ; Test of User Presence (least significant bit)

signCount   = 4OCTET    ; 4 octets

;--------  Attestation Data ----------

attData     = AAGUID credIDLen credID pkAlg credPkLen credPk

AAGUID      = 16OCTET   ; 16 octets

credIDLen   = 2OCTET    ; 2 octets

credID      = < sequence of credIDLen octets >

pkAlg       = EC / RSA  ; public key alg & encoding

    EC  = %x0100    ; ANSI X9.62 formatted EC public key

    RSA = %x0102    ; RSA PKCS1 or RSASSA-PSS public key

credPkLen   = 2OCTET    ; 2 octets

credPk      = < sequence of uauthPkLen octets >

;--------  Extension Data ----------

extData     = < CBOR [RFC7049] map with extension identifiers 
            as keys, and extension authenticator data values
            as values >

;-----------------------------------

A second way is to add a "name" column to the existing authenticatorData and "Attestation data" tables.

A third way is to do 2nd way as well as use ABNF within the tables to formally define the data fields and their relationships.

UPDATE: 2016-10-24: revise authenticatorData ABNF; also: uauthPk -> credPk [=JeffH]

equalsJeffH commented 7 years ago

see also #226

rlin1 commented 7 years ago

wait for Vijay's change in attestation #244 before working on this one.

equalsJeffH commented 7 years ago

@rlin1 said

wait for Vijay's change in attestation #244 before working on this one.

agreed.

equalsJeffH commented 7 years ago

Ok, #244 is closed and PR #321 merged. time to now double-check all the originally listed items in original issue to see if any remain. volunteers? :)

vijaybh commented 7 years ago

Of the list above, (5) and (6) and perhaps (4) are now addressed by #321. The other items remain. In general I believe our descriptions of DAA need cleaning up to align the terminology with some sort of published reference.

rlin1 commented 7 years ago

regarding 1. DAA root key and 4. daaKey: see fixes in branch DAA-root-key-233, PR #381

selfissued commented 7 years ago

If addressing what remains of this issue will result in field name changes (and therefore API changes), we should finish addressing this before declaring an Implementer's Draft.

rlin1 commented 7 years ago

Note: the daaKey issue has been addressed by another issue.

selfissued commented 7 years ago

This is waiting on a review from @vijaybh

equalsJeffH commented 7 years ago

see also #296 'Refine meaning of PublicKeyCredentialType to be "signature & assertion format (and version thereof)" '

equalsJeffH commented 6 years ago

points (2) and (3) (see OP: https://github.com/w3c/webauthn/issues/233#issue-183487783) addressed in PR #614

rlin1 commented 6 years ago

point (1) addressed by #381

equalsJeffH commented 6 years ago

this is largely addressed at this point closing