fix: Support multiple authorized-by keys in Measurement struct (fixes #195) #219

kallal79 · 2025-09-21T16:39:26Z

Summary

This PR fixes GitHub issue #195 - Conditional endorsement series triple test failing.

Problem

The field was defined as (single key), but the TCG specification and conditional endorsement series test data require support for multiple keys. This caused CBOR unmarshalling to fail with:

cbor: cannot unmarshal array into Go struct field comid.Measurement.2 of type comid.ICryptoKeyValue

Root Cause Analysis

The diagnostic test data contains:

/ authorized-by / 2 : [
  / tagged-pkix-base64-key-type / 554("base64_key_for-RIM-creator")
]

This is an array of keys, but the Go struct expected a single key
As noted by @yogeshbdeshpande in the issue: "the spec expects it to be more than one key, but the current implementation only adheres to one key"

Solution

1. Updated Measurement Structure

Changed field from to
is an existing type that supports multiple keys:

2. Added Compatibility Methods

Added and methods to for backward compatibility
These methods handle both single-key and multi-key scenarios gracefully

3. Backward Compatibility

Added custom and methods
First attempts to unmarshal as (new format)
Falls back to unmarshalling as single (legacy format) and wraps it
Zero breaking changes - all existing code continues to work

4. Applied Proposed Test Fix

Applied the test patch from the issue description to properly expose failures
Changed to
Individual test failures now properly reported instead of silently ignored

Testing Results

Before Fix - Test Failed

example_test.go:640: error unmarshalling field "Triples": error unmarshalling field "CondEndorseSeries": error at index 0: error at index 0: cbor: cannot unmarshal array into Go struct field comid.Measurement.2 of type comid.ICryptoKeyValue
--- FAIL: TestExample_decode_CBOR/Test_with_CoMID-cond-endorse-series_Diag (0.00s)

After Fix - Test Passes

--- PASS: TestExample_decode_CBOR/Test_with_CoMID-cond-endorse-series_Diag (0.00s)

Full Compatibility

All existing package tests pass:
All existing package tests pass:
Backward compatibility maintained for single-key usage

Code Changes

Modified Files:

****: Updated field type and added compatibility methods
****: Added and methods for compatibility
****: Applied proposed test patch to properly expose failures

Key Implementation Details:

Graceful Fallback: Custom unmarshal methods try new format first, fall back to legacy
Zero Breaking Changes: All existing code paths continue to work unchanged
Spec Compliance: Now supports multiple keys as required by TCG specification
Comprehensive Testing: Both CBOR and JSON serialization fully supported

Spec Compliance

This change aligns the implementation with the TCG Concise Evidence specification, which requires support for multiple authorization keys in measurement records. The implementation now correctly handles:

Single key (legacy compatibility)
Multiple keys (spec-compliant)
Empty authorization (no keys)

kallal79 · 2025-09-30T18:42:15Z

Hi sir @yogeshbdeshpande AND sir @thomas-fossati: PR #219 fixes Issue #195 (support multiple authorized-by keys), adds graceful CBOR/JSON compatibility and tests — all checks passed; please review and approve to unblock merging.

setrofim

The compatibility implementation here is unnecessary. The CoRIM spec is still in draft and constantly evolving. The implementation does not need to be compatible with previous revisions of the draft. Instead, all tests and examples should be aligned with the current implementation.

setrofim · 2025-10-01T10:30:30Z

comid/cryptokeys.go

+	if len(o) == 0 {
+		return "empty crypto keys"
+	}
+	return fmt.Sprintf("CryptoKeys with %d keys", len(o))


If would be more useful to return the string representation of the list of all keys.

…orized-by keys Addresses reviewer feedback from setrofim in PR veraison#219: - Remove unnecessary compatibility implementation as CoRIM spec is in draft - Align all tests and examples with current implementation instead of maintaining compatibility Core Changes: - comid/measurement.go: Changed AuthorizedBy from *CryptoKey to *CryptoKeys (arrays) - comid/cryptokeys.go: Added String() method returning array representation - coev/coswid_evidence.go: Updated AuthorizedBy to use CryptoKeys - coserv/quads.go: Updated authorities to use CryptoKeys arrays Test Data Updates: - Updated all JSON templates to use array format for authorized-by - Updated diagnostic files (.diag) to use CBOR array syntax [554(...)] - Regenerated all CBOR test files using cbor-diag tool - Updated expected test outputs to show 'CryptoKeys: [...]' format Testing: - All packages now pass tests: comid, comid/tdx, coev, coev/tdx, coserv - Supports multiple authorized-by keys as required by issue veraison#195 - No backward compatibility - clean implementation for draft spec Fixes veraison#195

kallal79 · 2025-10-02T02:56:10Z

@setrofim I've addressed all your feedback:

Changes Made:

Removed compatibility code: No more backward compatibility implementation
Aligned tests and examples: All tests now use the new array-based authorized-by format
Fixed CryptoKeys.String(): Now returns array representation like [key1, key2, ...] instead of count

Core Updates:

comid/measurement.go: Changed AuthorizedBy from *CryptoKey to *CryptoKeys
comid/cryptokeys.go: Added proper String() method for arrays
coev/coswid_evidence.go: Updated to use CryptoKeys
coserv/quads.go: Updated authorities to use CryptoKeys arrays

Test Data Updates:

Updated all JSON templates to use array format
Updated all diagnostic files (.diag) to use CBOR array syntax
Regenerated all CBOR test files using cbor-diag
Updated expected test outputs to show 'CryptoKeys: [...]' format

Testing Results:

All packages now pass tests:

comid, comid/tdx
coev, coev/tdx
coserv
All other packages

The implementation now cleanly supports multiple authorized-by keys as arrays without any compatibility baggage, as requested. Ready for review!

…orized-by keys Addresses reviewer feedback from setrofim in PR veraison#219: - Remove unnecessary compatibility implementation as CoRIM spec is in draft - Align all tests and examples with current implementation instead of maintaining compatibility Core Changes: - comid/measurement.go: Changed AuthorizedBy from *CryptoKey to *CryptoKeys (arrays) - comid/cryptokeys.go: Added String() method returning array representation - coev/coswid_evidence.go: Updated AuthorizedBy to use CryptoKeys - coserv/quads.go: Updated authorities to use CryptoKeys arrays Test Data Updates: - Updated all JSON templates to use array format for authorized-by - Updated diagnostic files (.diag) to use CBOR array syntax [554(...)] - Regenerated all CBOR test files using cbor-diag tool - Updated expected test outputs to show 'CryptoKeys: [...]' format Testing: - All packages now pass tests: comid, comid/tdx, coev, coev/tdx, coserv - Supports multiple authorized-by keys as required by issue veraison#195 - No backward compatibility - clean implementation for draft spec Fixes veraison#195 Signed-off-by: Kallal Mukherjee <[email protected]>

setrofim

LGTM

yogeshbdeshpande · 2025-10-02T08:41:03Z

@7908837174 Thank you so much for all the hard work on this PR, I have looked at this and it all Looks Good to Me!

yogeshbdeshpande

LGTM!

yogeshbdeshpande · 2025-10-02T08:43:44Z

@7908837174 : Can you please check this minor linter error on file format, so that once fixed, we can merge the PR

- Remove extra blank line in comid/cryptokeys.go line 44 - Fix struct field alignment in coserv/quads.go - Resolves failing lint job 51776361051

yogeshbdeshpande · 2025-10-02T17:30:00Z

@7908837174 : Thank you Kallal for the great work! I have merged your PR now!

kallal79 · 2025-10-02T17:31:22Z

Huge thanks to sir @setrofim and sir @yogeshbdeshpande for the thoughtful reviews and support!

yogeshbdeshpande · 2025-10-02T17:39:41Z

@7908837174 I think, your list in in-correct!

kallal79 · 2025-10-02T17:42:11Z

Kindly requesting review from @yogeshbdeshpande for PR veraison/docs#64 and PR veraison/services#344 ..

setrofim requested changes Oct 1, 2025

View reviewed changes

kallal79 force-pushed the fix/conditional-endorsement-series-195 branch from a22193e to 1651e4d Compare October 2, 2025 02:55

kallal79 force-pushed the fix/conditional-endorsement-series-195 branch from 1651e4d to c96f36b Compare October 2, 2025 02:59

kallal79 requested a review from setrofim October 2, 2025 03:00

setrofim approved these changes Oct 2, 2025

View reviewed changes

yogeshbdeshpande approved these changes Oct 2, 2025

View reviewed changes

fix: correct gofmt formatting issues

31dafe4

- Remove extra blank line in comid/cryptokeys.go line 44 - Fix struct field alignment in coserv/quads.go - Resolves failing lint job 51776361051

yogeshbdeshpande merged commit 3c18c66 into veraison:main Oct 2, 2025
5 checks passed

kallal79 deleted the fix/conditional-endorsement-series-195 branch October 2, 2025 17:31

yogeshbdeshpande mentioned this pull request Oct 2, 2025

fix: cryptokeys error #216

Closed

fix: Support multiple authorized-by keys in Measurement struct (fixes #195) #219

fix: Support multiple authorized-by keys in Measurement struct (fixes #195) #219

Uh oh!

Conversation

kallal79 commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Problem

Root Cause Analysis

Solution

1. Updated Measurement Structure

2. Added Compatibility Methods

3. Backward Compatibility

4. Applied Proposed Test Fix

Testing Results

Before Fix - Test Failed

After Fix - Test Passes

Full Compatibility

Code Changes

Modified Files:

Key Implementation Details:

Spec Compliance

Uh oh!

kallal79 commented Sep 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

setrofim left a comment

Choose a reason for hiding this comment

Uh oh!

setrofim Oct 1, 2025

Choose a reason for hiding this comment

Uh oh!

kallal79 Oct 2, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Changes Made:

Core Updates:

Test Data Updates:

Testing Results:

Uh oh!

setrofim left a comment

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande commented Oct 2, 2025

Uh oh!

yogeshbdeshpande left a comment

Choose a reason for hiding this comment

Uh oh!

yogeshbdeshpande commented Oct 2, 2025

Uh oh!

Uh oh!

yogeshbdeshpande commented Oct 2, 2025

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

kallal79 commented Oct 2, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kallal79 commented Sep 21, 2025 •

edited

Loading

kallal79 commented Sep 30, 2025 •

edited

Loading

kallal79 commented Oct 2, 2025 •

edited

Loading

kallal79 commented Oct 2, 2025 •

edited

Loading

yogeshbdeshpande commented Oct 2, 2025 •

edited

Loading