test: add Ed25519 conformance tests #219

Sukuna0007Abhi · 2025-10-03T01:04:21Z

Description

This PR adds conformance tests for Ed25519 signatures to address issue #171.

Changes

Added 3 new test vector files in testdata/:
- sign1-sign-0007.json: Ed25519 signing test vector
- sign1-verify-0007.json: Ed25519 verification test vector
- sign1-verify-negative-0004.json: Ed25519 negative verification test
Updated conformance_test.go to support OKP (Octet Key Pair) key type and EdDSA algorithm
Added mustBase64ToBytes() helper function for base64url decoding
Modified testSign1() to use nil rand parameter for deterministic EdDSA signatures

Test Results

All 21 conformance tests now pass (up from 18):

8 signing tests (including new Ed25519)
8 verification tests (including new Ed25519)
5 negative verification tests (including new Ed25519)

Technical Notes

Ed25519 signatures are deterministic and require nil for the rand parameter, unlike ECDSA/RSA which use zeroSource
Test vectors are based on COSE working group examples from RFC 8152
All existing tests continue to pass

Fixes #171

Ready for review sir @thomas-fossati @setrofim @yogeshbdeshpande @SimonFrost-Arm @OR13 @henkbirkholz

- Add sign1-sign-0007.json: Ed25519 signing test vector - Add sign1-verify-0007.json: Ed25519 verification test vector - Add sign1-verify-negative-0004.json: Ed25519 negative verification test - Update conformance_test.go to support OKP key type and EdDSA algorithm - Add mustBase64ToBytes() helper for base64url decoding - Modify testSign1() to use nil rand for deterministic EdDSA signatures Fixes veraison#171 Signed-off-by: Sukuna0007Abhi <[email protected]>

OR13 · 2025-10-03T01:10:52Z

conformance_test.go

 		return cose.AlgorithmES384
 	case "ES512":
 		return cose.AlgorithmES512
+	case "EdDSA":


Technically this could also mean Ed448.

Consider if anything here is helpful:

https://datatracker.ietf.org/doc/draft-ietf-jose-fully-specified-algorithms/13/

Sure sir @OR13 , thanks for the approval

Hi sir @OR13 Good point about Ed448. I've added placeholder support for it with proper error messages and comments about the key size differences (57-byte vs 32-byte seeds). The code structure now makes it easy to add Ed448 implementation later when golang.org/x/crypto/ed448 is integrated. Both curves correctly use AlgorithmEdDSA = -8 with curve differentiation in the key's "crv" field.

- Add Ed448 case with clear 'not yet implemented' message - Add comments about Ed448 key size differences (57 vs 32 bytes) - Document that Ed448 would require golang.org/x/crypto/ed448 - Maintain backward compatibility while preparing for future expansion This addresses the mentor's suggestion about supporting both Ed25519 and Ed448 under the EdDSA algorithm identifier. Signed-off-by: Sukuna0007Abhi <[email protected]>

Sukuna0007Abhi · 2025-10-04T13:32:59Z

Ready for review and merge as sir @OR13 approved it

SteveLasker · 2025-10-05T05:14:47Z

@shizhMSFT, @qmuntal can either of you PTAL?

