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

Use COSE_Key and COSE Algorithm Identifiers #514

Merged
merged 12 commits into from
Aug 4, 2017

Conversation

selfissued
Copy link
Contributor

@selfissued selfissued commented Aug 2, 2017

This implements the decision we documented in issue #366 to use the COSE_Key data structure to represent keys, rather than our current ad-hoc structure. This PR also fixes issue #509 and issue #512 by using COSE algorithm identifiers, such as -7 for "ES256" - eliminating the current ambiguities and discrepancies in algorithm representations.


Preview | Diff

@selfissued
Copy link
Contributor Author

This fixes #509 .
This fixes #512 .

@nadalin
Copy link
Contributor

nadalin commented Aug 3, 2017

Please review @jyasskin , @equalsJeffH , @jcjones

Copy link
Contributor

@equalsJeffH equalsJeffH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is nominally correct but there's various rough edges to polish, some of which I'm working on. Also there's a host of linking errors reported by bikeshed, working on those too.

1. Let |normalizedParameters| be a new [=list=] whose [=list/items=] are pairs of {{PublicKeyCredentialType}} and a
[=dictionary=] type (as returned by [=normalizing an algorithm=]).
1. Let |normalizedParameters| be a new [=list=] whose [=list/items=] are pairs of {{PublicKeyCredentialType}} and
an {{AlgorithmIdentifier}}.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the type {{AlgorithmIdentifier}} is now a dangling pointer in that it is not itself defined.

index.bs Outdated

A number or string identifying a cryptographic algorithm.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there needs to be IDL here defining AlgorithmIdentifier -- I'm working on that now.

index.bs Outdated
definitions in [[!RFC7518]] section 6. Specifically, for ECC keys, the semantics of the `x` and `y` fields are
defined in [[!RFC7518]] sections 6.2.1.2 and 6.2.1.3, while for RSA keys, the semantics of the `n` and `e` fields
are defined in [[!RFC7518]] sections 6.3.1.1 and 6.3.1.2.
The Credential ID is the credential public key encoded in COSE_Key format, as defined in Section 7 of [[!RFC8152]].
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Cred ID is separate from the Cred Pub Key. They are two different things.

@@ -2534,7 +2504,7 @@ This attestation statement format is used with FIDO U2F authenticators using the
:: The [=attestation signature=].

: Signing procedure
:: If the credential public key of the given credential is not of algorithm "ES256", stop and return an error.
:: If the credential public key of the given credential is not of algorithm -7 ("ES256"), stop and return an error.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-7 should be <code>-7</code> -- I'm fixing that.

@@ -3304,11 +3274,11 @@ The sample code for generating and registering a new key follows:
pubKeyCredParams: [
{
type: "public-key",
alg: "ES256",
alg: -7 // "ES256" as registered in the IANA COSE Algorithms registry
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need refs for IANA COSE Algorithms registry and for JOSE Algs registry. I'm working on adding those.

index.bs Outdated
text: recognized algorithm name
type: dictionary
text: AlgorithmIdentifier; url: dfn-AlgorithmIdentifier

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

when one deletes stuff like this, one needs to run BikeShed and check for errors -- in this case, we still use the "recognized algorithm name" from WebCrypto, and that is causing linking errors without at least that portion the above in the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where we are using WebCrypto "recognized algorithm names" for the hash algorithms (which are listed at http://www.w3.org/TR/WebCryptoAPI/#sha-registration - such as "SHA-256"), maybe we should give an example value, such as "SHA-256".

Interestingly, the Travis-CI build succeeded for the PR without Bikeshed errors.

@equalsJeffH
Copy link
Contributor

fixes issue #366

@equalsJeffH equalsJeffH mentioned this pull request Aug 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants