Skip to content

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

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.

Sign up for GitHub

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

Open
kentakayama wants to merge 8 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
cb33f56
update: always handle x, y and d with full size for the curve
kentakayama Dec 4, 2025
1337f29
add: test ensuring the NewKeyEC2 always returns correct byte size of …
kentakayama Dec 4, 2025
f3b1be1
add: failure test ensuring invalid size of x and d for OKP
kentakayama Dec 4, 2025
b9d3c0b
update: Normalize and validate EC2 key lengths to ensure compliance w…
kentakayama Dec 4, 2025
52af599
fix: typo
kentakayama Dec 4, 2025
88057f2
refactor: clarify error handling for missing vs malformed params for …
kentakayama Dec 6, 2025
13e02ff
refactor: comments
kentakayama Dec 6, 2025
b745bfa
refactor: split curveSize into keySizeEC2 and keySizeOKP
kentakayama Dec 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 16 additions & 12 deletions key.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,14 +336,19 @@ func NewKeyEC2(alg Algorithm, x, y, d []byte) (*Key, error) {
KeyLabelEC2Curve: curve,
},
}

// RFC 9053 Section 7.1.1 says that x and y leading zero octets
// MUST be preserved, but the Go crypto/elliptic package trims them.
// Since x, y might be used before marshaling, we add 0x00 padding here.
size := curveSize(curve)
if x != nil {
key.Params[KeyLabelEC2X] = x
key.Params[KeyLabelEC2X] = append(make([]byte, size-len(x), size), x...)
}
if y != nil {
key.Params[KeyLabelEC2Y] = y
key.Params[KeyLabelEC2Y] = append(make([]byte, size-len(y), size), y...)
}
if d != nil {
key.Params[KeyLabelEC2D] = d
key.Params[KeyLabelEC2D] = append(make([]byte, size-len(d), size), d...)
}
if err := key.validate(KeyOpReserved); err != nil {
return nil, err
Expand Down Expand Up @@ -448,12 +453,14 @@ func (k Key) validate(op KeyOp) error {
return errReqParamsMissing
}
if size := curveSize(crv); size > 0 {
// RFC 8152 Section 13.1.1 says that x and y leading zero octets
// MUST be preserved, but the Go crypto/elliptic package trims them.
// So we relax the check here to allow for omitted leading zero
// octets, we will add them back when marshaling.
if len(x) > size || len(y) > size || len(d) > size {
return errCoordOverflow
if len(y) == 0 && len(x) == size+1 {
return fmt.Errorf("%w: compressed point not supported", ErrInvalidPubKey)
}
if len(x) != size || len(y) != size {
Copy link
Author

@kentakayama kentakayama Dec 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

@OR13 OR13 Dec 4, 2025

Choose a reason for hiding this comment

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

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?

Copy link
Author

@kentakayama kentakayama Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Author

@kentakayama kentakayama Dec 6, 2025

Choose a reason for hiding this comment

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

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.

return ErrInvalidPubKey
}
if len(d) > 0 && len(d) != size {
return ErrInvalidPrivKey
}
}
switch crv {
Expand Down Expand Up @@ -705,9 +712,6 @@ func (k *Key) PrivateKey() (crypto.PrivateKey, error) {
switch alg {
case AlgorithmES256, AlgorithmES384, AlgorithmES512:
_, x, y, d := k.EC2()
if len(x) == 0 || len(y) == 0 {
return nil, fmt.Errorf("%w: compressed point not supported", ErrInvalidPrivKey)
}

var curve elliptic.Curve
switch alg {
Expand Down
51 changes: 46 additions & 5 deletions key_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -866,6 +866,14 @@ func TestNewKeyOKP(t *testing.T) {
name: "x and d missing", args: args{AlgorithmEdDSA, nil, nil},
want: nil,
wantErr: "invalid key: required parameters missing",
}, {
name: "invalid x", args: args{AlgorithmEdDSA, x[:31], d},
want: nil,
wantErr: errCoordOverflow.Error(),
}, {
name: "invalid d", args: args{AlgorithmEdDSA, x, d[:31]},
want: nil,
wantErr: errCoordOverflow.Error(),
},
}
for _, tt := range tests {
Expand All @@ -883,6 +891,7 @@ func TestNewKeyOKP(t *testing.T) {
}

func TestNewNewKeyEC2(t *testing.T) {
// newEC2 always return the full size []byte
ec256x, ec256y, ec256d := newEC2(t, elliptic.P256())
ec384x, ec384y, ec384d := newEC2(t, elliptic.P384())
ec521x, ec521y, ec521d := newEC2(t, elliptic.P521())
Expand Down Expand Up @@ -911,6 +920,19 @@ func TestNewNewKeyEC2(t *testing.T) {
},
},
wantErr: "",
}, {
name: "short x, y and d but valid", args: args{AlgorithmES256, ec256x[:31], ec256y[:31], ec256d[:31]},
want: &Key{
Type: KeyTypeEC2,
Algorithm: AlgorithmES256,
Params: map[any]any{
KeyLabelEC2Curve: CurveP256,
KeyLabelEC2X: append([]byte{0x00}, ec256x[:31]...),
KeyLabelEC2Y: append([]byte{0x00}, ec256y[:31]...),
KeyLabelEC2D: append([]byte{0x00}, ec256d[:31]...),
},
},
wantErr: "",
}, {
name: "valid ES384", args: args{AlgorithmES384, ec384x, ec384y, ec384d},
want: &Key{
Expand Down Expand Up @@ -1479,6 +1501,17 @@ func TestKey_PrivateKey(t *testing.T) {
D: new(big.Int).SetBytes(ec256d),
},
"",
}, {
"CurveP256 compressed x", &Key{
Type: KeyTypeEC2,
Params: map[any]any{
KeyLabelEC2Curve: CurveP256,
KeyLabelEC2X: append([]byte{0x02}, ec256x...),
KeyLabelEC2D: ec256d,
},
},
nil,
"invalid public key: compressed point not supported",
}, {
"CurveP256 missing x and y", &Key{
Type: KeyTypeEC2,
Expand All @@ -1488,7 +1521,7 @@ func TestKey_PrivateKey(t *testing.T) {
},
},
nil,
"invalid private key: compressed point not supported",
ErrInvalidPubKey.Error(),
}, {
"CurveP384", &Key{
Type: KeyTypeEC2,
Expand Down Expand Up @@ -1610,7 +1643,7 @@ func TestKey_PrivateKey(t *testing.T) {
},
},
nil,
"invalid key: overflowing coordinate",
ErrInvalidPubKey.Error(),
}, {
"EC2 incorrect y size", &Key{
Type: KeyTypeEC2,
Expand All @@ -1622,7 +1655,7 @@ func TestKey_PrivateKey(t *testing.T) {
},
},
nil,
"invalid key: overflowing coordinate",
ErrInvalidPubKey.Error(),
}, {
"EC2 incorrect d size", &Key{
Type: KeyTypeEC2,
Expand All @@ -1634,7 +1667,7 @@ func TestKey_PrivateKey(t *testing.T) {
},
},
nil,
"invalid key: overflowing coordinate",
ErrInvalidPrivKey.Error(),
},
}
for _, tt := range tests {
Expand Down Expand Up @@ -1890,5 +1923,13 @@ func newEC2(t *testing.T, crv elliptic.Curve) (x, y, d []byte) {
if err != nil {
t.Fatal(err)
}
return priv.X.Bytes(), priv.Y.Bytes(), priv.D.Bytes()

size := (crv.Params().BitSize + 7) / 8
x = make([]byte, size)
copy(x[size-len(priv.X.Bytes()):], priv.X.Bytes())
y = make([]byte, size)
copy(y[size-len(priv.Y.Bytes()):], priv.Y.Bytes())
d = make([]byte, size)
copy(d[size-len(priv.D.Bytes()):], priv.D.Bytes())
return x, y, d
}