Centralize key validations and check more invalid cases (#156)

Signed-off-by: qmuntal <qmuntaldiaz@microsoft.com>
veraison · Jul 10, 2023 · d16fe13 · d16fe13
1 parent 2f778da
commit d16fe13
Show file tree

Hide file tree

Showing 3 changed files with 362 additions and 113 deletions.
diff --git a/errors.go b/errors.go
@@ -13,11 +13,11 @@ var (
 	ErrNoSignatures          = errors.New("no signatures attached")
 	ErrUnavailableHashFunc   = errors.New("hash function is not available")
 	ErrVerification          = errors.New("verification error")
+	ErrInvalidKey            = errors.New("invalid key")
 	ErrInvalidPubKey         = errors.New("invalid public key")
 	ErrInvalidPrivKey        = errors.New("invalid private key")
 	ErrNotPrivKey            = errors.New("not a private key")
-	ErrSignOpNotSupported    = errors.New("sign key_op not supported by key")
-	ErrVerifyOpNotSupported  = errors.New("verify key_op not supported by key")
-	ErrEC2NoPub              = errors.New("cannot create PrivateKey from EC2 key: missing X or Y")
-	ErrOKPNoPub              = errors.New("cannot create PrivateKey from OKP key: missing X")
+	ErrOpNotSupported        = errors.New("key_op not supported by key")
+	ErrEC2NoPub              = errors.New("cannot create PrivateKey from EC2 key: missing x or y")
+	ErrOKPNoPub              = errors.New("cannot create PrivateKey from OKP key: missing x")
 )
diff --git a/key.go b/key.go
@@ -276,7 +276,7 @@ func NewOKPKey(alg Algorithm, x, d []byte) (*Key, error) {
 		X:         x,
 		D:         d,
 	}
-	return key, key.Validate()
+	return key, key.validate(KeyOpInvalid)
 }
 
 // NewEC2Key returns a Key created using the provided elliptic curve key
@@ -303,17 +303,17 @@ func NewEC2Key(alg Algorithm, x, y, d []byte) (*Key, error) {
 		Y:         y,
 		D:         d,
 	}
-	return key, key.Validate()
+	return key, key.validate(KeyOpInvalid)
 }
 
 // NewSymmetricKey returns a Key created using the provided Symmetric key
 // bytes.
