fix: self-signed leaf certificate is not self-issued (#236)

Fix: - added self-issued check for self-signed leaf certificate - improved test coverage for `x509` package - fixed test case for `revocation` package Resolves #126 --------- Signed-off-by: Junjie Gao <[email protected]>
notaryproject · Nov 5, 2024 · 0c651a4 · 0c651a4
1 parent 1683ddb
commit 0c651a4
Show file tree

Hide file tree

Showing 4 changed files with 134 additions and 3 deletions.
diff --git a/x509/codesigning_cert_validations.go b/x509/codesigning_cert_validations.go
@@ -14,6 +14,7 @@
 package x509
 
 import (
+	"bytes"
 	"crypto/x509"
 	"errors"
 	"fmt"
@@ -34,12 +35,17 @@ func ValidateCodeSigningCertChain(certChain []*x509.Certificate, signingTime *ti
 	// For self-signed signing certificate (not a CA)
 	if len(certChain) == 1 {
 		cert := certChain[0]
-		if signedTimeError := validateSigningTime(cert, signingTime); signedTimeError != nil {
-			return signedTimeError
-		}
+		// check self-signed
 		if err := cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature); err != nil {
 			return fmt.Errorf("invalid self-signed certificate. subject: %q. Error: %w", cert.Subject, err)
 		}
+		// check self-issued
+		if !bytes.Equal(cert.RawSubject, cert.RawIssuer) {
+			return fmt.Errorf("invalid self-signed certificate. subject: %q. Error: issuer(%s) and subject(%s) are not the same", cert.Subject, cert.Issuer, cert.Subject)
+		}
+		if signedTimeError := validateSigningTime(cert, signingTime); signedTimeError != nil {
+			return signedTimeError
+		}
 		if err := validateCodeSigningLeafCertificate(cert); err != nil {
 			return fmt.Errorf("invalid self-signed certificate. Error: %w", err)
 		}

diff --git a/x509/codesigning_cert_validations_test.go b/x509/codesigning_cert_validations_test.go
@@ -19,6 +19,7 @@ import (
 	_ "embed"
 	"errors"
 	"os"
+	"strings"
 	"testing"
 	"time"
 
@@ -200,6 +201,35 @@ func TestFailEmptyChain(t *testing.T) {
 	assertErrorEqual("certificate chain must contain at least one certificate", err, t)
 }
 
+func TestFailNonSelfSignedLeafCert(t *testing.T) {
+	signingTime := time.Now()
+	err := ValidateCodeSigningCertChain([]*x509.Certificate{codeSigningCert}, &signingTime)
+
+	assertErrorEqual("invalid self-signed certificate. subject: \"CN=CodeSigningLeaf\". Error: crypto/rsa: verification error", err, t)
+}
+
+func TestFailSelfIssuedCodeSigningCert(t *testing.T) {
+	chainTuple := testhelper.GetRevokableRSATimestampChain(2)
+	// the leaf certiifcate and the root certificate share the same private key
+	// so the leaf is also self-signed but issuer and subject are different
+	chain := []*x509.Certificate{chainTuple[0].Cert}
+	signingTime := time.Now()
+	err := ValidateCodeSigningCertChain(chain, &signingTime)
+	assertErrorEqual("invalid self-signed certificate. subject: \"CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US\". Error: issuer(CN=Notation Test Revokable RSA Chain Cert Root,O=Notary,L=Seattle,ST=WA,C=US) and subject(CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US) are not the same", err, t)
+}
+
+func TestInvalidCodeSigningCertSigningTime(t *testing.T) {
+	chainTuple := testhelper.GetRevokableRSATimestampChain(2)
+	chain := []*x509.Certificate{chainTuple[1].Cert}
+	signingTime := time.Date(2021, 7, 7, 20, 48, 42, 0, time.UTC)
+
+	expectPrefix := "certificate with subject \"CN=Notation Test Revokable RSA Chain Cert Root,O=Notary,L=Seattle,ST=WA,C=US\" was invalid at signing time of 2021-07-07 20:48:42 +0000 UTC"
+	err := ValidateCodeSigningCertChain(chain, &signingTime)
+	if !strings.HasPrefix(err.Error(), expectPrefix) {
+		t.Errorf("expected error to start with %q, got %q", expectPrefix, err)
+	}
+}
+
 func TestFailInvalidSigningTime(t *testing.T) {
 	certChain := []*x509.Certificate{codeSigningCert, intermediateCert2, intermediateCert1, rootCert}
 

diff --git a/x509/timestamp_cert_validations.go b/x509/timestamp_cert_validations.go
@@ -14,6 +14,7 @@
 package x509
 
 import (
+	"bytes"
 	"crypto/x509"
 	"errors"
 	"fmt"
@@ -33,9 +34,14 @@ func ValidateTimestampingCertChain(certChain []*x509.Certificate) error {
 	// For self-signed signing certificate (not a CA)
 	if len(certChain) == 1 {
 		cert := certChain[0]
+		// check self-signed
 		if err := cert.CheckSignature(cert.SignatureAlgorithm, cert.RawTBSCertificate, cert.Signature); err != nil {
 			return fmt.Errorf("invalid self-signed certificate. subject: %q. Error: %w", cert.Subject, err)
 		}
+		// check self-issued
+		if !bytes.Equal(cert.RawSubject, cert.RawIssuer) {
+			return fmt.Errorf("invalid self-signed certificate. subject: %q. Error: issuer (%s) and subject (%s) are not the same", cert.Subject, cert.Issuer, cert.Subject)
+		}
 		if err := validateTimestampingLeafCertificate(cert); err != nil {
 			return fmt.Errorf("invalid self-signed certificate. Error: %w", err)
 		}

diff --git a/x509/timestamp_cert_validations_test.go b/x509/timestamp_cert_validations_test.go
@@ -14,9 +14,18 @@
 package x509
 
 import (
+	"crypto/rand"
+	"crypto/rsa"
 	"crypto/x509"
 	"crypto/x509/pkix"
+	"encoding/asn1"
+	"math/big"
+	"strings"
 	"testing"
+	"time"
+
+	"github.com/notaryproject/notation-core-go/internal/oid"
+	"github.com/notaryproject/notation-core-go/testhelper"
 )
 
 func TestValidTimestampingChain(t *testing.T) {
@@ -39,6 +48,42 @@ func TestValidTimestampingChain(t *testing.T) {
 	}
 }
 
+func TestFailSelfIssuedTimestampingCert(t *testing.T) {
+	chainTuple := testhelper.GetRevokableRSATimestampChain(2)
+	// the leaf certiifcate and the root certificate share the same private key
+	// so the leaf is also self-signed but issuer and subject are different
+	chain := []*x509.Certificate{chainTuple[0].Cert}
+	err := ValidateTimestampingCertChain(chain)
+	assertErrorEqual("invalid self-signed certificate. subject: \"CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US\". Error: issuer (CN=Notation Test Revokable RSA Chain Cert Root,O=Notary,L=Seattle,ST=WA,C=US) and subject (CN=Notation Test Revokable RSA Chain Cert 2,O=Notary,L=Seattle,ST=WA,C=US) are not the same", err, t)
+}
+
+func TestInvalidTimestampSelfSignedCert(t *testing.T) {
+	cert, err := createSelfSignedCert("valid cert", "valid cert", false)
+	if err != nil {
+		t.Error(err)
+	}
+	certChain := []*x509.Certificate{cert}
+
+	expectPrefix := "invalid self-signed certificate. Error: timestamp signing certificate with subject \"CN=valid cert\" must have and only have Timestamping as extended key usage"
+	err = ValidateTimestampingCertChain(certChain)
+	if !strings.HasPrefix(err.Error(), expectPrefix) {
+		t.Errorf("expected error to start with %q, got %q", expectPrefix, err)
+	}
+}
+
+func TestValidTimestampSelfSignedCert(t *testing.T) {
+	cert, err := createSelfSignedCert("valid cert", "valid cert", true)
+	if err != nil {
+		t.Error(err)
+	}
+	certChain := []*x509.Certificate{cert}
+
+	err = ValidateTimestampingCertChain(certChain)
+	if err != nil {
+		t.Error(err)
+	}
+}
+
 func TestInvalidTimestampingChain(t *testing.T) {
 	timestamp_leaf, err := readSingleCertificate("testdata/timestamp_leaf.crt")
 	if err != nil {
@@ -215,3 +260,47 @@ func TestEkuToString(t *testing.T) {
 		t.Fatalf("expected 7")
 	}
 }
+
+func createSelfSignedCert(subject string, issuer string, isTimestamp bool) (*x509.Certificate, error) {
+	priv, err := rsa.GenerateKey(rand.Reader, 2048)
+	if err != nil {
+		return nil, err
+	}
+
+	template := &x509.Certificate{
+		SerialNumber: big.NewInt(1),
+		Subject:      pkix.Name{CommonName: subject},
+		NotBefore:    time.Now(),
+		NotAfter:     time.Now().Add(365 * 24 * time.Hour),
+		KeyUsage:     x509.KeyUsageDigitalSignature,
+	}
+
+	if isTimestamp {
+		oids := []asn1.ObjectIdentifier{{1, 3, 6, 1, 5, 5, 7, 3, 8}}
+		value, err := asn1.Marshal(oids)
+		if err != nil {
+			return nil, err
+		}
+		template.ExtraExtensions = []pkix.Extension{{
+			Id:       oid.ExtKeyUsage,
+			Critical: true,
+			Value:    value,
+		}}
+		template.ExtKeyUsage = []x509.ExtKeyUsage{x509.ExtKeyUsageTimeStamping}
+	}
+
+	parentTemplate := &x509.Certificate{
+		SerialNumber: big.NewInt(2),
+		Subject:      pkix.Name{CommonName: issuer},
+		NotBefore:    time.Now(),
+		NotAfter:     time.Now().Add(365 * 24 * time.Hour),
+		KeyUsage:     x509.KeyUsageCertSign,
+	}
+
+	certDER, err := x509.CreateCertificate(rand.Reader, template, parentTemplate, &priv.PublicKey, priv)
+	if err != nil {
+		return nil, err
+	}
+
+	return x509.ParseCertificate(certDER)
+}