Normalize EC2 key size at input time to ensure RFC 9053 compliance

[kentakayama]

When EC2 keys are represented using big.Int, calling x.Bytes() may not yield a byte slice of the expected length as specified in RFC 9053, Section 7.1.1 (previously RFC 8152, Section 13.1.1).

Currently, the code adjusts the length of these values (X, Y, D) during UnmarshalCBOR, based on the expected size for the given curve. However, it's possible that these fields may be accessed before unmarshaling occurs.
To prevent accidental misuse of incorrectly sized []byte values, this change standardizes the length of these fields before storing them in the Key struct. It also adds validation to ensure the lengths are correct.
One concrete example of where this matters is the COSE Key Thumbprint computation introduced in PR #222, which accesses these fields directly. There may be other similar use cases, and requiring every caller to manually handle length normalization increases the risk of subtle bugs.

Therefore, I propose performing the normalization near NewEC2Key, immediately after calling Bytes(), to ensure consistency and safety throughout the codebase.

Commit b9d3c0b may warrant further discussion, as it changes the error codes returned and could lead to behavior that differs from what callers of the package expect.
If this change in behavior is not desired, I’m happy to revert it.

[kentakayama]

@OR13
I've noticed that I made a significant change to how X and Y are handled in the existing code. My apologies.
For EC2 private keys, the previous implementation allowed both X and Y to be nil and did not raise an error in those cases.
For OKP private keys, the implementation likewise permitted X to be nil without producing an error.
See:

go-cose/key.go

Lines 455 to 457 in 022cb54

	if len(x) > size || len(y) > size || len(d) > size {
	return errCoordOverflow
	}

Specifically, the old code returned an error when len(x) > size, so if x was nil it did not raise an error.

However, in the modified code I introduced, an error is returned when len(x) != size, which means that a nil x now triggers an error.

RFC 9053 Section 7.1.1 states the following, so this change may need to be considered carefully.
From the perspective of simplifying error handling, it might be preferable to require that both X and Y are always present and conform to the lengths specified in RFC 9053. What do you think?

https://www.rfc-editor.org/rfc/rfc9053#section-7.1.1-4

For private keys, it is RECOMMENDED that "x" also be present, but it can be recomputed from the required elements, and omitting it saves on space.

[OR13]

Perhaps the best solution here is a separate key representation for the "minimal cose key".
I would have x, and y be present and yield errors when omitted in the default.
I would also defer implementation of the "minimal cose key", until someone asks for it, or is willing to implement it.

@thomas-fossati @shizhMSFT wdyt?

[kentakayama]

Thanks, @OR13 .
The policy to "require both x and y by default for EC2 public and private keys" aligns with my own view, and it also simplifies handling of other cases—such as compressed point encoding. With this approach, we can compute the y-coordinate (from x or d, if present) during UnmarshalCBOR, which avoids the need for the Key struct to retain whether the original CBOR data used compressed mode.
Hence, the following compressed mode check will not be required:

			if len(y) == 0 && len(x) == size+1 {
				return fmt.Errorf("%w: compressed point not supported", ErrInvalidPubKey)
			}

(Also, I realized that the code above in my PR is incorrect — the correct check should be len(x) == size. That said, the Key struct doesn't explicitly indicate whether the y-coordinate is compressed. In any case, if we enforce the presence of both x and y with standard lengths, we can drop this logic entirely and avoid handling compressed mode.)

When marshaling with MarshalCBOR, we generally avoid using compressed mode. By requiring both x and y as bstr, we can support both private keys (with d) and public keys (without d) in a consistent way. Calling k.validate() at the beginning of MarshalCBOR would help ensure that we don't emit CBOR data that violates RFC 9053.

[kentakayama]

Hi @OR13 ,
I hope the commit 88057f2 resolves this conversation.
At this point, missing x and y for PrivateKey during validate(KeyOpSign) is acceptable, since RFC 9053 defines them as RECOMMENDED, not required.

That said, I've refactored the validation logic to align more precisely with RFC 9053. In particular:

For public keys, it is REQUIRED that "crv", "x", and "y" be present in the structure. For private keys, it is REQUIRED that "crv" and "d" be present in the structure.

Previously, both x := []byte{}; len(x) == 0 (presents but empty) and var x []byte; len(x) == 0 (absent) would pass the same check.
To distinguish these cases, I've updated the conditionals in the first block from len(x) == 0 to x == nil.

In the second block, if x, y or d are present, their lengths are now explicitly checked.

As for deriving (x, y) from d, or recovering compressed y from x, I believe that should be handled in a separate issue or pull request.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.35%. Comparing base (58542e2) to head (52af599).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #223      +/-   ##
==========================================
+ Coverage   91.23%   92.35%   +1.11%     
==========================================
  Files          13       13              
  Lines        2133     1726     -407     
==========================================
- Hits         1946     1594     -352     
+ Misses        128       73      -55     
  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.