From 14a563bb51b5c40372e98185cce7928287b6f7d5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 6 Nov 2015 23:16:14 +0100 Subject: [PATCH 1/8] Add a few more descriptive comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Miloslav Trmač --- certs/certs.go | 5 +++-- client/client.go | 28 ++++++++++++++++++++++++---- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/certs/certs.go b/certs/certs.go index d8ba0d9255..e2a16bdb77 100644 --- a/certs/certs.go +++ b/certs/certs.go @@ -142,8 +142,9 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin return nil } -// validRootLeafCerts returns a list of non-exipired, non-sha1 certificates whose -// Common-Names match the provided GUN +// validRootLeafCerts returns a list of non-expired, non-sha1 certificates +// found in root whoose Common-Names match the provided GUN. Note that this +// "validity" alone does not imply any measure of trust. func validRootLeafCerts(root *data.SignedRoot, gun string) ([]*x509.Certificate, error) { // Get a list of all of the leaf certificates present in root allLeafCerts, _ := parseAllCerts(root) diff --git a/client/client.go b/client/client.go index a867ecc6c4..65e74845c6 100644 --- a/client/client.go +++ b/client/client.go @@ -160,7 +160,10 @@ func NewTarget(targetName string, targetPath string) (*Target, error) { } // Initialize creates a new repository by using rootKey as the root Key for the -// TUF repository. +// TUF repository. The server must be reachable (and is asked to generate a +// timestamp key and possibly other serverManagedRoles), but the created repository +// result is only stored on local disk, not published to the server. To do that, +// use r.Publish() eventually. func (r *NotaryRepository) Initialize(rootKeyID string, serverManagedRoles ...string) error { privKey, _, err := r.CryptoService.GetPrivateKey(rootKeyID) if err != nil { @@ -645,9 +648,10 @@ func (r *NotaryRepository) publish(cl changelist.Changelist) error { return remote.SetMultiMeta(updatedFiles) } -// bootstrapRepo loads the repository from the local file system. This attempts -// to load metadata for all roles. Since server snapshots are supported, -// if the snapshot metadata fails to load, that's ok. +// bootstrapRepo loads the repository from the local file system (i.e. +// a not yet published repo or a possibly obsolete local copy) into +// r.tufRepo. This attempts to load metadata for all roles. Since server +// snapshots are supported, if the snapshot metadata fails to load, that's ok. // This can also be unified with some cache reading tools from tuf/client. // This assumes that bootstrapRepo is only used by Publish() or RotateKey() func (r *NotaryRepository) bootstrapRepo() error { @@ -695,6 +699,8 @@ func (r *NotaryRepository) bootstrapRepo() error { return nil } +// saveMetadata saves contents of r.tufRepo onto the local disk, creating +// signatures as necessary, possibly prompting for passphrases. func (r *NotaryRepository) saveMetadata(ignoreSnapshot bool) error { logrus.Debugf("Saving changes to Trusted Collection.") @@ -776,6 +782,20 @@ func (r *NotaryRepository) Update(forWrite bool) error { // we should always attempt to contact the server to determine if the repository // is initialized or not. If set to true, we will always attempt to download // and return an error if the remote repository errors. +// +// Partially populates r.tufRepo with this root metadata (only; use +// tufclient.Client.Update to load the rest). +// +// As another side effect, r.CertManager's list of trusted certificates +// is updated with data from the loaded root.json. +// +// Fails if the remote server is reachable and does not know the repo +// (i.e. before the first r.Publish()), in which case the error is +// store.ErrMetaNotFound, or if the root metadata (from whichever source is used) +// is not trusted. +// +// Returns a tufclient.Client for the remote server, which may not be actually +// operational (if the URL is invalid but a root.json is cached). func (r *NotaryRepository) bootstrapClient(checkInitialized bool) (*tufclient.Client, error) { var ( rootJSON []byte From 28d1f487cf7a1a8d2e58966fa5f83e5a46f8ffe2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 6 Nov 2015 23:36:40 +0100 Subject: [PATCH 2/8] Use more descriptive variable names in keystoremanager.ValidateRoot MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit allValidCerts implies validity, and certsFromCN is neutral, when the roles are at least reversed. So, talk about neutral certsFromRoot, and trustedCerts. Signed-off-by: Miloslav Trmač --- certs/certs.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/certs/certs.go b/certs/certs.go index e2a16bdb77..aebe663f2d 100644 --- a/certs/certs.go +++ b/certs/certs.go @@ -71,14 +71,14 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin } // Retrieve all the leaf certificates in root for which the CN matches the GUN - allValidCerts, err := validRootLeafCerts(signedRoot, gun) + certsFromRoot, err := validRootLeafCerts(signedRoot, gun) if err != nil { logrus.Debugf("error retrieving valid leaf certificates for: %s, %v", gun, err) return &ErrValidationFail{Reason: "unable to retrieve valid leaf certificates"} } // Retrieve all the trusted certificates that match this gun - certsForCN, err := certStore.GetCertificatesByCN(gun) + trustedCerts, err := certStore.GetCertificatesByCN(gun) if err != nil { // If the error that we get back is different than ErrNoCertificatesFound // we couldn't check if there are any certificates with this CN already @@ -91,9 +91,9 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin // 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 root certificates for %s", len(certsForCN), gun) - err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(certsForCN)) + if len(trustedCerts) != 0 { + logrus.Debugf("found %d valid root certificates for %s", len(trustedCerts), gun) + err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(trustedCerts)) if err != nil { logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err) return &ErrValidationFail{Reason: "failed to validate data with current trusted certificates"} @@ -103,7 +103,7 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin } // Validate the integrity of the new root (does it have valid signatures) - err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(allValidCerts)) + err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(certsFromRoot)) if err != nil { logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err) return &ErrValidationFail{Reason: "failed to validate integrity of roots"} @@ -116,7 +116,7 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin // Do root certificate rotation: we trust only the certs present in the new root // First we add all the new certificates (even if they already exist) - for _, cert := range allValidCerts { + for _, cert := range certsFromRoot { err := certStore.AddCert(cert) if err != nil { // If the error is already exists we don't fail the rotation @@ -129,7 +129,7 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin } // Now we delete old certificates that aren't present in the new root - for certID, cert := range certsToRemove(certsForCN, allValidCerts) { + for certID, cert := range certsToRemove(trustedCerts, certsFromRoot) { logrus.Debugf("removing certificate with certID: %s", certID) err = certStore.RemoveCert(cert) if err != nil { From 01da4b1a111486b65e57daa9100a468dab515e08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Fri, 6 Nov 2015 23:45:28 +0100 Subject: [PATCH 3/8] Drop an unexpected corner case in certificate rotation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit keystoremanager.certsToRemove unexpectedly returns an empty map of old certificates when there are no new ones. While there is nothing really great to do in that case (we don't want rotation to make a repo completely unusable), this already can not happen so just fail instead of having this weird case. Signed-off-by: Miloslav Trmač --- certs/certs.go | 17 ++++++++++++----- certs/certs_test.go | 22 ++++++++++------------ 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/certs/certs.go b/certs/certs.go index aebe663f2d..661ed5b2fb 100644 --- a/certs/certs.go +++ b/certs/certs.go @@ -129,7 +129,12 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin } // Now we delete old certificates that aren't present in the new root - for certID, cert := range certsToRemove(trustedCerts, certsFromRoot) { + oldCertsToRemove, err := certsToRemove(trustedCerts, certsFromRoot) + if err != nil { + logrus.Debugf("inconsistency when removing old certificates: %v", err) + return err + } + for certID, cert := range oldCertsToRemove { logrus.Debugf("removing certificate with certID: %s", certID) err = certStore.RemoveCert(cert) if err != nil { @@ -246,12 +251,14 @@ func parseAllCerts(signedRoot *data.SignedRoot) (map[string]*x509.Certificate, m // certsToRemove returns all the certifificates from oldCerts that aren't present // in newCerts -func certsToRemove(oldCerts, newCerts []*x509.Certificate) map[string]*x509.Certificate { +func certsToRemove(oldCerts, newCerts []*x509.Certificate) (map[string]*x509.Certificate, error) { certsToRemove := make(map[string]*x509.Certificate) - // If no newCerts were provided + // We don't want to "rotate" certificates to an empty set, nor keep old certificates if the + // new root does not trust them. newCerts should come from validRootLeafCerts, which refuses + // to return an empty set, so this should never happen - fail just to be sure. if len(newCerts) == 0 { - return certsToRemove + return nil, &ErrRootRotationFail{Reason: "internal error, got no certificates to rotate to"} } // Populate a map with all the IDs from newCert @@ -277,5 +284,5 @@ func certsToRemove(oldCerts, newCerts []*x509.Certificate) map[string]*x509.Cert } } - return certsToRemove + return certsToRemove, nil } diff --git a/certs/certs_test.go b/certs/certs_test.go index 62817ddd9c..4ea992308c 100644 --- a/certs/certs_test.go +++ b/certs/certs_test.go @@ -55,7 +55,8 @@ func TestCertsToRemove(t *testing.T) { oldCerts := []*x509.Certificate{cert1} newCerts := []*x509.Certificate{cert2} - certificates := certsToRemove(oldCerts, newCerts) + certificates, err := certsToRemove(oldCerts, newCerts) + require.NoError(t, err) require.Len(t, certificates, 1) _, ok := certificates[cert1KeyID] require.True(t, ok) @@ -64,7 +65,8 @@ func TestCertsToRemove(t *testing.T) { oldCerts = []*x509.Certificate{cert1, cert2} newCerts = []*x509.Certificate{cert3} - certificates = certsToRemove(oldCerts, newCerts) + certificates, err = certsToRemove(oldCerts, newCerts) + require.NoError(t, err) require.Len(t, certificates, 2) _, ok = certificates[cert1KeyID] require.True(t, ok) @@ -77,7 +79,8 @@ func TestCertsToRemove(t *testing.T) { oldCerts = []*x509.Certificate{cert3} newCerts = []*x509.Certificate{cert2, cert1} - certificates = certsToRemove(oldCerts, newCerts) + certificates, err = certsToRemove(oldCerts, newCerts) + require.NoError(t, err) require.Len(t, certificates, 1) _, ok = certificates[cert3KeyID] require.True(t, ok) @@ -90,20 +93,15 @@ func TestCertsToRemove(t *testing.T) { oldCerts = []*x509.Certificate{cert1, cert2, cert3} newCerts = []*x509.Certificate{} - certificates = certsToRemove(oldCerts, newCerts) - require.Len(t, certificates, 0) - _, ok = certificates[cert1KeyID] - require.False(t, ok) - _, ok = certificates[cert2KeyID] - require.False(t, ok) - _, ok = certificates[cert3KeyID] - require.False(t, ok) + certificates, err = certsToRemove(oldCerts, newCerts) + require.Error(t, err) // Call CertsToRemove with three new certificates and no old oldCerts = []*x509.Certificate{} newCerts = []*x509.Certificate{cert1, cert2, cert3} - certificates = certsToRemove(oldCerts, newCerts) + certificates, err = certsToRemove(oldCerts, newCerts) + require.NoError(t, err) require.Len(t, certificates, 0) _, ok = certificates[cert1KeyID] require.False(t, ok) From 12879622ac9bdf2077a07fab80b19981f95c690f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Tue, 26 Jan 2016 21:12:22 +0100 Subject: [PATCH 4/8] Clean up tuf/signed/sign_test.go MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Remove Sign, no longer needed for CryptoService implementations - Remove unused testKeyID1 - Drop the intermediate MockCryptoService with no users; rename StrictMockCryptoService to MockCryptoService - Actually be strict in the new MockCryptoService, and fix key ID use - Clearly document what FailingCryptoService and MockCryptoService are supposed to do Signed-off-by: Miloslav Trmač --- tuf/signed/sign_test.go | 73 +++++++---------------------------------- 1 file changed, 11 insertions(+), 62 deletions(-) diff --git a/tuf/signed/sign_test.go b/tuf/signed/sign_test.go index 3a8d172da1..b02338db55 100644 --- a/tuf/signed/sign_test.go +++ b/tuf/signed/sign_test.go @@ -14,24 +14,18 @@ import ( const ( testKeyPEM1 = "-----BEGIN PUBLIC KEY-----\nMIIBojANBgkqhkiG9w0BAQEFAAOCAY8AMIIBigKCAYEAnKuXZeefa2LmgxaL5NsM\nzKOHNe+x/nL6ik+lDBCTV6OdcwAhHQS+PONGhrChIUVR6Vth3hUCrreLzPO73Oo5\nVSCuRJ53UronENl6lsa5mFKP8StYLvIDITNvkoT3j52BJIjyNUK9UKY9As2TNqDf\nBEPIRp28ev/NViwGOEkBu2UAbwCIdnDXm8JQErCZA0Ydm7PKGgjLbFsFGrVzqXHK\n6pdzJXlhr9yap3UpgQ/iO9JtoEYB2EXsnSrPc9JRjR30bNHHtnVql3fvinXrAEwq\n3xmN4p+R4VGzfdQN+8Kl/IPjqWB535twhFYEG/B7Ze8IwbygBjK3co/KnOPqMUrM\nBI8ztvPiogz+MvXb8WvarZ6TMTh8ifZI96r7zzqyzjR1hJulEy3IsMGvz8XS2J0X\n7sXoaqszEtXdq5ef5zKVxkiyIQZcbPgmpHLq4MgfdryuVVc/RPASoRIXG4lKaTJj\n1ANMFPxDQpHudCLxwCzjCb+sVa20HBRPTnzo8LSZkI6jAgMBAAE=\n-----END PUBLIC KEY-----" - testKeyID1 = "51324b59d4888faa91219ebbe5a3876bb4efb21f0602ddf363cd4c3996ded3d4" ) +// A CryptoService which does not contain any keys. type FailingCryptoService struct { - testKey data.PrivateKey -} - -func (mts *FailingCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) { - sigs := make([]data.Signature, 0, len(keyIDs)) - return sigs, nil } func (mts *FailingCryptoService) Create(_, _, _ string) (data.PublicKey, error) { - return mts.testKey, nil + return nil, nil } func (mts *FailingCryptoService) ListKeys(role string) []string { - return []string{mts.testKey.ID()} + return []string{} } func (mts *FailingCryptoService) AddKey(role, gun string, key data.PrivateKey) error { @@ -39,25 +33,14 @@ func (mts *FailingCryptoService) AddKey(role, gun string, key data.PrivateKey) e } func (mts *FailingCryptoService) ListAllKeys() map[string]string { - return map[string]string{ - mts.testKey.ID(): data.CanonicalRootRole, - mts.testKey.ID(): data.CanonicalTargetsRole, - mts.testKey.ID(): data.CanonicalSnapshotRole, - mts.testKey.ID(): data.CanonicalTimestampRole, - } + return map[string]string{} } func (mts *FailingCryptoService) GetKey(keyID string) data.PublicKey { - if keyID == "testID" { - return data.PublicKeyFromPrivate(mts.testKey) - } return nil } func (mts *FailingCryptoService) GetPrivateKey(keyID string) (data.PrivateKey, string, error) { - if mts.testKey != nil { - return mts.testKey, "testRole", nil - } return nil, "", trustmanager.ErrKeyNotFound{KeyID: keyID} } @@ -65,18 +48,11 @@ func (mts *FailingCryptoService) RemoveKey(keyID string) error { return nil } +// A CryptoService which only only allows using one key type MockCryptoService struct { testKey data.PrivateKey } -func (mts *MockCryptoService) Sign(keyIDs []string, _ []byte) ([]data.Signature, error) { - sigs := make([]data.Signature, 0, len(keyIDs)) - for _, keyID := range keyIDs { - sigs = append(sigs, data.Signature{KeyID: keyID}) - } - return sigs, nil -} - func (mts *MockCryptoService) Create(_, _, _ string) (data.PublicKey, error) { return mts.testKey, nil } @@ -86,7 +62,7 @@ func (mts *MockCryptoService) AddKey(role, gun string, key data.PrivateKey) erro } func (mts *MockCryptoService) GetKey(keyID string) data.PublicKey { - if keyID == "testID" { + if keyID == mts.testKey.ID() { return data.PublicKeyFromPrivate(mts.testKey) } return nil @@ -106,43 +82,16 @@ func (mts *MockCryptoService) ListAllKeys() map[string]string { } func (mts *MockCryptoService) GetPrivateKey(keyID string) (data.PrivateKey, string, error) { - return mts.testKey, "testRole", nil + if keyID == mts.testKey.ID() { + return mts.testKey, "testRole", nil + } + return nil, "", trustmanager.ErrKeyNotFound{KeyID: keyID} } func (mts *MockCryptoService) RemoveKey(keyID string) error { return nil } -var _ CryptoService = &MockCryptoService{} - -type StrictMockCryptoService struct { - MockCryptoService -} - -func (mts *StrictMockCryptoService) GetKey(keyID string) data.PublicKey { - if keyID == mts.testKey.ID() { - return data.PublicKeyFromPrivate(mts.testKey) - } - return nil -} - -func (mts *StrictMockCryptoService) ListKeys(role string) []string { - return []string{mts.testKey.ID()} -} - -func (mts *StrictMockCryptoService) ListAllKeys() map[string]string { - return map[string]string{ - mts.testKey.ID(): data.CanonicalRootRole, - mts.testKey.ID(): data.CanonicalTargetsRole, - mts.testKey.ID(): data.CanonicalSnapshotRole, - mts.testKey.ID(): data.CanonicalTimestampRole, - } -} - -func (mts *StrictMockCryptoService) AddKey(role, gun string, key data.PrivateKey) error { - return nil -} - // Test signing and ensure the expected signature is added func TestBasicSign(t *testing.T) { cs := NewEd25519() @@ -261,7 +210,7 @@ func TestSignWithX509(t *testing.T) { // test signing against a service that only recognizes a RSAKey (not // RSAx509 key) - mockCryptoService := &StrictMockCryptoService{MockCryptoService{privKey}} + mockCryptoService := &MockCryptoService{privKey} testData := data.Signed{ Signed: &json.RawMessage{}, } From 229d64e0e05318e1c761c7d932d5ededf49d5f15 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 11 Jan 2016 19:12:09 +0100 Subject: [PATCH 5/8] Use ErrKeyNotFound in KeyStore.GetKey MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 3 out of 4 KeyStore implementations have already been returning this type or a pointer to it; document this as a requirement, and modify implementations to comply. Signed-off-by: Miloslav Trmač --- cryptoservice/crypto_service_test.go | 2 +- trustmanager/keyfilestore.go | 2 +- trustmanager/keystore.go | 2 ++ trustmanager/yubikey/yubikeystore.go | 29 ++++++++++++++++++++++++---- 4 files changed, 29 insertions(+), 6 deletions(-) diff --git a/cryptoservice/crypto_service_test.go b/cryptoservice/crypto_service_test.go index 57182eb203..6589fc60a5 100644 --- a/cryptoservice/crypto_service_test.go +++ b/cryptoservice/crypto_service_test.go @@ -97,7 +97,7 @@ func (c CryptoServiceTester) TestGetNonexistentKey(t *testing.T) { _, _, err := cryptoService.GetPrivateKey("boguskeyid") require.Error(t, err) // The underlying error has been correctly propagated. - _, ok := err.(*trustmanager.ErrKeyNotFound) + _, ok := err.(trustmanager.ErrKeyNotFound) require.True(t, ok) } diff --git a/trustmanager/keyfilestore.go b/trustmanager/keyfilestore.go index 13fe4704f3..3a5be1cf62 100644 --- a/trustmanager/keyfilestore.go +++ b/trustmanager/keyfilestore.go @@ -361,7 +361,7 @@ func getKeyRole(s LimitedFileStore, keyID string) (string, bool, error) { } } - return "", false, &ErrKeyNotFound{KeyID: keyID} + return "", false, ErrKeyNotFound{KeyID: keyID} } // GetKey returns the PrivateKey given a KeyID diff --git a/trustmanager/keystore.go b/trustmanager/keystore.go index 4f1338e47b..237f570bdf 100644 --- a/trustmanager/keystore.go +++ b/trustmanager/keystore.go @@ -43,6 +43,8 @@ type KeyStore interface { // AddKey adds a key to the KeyStore, and if the key already exists, // succeeds. Otherwise, returns an error if it cannot add. AddKey(keyInfo KeyInfo, privKey data.PrivateKey) error + // Should fail with ErrKeyNotFound if the keystore is operating normally + // and knows that it does not store the requested key. GetKey(keyID string) (data.PrivateKey, string, error) GetKeyInfo(keyID string) (KeyInfo, error) ListKeys() map[string]KeyInfo diff --git a/trustmanager/yubikey/yubikeystore.go b/trustmanager/yubikey/yubikeystore.go index e1653aa63f..067e898360 100644 --- a/trustmanager/yubikey/yubikeystore.go +++ b/trustmanager/yubikey/yubikeystore.go @@ -103,6 +103,17 @@ func (err ErrBackupFailed) Error() string { return fmt.Sprintf("Failed to backup private key to: %s", err.err) } +// An error indicating that the HSM is not present (as opposed to failing), +// i.e. that we can confidently claim that the key is not stored in the HSM +// without notifying the user about a missing or failing HSM. +type errHSMNotPresent struct { + err string +} + +func (err errHSMNotPresent) Error() string { + return err.err +} + type yubiSlot struct { role string slotID []byte @@ -706,13 +717,16 @@ func (s *YubiKeyStore) GetKey(keyID string) (data.PrivateKey, string, error) { ctx, session, err := SetupHSMEnv(pkcs11Lib, s.libLoader) if err != nil { logrus.Debugf("Failed to initialize PKCS11 environment: %s", err.Error()) + if _, ok := err.(errHSMNotPresent); ok { + err = trustmanager.ErrKeyNotFound{KeyID: keyID} + } return nil, "", err } defer cleanup(ctx, session) key, ok := s.keys[keyID] if !ok { - return nil, "", errors.New("no matching keys found inside of yubikey") + return nil, "", trustmanager.ErrKeyNotFound{KeyID: keyID} } pubKey, alias, err := getECDSAKey(ctx, session, key.slotID) @@ -788,7 +802,7 @@ func SetupHSMEnv(libraryPath string, libLoader pkcs11LibLoader) ( IPKCS11Ctx, pkcs11.SessionHandle, error) { if libraryPath == "" { - return nil, 0, fmt.Errorf("no library found.") + return nil, 0, errHSMNotPresent{err: "no library found."} } p := libLoader(libraryPath) @@ -798,8 +812,15 @@ func SetupHSMEnv(libraryPath string, libLoader pkcs11LibLoader) ( if err := p.Initialize(); err != nil { defer finalizeAndDestroy(p) - return nil, 0, fmt.Errorf( - "found library %s, but initialize error %s", libraryPath, err.Error()) + // Sadly, if ykcs11's C_Initialize fails at all, it is likely with this + // error code; but because it is only discovering the slots and not interacting + // with individual tokens, assume that this is not a Yubikey failure, simply + // no slots are present. + message := fmt.Sprintf("found library %s, but initialize error %s", libraryPath, err.Error()) + if err, ok := err.(pkcs11.Error); ok && err == pkcs11.CKR_FUNCTION_FAILED { + return nil, 0, errHSMNotPresent{err: message} + } + return nil, 0, errors.New(message) } slots, err := p.GetSlotList(true) From bcc97ed2212281b20d94d388303575fbb1ffb5ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 11 Jan 2016 18:59:15 +0100 Subject: [PATCH 6/8] Fill the KeyID member of ErrKeyNotFound in KeyDBStore MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit (Not necessary for the rest of this PR, just a related cleanup.) Signed-off-by: Miloslav Trmač --- signer/keydbstore/keydbstore.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/signer/keydbstore/keydbstore.go b/signer/keydbstore/keydbstore.go index ac57263b3a..ebd0c49ae2 100644 --- a/signer/keydbstore/keydbstore.go +++ b/signer/keydbstore/keydbstore.go @@ -117,7 +117,7 @@ func (s *KeyDBStore) GetKey(name string) (data.PrivateKey, string, error) { // Retrieve the GORM private key from the database dbPrivateKey := GormPrivateKey{} if s.db.Where(&GormPrivateKey{KeyID: name}).First(&dbPrivateKey).RecordNotFound() { - return nil, "", trustmanager.ErrKeyNotFound{} + return nil, "", trustmanager.ErrKeyNotFound{KeyID: name} } // Get the passphrase to use for this key @@ -165,7 +165,7 @@ func (s *KeyDBStore) RemoveKey(keyID string) error { // Retrieve the GORM private key from the database dbPrivateKey := GormPrivateKey{} if s.db.Where(&GormPrivateKey{KeyID: keyID}).First(&dbPrivateKey).RecordNotFound() { - return trustmanager.ErrKeyNotFound{} + return trustmanager.ErrKeyNotFound{KeyID: keyID} } // Delete the key from the database @@ -179,7 +179,7 @@ func (s *KeyDBStore) RotateKeyPassphrase(name, newPassphraseAlias string) error // Retrieve the GORM private key from the database dbPrivateKey := GormPrivateKey{} if s.db.Where(&GormPrivateKey{KeyID: name}).First(&dbPrivateKey).RecordNotFound() { - return trustmanager.ErrKeyNotFound{} + return trustmanager.ErrKeyNotFound{KeyID: name} } // Get the current passphrase to use for this key From 4a271d4b3e11c057681225a2867ef538fd321d97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Miloslav=20Trma=C4=8D?= Date: Mon, 1 Feb 2016 02:01:36 +0100 Subject: [PATCH 7/8] Log certificate IDs during root verification MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "found 1 certificate" does not say which certificates are involved; log their IDs. Signed-off-by: Miloslav Trmač --- certs/certs.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/certs/certs.go b/certs/certs.go index 661ed5b2fb..68e4af4bc2 100644 --- a/certs/certs.go +++ b/certs/certs.go @@ -4,6 +4,7 @@ import ( "crypto/x509" "errors" "fmt" + "strings" "time" "github.com/Sirupsen/logrus" @@ -36,6 +37,18 @@ func (err ErrRootRotationFail) Error() string { return fmt.Sprintf("could not rotate trust to a new trusted root: %s", err.Reason) } +func certIDs(certs []*x509.Certificate) string { + ids := make([]string, 0, len(certs)) + for _, cert := range certs { + id, err := trustmanager.FingerprintCert(cert) + if err != nil { + id = fmt.Sprintf("[Error %s]", err) + } + ids = append(ids, id) + } + return strings.Join(ids, ", ") +} + /* ValidateRoot receives a new root, validates its correctness and attempts to do root key rotation if needed. @@ -92,7 +105,8 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin // 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(trustedCerts) != 0 { - logrus.Debugf("found %d valid root certificates for %s", len(trustedCerts), gun) + logrus.Debugf("found %d valid root certificates for %s: %s", len(trustedCerts), gun, + certIDs(trustedCerts)) err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(trustedCerts)) if err != nil { logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err) @@ -186,7 +200,8 @@ func validRootLeafCerts(root *data.SignedRoot, gun string) ([]*x509.Certificate, 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) + logrus.Debugf("found %d valid leaf certificates for %s: %s", len(validLeafCerts), gun, + certIDs(validLeafCerts)) return validLeafCerts, nil } From dcc41be3e17b0fe0c357d8259f7b9db3bd4f98c4 Mon Sep 17 00:00:00 2001 From: Ying Li Date: Thu, 24 Mar 2016 11:20:12 -0700 Subject: [PATCH 8/8] Rename cert function, and also check if newCertMap is empty. Also do not check for CKR_FUNCTION_FAILED for error translation Signed-off-by: Ying Li --- certs/certs.go | 28 +++++++++++++++------------- trustmanager/yubikey/yubikeystore.go | 10 +--------- 2 files changed, 16 insertions(+), 22 deletions(-) diff --git a/certs/certs.go b/certs/certs.go index 68e4af4bc2..4b81543b3e 100644 --- a/certs/certs.go +++ b/certs/certs.go @@ -37,7 +37,7 @@ func (err ErrRootRotationFail) Error() string { return fmt.Sprintf("could not rotate trust to a new trusted root: %s", err.Reason) } -func certIDs(certs []*x509.Certificate) string { +func prettyFormatCertIDs(certs []*x509.Certificate) string { ids := make([]string, 0, len(certs)) for _, cert := range certs { id, err := trustmanager.FingerprintCert(cert) @@ -106,7 +106,7 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin // use them first to validate that this new root is valid. if len(trustedCerts) != 0 { logrus.Debugf("found %d valid root certificates for %s: %s", len(trustedCerts), gun, - certIDs(trustedCerts)) + prettyFormatCertIDs(trustedCerts)) err = signed.VerifyRoot(root, 0, trustmanager.CertsToKeys(trustedCerts)) if err != nil { logrus.Debugf("failed to verify TUF data for: %s, %v", gun, err) @@ -162,7 +162,7 @@ func ValidateRoot(certStore trustmanager.X509Store, root *data.Signed, gun strin } // validRootLeafCerts returns a list of non-expired, non-sha1 certificates -// found in root whoose Common-Names match the provided GUN. Note that this +// found in root whose Common-Names match the provided GUN. Note that this // "validity" alone does not imply any measure of trust. func validRootLeafCerts(root *data.SignedRoot, gun string) ([]*x509.Certificate, error) { // Get a list of all of the leaf certificates present in root @@ -201,7 +201,7 @@ func validRootLeafCerts(root *data.SignedRoot, gun string) ([]*x509.Certificate, } logrus.Debugf("found %d valid leaf certificates for %s: %s", len(validLeafCerts), gun, - certIDs(validLeafCerts)) + prettyFormatCertIDs(validLeafCerts)) return validLeafCerts, nil } @@ -264,18 +264,12 @@ func parseAllCerts(signedRoot *data.SignedRoot) (map[string]*x509.Certificate, m return leafCerts, intCerts } -// certsToRemove returns all the certifificates from oldCerts that aren't present -// in newCerts +// certsToRemove returns all the certificates from oldCerts that aren't present +// in newCerts. Note that newCerts should never be empty, else this function will error. +// We expect newCerts to come from validateRootLeafCerts, which does not return empty sets. func certsToRemove(oldCerts, newCerts []*x509.Certificate) (map[string]*x509.Certificate, error) { certsToRemove := make(map[string]*x509.Certificate) - // We don't want to "rotate" certificates to an empty set, nor keep old certificates if the - // new root does not trust them. newCerts should come from validRootLeafCerts, which refuses - // to return an empty set, so this should never happen - fail just to be sure. - if len(newCerts) == 0 { - return nil, &ErrRootRotationFail{Reason: "internal error, got no certificates to rotate to"} - } - // Populate a map with all the IDs from newCert var newCertMap = make(map[string]struct{}) for _, cert := range newCerts { @@ -287,6 +281,14 @@ func certsToRemove(oldCerts, newCerts []*x509.Certificate) (map[string]*x509.Cer newCertMap[certID] = struct{}{} } + // We don't want to "rotate" certificates to an empty set, nor keep old certificates if the + // new root does not trust them. newCerts should come from validRootLeafCerts, which refuses + // to return an empty set, and they should all be fingerprintable, so this should never happen + // - fail just to be sure. + if len(newCertMap) == 0 { + return nil, &ErrRootRotationFail{Reason: "internal error, got no certificates to rotate to"} + } + // Iterate over all the old certificates and check to see if we should remove them for _, cert := range oldCerts { certID, err := trustmanager.FingerprintCert(cert) diff --git a/trustmanager/yubikey/yubikeystore.go b/trustmanager/yubikey/yubikeystore.go index 067e898360..2e8300b396 100644 --- a/trustmanager/yubikey/yubikeystore.go +++ b/trustmanager/yubikey/yubikeystore.go @@ -812,15 +812,7 @@ func SetupHSMEnv(libraryPath string, libLoader pkcs11LibLoader) ( if err := p.Initialize(); err != nil { defer finalizeAndDestroy(p) - // Sadly, if ykcs11's C_Initialize fails at all, it is likely with this - // error code; but because it is only discovering the slots and not interacting - // with individual tokens, assume that this is not a Yubikey failure, simply - // no slots are present. - message := fmt.Sprintf("found library %s, but initialize error %s", libraryPath, err.Error()) - if err, ok := err.(pkcs11.Error); ok && err == pkcs11.CKR_FUNCTION_FAILED { - return nil, 0, errHSMNotPresent{err: message} - } - return nil, 0, errors.New(message) + return nil, 0, fmt.Errorf("found library %s, but initialize error %s", libraryPath, err.Error()) } slots, err := p.GetSlotList(true)