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

Deprecate AlgorithmEd25519 and provide AlgorithmEdDSA instead #160

Merged
merged 1 commit into from
Jul 24, 2023

Conversation

qmuntal
Copy link
Contributor

@qmuntal qmuntal commented Jul 12, 2023

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
Copy link

codecov bot commented Jul 12, 2023

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

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

Copy link
Contributor

@yogeshbdeshpande yogeshbdeshpande left a comment

Choose a reason for hiding this comment

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

LGTM!

@SteveLasker
Copy link
Contributor

@shizhMSFT can you PTAL?

@OR13
Copy link
Contributor

OR13 commented Jul 14, 2023

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
Copy link

dwaite commented Jul 14, 2023

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
Copy link

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
Copy link
Contributor

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

@shizhMSFT
Copy link
Contributor

This PR targets the branch keyapiimpr. Is it intended?

@qmuntal qmuntal changed the base branch from keyapiimpr to main July 19, 2023 14:15
Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
@qmuntal
Copy link
Contributor Author

qmuntal commented Jul 19, 2023

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

Fixed. @shizhMSFT

Copy link
Contributor

@shizhMSFT shizhMSFT left a comment

Choose a reason for hiding this comment

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

LGTM

@qmuntal qmuntal merged commit 88e7f1e into main Jul 24, 2023
6 checks passed
@qmuntal qmuntal deleted the eddssaalg branch July 24, 2023 10:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants