Signature field clarification in attestation statement #805

akshayku · 2018-02-16T18:56:32Z

Fixes #799

agl · 2018-02-16T21:33:19Z

This specifies the signature format for the packed attestation, which is good, but that would leave the assertion signature format still under-specified. (The assertion signature was what I was testing with Chrome and Firefox since I do not have any tokens that speak CTAP2 and thus could have a packed attestation.)

akshayku · 2018-02-16T21:59:18Z

Moved that information is individual section in the same vicinity of both assertion signature and attestation statements signature.

agl · 2018-02-16T22:29:01Z

index.bs

@@ -2789,6 +2789,50 @@ the [=authenticator=] MUST:
    attStmtTemplate .within $$attStmtType
    ```

+### Signature Formats ### {#signature-attestation-types}
+    - For COSEAlgorithmIdentifier -7 (ES256), `sig` contains a pair of 32-byte integers,


The r and s values might not be 32 bytes long in this encoding, and I think the reference for the encoding is RFC 3278 section 3.3.2. So I've say something like:

For {{COSEAlgorithmIdentifier}} -7 (ES256), and other ECDSA-based algorithms, a signature is encoded as an ASN.1 DER Ecdsa-Sig-Value, as defined in [[RFC3279]] section 3.3.1.

(Then drop the added reference to X9.62.)

agl · 2018-02-16T22:29:36Z

index.bs

+
+        ```
+        Example:
+        30 44                               ; SEQUENCE (44 Bytes)


The lengths in the comments here are in hex (i.e. "44 Bytes" is 0x44 bytes). But typically such numbers are in base 10.

Ah.. Yeah, I was using my certutil tool. Will fix it.

agl · 2018-02-16T22:30:07Z

index.bs

+        Example:
+        30 44                               ; SEQUENCE (44 Bytes)
+            02 20                            ; INTEGER (20 Bytes)
+            |  3d 46 28 7b 8c 6e 8c 8c  26 1c 1b 88 f2 73 b0 9a


(There are | symbols at the start of these two lines, but not below.)

agl · 2018-02-16T22:31:34Z

index.bs

+        09 24 2c 13 0b af 30 16  a5 0f 75 ef 99 5f 9b f7
+        ed b0 a6 36 74 0e 90 dc  52 3b 4b 7d c5 eb f8 19
+        ```
+    - For COSEAlgorithmIdentifier -37 (RS256), `sig` contains the signature generated using the


s/RS256/PS256/

oops. thanks.

agl · 2018-02-16T22:32:08Z

index.bs

+        RSASSA-PKCS1-v1_5 signature scheme defined in [[RFC8017]] with SHA256 as the hash function.
+        ```
+        Example:
+        87 38 c6 f7 7d 53 3a 7e  73 27 2e 4b 9d 45 c7 19


This example is just 2,048 random bits, right? Dunno how helpful that is :)

Yeah.. Not helpful. Lets remove that.

selfissued

This is close but still ambiguous in some important ways. Please apply the corrections suggested in the line-by-line comments.

selfissued · 2018-02-16T22:30:38Z

index.bs

@@ -2789,6 +2789,50 @@ the [=authenticator=] MUST:
    attStmtTemplate .within $$attStmtType
    ```

+### Signature Formats ### {#signature-attestation-types}


Please make this title more specific clearly restrict its scope to only the kinds of signatures that it applies to. Right now it reads like it applies to all signatures.

For instance, the new title could be "Signature Formats for Packed Attestations and Assertion Signatures" (if that's the correct scoping).

Changed to added FIDO-U2F attestation format also.

selfissued · 2018-02-16T22:31:35Z

index.bs

+        ASN.1 DER format defined in [[ANSI.X9-62.2005]] and [[FIPS.186-4.2013]].
+
+        Note: As CTAP1/U2F devices are already producing signatures in this format, CTAP2
+        devices will also produce signatures in same format for consistency. Otherwise,


Change "in same format for consistency" to "in same format, for consistency reasons"

selfissued · 2018-02-16T22:35:10Z

index.bs

+
+        Note: As CTAP1/U2F devices are already producing signatures in this format, CTAP2
+        devices will also produce signatures in same format for consistency. Otherwise,
+        newer attestation formats MUST use COSE_KEY signature formats as defined in


Change "newer attestation formats MUST use COSE_KEY signature formats" to "it is recommended that any new attestation formats defined not use ASN.1 encodings, but instead represent signatures as equivalent fixed-length byte arrays without internal structure, using the same representations as used by COSE signatures"

selfissued · 2018-02-16T22:36:27Z

index.bs

+        Note: As CTAP1/U2F devices are already producing signatures in this format, CTAP2
+        devices will also produce signatures in same format for consistency. Otherwise,
+        newer attestation formats MUST use COSE_KEY signature formats as defined in
+        [[IANA-COSE-ALGS-REG]].


Change [[IANA-COSE-ALGS-REG]] to [[!RFC8152]].

selfissued · 2018-02-16T22:38:20Z

index.bs

+              17 6c 92 bb 8e 36 c0 41  98 a2 7b 90 9b 6e 8f 13
+        ```
+    - For COSEAlgorithmIdentifier -257 (RS256), `sig` contains the signature generated using the
+        RSASSA-PKCS1-v1_5 signature scheme defined in [[RFC8017]] with SHA256 as the hash function.


