Merge pull request #108 from docker/fixing-filestores

Fixed but with loadCertsFromDir and added tests

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
 }
 

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)
 		}

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{})
 	}
 }
 

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

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