From e5957a4d8b7be785924d718b421e82a51f9f2db6 Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Fri, 8 Jul 2016 15:13:48 -0700 Subject: [PATCH] Unify certificate validation Signed-off-by: Riyaz Faizullabhoy --- trustpinning/ca.crt | 37 +++++++++++++++ trustpinning/certs.go | 36 ++------------- trustpinning/certs_test.go | 4 +- trustpinning/test.crt | 31 +++++++++++++ trustpinning/trustpin.go | 2 +- tuf/tuf.go | 11 +++++ tuf/utils/x509.go | 29 +++++++----- tuf/utils/x509_test.go | 95 ++++++++++++++++++++++++++++++++++++++ 8 files changed, 198 insertions(+), 47 deletions(-) create mode 100644 trustpinning/ca.crt create mode 100644 trustpinning/test.crt diff --git a/trustpinning/ca.crt b/trustpinning/ca.crt new file mode 100644 index 0000000000..df95eacf4a --- /dev/null +++ b/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----- + diff --git a/trustpinning/certs.go b/trustpinning/certs.go index a9e6861ea4..7221f96a6e 100644 --- a/trustpinning/certs.go +++ b/trustpinning/certs.go @@ -5,7 +5,6 @@ import ( "errors" "fmt" "strings" - "time" "github.com/Sirupsen/logrus" "github.com/docker/notary/tuf/data" @@ -153,7 +152,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"} @@ -179,12 +178,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 } @@ -209,10 +203,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) @@ -284,24 +275,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 -} diff --git a/trustpinning/certs_test.go b/trustpinning/certs_test.go index 46a9ca564d..95269cfdf2 100644 --- a/trustpinning/certs_test.go +++ b/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) diff --git a/trustpinning/test.crt b/trustpinning/test.crt new file mode 100644 index 0000000000..88d806b7e5 --- /dev/null +++ b/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----- + diff --git a/trustpinning/trustpin.go b/trustpinning/trustpin.go index 5ddfe14e5b..8845ea66f3 100644 --- a/trustpinning/trustpin.go +++ b/trustpinning/trustpin.go @@ -45,7 +45,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 { continue } caRootPool.AddCert(caCert) diff --git a/tuf/tuf.go b/tuf/tuf.go index eb838957ad..a6dacd3551 100644 --- a/tuf/tuf.go +++ b/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{} } diff --git a/tuf/utils/x509.go b/tuf/utils/x509.go index 0aa0789444..98162328ce 100644 --- a/tuf/utils/x509.go +++ b/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 { diff --git a/tuf/utils/x509_test.go b/tuf/utils/x509_test.go index 4b98f05c3e..34b85f4152 100644 --- a/tuf/utils/x509_test.go +++ b/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)) +}