Add a section number reference within RFC 8017 and say that you're including (or not including) the ASN.1 wrapper for the signature. Just saying RFC 8017 is ambiguous, because it defines both the raw signature bytes and an ASN.1 wrapper for those bytes.

(Referencing the section is a good idea in any case, but I don't believe that there is an ASN.1-based format for an RSA signature.)

selfissued · 2018-02-16T22:38:31Z

index.bs

+        ed b0 a6 36 74 0e 90 dc  52 3b 4b 7d c5 eb f8 19
+        ```
+    - For COSEAlgorithmIdentifier -37 (RS256), `sig` contains the signature generated using the
+        RSASSA-PSS signature scheme defined in [[RFC8017]] with SHA256 as the hash function.


Add a section number reference within RFC 8017 and say that you're including (or not including) the ASN.1 wrapper for the signature. Just saying RFC 8017 is ambiguous, because it defines both the raw signature bytes and an ASN.1 wrapper for those bytes.

selfissued

Please apply the four punctuation corrections below before merging.

selfissued · 2018-02-16T22:59:29Z

index.bs

@@ -2789,6 +2789,31 @@ the [=authenticator=] MUST:
    attStmtTemplate .within $$attStmtType
    ```

+### Signature Formats for Packed Attestation, FIDO U2F Attestation  and Assertion Signatures ### {#signature-attestation-types}


Put a comma after "FIDO U2F Attestation"

selfissued · 2018-02-16T23:00:09Z

index.bs

+        devices will also produce signatures in same format, for consistency reasons.
+        It is recommended that any new attestation formats defined not use ASN.1 encodings,
+        but instead represent signatures as equivalent fixed-length byte arrays without internal structure,
+        using the same representations as used by COSE signatures as defined in [[!RFC8152]]


You're missing a period at the end of this sentence.

selfissued · 2018-02-16T23:00:38Z

index.bs

+            |  17 6c 92 bb 8e 36 c0 41  98 a2 7b 90 9b 6e 8f 13
+        ```
+    - For COSEAlgorithmIdentifier -257 (RS256), `sig` contains the signature generated using the
+        RSASSA-PKCS1-v1_5 signature scheme defined in section 8.2.1 in [[RFC8017]] with SHA256 as the hash function.


Change SHA256 to SHA-256.

selfissued · 2018-02-16T23:00:53Z

index.bs

+    - For COSEAlgorithmIdentifier -257 (RS256), `sig` contains the signature generated using the
+        RSASSA-PKCS1-v1_5 signature scheme defined in section 8.2.1 in [[RFC8017]] with SHA256 as the hash function.
+    - For COSEAlgorithmIdentifier -37 (PS256), `sig` contains the signature generated using the
+        RSASSA-PSS signature scheme defined in section 8.1.1 in [[RFC8017]] with SHA256 as the hash function.


Change SHA256 to SHA-256.

equalsJeffH

This seems nominally ok though could use some further polishing as noted below.

equalsJeffH · 2018-02-16T23:54:54Z

index.bs

@@ -2789,6 +2789,31 @@ the [=authenticator=] MUST:
    attStmtTemplate .within $$attStmtType
    ```

+### Signature Formats for Packed Attestation, FIDO U2F Attestation, and Assertion Signatures ### {#signature-attestation-types}
+    - For COSEAlgorithmIdentifier -7 (ES256),  and other ECDSA-based algorithms,
+        a signature is encoded as an ASN.1 DER Ecdsa-Sig-Value, as defined in [[RFC3279]] section 3.3.1.


AFAICT there is no "section 3.3.1" in RFC3279. Is the section you actually want to ref https://tools.ietf.org/html/rfc3279#section-2.2.3 ?

Oh Yes. I mistakenly blindly copied.

equalsJeffH · 2018-02-16T23:55:38Z

index.bs

@@ -2789,6 +2789,31 @@ the [=authenticator=] MUST:
    attStmtTemplate .within $$attStmtType
    ```

+### Signature Formats for Packed Attestation, FIDO U2F Attestation, and Assertion Signatures ### {#signature-attestation-types}
+    - For COSEAlgorithmIdentifier -7 (ES256),  and other ECDSA-based algorithms,
+        a signature is encoded as an ASN.1 DER Ecdsa-Sig-Value, as defined in [[RFC3279]] section 3.3.1.


suggest:
s/ a signature is / a signature value is /

equalsJeffH · 2018-02-16T23:56:39Z

index.bs

+        a signature is encoded as an ASN.1 DER Ecdsa-Sig-Value, as defined in [[RFC3279]] section 3.3.1.
+
+        Note: As CTAP1/U2F devices are already producing signatures in this format, CTAP2
+        devices will also produce signatures in same format, for consistency reasons.


suggest:
s/ signatures / signature values / in both places above

equalsJeffH · 2018-02-17T00:01:03Z

index.bs

+        devices will also produce signatures in same format, for consistency reasons.
+        It is recommended that any new attestation formats defined not use ASN.1 encodings,
+        but instead represent signatures as equivalent fixed-length byte arrays without internal structure,
+        using the same representations as used by COSE signatures as defined in [[!RFC8152]].


suggest:
s/ [[!RFC8152]] / [[!RFC8152]] and [[!RFC8230]] /

equalsJeffH · 2018-02-17T00:02:21Z

index.bs

+        but instead represent signatures as equivalent fixed-length byte arrays without internal structure,
+        using the same representations as used by COSE signatures as defined in [[!RFC8152]].
+
+        ```


it appears that the below example is ASN.1-wrapped. If so, I would move it up above the Note: (presently above), because having it down here is confusing -- i was expecting a "raw" fixed-length example to go with the Note.

equalsJeffH · 2018-02-17T00:12:26Z

index.bs

@@ -4963,5 +4988,15 @@ for their contributions as our W3C Team Contacts.
    "href": "https://www.internet2.edu/media/medialibrary/2013/09/04/internet2-mace-dir-eduperson-200604.html",
    "date": "May 15, 2007"
  }


