refactor: address comments

Signed-off-by: Binbin Li <libinbin@microsoft.com>

signature/algorithm.go

@@ -33,7 +33,10 @@ const (
 
 // KeySpec defines a key type and size.
 type KeySpec struct {
+	// KeyType is the type of the key.
 	Type KeyType
+
+	// KeySize is the size of the key in bits.
 	Size int
 }
 
@@ -62,7 +65,7 @@ func ExtractKeySpec(signingCert *x509.Certificate) (KeySpec, error) {
 			}, nil
 		default:
 			return KeySpec{}, &UnsupportedSigningKeyError{
-				Msg: fmt.Sprintf("rsa key size %d is not supported", bitSize),
+				Msg: fmt.Sprintf("rsa key size %d bits is not supported", bitSize),
 			}
 		}
 	case *ecdsa.PublicKey:
@@ -74,12 +77,12 @@ func ExtractKeySpec(signingCert *x509.Certificate) (KeySpec, error) {
 			}, nil
 		default:
 			return KeySpec{}, &UnsupportedSigningKeyError{
-				Msg: fmt.Sprintf("ecdsa key size %d is not supported", bitSize),
+				Msg: fmt.Sprintf("ecdsa key size %d bits is not supported", bitSize),
 			}
 		}
 	}
 	return KeySpec{}, &UnsupportedSigningKeyError{
-		Msg: "invalid public key type",
+		Msg: "unsupported public key type",
 	}
 }
 

signature/envelope.go

@@ -46,6 +46,8 @@ var envelopeFuncs map[string]envelopeFunc
 // RegisterEnvelopeType registers newFunc and parseFunc for the given mediaType.
 // Those functions are intended to be called when creating a new envelope.
 // It will be called while inializing the built-in envelopes(JWS/COSE).
+//
+// This method is not go-routine safe.
 func RegisterEnvelopeType(mediaType string, newFunc NewEnvelopeFunc, parseFunc ParseEnvelopeFunc) error {
 	if newFunc == nil || parseFunc == nil {
 		return fmt.Errorf("required functions not provided")
@@ -81,7 +83,7 @@ func NewEnvelope(mediaType string) (Envelope, error) {
 	return envelopeFunc.newFunc(), nil
 }
 
-// ParseEnvelope generates an envelope by given envelope bytes with specified
+// ParseEnvelope generates an envelope for given envelope bytes with specified
 // media type.
 func ParseEnvelope(mediaType string, envelopeBytes []byte) (Envelope, error) {
 	envelopeFunc, ok := envelopeFuncs[mediaType]

signature/errors.go

@@ -18,17 +18,17 @@ func (e *SignatureIntegrityError) Unwrap() error {
 	return e.Err
 }
 
-// MalformedSignatureError is used when Signature envelope is malformed.
-type MalformedSignatureError struct {
+// InvalidSignatureError is used when Signature envelope is invalid.
+type InvalidSignatureError struct {
 	Msg string
 }
 
 // Error returns the error message or the default message if not provided.
-func (e MalformedSignatureError) Error() string {
+func (e InvalidSignatureError) Error() string {
 	if e.Msg != "" {
 		return e.Msg
 	}
-	return "signature envelope format is malformed"
+	return "signature envelope format is invalid"
 
 }
 
@@ -71,45 +71,45 @@ func (e UnsupportedSigningKeyError) Error() string {
 	return "signing key is not supported"
 }
 
-// MalformedArgumentError is used when an argument to a function is malformed.
-type MalformedArgumentError struct {
+// InvalidArgumentError is used when an argument to a function is invalid.
+type InvalidArgumentError struct {
 	Param string
 	Err   error
 }
 
 // Error returns the error message.
-func (e *MalformedArgumentError) Error() string {
+func (e *InvalidArgumentError) Error() string {
 	if e.Err != nil {
-		return fmt.Sprintf("%q param is malformed. Error: %s", e.Param, e.Err.Error())
+		return fmt.Sprintf("%q param is invalid. Error: %s", e.Param, e.Err.Error())
 	}
-	return fmt.Sprintf("%q param is malformed", e.Param)
+	return fmt.Sprintf("%q param is invalid", e.Param)
 }
 
 // Unwrap returns the unwrapped error
-func (e *MalformedArgumentError) Unwrap() error {
+func (e *InvalidArgumentError) Unwrap() error {
 	return e.Err
 }
 
-// MalformedSignRequestError is used when SignRequest is malformed.
-type MalformedSignRequestError struct {
+// InvalidSignRequestError is used when SignRequest is invalid.
+type InvalidSignRequestError struct {
 	Msg string
 }
 
 // Error returns the error message or the default message if not provided.
-func (e *MalformedSignRequestError) Error() string {
+func (e *InvalidSignRequestError) Error() string {
 	if e.Msg != "" {
 		return e.Msg
 	}
-	return "SignRequest is malformed"
+	return "SignRequest is invalid"
 }
 
-// SignatureAlgoNotSupportedError is used when signing algo is not supported.
-type SignatureAlgoNotSupportedError struct {
+// UnsupportedSignatureAlgoError is used when signing algo is not supported.
+type UnsupportedSignatureAlgoError struct {
 	Alg string
 }
 
 // Error returns the formatted error message.
-func (e *SignatureAlgoNotSupportedError) Error() string {
+func (e *UnsupportedSignatureAlgoError) Error() string {
 	return fmt.Sprintf("signature algorithm %q is not supported", e.Alg)
 }
 
@@ -121,12 +121,12 @@ func (e *SignatureEnvelopeNotFoundError) Error() string {
 	return "signature envelope is not present"
 }
 
-// EnvelopeKeyRepeatedError is used when repeated key name found in the envelope.
-type EnvelopeKeyRepeatedError struct {
+// DuplicateKeyError is used when repeated key name found.
+type DuplicateKeyError struct {
 	Key string
 }
 
 // Error returns the formatted error message.
-func (e *EnvelopeKeyRepeatedError) Error() string {
-	return fmt.Sprintf("repeated key: %q exists in the envelope.", e.Key)
+func (e *DuplicateKeyError) Error() string {
+	return fmt.Sprintf("repeated key: %q exists.", e.Key)
 }

signature/errors_test.go

@@ -28,20 +28,20 @@ func TestSignatureIntegrityError(t *testing.T) {
 	}
 }
 
-func TestMalformedSignatureError(t *testing.T) {
+func TestInvalidSignatureError(t *testing.T) {
 	tests := []struct {
 		name   string
-		err    *MalformedSignatureError
+		err    *InvalidSignatureError
 		expect string
 	}{
 		{
 			name:   "err msg set",
-			err:    &MalformedSignatureError{Msg: errMsg},
+			err:    &InvalidSignatureError{Msg: errMsg},
 			expect: errMsg,
 		},
 		{
 			name:   "err msg not set",
-			err:    &MalformedSignatureError{},
+			err:    &InvalidSignatureError{},
 			expect: "signature envelope format is malformed",
 		},
 	}
@@ -93,16 +93,16 @@ func TestUnsupportedSigningKeyError(t *testing.T) {
 	}
 }
 
-func TestMalformedArgumentError(t *testing.T) {
-	expectedMsg := "\"hola\" param is malformed"
-	validateErrorMsg(&MalformedArgumentError{Param: "hola"}, expectedMsg, t)
+func TestInvalidArgumentError(t *testing.T) {
+	expectedMsg := "\"hola\" param is invalid"
+	validateErrorMsg(&InvalidArgumentError{Param: "hola"}, expectedMsg, t)
 
-	expectedMsg = "\"hola\" param is malformed. Error: se produjo un error"
-	validateErrorMsg(&MalformedArgumentError{Param: "hola", Err: fmt.Errorf("se produjo un error")}, expectedMsg, t)
+	expectedMsg = "\"hola\" param is invalid. Error: se produjo un error"
+	validateErrorMsg(&InvalidArgumentError{Param: "hola", Err: fmt.Errorf("se produjo un error")}, expectedMsg, t)
 }
 
-func TestSignatureAlgoNotSupportedError(t *testing.T) {
-	err := &SignatureAlgoNotSupportedError{
+func TestUnsupportedSignatureAlgoError(t *testing.T) {
+	err := &UnsupportedSignatureAlgoError{
 		Alg: testAlg,
 	}
 
@@ -112,12 +112,12 @@ func TestSignatureAlgoNotSupportedError(t *testing.T) {
 	}
 }
 
-func TestMalformedSignRequestError(t *testing.T) {
+func TestInvalidSignRequestError(t *testing.T) {
 	expectedMsg := "SignRequest is malformed"
-	validateErrorMsg(&MalformedSignRequestError{}, expectedMsg, t)
+	validateErrorMsg(&InvalidSignRequestError{}, expectedMsg, t)
 
 	expectedMsg = "Se produjo un error"
-	validateErrorMsg(&MalformedSignRequestError{Msg: expectedMsg}, expectedMsg, t)
+	validateErrorMsg(&InvalidSignRequestError{Msg: expectedMsg}, expectedMsg, t)
 }
 
 func validateErrorMsg(err error, expectedMsg string, t *testing.T) {
@@ -128,7 +128,7 @@ func validateErrorMsg(err error, expectedMsg string, t *testing.T) {
 }
 
 func TestMalformedArgumentError_Unwrap(t *testing.T) {
-	err := &MalformedArgumentError{
+	err := &InvalidArgumentError{
 		Param: testParam,
 		Err:   errors.New(errMsg),
 	}
@@ -157,7 +157,7 @@ func TestSignatureAuthenticityError(t *testing.T) {
 }
 
 func TestEnvelopeKeyRepeatedError(t *testing.T) {
-	err := &EnvelopeKeyRepeatedError{Key: errMsg}
+	err := &DuplicateKeyError{Key: errMsg}
 	expectMsg := fmt.Sprintf("repeated key: %q exists in the envelope.", errMsg)
 
 	if err.Error() != expectMsg {

signature/internal/base/envelope.go

@@ -62,7 +62,7 @@ func (e *Envelope) Sign(req *signature.SignRequest) ([]byte, error) {
 func (e *Envelope) Verify() (*signature.EnvelopeContent, error) {
 	// validation before the core verify process.
 	if len(e.Raw) == 0 {
-		return nil, &signature.MalformedSignatureError{}
+		return nil, &signature.SignatureNotFoundError{}
 	}
 
 	// core verify process.
@@ -82,7 +82,7 @@ func (e *Envelope) Verify() (*signature.EnvelopeContent, error) {
 // Content returns the validated signature information and payload.
 func (e *Envelope) Content() (*signature.EnvelopeContent, error) {
 	if len(e.Raw) == 0 {
-		return nil, &signature.MalformedSignatureError{Msg: "raw signature is empty"}
+		return nil, &signature.SignatureNotFoundError{}
 	}
 
 	content, err := e.Envelope.Content()
@@ -103,12 +103,16 @@ func validateSignRequest(req *signature.SignRequest) error {
 		return err
 	}
 
-	if err := validateSigningTime(req.SigningTime, req.Expiry); err != nil {
+	if err := validateSigningAndExpiryTime(req.SigningTime, req.Expiry); err != nil {
 		return err
 	}
 
 	if req.Signer == nil {
-		return &signature.MalformedSignatureError{Msg: "signer is nil"}
+		return &signature.InvalidSignRequestError{Msg: "signer is nil"}
+	}
+
+	if req.SigningScheme == "" {
+		return &signature.InvalidSignRequestError{Msg: "SigningScheme not present"}
 	}
 
 	_, err := req.Signer.KeySpec()
@@ -127,49 +131,46 @@ func validateEnvelopeContent(content *signature.EnvelopeContent) error {
 // validateSignerInfo performs basic set of validations on SignerInfo struct.
 func validateSignerInfo(info *signature.SignerInfo) error {
 	if len(info.Signature) == 0 {
-		return &signature.MalformedSignatureError{Msg: "signature not present or is empty"}
+		return &signature.InvalidSignatureError{Msg: "signature not present or is empty"}
 	}
 
 	if info.SignatureAlgorithm == 0 {
-		return &signature.MalformedSignatureError{Msg: "SignatureAlgorithm is not present"}
+		return &signature.InvalidSignatureError{Msg: "SignatureAlgorithm is not present"}
 	}
 
 	signingTime := info.SignedAttributes.SigningTime
-	if err := validateSigningTime(signingTime, info.SignedAttributes.Expiry); err != nil {
+	if err := validateSigningAndExpiryTime(signingTime, info.SignedAttributes.Expiry); err != nil {
 		return err
 	}
 
+	if info.SignedAttributes.SigningScheme == "" {
+		return &signature.InvalidSignatureError{Msg: "SigningScheme not present"}
+	}
+
 	return validateCertificateChain(
 		info.CertificateChain,
 		signingTime,
 		info.SignatureAlgorithm,
 	)
 }
 
-// validateSigningTime checks that signing time is within the valid range of
-// time duration.
-func validateSigningTime(signingTime, expireTime time.Time) error {
+// validateSigningAndExpiryTime checks that signing time is within the valid
+// range of time duration and expire time is valid.
+func validateSigningAndExpiryTime(signingTime, expireTime time.Time) error {
 	if signingTime.IsZero() {
-		return &signature.MalformedSignatureError{Msg: "signing-time not present"}
+		return &signature.InvalidSignatureError{Msg: "signing-time not present"}
 	}
 
 	if !expireTime.IsZero() && (expireTime.Before(signingTime) || expireTime.Equal(signingTime)) {
-		return &signature.MalformedSignatureError{Msg: "expiry cannot be equal or before the signing time"}
+		return &signature.InvalidSignatureError{Msg: "expiry cannot be equal or before the signing time"}
 	}
 	return nil
 }
 
 // validatePayload performs validation of the payload.
 func validatePayload(payload *signature.Payload) error {
-	switch payload.ContentType {
-	case signature.MediaTypePayloadV1:
-		if len(payload.Content) == 0 {
-			return &signature.MalformedSignatureError{Msg: "content not present"}
-		}
-	default:
-		return &signature.MalformedSignatureError{
-			Msg: fmt.Sprintf("payload content type: {%s} not supported", payload.ContentType),
-		}
+	if len(payload.Content) == 0 {
+		return &signature.InvalidSignatureError{Msg: "content not present"}
 	}
 
 	return nil
@@ -178,22 +179,22 @@ func validatePayload(payload *signature.Payload) error {
 // validateCertificateChain performs the validation of the certificate chain.
 func validateCertificateChain(certChain []*x509.Certificate, signTime time.Time, expectedAlg signature.Algorithm) error {
 	if len(certChain) == 0 {
-		return &signature.MalformedSignatureError{Msg: "certificate-chain not present or is empty"}
+		return &signature.InvalidSignatureError{Msg: "certificate-chain not present or is empty"}
 	}
 
 	err := nx509.ValidateCodeSigningCertChain(certChain, signTime)
 	if err != nil {
-		return &signature.MalformedSignatureError{
+		return &signature.InvalidSignatureError{
 			Msg: fmt.Sprintf("certificate-chain is invalid, %s", err),
 		}
 	}
 
 	signingAlg, err := getSignatureAlgorithm(certChain[0])
 	if err != nil {
-		return &signature.MalformedSignatureError{Msg: err.Error()}
+		return &signature.InvalidSignatureError{Msg: err.Error()}
 	}
 	if signingAlg != expectedAlg {
-		return &signature.MalformedSignatureError{
+		return &signature.InvalidSignatureError{
 			Msg: fmt.Sprintf("mismatch between signature algorithm derived from signing certificate (%v) and signing algorithm specified (%vs)", signingAlg, expectedAlg),
 		}
 	}