veraison / go-cose

go library for CBOR Object Signing and Encryption (COSE)
Mozilla Public License 2.0
49 stars 26 forks source link

Deprecate AlgorithmEd25519 and provide AlgorithmEdDSA instead #160

Closed qmuntal closed 1 year ago

qmuntal commented 1 year ago

RFC 88152 Section 8.2 names the Edwards-Curve Digital Signature algorithm as EdDSA, but go-cose defines it as const AlgorithmEd25519 = -8. Our naming is misleading, the -8 value can be used with other curves as well, e.g., Ed448, and the concept of an Ed25519 algorithm does not appear in the spec. We are mixing algorithms with curves.

We can't just rename the algorithm constant, it would break the API, but we can deprecate it and create a new one with a more accurate name: AlgorithmEdDSA,

codecov[bot] commented 1 year ago

Codecov Report

Merging #160 (342ec02) into main (ec64bcb) will increase coverage by 0.17%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #160      +/-   ##
==========================================
+ Coverage   93.38%   93.56%   +0.17%     
==========================================
  Files          11       11              
  Lines        1678     1678              
==========================================
+ Hits         1567     1570       +3     
+ Misses         78       75       -3     
  Partials       33       33              
Impacted Files Coverage Δ
algorithm.go 85.36% <100.00%> (ø)
ed25519.go 100.00% <100.00%> (ø)
key.go 95.90% <100.00%> (ø)
signer.go 100.00% <100.00%> (ø)
verifier.go 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

SteveLasker commented 1 year ago

@shizhMSFT can you PTAL?

OR13 commented 1 year ago

EdDSA is sadly more ambiguous.

Internally I suggest a more specific name like Ed25519EdDSA... externally you will need to comply with the standards.

Presently there’s no way to negotiate Ed448 because the alg is just EdDSA, and it was already claimed to be Ed25519...

This has awful consequences for things like WebAuthN / Passkeys, etc....

dwaite commented 1 year ago

Since @OR13 mentioned this to me, I'll leave a reference to one issue on this open problem within the WebAuthn issue w3c/webauthn#1446 . A more relevant section of the Editor's Draft: https://w3c.github.io/webauthn/#sctn-alg-identifier

There has been a few faint discussions of having an additional registry of integer 'fully parameterized algorithm' values in the past which I can't find reference to at the moment. Not sure if this would be WebAuthn-specific or live within COSE. WebAuthn does not currently have intention to expand beyond representing negotiation of algorithms as a list of integer values.

selfissued commented 1 year ago

Creating pure non-polymorphic "alg" values for JOSE and COSE that fully specify their parameters has been on my to-do list for a while. This is needed for lots of systems that use "alg" to determine the cryptographic operation to be performed. WebAuthn/FIDO2 and OpenID Connect are just a few prominent examples.

Note that when I wrote RFC 8812, I explicitly prohibited using the potentially polymorphic COSE algorithm identifier ES256 for ES256K signatures. See this text at https://www.rfc-editor.org/rfc/rfc8812.html#name-other-uses-of-the-secp256k1:

When used for ECDSA, the secp256k1 curve MUST be used only with the ES256K algorithm identifier and not any others, including not with the COSE ES256 identifier. Note that the ES256K algorithm identifier needed to be introduced for JOSE to sign with the secp256k1 curve because the JOSE ES256 algorithm is defined to be used only with the P-256 curve. The COSE treatment of how to sign with secp256k1 is intentionally parallel to that for JOSE, where the secp256k1 curve MUST be used with the ES256K algorithm identifier.

shizhMSFT commented 1 year ago

@qmuntal It becomes harder to review if those file conflicts are not resolved.

shizhMSFT commented 1 year ago

This PR targets the branch keyapiimpr. Is it intended?

qmuntal commented 1 year ago

@qmuntal It becomes harder to review if those file conflicts are not resolved. This PR targets the branch keyapiimpr. Is it intended?

Fixed. @shizhMSFT