need a comma here to get spec to build properly

equalsJeffH · 2018-02-17T00:12:37Z

index.bs

+  "ANSI.X9-62.2005": {
+    "title": "Public Key Cryptography for the Financial Services Industry: The Elliptic Curve Digital Signature Algorithm (ECDSA), ANSI X9.62-2005",
+    "date": "November 2005"
+  }


need a comma here to get spec to build properly

equalsJeffH · 2018-02-17T00:16:53Z

index.bs

@@ -4963,5 +4988,15 @@ for their contributions as our W3C Team Contacts.
    "href": "https://www.internet2.edu/media/medialibrary/2013/09/04/internet2-mace-dir-eduperson-200604.html",
    "date": "May 15, 2007"
  }
+


neither of the below references are cited in the above prose re "COSEAlgorithmIdentifier -7 (ES256)", so am wondering why they are here? Yes, tokbind-protocol cites them, but we are instead citing [[RFC3279]] which seems to obviate citing the former two refs.

seems like we can either cite [[RFC3279]] or do it like tokbind and cite the below. I suppose either works for me.

equalsJeffH · 2018-02-17T00:21:30Z

index.bs

@@ -2789,6 +2789,31 @@ the [=authenticator=] MUST:
    attStmtTemplate .within $$attStmtType
    ```

+### Signature Formats for Packed Attestation, FIDO U2F Attestation, and Assertion Signatures ### {#signature-attestation-types}


I would more clearly explain that with respect to [=assertion signatures=] that this is for assertions produced using the three listed signature algorithms.

what about any of the other COSE sig algorithms ?

They will refer to COSE document.

equalsJeffH · 2018-02-17T00:35:34Z

index.bs

+            |  4e 72 23 6e a3 90 a9 a1  7b cf 5f 7a 09 d6 3a b2
+            |  17 6c 92 bb 8e 36 c0 41  98 a2 7b 90 9b 6e 8f 13
+        ```
+    - For COSEAlgorithmIdentifier -257 (RS256), `sig` contains the signature generated using the


AFAICT the sig values produced by [[RFC8017]] sections 8.1.1 and 8.2.1 are not ASN.1-wrapped. If that is correct, would a Note: to that effect be helpful to readers here?

…sig-clarification

selfissued · 2018-02-20T21:22:55Z

Fixes #799

selfissued · 2018-02-20T21:27:19Z

index.bs

@@ -4968,6 +4997,7 @@ for their contributions as our W3C Team Contacts.
    "title": "EduPerson Object Class Specification (200604a)",
    "href": "https://www.internet2.edu/media/medialibrary/2013/09/04/internet2-mace-dir-eduperson-200604.html",
    "date": "May 15, 2007"
-  }
+  },


Delete the common from this line so that it becomes legal JSON.

selfissued

Good to go once the one missing "the" is added.

selfissued · 2018-02-20T23:07:02Z

index.bs

+        ```
+
+        Note: As CTAP1/U2F devices are already producing signatures values in this format, CTAP2
+        devices will also produce signatures values in same format, for consistency reasons.


Change "same format" to "the same format"

…ion statement (#805)

* Sig Clarification * Sig Clarification 2 * Clarify that ECDSA is ASN.1 DER format * Exception for ECDSA for consistency * Put Signature formats in its own section * typo * Incorporating comments * Incorporating comments-2 * Incorporating comments-3 * Incorporating comments-4 * Json comma issue * Incorporating comments - 6

Akshay Kumar added 2 commits February 15, 2018 12:57

Sig Clarification

7b518bd

Sig Clarification 2

2aed87c

akshayku added this to the Last Working Draft milestone Feb 16, 2018

akshayku requested review from agl, equalsJeffH and selfissued February 16, 2018 18:56

Akshay Kumar added 2 commits February 16, 2018 13:08

Clarify that ECDSA is ASN.1 DER format

ec49211

Exception for ECDSA for consistency

f13bbc3

Put Signature formats in its own section

61bc863

typo

861667a

agl reviewed Feb 16, 2018

View reviewed changes

selfissued requested changes Feb 16, 2018

View reviewed changes

Akshay Kumar added 2 commits February 16, 2018 14:53

Incorporating comments

02f96cd

Incorporating comments-2

feca22b

selfissued approved these changes Feb 16, 2018

View reviewed changes

Incorporating comments-3

18f342c

selfissued approved these changes Feb 16, 2018

View reviewed changes

equalsJeffH reviewed Feb 17, 2018

View reviewed changes

Akshay Kumar added 2 commits February 16, 2018 16:54

Incorporating comments-4

4514d10

Merge branch 'master' of https://github.com/w3c/webauthn into akshay-…

cde648a

…sig-clarification

nadalin assigned akshayku Feb 20, 2018

nadalin added the type:technical label Feb 20, 2018

selfissued reviewed Feb 20, 2018

View reviewed changes

Json comma issue

4bdb54c

selfissued approved these changes Feb 20, 2018

View reviewed changes

Incorporating comments - 6

c8c43e2

akshayku merged commit 3a3700a into master Feb 21, 2018

akshayku deleted the akshay-sig-clarification branch February 21, 2018 01:45

WebAuthnBot pushed a commit that referenced this pull request Feb 21, 2018

Built by Travis-CI: 3a3700a Signature field clarification in attestat…

6f3dbc5

…ion statement (#805)

Signature field clarification in attestation statement #805

Signature field clarification in attestation statement #805

Conversation

akshayku commented Feb 16, 2018 • edited by pr-preview bot Loading

agl commented Feb 16, 2018

akshayku commented Feb 16, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfissued left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfissued left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

equalsJeffH Feb 16, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfissued commented Feb 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

selfissued left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayku commented Feb 16, 2018 •

edited by pr-preview bot

Loading

equalsJeffH Feb 16, 2018 •

edited

Loading