Unify certificate validation

Signed-off-by: Riyaz Faizullabhoy <riyaz.faizullabhoy@docker.com>

Author: riyazdf

trustpinning/ca.crt

@@ -0,0 +1,37 @@
+-----BEGIN CERTIFICATE-----
+MIIGMzCCBBugAwIBAgIBATANBgkqhkiG9w0BAQsFADBfMQswCQYDVQQGEwJVUzEL
+MAkGA1UECAwCQ0ExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDzANBgNVBAoMBkRv
+Y2tlcjEaMBgGA1UEAwwRTm90YXJ5IFRlc3RpbmcgQ0EwHhcNMTUwNzE2MDQyNTAz
+WhcNMjUwNzEzMDQyNTAzWjBfMRowGAYDVQQDDBFOb3RhcnkgVGVzdGluZyBDQTEL
+MAkGA1UEBhMCVVMxFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDzANBgNVBAoMBkRv
+Y2tlcjELMAkGA1UECAwCQ0EwggIiMA0GCSqGSIb3DQEBAQUAA4ICDwAwggIKAoIC
+AQCwVVD4pK7z7pXPpJbaZ1Hg5eRXIcaYtbFPCnN0iqy9HsVEGnEn5BPNSEsuP+m0
+5N0qVV7DGb1SjiloLXD1qDDvhXWk+giS9ppqPHPLVPB4bvzsqwDYrtpbqkYvO0YK
+0SL3kxPXUFdlkFfgu0xjlczm2PhWG3Jd8aAtspL/L+VfPA13JUaWxSLpui1In8rh
+gAyQTK6Q4Of6GbJYTnAHb59UoLXSzB5AfqiUq6L7nEYYKoPflPbRAIWL/UBm0c+H
+ocms706PYpmPS2RQv3iOGmnn9hEVp3P6jq7WAevbA4aYGx5EsbVtYABqJBbFWAuw
+wTGRYmzn0Mj0eTMge9ztYB2/2sxdTe6uhmFgpUXngDqJI5O9N3zPfvlEImCky3HM
+jJoL7g5smqX9o1P+ESLh0VZzhh7IDPzQTXpcPIS/6z0l22QGkK/1N1PaADaUHdLL
+vSav3y2BaEmPvf2fkZj8yP5eYgi7Cw5ONhHLDYHFcl9Zm/ywmdxHJETz9nfgXnsW
+HNxDqrkCVO46r/u6rSrUt6hr3oddJG8s8Jo06earw6XU3MzM+3giwkK0SSM3uRPq
+4AscR1Tv+E31AuOAmjqYQoT29bMIxoSzeljj/YnedwjW45pWyc3JoHaibDwvW9Uo
+GSZBVy4hrM/Fa7XCWv1WfHNW1gDwaLYwDnl5jFmRBvcfuQIDAQABo4H5MIH2MIGR
+BgNVHSMEgYkwgYaAFHUM1U3E4WyL1nvFd+dPY8f4O2hZoWOkYTBfMQswCQYDVQQG
+EwJVUzELMAkGA1UECAwCQ0ExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xDzANBgNV
+BAoMBkRvY2tlcjEaMBgGA1UEAwwRTm90YXJ5IFRlc3RpbmcgQ0GCCQDCeDLbemIT
+SzASBgNVHRMBAf8ECDAGAQH/AgEAMB0GA1UdJQQWMBQGCCsGAQUFBwMCBggrBgEF
+BQcDATAOBgNVHQ8BAf8EBAMCAUYwHQYDVR0OBBYEFHe48hcBcAp0bUVlTxXeRA4o
+E16pMA0GCSqGSIb3DQEBCwUAA4ICAQAWUtAPdUFpwRq+N1SzGUejSikeMGyPZscZ
+JBUCmhZoFufgXGbLO5OpcRLaV3Xda0t/5PtdGMSEzczeoZHWknDtw+79OBittPPj
+Sh1oFDuPo35R7eP624lUCch/InZCphTaLx9oDLGcaK3ailQ9wjBdKdlBl8KNKIZp
+a13aP5rnSm2Jva+tXy/yi3BSds3dGD8ITKZyI/6AFHxGvObrDIBpo4FF/zcWXVDj
+paOmxplRtM4Hitm+sXGvfqJe4x5DuOXOnPrT3dHvRT6vSZUoKobxMqmRTOcrOIPa
+EeMpOobshORuRntMDYvvgO3D6p6iciDW2Vp9N6rdMdfOWEQN8JVWvB7IxRHk9qKJ
+vYOWVbczAt0qpMvXF3PXLjZbUM0knOdUKIEbqP4YUbgdzx6RtgiiY930Aj6tAtce
+0fpgNlvjMRpSBuWTlAfNNjG/YhndMz9uI68TMfFpR3PcgVIv30krw/9VzoLi2Dpe
+ow6DrGO6oi+DhN78P4jY/O9UczZK2roZL1Oi5P0RIxf23UZC7x1DlcN3nBr4sYSv
+rBx4cFTMNpwU+nzsIi4djcFDKmJdEOyjMnkP2v0Lwe7yvK08pZdEu+0zbrq17kue
+XpXLc7K68QB15yxzGylU5rRwzmC/YsAVyE4eoGu8PxWxrERvHby4B8YP0vAfOraL
+lKmXlK4dTg==
+-----END CERTIFICATE-----
+

