refactor: clarify error handling for missing vs malformed params for EC2 and OKP keys

Signed-off-by: Ken Takayama <[email protected]>

Author: kentakayama

key.go

@@ -427,9 +427,9 @@ var (
 	// The following errors are used multiple times
 	// in Key.validate. We declare them here to avoid
 	// duplication. They are not considered public errors.
-	errCoordOverflow    = fmt.Errorf("%w: overflowing coordinate", ErrInvalidKey)
-	errReqParamsMissing = fmt.Errorf("%w: required parameters missing", ErrInvalidKey)
-	errInvalidCurve     = fmt.Errorf("%w: curve not supported for the given key type", ErrInvalidKey)
+	errCoordSizeMismatch = fmt.Errorf("%w: coordinate size mismatch", ErrInvalidKey)
+	errReqParamsMissing  = fmt.Errorf("%w: required parameters missing", ErrInvalidKey)
+	errInvalidCurve      = fmt.Errorf("%w: curve not supported for the given key type", ErrInvalidKey)
 )
 
 // Validate ensures that the parameters set inside the Key are internally
@@ -439,28 +439,49 @@ func (k Key) validate(op KeyOp) error {
 	switch k.Type {
 	case KeyTypeEC2:
 		crv, x, y, d := k.EC2()
+		// Check required paraemeters exist
 		switch op {
 		case KeyOpVerify:
-			if len(x) == 0 || len(y) == 0 {
+			if x == nil || y == nil {
 				return ErrEC2NoPub
 			}
 		case KeyOpSign:
-			if len(d) == 0 {
+			if d == nil {
 				return ErrNotPrivKey
 			}
 		}
-		if crv == CurveReserved || (len(x) == 0 && len(y) == 0 && len(d) == 0) {
+		if crv == CurveReserved || (x == nil && y == nil && d == nil) {
 			return errReqParamsMissing
 		}
+
+		// Then, validate their length if exist and if the size is known
 		if size := curveSize(crv); size > 0 {
 			if len(y) == 0 && len(x) == size+1 {
+				// NOTE: RFC 9053 Section 7.1.1 describes compressed points in COSE_Key
+				// using a boolean y-coordinate value (false/true). However, this code
+				// currently assumes SEC1-style compression, where 0x02 or 0x03 is prepended
+				// to the x-coordinate.
+				//
+				// This behavior may change in the future, for example, we might compute the
+				// y-coordinate during UnmarshalCBOR, and MarshalCBOR would avoid emitting
+				// compressed points entirely.
+				//
+				// In that case, this conditional may become unnecessary, since the general
+				// length check below (`len(x) > 0 && len(x) != size`) would already catch
+				// invalid compressed input.
+				//
+				// See discussion in https://github.com/veraison/go-cose/pull/223 .
+				// Consider revisiting this logic in a future update.
 				return fmt.Errorf("%w: compressed point not supported", ErrInvalidPubKey)
 			}
-			if len(x) != size || len(y) != size {
-				return ErrInvalidPubKey
+			if len(x) > 0 && len(x) != size {
+				return errCoordSizeMismatch
+			}
+			if len(y) > 0 && len(y) != size {
+				return errCoordSizeMismatch
 			}
 			if len(d) > 0 && len(d) != size {
-				return ErrInvalidPrivKey
+				return errCoordSizeMismatch
 			}
 		}
 		switch crv {
@@ -472,21 +493,27 @@ func (k Key) validate(op KeyOp) error {
 		}
 	case KeyTypeOKP:
 		crv, x, d := k.OKP()
+		// Check required paraemeters exist
 		switch op {
 		case KeyOpVerify:
-			if len(x) == 0 {
+			if x == nil {
 				return ErrOKPNoPub
 			}
 		case KeyOpSign:
-			if len(d) == 0 {
+			if d == nil {
 				return ErrNotPrivKey
 			}
 		}
-		if crv == CurveReserved || (len(x) == 0 && len(d) == 0) {
+		if crv == CurveReserved || (x == nil && d == nil) {
 			return errReqParamsMissing
 		}
-		if (len(x) > 0 && len(x) != ed25519.PublicKeySize) || (len(d) > 0 && len(d) != ed25519.SeedSize) {
-			return errCoordOverflow
+
+		// Then, validate their length if exist and if the size is known
+		if len(x) > 0 && len(x) != ed25519.PublicKeySize {
+			return errCoordSizeMismatch
+		}
+		if len(d) > 0 && len(d) != ed25519.SeedSize {
+			return errCoordSizeMismatch
 		}
 		switch crv {
 		case CurveP256, CurveP384, CurveP521:

key_test.go

@@ -869,11 +869,11 @@ func TestNewKeyOKP(t *testing.T) {
 		}, {
 			name: "invalid x", args: args{AlgorithmEdDSA, x[:31], d},
 			want:    nil,
-			wantErr: errCoordOverflow.Error(),
+			wantErr: errCoordSizeMismatch.Error(),
 		}, {
 			name: "invalid d", args: args{AlgorithmEdDSA, x, d[:31]},
 			want:    nil,
-			wantErr: errCoordOverflow.Error(),
+			wantErr: errCoordSizeMismatch.Error(),
 		},
 	}
 	for _, tt := range tests {
@@ -1520,8 +1520,15 @@ func TestKey_PrivateKey(t *testing.T) {
 					KeyLabelEC2D:     ec256d,
 				},
 			},
-			nil,
-			ErrInvalidPubKey.Error(),
+			&ecdsa.PrivateKey{
+				PublicKey: ecdsa.PublicKey{
+					Curve: elliptic.P256(),
+					X:     new(big.Int).SetBytes([]byte{}),
+					Y:     new(big.Int).SetBytes([]byte{}),
+				},
+				D: new(big.Int).SetBytes(ec256d),
+			},
+			"",
 		}, {
 			"CurveP384", &Key{
 				Type: KeyTypeEC2,
@@ -1597,7 +1604,7 @@ func TestKey_PrivateKey(t *testing.T) {
 				},
 			},
 			nil,
-			"invalid key: overflowing coordinate",
+			errCoordSizeMismatch.Error(),
 		}, {
 			"OKP incorrect d size", &Key{
 				Type: KeyTypeOKP,
@@ -1608,7 +1615,7 @@ func TestKey_PrivateKey(t *testing.T) {
 				},
 			},
 			nil,
-			"invalid key: overflowing coordinate",
+			errCoordSizeMismatch.Error(),
 		}, {
 			"EC2 missing D", &Key{
 				Type: KeyTypeEC2,
@@ -1643,7 +1650,7 @@ func TestKey_PrivateKey(t *testing.T) {
 				},
 			},
 			nil,
-			ErrInvalidPubKey.Error(),
+			errCoordSizeMismatch.Error(),
 		}, {
 			"EC2 incorrect y size", &Key{
 				Type: KeyTypeEC2,
@@ -1655,7 +1662,7 @@ func TestKey_PrivateKey(t *testing.T) {
 				},
 			},
 			nil,
-			ErrInvalidPubKey.Error(),
+			errCoordSizeMismatch.Error(),
 		}, {
 			"EC2 incorrect d size", &Key{
 				Type: KeyTypeEC2,
@@ -1667,7 +1674,7 @@ func TestKey_PrivateKey(t *testing.T) {
 				},
 			},
 			nil,
-			ErrInvalidPrivKey.Error(),
+			errCoordSizeMismatch.Error(),
 		},
 	}
 	for _, tt := range tests {