-func NewSymmetricKey(k []byte) (*Key, error) {
+func NewSymmetricKey(k []byte) *Key {
 	key := &Key{
 		KeyType: KeyTypeSymmetric,
 		K:       k,
 	}
-	return key, key.Validate()
+	return key
 }
 
 // NewKeyFromPublic returns a Key created using the provided crypto.PublicKey.
@@ -355,10 +355,24 @@ func NewKeyFromPrivate(priv crypto.PrivateKey) (*Key, error) {
 }
 
 // Validate ensures that the parameters set inside the Key are internally
-// consistent (e.g., that the key type is appropriate to the curve.)
-func (k Key) Validate() error {
+// consistent (e.g., that the key type is appropriate to the curve).
+// It also checks that the key is valid for the requested operation.
+func (k Key) validate(op KeyOp) error {
 	switch k.KeyType {
 	case KeyTypeEC2:
+		switch op {
+		case KeyOpVerify:
+			if len(k.X) == 0 || len(k.Y) == 0 {
+				return ErrEC2NoPub
+			}
+		case KeyOpSign:
+			if len(k.D) == 0 {
+				return ErrNotPrivKey
+			}
+		}
+		if k.Curve == CurveInvalid || (len(k.X) == 0 && len(k.Y) == 0 && len(k.D) == 0) {
+			return ErrInvalidKey
+		}
 		switch k.Curve {
 		case CurveX25519, CurveX448, CurveEd25519, CurveEd448:
 			return fmt.Errorf(
@@ -370,6 +384,19 @@ func (k Key) Validate() error {
 			// see https://www.rfc-editor.org/rfc/rfc8152#section-13.1.1
 		}
 	case KeyTypeOKP:
+		switch op {
+		case KeyOpVerify:
+			if len(k.X) == 0 {
+				return ErrOKPNoPub
+			}
+		case KeyOpSign:
+			if len(k.D) == 0 {
+				return ErrNotPrivKey
+			}
+		}
+		if k.Curve == CurveInvalid || (len(k.X) == 0 && len(k.D) == 0) {
+			return ErrInvalidKey
+		}
 		switch k.Curve {
 		case CurveP256, CurveP384, CurveP521:
 			return fmt.Errorf(
@@ -381,8 +408,9 @@ func (k Key) Validate() error {
 			// see https://www.rfc-editor.org/rfc/rfc8152#section-13.2
 		}
 	case KeyTypeSymmetric:
+		// Nothing to validate
 	default:
-		return errors.New(k.KeyType.String())
+		// Unknown key type, we can't validate custom parameters.
 	}
 
 	// If Algorithm is set, it must match the specified key parameters.
@@ -404,6 +432,18 @@ func (k Key) Validate() error {
 	return nil
 }
 
+func (k Key) canOp(op KeyOp) bool {
+	if k.KeyOps == nil {
+		return true
+	}
+	for _, kop := range k.KeyOps {
+		if kop == op {
+			return true
+		}
+	}
+	return false
+}
+
 type keyalias Key
 
 type marshaledKey struct {
@@ -483,11 +523,14 @@ func (k *Key) UnmarshalCBOR(data []byte) error {
 		return fmt.Errorf("unexpected key type %q", k.KeyType.String())
 	}
 
-	return k.Validate()
+	return k.validate(KeyOpInvalid)
 }
 
 // PublicKey returns a crypto.PublicKey generated using Key's parameters.
 func (k *Key) PublicKey() (crypto.PublicKey, error) {
+	if err := k.validate(KeyOpVerify); err != nil {
+		return nil, err
+	}
 	alg, err := k.deriveAlgorithm()
 	if err != nil {
 		return nil, err
@@ -520,15 +563,14 @@ func (k *Key) PublicKey() (crypto.PublicKey, error) {
 
 // PrivateKey returns a crypto.PrivateKey generated using Key's parameters.
 func (k *Key) PrivateKey() (crypto.PrivateKey, error) {
+	if err := k.validate(KeyOpSign); err != nil {
+		return nil, err
+	}
 	alg, err := k.deriveAlgorithm()
 	if err != nil {
 		return nil, err
 	}
 
-	if len(k.D) == 0 {
-		return nil, ErrNotPrivKey
-	}
-
 	switch alg {
 	case AlgorithmES256, AlgorithmES384, AlgorithmES512:
 		// RFC8152 allows omitting X and Y from private keys;
@@ -591,25 +633,9 @@ func (k *Key) AlgorithmOrDefault() (Algorithm, error) {
 
 // Signer returns a Signer created using Key.
 func (k *Key) Signer() (Signer, error) {
-	if err := k.Validate(); err != nil {
-		return nil, err
-	}
-
-	if k.KeyOps != nil {
-		signFound := false
-
-		for _, kop := range k.KeyOps {
-			if kop == KeyOpSign {
-				signFound = true
-				break
-			}
-		}
-
-		if !signFound {
-			return nil, ErrSignOpNotSupported
-		}
+	if !k.canOp(KeyOpSign) {
+		return nil, ErrOpNotSupported
 	}
-
 	priv, err := k.PrivateKey()
 	if err != nil {
 		return nil, err
@@ -620,48 +646,19 @@ func (k *Key) Signer() (Signer, error) {
 		return nil, err
 	}
 
-	var signer crypto.Signer
-	var ok bool
-
-	switch alg {
-	case AlgorithmES256, AlgorithmES384, AlgorithmES512:
-		signer, ok = priv.(*ecdsa.PrivateKey)
-		if !ok {
-			return nil, ErrInvalidPrivKey
-		}
-	case AlgorithmEd25519:
-		signer, ok = priv.(ed25519.PrivateKey)
-		if !ok {
-			return nil, ErrInvalidPrivKey
-		}
-	default:
-		return nil, ErrAlgorithmNotSupported
+	signer, ok := priv.(crypto.Signer)
+	if !ok {
+		return nil, ErrInvalidPrivKey
 	}
 
 	return NewSigner(alg, signer)
 }
 
 // Verifier returns a Verifier created using Key.
 func (k *Key) Verifier() (Verifier, error) {
-	if err := k.Validate(); err != nil {
-		return nil, err
+	if !k.canOp(KeyOpVerify) {
+		return nil, ErrOpNotSupported
 	}
-
-	if k.KeyOps != nil {
-		verifyFound := false
-
-		for _, kop := range k.KeyOps {
-			if kop == KeyOpVerify {
-				verifyFound = true
-				break
-			}
-		}
-
-		if !verifyFound {
-			return nil, ErrVerifyOpNotSupported
-		}
-	}
-
 	pub, err := k.PublicKey()
 	if err != nil {
 		return nil, err