From b8b59dbc20d2683f6599bcfd89863bb9b1c28381 Mon Sep 17 00:00:00 2001 From: Diogo Monica Date: Mon, 20 Jul 2015 19:48:17 -0700 Subject: [PATCH] Fixed but with listDirectory and added tests Signed-off-by: Diogo Monica --- keystoremanager/keystoremanager.go | 10 ++++++---- trustmanager/x509filestore.go | 7 +++++-- trustmanager/x509filestore_test.go | 29 +++++++++++++++++++---------- trustmanager/x509store.go | 11 +++++++++++ trustmanager/x509utils.go | 22 +++++++++++++++++++--- 5 files changed, 60 insertions(+), 19 deletions(-) diff --git a/keystoremanager/keystoremanager.go b/keystoremanager/keystoremanager.go index ecc4c1bcec..af5723f487 100644 --- a/keystoremanager/keystoremanager.go +++ b/keystoremanager/keystoremanager.go @@ -239,12 +239,14 @@ func (km *KeyStoreManager) ValidateRoot(root *data.Signed, gun string) error { // If we have certificates that match this specific GUN, let's make sure to // use them first to validate that this new root is valid. if len(certsForCN) != 0 { - logrus.Debugf("found %d valid certificates for %s", len(certsForCN), gun) + logrus.Debugf("found %d valid root certificates for %s", len(certsForCN), gun) err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(certsForCN)) if err != nil { logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err) return &ErrValidationFail{Reason: "failed to validate integrity of roots"} } + } else { + logrus.Debugf("found no currently valid root certificates for %s", gun) } // Validate the integrity of the new root (does it have valid signatures) @@ -266,13 +268,11 @@ func (km *KeyStoreManager) ValidateRoot(root *data.Signed, gun string) error { if err != nil { // If the error is already exists we don't fail the rotation if _, ok := err.(*trustmanager.ErrCertExists); ok { + logrus.Debugf("ignoring certificate addition to: %s", gun) continue } logrus.Debugf("error adding new trusted certificate for: %s, %v", gun, err) } - // Getting the CertID for debug only, don't care about the error - certID, _ := trustmanager.FingerprintCert(cert) - logrus.Debugf("adding trust certificate with certID %s for %s", certID, gun) } // Now we delete old certificates that aren't present in the new root @@ -322,9 +322,11 @@ func validRootLeafCerts(root *data.SignedRoot, gun string) ([]*x509.Certificate, } if len(validLeafCerts) < 1 { + logrus.Debugf("didn't find any valid leaf certificates for %s", gun) return nil, errors.New("no valid leaf certificates found in any of the root keys") } + logrus.Debugf("found %d valid leaf certificates for %s", len(validLeafCerts), gun) return validLeafCerts, nil } diff --git a/trustmanager/x509filestore.go b/trustmanager/x509filestore.go index 9e561d4f19..1df6233191 100644 --- a/trustmanager/x509filestore.go +++ b/trustmanager/x509filestore.go @@ -44,7 +44,10 @@ func newX509FileStore(directory string, validate func(*x509.Certificate) bool) ( fileStore: fileStore, } - loadCertsFromDir(s) + err = loadCertsFromDir(s) + if err != nil { + return nil, err + } return s, nil } @@ -227,7 +230,7 @@ func (s *X509FileStore) GetCertificatesByCN(cn string) ([]*x509.Certificate, err if err != nil { // This error should never happen. This would mean that we have // an inconsistent X509FileStore - return nil, err + return nil, &ErrBadCertificateStore{} } certs = append(certs, cert) } diff --git a/trustmanager/x509filestore_test.go b/trustmanager/x509filestore_test.go index a90a0a194a..21b1a0b36b 100644 --- a/trustmanager/x509filestore_test.go +++ b/trustmanager/x509filestore_test.go @@ -6,6 +6,8 @@ import ( "io/ioutil" "os" "testing" + + "github.com/stretchr/testify/assert" ) func TestNewX509FileStore(t *testing.T) { @@ -59,17 +61,24 @@ func TestAddCertX509FileStore(t *testing.T) { func TestAddCertFromFileX509FileStore(t *testing.T) { tempDir, err := ioutil.TempDir("", "cert-test") - if err != nil { - t.Fatal(err) - } - store, _ := NewX509FileStore(tempDir) + assert.NoError(t, err, "failed to create temporary directory") + + store, err := NewX509FileStore(tempDir) + assert.NoError(t, err, "failed to load x509 filestore") + err = store.AddCertFromFile("../fixtures/root-ca.crt") - if err != nil { - t.Fatalf("failed to load certificate from file: %v", err) - } - numCerts := len(store.GetCertificates()) - if numCerts != 1 { - t.Fatalf("unexpected number of certificates in store: %d", numCerts) + assert.NoError(t, err, "failed to add certificate from file") + assert.Len(t, store.GetCertificates(), 1) + + // Now load the x509 filestore with the same path and expect the same result + newStore, err := NewX509FileStore(tempDir) + assert.NoError(t, err, "failed to load x509 filestore") + assert.Len(t, newStore.GetCertificates(), 1) + + // Test that adding the same certificate returns an error + err = newStore.AddCert(newStore.GetCertificates()[0]) + if assert.Error(t, err, "expected error when adding certificate twice") { + assert.Equal(t, err, &ErrCertExists{}) } } diff --git a/trustmanager/x509store.go b/trustmanager/x509store.go index 05aaaba248..3736ff632c 100644 --- a/trustmanager/x509store.go +++ b/trustmanager/x509store.go @@ -40,6 +40,17 @@ func (err ErrCertExists) Error() string { return fmt.Sprintf("certificate already in the store") } +// ErrBadCertificateStore is returned when there is an internal inconsistency +// in our x509 store +type ErrBadCertificateStore struct { +} + +// ErrBadCertificateStore is returned when there is an internal inconsistency +// in our x509 store +func (err ErrBadCertificateStore) Error() string { + return fmt.Sprintf("inconsistent certificate store") +} + // X509Store is the interface for all X509Stores type X509Store interface { AddCert(cert *x509.Certificate) error diff --git a/trustmanager/x509utils.go b/trustmanager/x509utils.go index 7958c86948..5c2f5908b5 100644 --- a/trustmanager/x509utils.go +++ b/trustmanager/x509utils.go @@ -15,6 +15,7 @@ import ( "math/big" "net/http" "net/url" + "path/filepath" "time" "github.com/Sirupsen/logrus" @@ -118,11 +119,26 @@ func fingerprintCert(cert *x509.Certificate) (CertID, error) { } // loadCertsFromDir receives a store AddCertFromFile for each certificate found -func loadCertsFromDir(s *X509FileStore) { +func loadCertsFromDir(s *X509FileStore) error { certFiles := s.fileStore.ListFiles(true) - for _, c := range certFiles { - s.AddCertFromFile(c) + for _, f := range certFiles { + // ListFiles returns relative paths + fullPath := filepath.Join(s.fileStore.BaseDir(), f) + err := s.AddCertFromFile(fullPath) + if err != nil { + if _, ok := err.(*ErrCertValidation); ok { + logrus.Debugf("ignoring certificate, did not pass validation: %s", f) + continue + } + if _, ok := err.(*ErrCertExists); ok { + logrus.Debugf("ignoring certificate, already exists in the store: %s", f) + continue + } + + return err + } } + return nil } // LoadCertFromFile loads the first certificate from the file provided. The