Skip to content

Add fully specified algorithms - #225

Open
kentakayama wants to merge 12 commits into
veraison:mainfrom
kentakayama:add-fully-specified-algorithms
Open

Add fully specified algorithms#225
kentakayama wants to merge 12 commits into
veraison:mainfrom
kentakayama:add-fully-specified-algorithms

Conversation

@kentakayama

@kentakayama kentakayama commented Dec 13, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

Resolve #224 based on the Option A.

Key modifications are:

  • AlgorithmESP256, AlgorithmESP384, AlgorithmESP512 and AlgorithmEd25519EdDSA are added
  • EdDSA Signer and Verifier are changed to have alg field, and it is returned in Algorithm()
  • deriveAlgorithm is changed to deriveAlgorithms, now it returns the slice of possible Algorithms (at least one Algorithm is included for better error handling)
  • tests are extended to cover {ES256, ESP256} and {EdDSA, Ed25519EdDSA}

@kentakayama
add: ESP256, ESP384, ESP512 and Ed25519EdDSA defined in RFC 9864
e88025e 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
fix: deriveAlgorithm to produce possible cose.Algorithms to avoid sel…
fe78f60 
…ecting one Algorithm for a Curve

Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
fix: test
0713313 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
refactor: function name and comments
8187606 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
test: add coverage for {ES256, ESP256} and {EdDSA, Ed25519} and fix r…
bf56e31 
…elated issues

Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
refactor: move new AlgorithmEd25519EdDSA to the bottom
486b46e 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@THS-on

THS-on commented Sep 15, 2026

Copy link
Copy Markdown
Member

Can any of the others reviewers have a look if we can merge this?

thomas-fossati
thomas-fossati reviewed Sep 23, 2026
View reviewed changes

@thomas-fossati thomas-fossati left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, thanks very much!

I have flagged a couple of minor things and a slightly more important missing condition.

Comment thread key.go Outdated

@thomas-fossati thomas-fossati Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

shouldn't this accept AlgorithmEd25519EdDSA?

Comment thread key.go Outdated
}
return fmt.Errorf(
"found algorithm %q (expected %q)",
"found algorithm %q (expected one of {%q})",

@thomas-fossati thomas-fossati Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I suspect this would not format the list in the way you wanted ;-)

Comment thread key.go Outdated
return nil
}

func containsAlg(algs []Algorithm, target Algorithm) bool {

@thomas-fossati thomas-fossati Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I believe there is no need to define our own function; we could just use the slices.Contains here.

Comment thread algorithm.go Outdated
Comment on lines +154 to +155
// As stated in RFC 8152 section 8.2, only the pure EdDSA version is
// used for COSE.

@thomas-fossati thomas-fossati Sep 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

c&p from L150-151

@kentakayama kentakayama Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I've resolved these 4 review comments in 80e06c3 .

@kentakayama
fix: address fully specified algorithm review
80e06c3 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
test: fix ECDSA algorithm test names
27aaea7 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
test: support generating ECDSA keys for each curve
8f56851 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
test: expand fully specified algorithm coverage
295f469 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
fix: validate fully specified ECDSA curves
4a9995f 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama
docs: clarify polymorphic algorithm interoperability
8beae37 
Signed-off-by: Ken Takayama <ken.takayama.ietf@gmail.com>
@kentakayama

kentakayama commented Sep 25, 2026

Copy link
Copy Markdown
Contributor Author

I added five follow-up commits to improve the implementation and test coverage for the fully specified algorithms.

The main changes are:

  • 295f469 expands the existing tests to cover the new fully specified algorithms. It checks their registered values, names, hash functions, signing and verification with all supported curves, and COSE key serialization/signing round trips.
  • 4a9995f adds validation and negative tests to ensure that each fully specified ECDSA algorithm is used only with its required curve: ESP256 with P-256, ESP384 with P-384, and ESP512 with P-521. NewSigner and NewVerifier now reject mismatched curves.
  • 8beae37 clarifies the documentation for the new and legacy algorithm identifiers. It now distinguishes the RFC 9864 deprecation of the polymorphic identifiers from the practical need to retain them when interoperating with implementations that do not yet support the fully specified identifiers.

The other two commits are small preparatory cleanups:

  • 27aaea7 corrects the ES512/ESP512 test names.
  • 8f56851 extracts a test helper for generating ECDSA keys with different curves.

All tests, including the race detector, pass after these changes.

@dogrucanemek-alt

dogrucanemek-alt commented Sep 26, 2026

Copy link
Copy Markdown

Tested this PR against Ed25519 (-19) COSE_Sign1 records produced by a different implementation (TypeScript, not go-cose): six decision records from verax-ai/verax, signed with signDecisionRecord from @cedulon/core 0.13.1. Fixture and public test key are in that repo at 13ab7e9 (packages/proxy/tests/fixtures/ledger-golden/decisions.jsonl, packages/proxy/tests/fixtures/keys/record-public.TEST-KEY-NOT-A-SECRET.pem).

  • v1.3.0: 0/6, algorithm mismatch: verifier EdDSA: header Algorithm(-19)
  • this PR at 8beae37, NewVerifier(AlgorithmEd25519EdDSA, pub): 6/6
  • the same records against AlgorithmEdDSA: 0/6, algorithm mismatch (no cross-acceptance, as intended)
  • one payload bit flipped in one record: 5/6

The suite also passes here on Go 1.21.13 and 1.22.12. Thanks for carrying this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thomas-fossati thomas-fossati thomas-fossati left review comments
@OR13 OR13 OR13 approved these changes
@henkbirkholz henkbirkholz Awaiting requested review from henkbirkholz henkbirkholz is a code owner
@qmuntal qmuntal Awaiting requested review from qmuntal qmuntal is a code owner
@roywill roywill Awaiting requested review from roywill roywill is a code owner
@setrofim setrofim Awaiting requested review from setrofim setrofim is a code owner
@shizhMSFT shizhMSFT Awaiting requested review from shizhMSFT shizhMSFT is a code owner
@SimonFrost-Arm SimonFrost-Arm Awaiting requested review from SimonFrost-Arm SimonFrost-Arm is a code owner
@SteveLasker SteveLasker Awaiting requested review from SteveLasker SteveLasker is a code owner
@yogeshbdeshpande yogeshbdeshpande Awaiting requested review from yogeshbdeshpande yogeshbdeshpande is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

RFC 9864: Fully-Specified Algorithms for COSE support

5 participants

@kentakayama @THS-on @dogrucanemek-alt @OR13 @thomas-fossati