Addressign PR feedback

Signed-off-by: Pritesh Bandi <[email protected]>

internal/testhelper/certificatetest.go

@@ -3,7 +3,6 @@
 package testhelper
 
 import (
-	_ "crypto"
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
@@ -70,20 +69,20 @@ func setupCertificates() {
 	ecdsaLeaf = getECCertTuple("Notation Test Leaf Cert", &ecdsaRoot)
 
 	k, _ := rsa.GenerateKey(rand.Reader, 1024)
-	unsupported = getRsaCertTupleWithPK(k, "Notation Unsupported Root", nil)
+	unsupported = getRSACertTupleWithPK(k, "Notation Unsupported Root", nil)
 }
 
 func getCertTuple(cn string, issuer *CertTuple) CertTuple {
 	pk, _ := rsa.GenerateKey(rand.Reader, 3072)
-	return getRsaCertTupleWithPK(pk, cn, issuer)
+	return getRSACertTupleWithPK(pk, cn, issuer)
 }
 
 func getECCertTuple(cn string, issuer *ECCertTuple) ECCertTuple {
 	k, _ := ecdsa.GenerateKey(elliptic.P384(), rand.Reader)
-	return getEcdsaCertTupleWithPK(k, cn, issuer)
+	return getECDSACertTupleWithPK(k, cn, issuer)
 }
 
-func getRsaCertTupleWithPK(privKey *rsa.PrivateKey, cn string, issuer *CertTuple) CertTuple {
+func getRSACertTupleWithPK(privKey *rsa.PrivateKey, cn string, issuer *CertTuple) CertTuple {
 	template := getCertTemplate(issuer == nil, cn)
 
 	var certBytes []byte
@@ -100,7 +99,7 @@ func getRsaCertTupleWithPK(privKey *rsa.PrivateKey, cn string, issuer *CertTuple
 	}
 }
 
-func getEcdsaCertTupleWithPK(privKey *ecdsa.PrivateKey, cn string, issuer *ECCertTuple) ECCertTuple {
+func getECDSACertTupleWithPK(privKey *ecdsa.PrivateKey, cn string, issuer *ECCertTuple) ECCertTuple {
 	template := getCertTemplate(issuer == nil, cn)
 
 	var certBytes []byte

signer/errors.go

@@ -2,12 +2,12 @@ package signer
 
 import "fmt"
 
-// InvalidSignatureError is used when the Signature associated is no longer valid.
-type InvalidSignatureError struct {
+// SignatureIntegrityError is used when the Signature associated is no longer valid.
+type SignatureIntegrityError struct {
 	err error
 }
 
-func (e InvalidSignatureError) Error() string {
+func (e SignatureIntegrityError) Error() string {
 	return fmt.Sprintf("signature is invalid. Error: %s", e.err.Error())
 }
 
@@ -40,10 +40,10 @@ func (e SignatureNotFoundError) Error() string {
 	return "signature envelope is not present"
 }
 
-// UntrustedSignatureError is used when signature is not generated using trusted certificates.
-type UntrustedSignatureError struct{}
+// SignatureAuthenticityError is used when signature is not generated using trusted certificates.
+type SignatureAuthenticityError struct{}
 
-func (e UntrustedSignatureError) Error() string {
+func (e SignatureAuthenticityError) Error() string {
 	return "signature is not produced by a trusted signer"
 }
 
@@ -66,7 +66,6 @@ func (e UnsupportedSigningKeyError) Error() string {
 		return fmt.Sprintf("%q signing key is not supported", e.keyType)
 	}
 	return "signing key is not supported"
-
 }
 
 // MalformedArgumentError is used when an argument to a function is malformed.

signer/jws.go

@@ -51,18 +51,18 @@ func (jws *jwsEnvelope) validateIntegrity() error {
 		return SignatureNotFoundError{}
 	}
 
-	sigInfo, err := jws.getSignerInfo()
-	if err != nil {
-		return err
+	if len(jws.internalEnv.Header.CertChain) == 0 {
+		return MalformedSignatureError{msg:"malformed leaf certificate"}
 	}
 
-	if valError := validateSignerInfo(sigInfo); valError != nil {
-		return valError
+	cert, err := x509.ParseCertificate(jws.internalEnv.Header.CertChain[0])
+	if err != nil {
+		return MalformedSignatureError{msg:"malformed leaf certificate"}
 	}
 
 	// verify JWT
 	compact := strings.Join([]string{jws.internalEnv.Protected, jws.internalEnv.Payload, jws.internalEnv.Signature}, ".")
-	return verifyJWT(compact, sigInfo.CertificateChain[0].PublicKey)
+	return verifyJWT(compact, cert.PublicKey)
 }
 
 func (jws *jwsEnvelope) signPayload(req SignRequest) ([]byte, error) {
@@ -415,7 +415,7 @@ func verifyJWT(tokenString string, key crypto.PublicKey) error {
 		t.Method = signingMethod
 		return key, nil
 	}); err != nil {
-		return err
+		return SignatureIntegrityError{err: err}
 	}
 	return nil
 }

signer/signer.go

@@ -2,6 +2,7 @@ package signer
 
 import (
 	"crypto/x509"
+	"fmt"
 	nx509 "github.com/notaryproject/notation-core-go/x509"
 	"time"
 )
@@ -82,35 +83,40 @@ type internalSignatureEnvelope interface {
 	signPayload(SignRequest) ([]byte, error)
 }
 
-// Verify performs integrity validation and validates that cert-chain stored in Signature leads to given set of trusted root.
-// Returns the certificate that is used for establishing trust i.e. authenticity of the signature.
-func (s *SignatureEnvelope) Verify(trustedCerts []*x509.Certificate) (*x509.Certificate, error) {
+// Verify performs integrity validation and validates that cert-chain stored in Signature leads to a given set of the trusted root.
+// Returns the certificate that is used for establishing trust i.e. authenticity of the signature, SignerInfo object
+// containing the information about the signature.
+func (s *SignatureEnvelope) Verify(trustedCerts []*x509.Certificate) (*x509.Certificate, *SignerInfo, error) {
 	if len(s.rawSignatureEnvelope) == 0 {
-		return nil, SignatureNotFoundError{}
+		return nil, nil, SignatureNotFoundError{}
 	}
 
 	if len(trustedCerts) == 0 {
-		return nil, MalformedArgumentError{param: "trustedCerts"}
+		return nil, nil, MalformedArgumentError{param: "trustedCerts"}
 	}
 
 	integrityError := s.internalEnvelope.validateIntegrity()
 	if integrityError != nil {
-		return nil, InvalidSignatureError{err: integrityError}
+		return nil, nil, integrityError
 	}
 
-	singerInfo, singerInfoErr := s.internalEnvelope.getSignerInfo()
+	singerInfo, singerInfoErr := s.GetSignerInfo()
 	if singerInfoErr != nil {
-		return nil, singerInfoErr
+		return nil, nil, singerInfoErr
 	}
 
 	certChain := singerInfo.CertificateChain
-
 	err := nx509.ValidateCertChain(certChain)
 	if err != nil {
-		return nil, singerInfoErr
+		return nil, nil, singerInfoErr
 	}
 
-	return verifySigner(certChain, trustedCerts)
+	resolvedTrust, err := validateAuthenticity(certChain, trustedCerts)
+	if err != nil {
+		return nil, nil, err
+	}
+
+	return resolvedTrust, singerInfo, nil
 }
 
 // Sign generates Signature using given SignRequest.
@@ -136,8 +142,9 @@ func (s SignatureEnvelope) GetSignerInfo() (*SignerInfo, error) {
 
 	signInfo, err := s.internalEnvelope.getSignerInfo()
 	if err != nil {
-		return nil, err
+		return nil, MalformedSignatureError{msg: fmt.Sprintf("signature envelope format is malformed. error: %s", err.Error())}
 	}
+
 	if err := validateSignerInfo(signInfo); err != nil {
 		return nil, err
 	}
@@ -236,14 +243,15 @@ func NewSignatureEnvelope(envelopeMediaType SignatureMediaType) (*SignatureEnvel
 	}
 }
 
-// verifySigner verifies the certificate chain in the signature matches with one of trusted certificate
-func verifySigner(sigCerts []*x509.Certificate, trustedCerts []*x509.Certificate) (*x509.Certificate, error) {
+// validateAuthenticity verifies the certificate chain in the signature matches with one of the trusted certificates
+// and returns a certificate that matches with one of the certificates in the signature envelope.
+func validateAuthenticity(sigCerts []*x509.Certificate, trustedCerts []*x509.Certificate) (*x509.Certificate, error) {
 	for _, trust := range trustedCerts {
 		for _, sig := range sigCerts {
 			if trust.Equal(sig) {
 				return trust, nil
 			}
 		}
 	}
-	return nil, UntrustedSignatureError{}
+	return nil, SignatureAuthenticityError{}
 }

signer/signer_test.go

@@ -121,7 +121,7 @@ func TestVerify(t *testing.T) {
 		t.Fatalf("NewSignatureEnvelopeFromBytes() error = %v", err)
 	}
 
-	cert, err := env.Verify([]*x509.Certificate{trustedCerts[1]})
+	cert, vSignInfo, err := env.Verify([]*x509.Certificate{trustedCerts[1]})
 	if err != nil {
 		t.Fatalf("Verify() error = %v", err)
 	}
@@ -140,36 +140,35 @@ func TestVerify(t *testing.T) {
 	req.Expiry = req.SigningTime.AddDate(0, 0, 1)
 	req.CertificateChain = []*x509.Certificate{trustedCerts[0], trustedCerts[1]}
 	verifySignerInfo(info, req, t)
+
+	if !reflect.DeepEqual(vSignInfo, info) {
+		t.Fatalf("SignerInfo object returned by Verify() and GetSignerInfo() are different.\n"+
+			"Verify=%+v \nGetSignerInfo=%+v", vSignInfo, info)
+	}
 }
 
 func TestVerifyErrors(t *testing.T) {
 	t.Run("when trustedCerts is absent", func(t *testing.T) {
 		env, _ := NewSignatureEnvelopeFromBytes([]byte("{}"), JwsJsonMediaType)
-		_, err := env.Verify([]*x509.Certificate{})
+		_, _, err := env.Verify([]*x509.Certificate{})
 		if !(err != nil && errors.As(err, new(MalformedArgumentError))) {
-			t.Errorf("Expected MalformedArgumentError but found %q", err)
+			t.Errorf("Expected MalformedArgumentError but found %T", err)
 		}
 	})
 
 	t.Run("when trustedCerts are not trusted", func(t *testing.T) {
 		env, _ := NewSignatureEnvelopeFromBytes([]byte(TEST_VALID_SIG), JwsJsonMediaType)
-		if _, err := env.Verify([]*x509.Certificate{testhelper.GetECRootCertificate().Cert}); err != nil {
-			if !errors.As(err, &UntrustedSignatureError{}) {
-				t.Errorf("Expected %T but found %T", &UntrustedSignatureError{}, err)
-			}
-		} else {
-			t.Errorf("Expected UntrustedSignatureError but not found")
+		_, _, err := env.Verify([]*x509.Certificate{testhelper.GetECRootCertificate().Cert})
+		if !(err != nil && errors.As(err, new(SignatureAuthenticityError))) {
+			t.Errorf("Expected SignatureAuthenticityError but found %T", err)
 		}
 	})
 
 	t.Run("when invalid signature is provided", func(t *testing.T) {
 		env, _ := NewSignatureEnvelopeFromBytes([]byte(TEST_INVALID_SIG), JwsJsonMediaType)
-		if _, err := env.Verify([]*x509.Certificate{testhelper.GetECRootCertificate().Cert}); err != nil {
-			if !errors.As(err, &InvalidSignatureError{}) {
-				t.Errorf("Expected %T but found %T", InvalidSignatureError{}, err)
-			}
-		} else {
-			t.Errorf("Expected MalformedArgumentError but not found")
+		_, _, err := env.Verify([]*x509.Certificate{testhelper.GetECRootCertificate().Cert})
+		if !(err != nil && errors.As(err, new(SignatureIntegrityError))) {
+			t.Errorf("Expected SignatureIntegrityError but found %T", err)
 		}
 	})
 }
@@ -189,7 +188,7 @@ func TestSignAndVerify(t *testing.T) {
 		}
 
 		// Verify using same env struct
-		cert, err := env.Verify([]*x509.Certificate{testhelper.GetRSARootCertificate().Cert})
+		cert, _, err := env.Verify([]*x509.Certificate{testhelper.GetRSARootCertificate().Cert})
 		if err != nil {
 			t.Fatalf("Verify() error = %v", err)
 		}
@@ -222,7 +221,7 @@ func TestSignAndVerify(t *testing.T) {
 		}
 
 		// Verify using same env struct
-		cert, err := env.Verify([]*x509.Certificate{testhelper.GetECRootCertificate().Cert})
+		cert, _, err := env.Verify([]*x509.Certificate{testhelper.GetECRootCertificate().Cert})
 		if err != nil {
 			t.Fatalf("Verify() error = %v", err)
 		}