From 3baf0e4c50d358a2e87b1e9b1d24c3f7d3894eca Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Fri, 24 Jun 2016 12:29:20 -0700 Subject: [PATCH 1/3] Output warnings for cert within 6 months expiry, check int cert expiry in root Signed-off-by: Riyaz Faizullabhoy --- trustpinning/certs.go | 63 ++++++++-- trustpinning/certs_test.go | 249 ++++++++++++++++++++++++++++++++++++- tuf/utils/x509.go | 6 +- 3 files changed, 302 insertions(+), 16 deletions(-) diff --git a/trustpinning/certs.go b/trustpinning/certs.go index 684277958..e7d08f847 100644 --- a/trustpinning/certs.go +++ b/trustpinning/certs.go @@ -98,6 +98,7 @@ func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trus // Retrieve all the leaf and intermediate certificates in root for which the CN matches the GUN allLeafCerts, allIntCerts := parseAllCerts(signedRoot) certsFromRoot, err := validRootLeafCerts(allLeafCerts, gun, true) + validIntCerts := validRootIntCerts(allIntCerts) if err != nil { logrus.Debugf("error retrieving valid leaf certificates for: %s, %v", gun, err) @@ -140,7 +141,7 @@ func ValidateRoot(prevRoot *data.SignedRoot, root *data.Signed, gun string, trus validPinnedCerts := map[string]*x509.Certificate{} for id, cert := range certsFromRoot { logrus.Debugf("checking trust-pinning for cert: %s", id) - if ok := trustPinCheckFunc(cert, allIntCerts[id]); !ok { + if ok := trustPinCheckFunc(cert, validIntCerts[id]); !ok { logrus.Debugf("trust-pinning check failed for cert: %s", id) continue } @@ -156,7 +157,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: utils.CertsToKeys(certsFromRoot, allIntCerts), Threshold: rootRole.Threshold}) + Keys: trustmanager.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"} @@ -181,17 +182,13 @@ func validRootLeafCerts(allLeafCerts map[string]*x509.Certificate, gun string, c continue } // Make sure the certificate is not expired if checkExpiry is true - if checkExpiry && time.Now().After(cert.NotAfter) { - logrus.Debugf("error leaf certificate is expired") - continue + // and warn if it hasn't expired yet but is within 6 months of expiry + if checkExpiry { + if err := checkCertExpiry(cert); err != nil { + continue + } } - - // 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)") + if err := checkCertSigAlgorithm(cert); err != nil { continue } @@ -208,6 +205,27 @@ func validRootLeafCerts(allLeafCerts map[string]*x509.Certificate, gun string, c return validLeafCerts, nil } +// validRootIntCerts filters the passed in structure of intermediate certificates to only include non-expired, non-sha1 certificates +// Note that this "validity" alone does not imply any measure of trust. +func validRootIntCerts(allIntCerts map[string][]*x509.Certificate) map[string][]*x509.Certificate { + validIntCerts := make(map[string][]*x509.Certificate) + + // 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 { + continue + } + validIntCerts[leafID] = append(validIntCerts[leafID], intCert) + } + + } + return validIntCerts +} + // parseAllCerts returns two maps, one with all of the leafCertificates and one // with all the intermediate certificates found in signedRoot func parseAllCerts(signedRoot *data.SignedRoot) (map[string]*x509.Certificate, map[string][]*x509.Certificate) { @@ -270,3 +288,24 @@ 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 a037723c4..46a9ca564 100644 --- a/trustpinning/certs_test.go +++ b/trustpinning/certs_test.go @@ -9,15 +9,17 @@ import ( "crypto/x509/pkix" "encoding/json" "encoding/pem" + "fmt" "io/ioutil" "math/big" "os" "path/filepath" "testing" "text/template" - "time" + "github.com/Sirupsen/logrus" + "github.com/docker/notary" "github.com/docker/notary/cryptoservice" "github.com/docker/notary/trustmanager" "github.com/docker/notary/trustpinning" @@ -782,9 +784,9 @@ func testValidateRootRotationMissingNewSig(t *testing.T, keyAlg, rootKeyType str require.Error(t, err, "insuficient signatures on root") } -func generateTestingCertificate(rootKey data.PrivateKey, gun string) (*x509.Certificate, error) { +func generateTestingCertificate(rootKey data.PrivateKey, gun string, timeToExpire time.Duration) (*x509.Certificate, error) { startTime := time.Now() - return cryptoservice.GenerateCertificate(rootKey, gun, startTime, startTime.AddDate(10, 0, 0)) + return cryptoservice.GenerateCertificate(rootKey, gun, startTime, startTime.Add(timeToExpire)) } func generateExpiredTestingCertificate(rootKey data.PrivateKey, gun string) (*x509.Certificate, error) { @@ -800,3 +802,244 @@ func generateRootKeyIDs(r *data.SignedRoot) { } } } + +func TestCheckingCertExpiry(t *testing.T) { + gun := "notary" + pass := func(keyName, alias string, createNew bool, attempts int) (passphrase string, giveup bool, err error) { + return "password", false, nil + } + memStore := trustmanager.NewKeyMemoryStore(pass) + cs := cryptoservice.NewCryptoService(memStore) + testPubKey, err := cs.Create(data.CanonicalRootRole, gun, data.ECDSAKey) + require.NoError(t, err) + testPrivKey, _, err := memStore.GetKey(testPubKey.ID()) + require.NoError(t, err) + + almostExpiredCert, err := generateTestingCertificate(testPrivKey, gun, notary.Day*30) + require.NoError(t, err) + almostExpiredPubKey, err := trustmanager.ParsePEMPublicKey(trustmanager.CertToPEM(almostExpiredCert)) + require.NoError(t, err) + + // set up a logrus logger to capture warning output + origLevel := logrus.GetLevel() + logrus.SetLevel(logrus.WarnLevel) + defer logrus.SetLevel(origLevel) + logBuf := bytes.NewBuffer(nil) + logrus.SetOutput(logBuf) + + rootRole, err := data.NewRole(data.CanonicalRootRole, 1, []string{almostExpiredPubKey.ID()}, nil) + require.NoError(t, err) + testRoot, err := data.NewRoot( + map[string]data.PublicKey{almostExpiredPubKey.ID(): almostExpiredPubKey}, + map[string]*data.RootRole{ + data.CanonicalRootRole: &rootRole.RootRole, + data.CanonicalTimestampRole: &rootRole.RootRole, + data.CanonicalTargetsRole: &rootRole.RootRole, + data.CanonicalSnapshotRole: &rootRole.RootRole}, + false, + ) + testRoot.Signed.Version = 1 + require.NoError(t, err, "Failed to create new root") + + signedTestRoot, err := testRoot.ToSigned() + require.NoError(t, err) + + err = signed.Sign(cs, signedTestRoot, []data.PublicKey{almostExpiredPubKey}, 1, nil) + require.NoError(t, err) + + // This is a valid root certificate, but check that we get a Warn-level message that the certificate is near expiry + _, err = trustpinning.ValidateRoot(nil, signedTestRoot, gun, trustpinning.TrustPinConfig{}) + require.NoError(t, err) + require.Contains(t, logBuf.String(), fmt.Sprintf("certificate with CN %s is near expiry", gun)) + + expiredCert, err := generateExpiredTestingCertificate(testPrivKey, gun) + require.NoError(t, err) + expiredPubKey := trustmanager.CertToKey(expiredCert) + + rootRole, err = data.NewRole(data.CanonicalRootRole, 1, []string{expiredPubKey.ID()}, nil) + require.NoError(t, err) + testRoot, err = data.NewRoot( + map[string]data.PublicKey{expiredPubKey.ID(): expiredPubKey}, + map[string]*data.RootRole{ + data.CanonicalRootRole: &rootRole.RootRole, + data.CanonicalTimestampRole: &rootRole.RootRole, + data.CanonicalTargetsRole: &rootRole.RootRole, + data.CanonicalSnapshotRole: &rootRole.RootRole}, + false, + ) + testRoot.Signed.Version = 1 + require.NoError(t, err, "Failed to create new root") + + signedTestRoot, err = testRoot.ToSigned() + require.NoError(t, err) + + err = signed.Sign(cs, signedTestRoot, []data.PublicKey{expiredPubKey}, 1, nil) + require.NoError(t, err) + + // This is an invalid root certificate since it's expired + _, err = trustpinning.ValidateRoot(nil, signedTestRoot, gun, trustpinning.TrustPinConfig{}) + require.Error(t, err) +} + +func TestValidateRootWithExpiredIntermediate(t *testing.T) { + now := time.Now() + serialNumberLimit := new(big.Int).Lsh(big.NewInt(1), 128) + + pass := func(keyName, alias string, createNew bool, attempts int) (passphrase string, giveup bool, err error) { + return "password", false, nil + } + memStore := trustmanager.NewKeyMemoryStore(pass) + cs := cryptoservice.NewCryptoService(memStore) + + // generate CA cert + serialNumber, err := rand.Int(rand.Reader, serialNumberLimit) + require.NoError(t, err) + caTmpl := x509.Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{ + CommonName: "notary testing CA", + }, + NotBefore: now.Add(-time.Hour), + NotAfter: now.Add(time.Hour), + KeyUsage: x509.KeyUsageCertSign, + BasicConstraintsValid: true, + IsCA: true, + MaxPathLen: 3, + } + caPrivKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + _, err = x509.CreateCertificate( + rand.Reader, + &caTmpl, + &caTmpl, + caPrivKey.Public(), + caPrivKey, + ) + + // generate expired intermediate + intTmpl := x509.Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{ + CommonName: "EXPIRED notary testing intermediate", + }, + NotBefore: now.Add(-2 * notary.Year), + NotAfter: now.Add(-notary.Year), + KeyUsage: x509.KeyUsageCertSign, + BasicConstraintsValid: true, + IsCA: true, + MaxPathLen: 2, + } + intPrivKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + intCert, err := x509.CreateCertificate( + rand.Reader, + &intTmpl, + &caTmpl, + intPrivKey.Public(), + caPrivKey, + ) + require.NoError(t, err) + + // generate leaf + serialNumber, err = rand.Int(rand.Reader, serialNumberLimit) + require.NoError(t, err) + leafTmpl := x509.Certificate{ + SerialNumber: serialNumber, + Subject: pkix.Name{ + CommonName: "docker.io/notary/test", + }, + NotBefore: now.Add(-time.Hour), + NotAfter: now.Add(time.Hour), + + KeyUsage: x509.KeyUsageKeyEncipherment | x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageCodeSigning}, + BasicConstraintsValid: true, + } + + leafPubKey, err := cs.Create("root", "docker.io/notary/test", data.ECDSAKey) + require.NoError(t, err) + leafPrivKey, _, err := cs.GetPrivateKey(leafPubKey.ID()) + require.NoError(t, err) + signer := leafPrivKey.CryptoSigner() + leafCert, err := x509.CreateCertificate( + rand.Reader, + &leafTmpl, + &intTmpl, + signer.Public(), + intPrivKey, + ) + + rootBundleWriter := bytes.NewBuffer(nil) + pem.Encode( + rootBundleWriter, + &pem.Block{ + Type: "CERTIFICATE", + Bytes: leafCert, + }, + ) + pem.Encode( + rootBundleWriter, + &pem.Block{ + Type: "CERTIFICATE", + Bytes: intCert, + }, + ) + + rootBundle := rootBundleWriter.Bytes() + + ecdsax509Key := data.NewECDSAx509PublicKey(rootBundle) + + otherKey, err := cs.Create("targets", "docker.io/notary/test", data.ED25519Key) + require.NoError(t, err) + + root := data.SignedRoot{ + Signatures: make([]data.Signature, 0), + Signed: data.Root{ + SignedCommon: data.SignedCommon{ + Type: "Root", + Expires: now.Add(time.Hour), + Version: 1, + }, + Keys: map[string]data.PublicKey{ + ecdsax509Key.ID(): ecdsax509Key, + otherKey.ID(): otherKey, + }, + Roles: map[string]*data.RootRole{ + "root": { + KeyIDs: []string{ecdsax509Key.ID()}, + Threshold: 1, + }, + "targets": { + KeyIDs: []string{otherKey.ID()}, + Threshold: 1, + }, + "snapshot": { + KeyIDs: []string{otherKey.ID()}, + Threshold: 1, + }, + "timestamp": { + KeyIDs: []string{otherKey.ID()}, + Threshold: 1, + }, + }, + }, + Dirty: true, + } + + signedRoot, err := root.ToSigned() + require.NoError(t, err) + err = signed.Sign(cs, signedRoot, []data.PublicKey{ecdsax509Key}, 1, nil) + require.NoError(t, err) + + tempBaseDir, err := ioutil.TempDir("", "notary-test-") + defer os.RemoveAll(tempBaseDir) + require.NoError(t, err, "failed to create a temporary directory: %s", err) + + _, err = trustpinning.ValidateRoot( + nil, + signedRoot, + "docker.io/notary/test", + trustpinning.TrustPinConfig{}, + ) + require.Error(t, err, "failed to invalidate expired intermediate certificate") +} diff --git a/tuf/utils/x509.go b/tuf/utils/x509.go index b5faeb15a..0aa078944 100644 --- a/tuf/utils/x509.go +++ b/tuf/utils/x509.go @@ -267,7 +267,11 @@ func ValidateCertificate(c *x509.Certificate) error { 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 is expired") + 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.Warn("certificate with CN %s is near expiry", c.Subject.CommonName) } // If we have an RSA key, make sure it's long enough if c.PublicKeyAlgorithm == x509.RSA { From 29522292a9b3d245b6244e7469e904c982d45963 Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Fri, 8 Jul 2016 15:13:48 -0700 Subject: [PATCH 2/3] 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 000000000..df95eacf4 --- /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 e7d08f847..98f1e0428 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" @@ -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 -} diff --git a/trustpinning/certs_test.go b/trustpinning/certs_test.go index 46a9ca564..95269cfdf 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 000000000..88d806b7e --- /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 ae3e86d90..0b8a73176 100644 --- a/trustpinning/trustpin.go +++ b/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 } diff --git a/tuf/tuf.go b/tuf/tuf.go index 8f383852d..c76a485a4 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 0aa078944..98162328c 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 4b98f05c3..34b85f415 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)) +} From fb0334858aa077c4f6f98be0b01201838e74574b Mon Sep 17 00:00:00 2001 From: Riyaz Faizullabhoy Date: Mon, 8 Aug 2016 16:01:45 -0700 Subject: [PATCH 3/3] change ordering of expiry checks and add explicit error type for expired certs Signed-off-by: Riyaz Faizullabhoy --- tuf/data/errors.go | 9 +++++++++ tuf/tuf.go | 4 ++++ tuf/utils/x509.go | 24 ++++++++++++------------ 3 files changed, 25 insertions(+), 12 deletions(-) diff --git a/tuf/data/errors.go b/tuf/data/errors.go index 0cd0afba0..5c1397d3e 100644 --- a/tuf/data/errors.go +++ b/tuf/data/errors.go @@ -42,3 +42,12 @@ func (e ErrMismatchedChecksum) Error() string { return fmt.Sprintf("%s checksum for %s did not match: expected %s", e.alg, e.name, e.expected) } + +// ErrCertExpired is the error to be returned when a certificate has expired +type ErrCertExpired struct { + CN string +} + +func (e ErrCertExpired) Error() string { + return fmt.Sprintf("certificate with CN %s is expired", e.CN) +} diff --git a/tuf/tuf.go b/tuf/tuf.go index c76a485a4..267b009fc 100644 --- a/tuf/tuf.go +++ b/tuf/tuf.go @@ -253,6 +253,10 @@ func (tr *Repo) GetDelegationRole(name string) (data.DelegationRole, error) { continue } if err := utils.ValidateCertificate(certFromKey, true); err != nil { + if _, ok := err.(data.ErrCertExpired); !ok { + // do not allow other invalid cert errors + return err + } logrus.Warnf("error with delegation %s key ID %d: %s", delgRole.Name, keyID, err) } } diff --git a/tuf/utils/x509.go b/tuf/utils/x509.go index 98162328c..4abb16def 100644 --- a/tuf/utils/x509.go +++ b/tuf/utils/x509.go @@ -264,18 +264,6 @@ func ValidateCertificate(c *x509.Certificate, checkExpiry bool) error { if (c.NotBefore).After(c.NotAfter) { return fmt.Errorf("certificate validity window is invalid") } - 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) - } - } // 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) @@ -290,6 +278,18 @@ func ValidateCertificate(c *x509.Certificate, checkExpiry bool) error { return fmt.Errorf("RSA bit length is too short") } } + 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 data.ErrCertExpired{CN: 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) + } + } return nil }