trustpinning/certs.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"strings"
-	"time"
 
 	"github.com/Sirupsen/logrus"
 	"github.com/docker/notary/tuf/data"
@@ -157,7 +156,7 @@ func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trus
 	// Note that certsFromRoot is guaranteed to be unchanged only if we had prior cert data for this GUN or enabled TOFUS
 	// If we attempted to pin a certain certificate or CA, certsFromRoot could have been pruned accordingly
 	err = signed.VerifySignatures(root, data.BaseRole{
-		Keys: trustmanager.CertsToKeys(certsFromRoot, validIntCerts), Threshold: rootRole.Threshold})
+		Keys: utils.CertsToKeys(certsFromRoot, validIntCerts), Threshold: rootRole.Threshold})
 	if err != nil {
 		logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err)
 		return nil, &ErrValidationFail{Reason: "failed to validate integrity of roots"}
@@ -183,12 +182,7 @@ func validRootLeafCerts(allLeafCerts map[string]*x509.Certificate, gun string, c
 		}
 		// Make sure the certificate is not expired if checkExpiry is true
 		// and warn if it hasn't expired yet but is within 6 months of expiry
-		if checkExpiry {
-			if err := checkCertExpiry(cert); err != nil {
-				continue
-			}
-		}
-		if err := checkCertSigAlgorithm(cert); err != nil {
+		if err := utils.ValidateCertificate(cert, checkExpiry); err != nil {
 			continue
 		}
 
@@ -213,10 +207,7 @@ func validRootIntCerts(allIntCerts map[string][]*x509.Certificate) map[string][]
 	// Go through every leaf cert ID, and build its valid intermediate certificate list
 	for leafID, intCertList := range allIntCerts {
 		for _, intCert := range intCertList {
-			if err := checkCertExpiry(intCert); err != nil {
-				continue
-			}
-			if err := checkCertSigAlgorithm(intCert); err != nil {
+			if err := utils.ValidateCertificate(intCert, true); err != nil {
 				continue
 			}
 			validIntCerts[leafID] = append(validIntCerts[leafID], intCert)
@@ -288,24 +279,3 @@ func parseAllCerts(signedRoot *data.SignedRoot) (map[string]*x509.Certificate, m
 
 	return leafCerts, intCerts
 }
-
-func checkCertExpiry(cert *x509.Certificate) error {
-	if time.Now().After(cert.NotAfter) {
-		logrus.Debugf("certificate with CN %s is expired", cert.Subject.CommonName)
-		return fmt.Errorf("certificate expired: %s", cert.Subject.CommonName)
-	} else if cert.NotAfter.Before(time.Now().AddDate(0, 6, 0)) {
-		logrus.Warnf("certificate with CN %s is near expiry", cert.Subject.CommonName)
-	}
-	return nil
-}
-
-func checkCertSigAlgorithm(cert *x509.Certificate) error {
-	// We don't allow root certificates that use SHA1
-	if cert.SignatureAlgorithm == x509.SHA1WithRSA ||
-		cert.SignatureAlgorithm == x509.DSAWithSHA1 ||
-		cert.SignatureAlgorithm == x509.ECDSAWithSHA1 {
-		logrus.Debugf("error certificate uses deprecated hashing algorithm (SHA1)")
-		return fmt.Errorf("invalid signature algorithm for certificate with CN %s", cert.Subject.CommonName)
-	}
-	return nil
-}

trustpinning/certs_test.go

@@ -817,7 +817,7 @@ func TestCheckingCertExpiry(t *testing.T) {
 
 	almostExpiredCert, err := generateTestingCertificate(testPrivKey, gun, notary.Day*30)
 	require.NoError(t, err)
-	almostExpiredPubKey, err := trustmanager.ParsePEMPublicKey(trustmanager.CertToPEM(almostExpiredCert))
+	almostExpiredPubKey, err := utils.ParsePEMPublicKey(utils.CertToPEM(almostExpiredCert))
 	require.NoError(t, err)
 
 	// set up a logrus logger to capture warning output
@@ -854,7 +854,7 @@ func TestCheckingCertExpiry(t *testing.T) {
 
 	expiredCert, err := generateExpiredTestingCertificate(testPrivKey, gun)
 	require.NoError(t, err)
-	expiredPubKey := trustmanager.CertToKey(expiredCert)
+	expiredPubKey := utils.CertToKey(expiredCert)
 
 	rootRole, err = data.NewRole(data.CanonicalRootRole, 1, []string{expiredPubKey.ID()}, nil)
 	require.NoError(t, err)

trustpinning/test.crt

@@ -0,0 +1,31 @@
+-----BEGIN CERTIFICATE-----
+MIIFKzCCAxWgAwIBAgIQRyp9QqcJfd3ayqdjiz8xIDALBgkqhkiG9w0BAQswODEa
+MBgGA1UEChMRZG9ja2VyLmNvbS9ub3RhcnkxGjAYBgNVBAMTEWRvY2tlci5jb20v
+bm90YXJ5MB4XDTE1MDcxNzA2MzQyM1oXDTE3MDcxNjA2MzQyM1owODEaMBgGA1UE
+ChMRZG9ja2VyLmNvbS9ub3RhcnkxGjAYBgNVBAMTEWRvY2tlci5jb20vbm90YXJ5
+MIICIjANBgkqhkiG9w0BAQEFAAOCAg8AMIICCgKCAgEAoQffrzsYnsH8vGf4Jh55
+Cj5wrjUGzD/sHkaFHptjJ6ToJGJv5yMAPxzyInu5sIoGLJapnYVBoAU0YgI9qlAc
+YA6SxaSwgm6rpvmnl8Qn0qc6ger3inpGaUJylWHuPwWkvcimQAqHZx2dQtL7g6kp
+rmKeTWpWoWLw3JoAUZUVhZMd6a22ZL/DvAw+Hrogbz4XeyahFb9IH402zPxN6vga
+JEFTF0Ji1jtNg0Mo4pb9SHsMsiw+LZK7SffHVKPxvd21m/biNmwsgExA3U8OOG8p
+uygfacys5c8+ZrX+ZFG/cvwKz0k6/QfJU40s6MhXw5C2WttdVmsG9/7rGFYjHoIJ
+weDyxgWk7vxKzRJI/un7cagDIaQsKrJQcCHIGFRlpIR5TwX7vl3R7cRncrDRMVvc
+VSEG2esxbw7jtzIp/ypnVRxcOny7IypyjKqVeqZ6HgxZtTBVrF1O/aHo2kvlwyRS
+Aus4kvh6z3+jzTm9EzfXiPQzY9BEk5gOLxhW9rc6UhlS+pe5lkaN/Hyqy/lPuq89
+fMr2rr7lf5WFdFnze6WNYMAaW7dNA4NE0dyD53428ZLXxNVPL4WU66Gac6lynQ8l
+r5tPsYIFXzh6FVaRKGQUtW1hz9ecO6Y27Rh2JsyiIxgUqk2ooxE69uN42t+dtqKC
+1s8G/7VtY8GDALFLYTnzLvsCAwEAAaM1MDMwDgYDVR0PAQH/BAQDAgCgMBMGA1Ud
+JQQMMAoGCCsGAQUFBwMDMAwGA1UdEwEB/wQCMAAwCwYJKoZIhvcNAQELA4ICAQBM
+Oll3G/XBz8idiNdNJDWUh+5w3ojmwanrTBdCdqEk1WenaR6DtcflJx6Z3f/mwV4o
+b1skOAX1yX5RCahJHUMxMicz/Q38pOVelGPrWnc3TJB+VKjGyHXlQDVkZFb+4+ef
+wtj7HngXhHFFDSgjm3EdMndvgDQ7SQb4skOnCNS9iyX7eXxhFBCZmZL+HALKBj2B
+yhV4IcBDqmp504t14rx9/Jvty0dG7fY7I51gEQpm4S02JML5xvTm1xfboWIhZODI
+swEAO+ekBoFHbS1Q9KMPjIAw3TrCHH8x8XZq5zsYtAC1yZHdCKa26aWdy56A9eHj
+O1VxzwmbNyXRenVuBYP+0wr3HVKFG4JJ4ZZpNZzQW/pqEPghCTJIvIueK652ByUc
+//sv+nXd5f19LeES9pf0l253NDaFZPb6aegKfquWh8qlQBmUQ2GzaTLbtmNd28M6
+W7iL7tkKZe1ZnBz9RKgtPrDjjWGZInjjcOU8EtT4SLq7kCVDmPs5MD8vaAm96JsE
+jmLC3Uu/4k7HiDYX0i0mOWkFjZQMdVatcIF5FPSppwsSbW8QidnXt54UtwtFDEPz
+lpjs7ybeQE71JXcMZnVIK4bjRXsEFPI98RpIlEdedbSUdYAncLNJRT7HZBMPGSwZ
+0PNJuglnlr3srVzdW1dz2xQjdvLwxy6mNUF6rbQBWA==
+-----END CERTIFICATE-----
+

trustpinning/trustpin.go

@@ -48,7 +48,7 @@ func NewTrustPinChecker(trustPinConfig TrustPinConfig, gun string) (CertChecker,
 		// Now only consider certificates that are direct children from this CA cert chain
 		caRootPool := x509.NewCertPool()
 		for _, caCert := range caCerts {
-			if err = utils.ValidateCertificate(caCert); err != nil {
+			if err = utils.ValidateCertificate(caCert, true); err != nil {
 				logrus.Debugf("ignoring root CA certificate with CN %s in bundle: %s", caCert.Subject.CommonName, err)
 				continue
 			}

tuf/tuf.go

@@ -245,6 +245,17 @@ func (tr *Repo) GetDelegationRole(name string) (data.DelegationRole, error) {
 				if err != nil {
 					return err
 				}
+				// Check all public key certificates in the role for expiry
+				// Currently we do not reject expired delegation keys but warn if they might expire soon or have already
+				for keyID, pubKey := range delgRole.Keys {
+					certFromKey, err := utils.LoadCertFromPEM(pubKey.Public())
+					if err != nil {
+						continue
+					}
+					if err := utils.ValidateCertificate(certFromKey, true); err != nil {
+						logrus.Warnf("error with delegation %s key ID %d: %s", delgRole.Name, keyID, err)
+					}
+				}
 				foundRole = &delgRole
 				return StopWalk{}
 			}

tuf/utils/x509.go

@@ -247,7 +247,7 @@ func ParsePEMPublicKey(pubKeyBytes []byte) (data.PublicKey, error) {
 		if err != nil {
 			return nil, fmt.Errorf("could not parse provided certificate: %v", err)
 		}
-		err = ValidateCertificate(cert)
+		err = ValidateCertificate(cert, true)
 		if err != nil {
 			return nil, fmt.Errorf("invalid certificate: %v", err)
 		}
@@ -258,20 +258,27 @@ func ParsePEMPublicKey(pubKeyBytes []byte) (data.PublicKey, error) {
 }
 
 // ValidateCertificate returns an error if the certificate is not valid for notary
-// Currently this is only a time expiry check, and ensuring the public key has a large enough modulus if RSA
-func ValidateCertificate(c *x509.Certificate) error {
+// Currently this is only ensuring the public key has a large enough modulus if RSA,
+// using a non SHA1 signature algorithm, and an optional time expiry check
+func ValidateCertificate(c *x509.Certificate, checkExpiry bool) error {
 	if (c.NotBefore).After(c.NotAfter) {
 		return fmt.Errorf("certificate validity window is invalid")
 	}
-	now := time.Now()
-	tomorrow := now.AddDate(0, 0, 1)
-	// Give one day leeway on creation "before" time, check "after" against today
-	if (tomorrow).Before(c.NotBefore) || now.After(c.NotAfter) {
-		return fmt.Errorf("certificate with CN %s is expired", c.Subject.CommonName)
+	if checkExpiry {
+		now := time.Now()
+		tomorrow := now.AddDate(0, 0, 1)
+		// Give one day leeway on creation "before" time, check "after" against today
+		if (tomorrow).Before(c.NotBefore) || now.After(c.NotAfter) {
+			return fmt.Errorf("certificate with CN %s is expired", c.Subject.CommonName)
+		}
+		// If this certificate is expiring within 6 months, put out a warning
+		if (c.NotAfter).Before(time.Now().AddDate(0, 6, 0)) {
+			logrus.Warnf("certificate with CN %s is near expiry", c.Subject.CommonName)
+		}
 	}
-	// If this certificate is expiring within 6 months, put out a warning
-	if (c.NotAfter).Before(time.Now().AddDate(0, 6, 0)) {
-		logrus.Warn("certificate with CN %s is near expiry", c.Subject.CommonName)
+	// Can't have SHA1 sig algorithm
+	if c.SignatureAlgorithm == x509.SHA1WithRSA || c.SignatureAlgorithm == x509.DSAWithSHA1 || c.SignatureAlgorithm == x509.ECDSAWithSHA1 {
+		return fmt.Errorf("certificate with CN %s uses invalid SHA1 signature algorithm", c.Subject.CommonName)
 	}
 	// If we have an RSA key, make sure it's long enough
 	if c.PublicKeyAlgorithm == x509.RSA {

tuf/utils/x509_test.go

@@ -4,6 +4,7 @@ import (
 	"crypto/ecdsa"
 	"crypto/elliptic"
 	"crypto/rand"
+	"crypto/rsa"
 	"crypto/x509"
 	"io/ioutil"
 	"strings"
@@ -220,3 +221,97 @@ func TestECDSAX509PublickeyID(t *testing.T) {
 
 	require.Equal(t, tufPrivKey.ID(), tufID)
 }
+
+func TestValidateCertificateWithSHA1(t *testing.T) {
+	// Test against SHA1 signature algorithm cert first
+	startTime := time.Now()
+	template, err := NewCertificate("something", startTime, startTime.AddDate(10, 0, 0))
+	require.NoError(t, err)
+	// SHA1 signature algorithm is invalid
+	template.SignatureAlgorithm = x509.ECDSAWithSHA1
+	template.PublicKeyAlgorithm = x509.ECDSA
+
+	privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	derBytes, err := x509.CreateCertificate(
+		rand.Reader, template, template, &privKey.PublicKey, privKey)
+	require.NoError(t, err)
+
+	sha1Cert, err := x509.ParseCertificate(derBytes)
+	require.NoError(t, err)
+
+	// Regardless of expiry check, this certificate should fail to validate
+	require.Error(t, ValidateCertificate(sha1Cert, false))
+	require.Error(t, ValidateCertificate(sha1Cert, true))
+}
+
+func TestValidateCertificateWithExpiredCert(t *testing.T) {
+	// Test against an expired cert for 10 years ago, only valid for a day
+	startTime := time.Now().AddDate(-10, 0, 0)
+	template, err := NewCertificate("something", startTime, startTime.AddDate(0, 0, 1))
+	require.NoError(t, err)
+	template.SignatureAlgorithm = x509.ECDSAWithSHA256
+	template.PublicKeyAlgorithm = x509.ECDSA
+
+	privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	derBytes, err := x509.CreateCertificate(
+		rand.Reader, template, template, &privKey.PublicKey, privKey)
+	require.NoError(t, err)
+
+	expiredCert, err := x509.ParseCertificate(derBytes)
+	require.NoError(t, err)
+
+	// If we don't check expiry, this cert is perfectly valid
+	require.NoError(t, ValidateCertificate(expiredCert, false))
+	// We should get an error when we check expiry
+	require.Error(t, ValidateCertificate(expiredCert, true))
+}
+
+func TestValidateCertificateWithInvalidExpiry(t *testing.T) {
+	// Test against a cert with an invalid expiry window: from 10 years in the future to 10 years ago
+	startTime := time.Now().AddDate(10, 0, 0)
+	template, err := NewCertificate("something", startTime, startTime.AddDate(-10, 0, 0))
+	require.NoError(t, err)
+	template.SignatureAlgorithm = x509.ECDSAWithSHA256
+	template.PublicKeyAlgorithm = x509.ECDSA
+
+	privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	require.NoError(t, err)
+
+	derBytes, err := x509.CreateCertificate(
+		rand.Reader, template, template, &privKey.PublicKey, privKey)
+	require.NoError(t, err)
+
+	invalidCert, err := x509.ParseCertificate(derBytes)
+	require.NoError(t, err)
+
+	// Regardless of expiry check, this certificate should fail to validate
+	require.Error(t, ValidateCertificate(invalidCert, false))
+	require.Error(t, ValidateCertificate(invalidCert, true))
+}
+
+func TestValidateCertificateWithShortKey(t *testing.T) {
+	startTime := time.Now()
+	template, err := NewCertificate("something", startTime, startTime.AddDate(10, 0, 0))
+	require.NoError(t, err)
+	template.SignatureAlgorithm = x509.SHA256WithRSA
+	template.PublicKeyAlgorithm = x509.RSA
+
+	// Use only 1024 bit modulus, this will fail
+	weakPrivKey, err := rsa.GenerateKey(rand.Reader, 1024)
+	require.NoError(t, err)
+
+	derBytes, err := x509.CreateCertificate(
+		rand.Reader, template, template, &weakPrivKey.PublicKey, weakPrivKey)
+	require.NoError(t, err)
+
+	weakKeyCert, err := x509.ParseCertificate(derBytes)
+	require.NoError(t, err)
+
+	// Regardless of expiry check, this certificate should fail to validate
+	require.Error(t, ValidateCertificate(weakKeyCert, false))
+	require.Error(t, ValidateCertificate(weakKeyCert, true))
+}