codecov · 2025-10-06T05:11:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.37%. Comparing base (a633822) to head (350a3a9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #219      +/-   ##
==========================================
+ Coverage   91.23%   91.37%   +0.14%     
==========================================
  Files          13       13              
  Lines        2133     2133              
==========================================
+ Hits         1946     1949       +3     
+ Misses        128      125       -3     
  Partials       59       59

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

Pull Request Overview

This PR adds conformance tests for Ed25519 signatures using the EdDSA algorithm to address issue #171. The change extends the existing test suite with Ed25519 cryptographic support and ensures deterministic signature generation.

Adds Ed25519 test vectors for signing, verification, and negative verification scenarios
Implements OKP (Octet Key Pair) key type support with Ed25519 curve handling
Modifies signature generation to use nil rand parameter for deterministic EdDSA signatures

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
testdata/sign1-sign-0007.json	Ed25519 signing test vector with expected deterministic output
testdata/sign1-verify-0007.json	Ed25519 verification test vector for valid signature
testdata/sign1-verify-negative-0004.json	Ed25519 negative test case with invalid signature
conformance_test.go	Adds OKP key support, EdDSA algorithm handling, and deterministic signing logic

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-10-06T05:12:08Z

conformance_test.go

+	case "OKP":
+		// Only Ed25519 is supported for now
+		switch key["crv"] {
+		case "Ed25519":
+			publicKey := mustBase64ToBytes(key["x"])
+			if len(publicKey) != ed25519.PublicKeySize {
+				return nil, errors.New("invalid Ed25519 public key size")
+			}
+			if private {
+				privateKey := mustBase64ToBytes(key["d"])
+				if len(privateKey) != ed25519.SeedSize {
+					return nil, errors.New("invalid Ed25519 private key size")
+				}
+				// Ed25519 private key is 64 bytes: 32-byte seed + 32-byte public key
+				fullPrivateKey := ed25519.NewKeyFromSeed(privateKey)
+				return fullPrivateKey, nil
+			}
+			// For public key only operations, we need to return a type that satisfies crypto.Signer
+			// but this won't work for verify-only operations. We'll handle this in getSigner.
+			return nil, errors.New("OKP public-only key not supported in this context")


The OKP key handling logic is duplicated between the getKey function and the getSigner function. Consider extracting the Ed25519 key creation logic into a separate helper function to reduce code duplication.

This is a very valid comment, which needs to be incorporated!

Ok sir @yogeshbdeshpande I will more review this one...

Copilot · 2025-10-06T05:12:09Z

conformance_test.go

 	if err != nil {
 		t.Fatal(err)
 	}
+	t.Logf("Test case: %s, Key type: %s", tc.Title, tc.Key["kty"])


The debug logging statements should be removed or replaced with conditional debug logging. These logs will clutter test output in normal runs and are typically only needed during development.

This one is great, changes will be made fr...

Copilot · 2025-10-06T05:12:09Z

conformance_test.go

+	}
+	t.Logf("Algorithm: %s, using rand: %v", tc.Alg, rand)
+	t.Logf("Payload: %x", sigMsg.Payload)
+	t.Logf("Protected: %+v", sigMsg.Headers.Protected)


The debug logging statements should be removed or replaced with conditional debug logging. These logs will clutter test output in normal runs and are typically only needed during development.

Suggested change

t.Logf("Protected: %+v", sigMsg.Headers.Protected)

if testing.Verbose() {

t.Logf("Protected: %+v", sigMsg.Headers.Protected)

}

shizhMSFT

Thanks for your contribution! No special concerns and LGTM.

I've changed the PR title to conventional commits as this PR will be squash merged.

Please also take a look at the comments provided by Copilot and validate if they are relevant.

Sukuna0007Abhi · 2025-10-06T05:33:32Z

Thanks sir @shizhMSFT for the approval, I will review the suggestions of copilot and if needed I will update the code,
& No issue with changes of PR title..

@yogeshbdeshpande

- Remove verbose debug logging to match existing test style - Extract Ed25519 key creation logic into createEd25519Key() helper function - Eliminate code duplication between getSigner() and getKey() functions - Improve code maintainability while preserving all functionality Addresses feedback from @yogeshbdeshpande and GitHub Copilot code review. Signed-off-by: Sukuna0007Abhi <[email protected]>

Sukuna0007Abhi · 2025-10-07T17:06:42Z

Hi sir @shizhMSFT @SteveLasker @yogeshbdeshpande @henkbirkholz @OR13 @thomas-fossati @qmuntal ready for review and merge, as I addressed all copilot suggestions also

shizhMSFT

LGTM

Sukuna0007Abhi · 2025-10-08T05:08:13Z

Thanks sir @shizhMSFT

yogeshbdeshpande · 2025-10-08T08:31:41Z

conformance_test.go

+	if tc.Key["kty"] == "OKP" {
+		switch tc.Key["crv"] {
+		case "Ed25519":
+			// Note: Ed448 would require different key size validation (57 bytes)


Is comment on line 207 correct ?
Should it not be Ed25519 ?

Yes sir @yogeshbdeshpande you're right I didn't put another line though,

I am actually thought of putting "We're currently handling Ed25519, but if we added Ed448, it would require different key size validation (57 bytes)"

But now I think it's merged so any member can change and commit this comment change part ,

Thanks great catching sir...

@Sukuna0007Abhi , please create a small minor edit PR and work on improving this...

Sure sir @yogeshbdeshpande will do that

SteveLasker

Thank you @Sukuna0007Abhi for the contribution and thanks @OR13, @shizhMSFT, @yogeshbdeshpande for the review.
LGTM

Sukuna0007Abhi · 2025-10-08T13:54:33Z

Thank you sir @SteveLasker @OR13 @yogeshbdeshpande @shizhMSFT @henkbirkholz @thomas-fossati @setrofim ;
First contribution on go-cose is successful ❤️‍🔥

Sukuna0007Abhi requested review from OR13, SimonFrost-Arm, SteveLasker, henkbirkholz, qmuntal, roywill, setrofim, shizhMSFT, thomas-fossati and yogeshbdeshpande as code owners October 3, 2025 01:04

Sukuna0007Abhi mentioned this pull request Oct 3, 2025

Add conformance tests for Ed25519 signatures #171

Closed

OR13 approved these changes Oct 3, 2025

View reviewed changes

shizhMSFT changed the title ~~Add Ed25519 conformance tests issue#171~~ test: add Ed25519 conformance tests Oct 6, 2025

shizhMSFT requested a review from Copilot October 6, 2025 05:11

Copilot AI reviewed Oct 6, 2025

View reviewed changes

shizhMSFT approved these changes Oct 6, 2025

View reviewed changes

shizhMSFT approved these changes Oct 8, 2025

View reviewed changes

yogeshbdeshpande reviewed Oct 8, 2025

View reviewed changes

SteveLasker approved these changes Oct 8, 2025

View reviewed changes

SteveLasker merged commit 58542e2 into veraison:main Oct 8, 2025
4 checks passed

test: add Ed25519 conformance tests #219

test: add Ed25519 conformance tests #219

Conversation

Sukuna0007Abhi commented Oct 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changes

Test Results

Technical Notes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sukuna0007Abhi commented Oct 4, 2025

Uh oh!

SteveLasker commented Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 6, 2025

Choose a reason for hiding this comment

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Sukuna0007Abhi commented Oct 6, 2025

Uh oh!

Sukuna0007Abhi commented Oct 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shizhMSFT left a comment

Choose a reason for hiding this comment

Uh oh!

Sukuna0007Abhi commented Oct 8, 2025

Uh oh!

yogeshbdeshpande Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SteveLasker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sukuna0007Abhi commented Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Sukuna0007Abhi commented Oct 3, 2025 •

edited

Loading

SteveLasker commented Oct 5, 2025 •

edited

Loading

codecov bot commented Oct 6, 2025 •

edited

Loading

Sukuna0007Abhi commented Oct 7, 2025 •

edited

Loading

yogeshbdeshpande Oct 8, 2025 •

edited

Loading

Sukuna0007Abhi commented Oct 8, 2025 •

edited

Loading