diff --git a/builtin/logical/pki/cert_util.go b/builtin/logical/pki/cert_util.go index b47d8eaa136c..30c3e570957a 100644 --- a/builtin/logical/pki/cert_util.go +++ b/builtin/logical/pki/cert_util.go @@ -186,33 +186,65 @@ func fetchCAInfo(req *logical.Request) (*caInfoBundle, error) { // Allows fetching certificates from the backend; it handles the slightly // separate pathing for CA, CRL, and revoked certificates. func fetchCertBySerial(req *logical.Request, prefix, serial string) (*logical.StorageEntry, error) { - var path string + var path, legacyPath string + var err error + var certEntry *logical.StorageEntry + + hyphenSerial := normalizeSerial(serial) + colonSerial := strings.Replace(strings.ToLower(serial), "-", ":", -1) switch { // Revoked goes first as otherwise ca/crl get hardcoded paths which fail if // we actually want revocation info case strings.HasPrefix(prefix, "revoked/"): - path = "revoked/" + strings.Replace(strings.ToLower(serial), "-", ":", -1) + legacyPath = "revoked/" + colonSerial + path = "revoked/" + hyphenSerial case serial == "ca": path = "ca" case serial == "crl": path = "crl" default: - path = "certs/" + strings.Replace(strings.ToLower(serial), "-", ":", -1) + legacyPath = "certs/" + colonSerial + path = "certs/" + hyphenSerial } - certEntry, err := req.Storage.Get(path) + certEntry, err = req.Storage.Get(path) if err != nil { return nil, errutil.InternalError{Err: fmt.Sprintf("error fetching certificate %s: %s", serial, err)} } - if certEntry == nil { + if certEntry != nil { + if certEntry.Value == nil || len(certEntry.Value) == 0 { + return nil, errutil.InternalError{Err: fmt.Sprintf("returned certificate bytes for serial %s were empty", serial)} + } + return certEntry, nil + } + + // No point checking these, no old/new style colons/hyphens + if legacyPath == "" { return nil, nil } + // Retrieve the old-style path + certEntry, err = req.Storage.Get(legacyPath) + if err != nil { + return nil, errutil.InternalError{Err: fmt.Sprintf("error fetching certificate %s: %s", serial, err)} + } + if certEntry == nil { + return nil, nil + } if certEntry.Value == nil || len(certEntry.Value) == 0 { return nil, errutil.InternalError{Err: fmt.Sprintf("returned certificate bytes for serial %s were empty", serial)} } + // Update old-style paths to new-style paths + certEntry.Key = path + if err = req.Storage.Put(certEntry); err != nil { + return nil, errutil.InternalError{Err: fmt.Sprintf("error saving certificate with serial %s to new location", serial)} + } + if err = req.Storage.Delete(legacyPath); err != nil { + return nil, errutil.InternalError{Err: fmt.Sprintf("error deleting certificate with serial %s from old location", serial)} + } + return certEntry, nil } diff --git a/builtin/logical/pki/cert_util_test.go b/builtin/logical/pki/cert_util_test.go new file mode 100644 index 000000000000..068a0a69a757 --- /dev/null +++ b/builtin/logical/pki/cert_util_test.go @@ -0,0 +1,124 @@ +package pki + +import ( + "fmt" + "testing" + + "strings" + + "github.com/hashicorp/vault/logical" +) + +func TestPki_FetchCertBySerial(t *testing.T) { + storage := &logical.InmemStorage{} + + cases := map[string]struct { + Req *logical.Request + Prefix string + Serial string + }{ + "valid cert": { + &logical.Request{ + Storage: storage, + }, + "certs/", + "00:00:00:00:00:00:00:00", + }, + "revoked cert": { + &logical.Request{ + Storage: storage, + }, + "revoked/", + "11:11:11:11:11:11:11:11", + }, + } + + // Test for colon-based paths in storage + for name, tc := range cases { + storageKey := fmt.Sprintf("%s%s", tc.Prefix, tc.Serial) + err := storage.Put(&logical.StorageEntry{ + Key: storageKey, + Value: []byte("some data"), + }) + if err != nil { + t.Fatalf("error writing to storage on %s colon-based storage path: %s", name, err) + } + + certEntry, err := fetchCertBySerial(tc.Req, tc.Prefix, tc.Serial) + if err != nil { + t.Fatalf("error on %s for colon-based storage path: %s", name, err) + } + + // Check for non-nil on valid/revoked certs + if certEntry == nil { + t.Fatalf("nil on %s for colon-based storage path", name) + } + + // Ensure that cert serials are converted/updated after fetch + expectedKey := tc.Prefix + normalizeSerial(tc.Serial) + se, err := storage.Get(expectedKey) + if err != nil { + t.Fatalf("error on %s for colon-based storage path:%s", name, err) + } + if strings.Compare(expectedKey, se.Key) != 0 { + t.Fatalf("expected: %s, got: %s", expectedKey, certEntry.Key) + } + } + + // Reset storage + storage = &logical.InmemStorage{} + + // Test for hyphen-base paths in storage + for name, tc := range cases { + storageKey := tc.Prefix + normalizeSerial(tc.Serial) + err := storage.Put(&logical.StorageEntry{ + Key: storageKey, + Value: []byte("some data"), + }) + if err != nil { + t.Fatalf("error writing to storage on %s hyphen-based storage path: %s", name, err) + } + + certEntry, err := fetchCertBySerial(tc.Req, tc.Prefix, tc.Serial) + if err != nil || certEntry == nil { + t.Fatalf("error on %s for hyphen-based storage path: err: %v, entry: %v", name, err, certEntry) + } + } + + noConvCases := map[string]struct { + Req *logical.Request + Prefix string + Serial string + }{ + "ca": { + &logical.Request{ + Storage: storage, + }, + "", + "ca", + }, + "crl": { + &logical.Request{ + Storage: storage, + }, + "", + "crl", + }, + } + + // Test for ca and crl case + for name, tc := range noConvCases { + err := storage.Put(&logical.StorageEntry{ + Key: tc.Serial, + Value: []byte("some data"), + }) + if err != nil { + t.Fatalf("error writing to storage on %s: %s", name, err) + } + + certEntry, err := fetchCertBySerial(tc.Req, tc.Prefix, tc.Serial) + if err != nil || certEntry == nil { + t.Fatalf("error on %s: err: %v, entry: %v", name, err, certEntry) + } + } +} diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index aa15f6cc617f..c40e759aab0f 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -65,7 +65,7 @@ func revokeCert(b *backend, req *logical.Request, serial string, fromLease bool) cert, err := x509.ParseCertificate(certEntry.Value) if err != nil { - return nil, fmt.Errorf("Error parsing certificate") + return nil, fmt.Errorf("Error parsing certificate: %s", err) } if cert == nil { return nil, fmt.Errorf("Got a nil certificate") @@ -86,7 +86,7 @@ func revokeCert(b *backend, req *logical.Request, serial string, fromLease bool) revInfo.RevocationTime = currTime.Unix() revInfo.RevocationTimeUTC = currTime.UTC() - revEntry, err = logical.StorageEntryJSON("revoked/"+serial, revInfo) + revEntry, err = logical.StorageEntryJSON("revoked/"+normalizeSerial(serial), revInfo) if err != nil { return nil, fmt.Errorf("Error creating revocation entry") } diff --git a/builtin/logical/pki/path_intermediate.go b/builtin/logical/pki/path_intermediate.go index 6bac720240ec..71a04555ebf5 100644 --- a/builtin/logical/pki/path_intermediate.go +++ b/builtin/logical/pki/path_intermediate.go @@ -196,7 +196,7 @@ func (b *backend) pathSetSignedIntermediate( return nil, err } - entry.Key = "certs/" + cb.SerialNumber + entry.Key = "certs/" + normalizeSerial(cb.SerialNumber) entry.Value = inputBundle.CertificateBytes err = req.Storage.Put(entry) if err != nil { diff --git a/builtin/logical/pki/path_issue_sign.go b/builtin/logical/pki/path_issue_sign.go index f615f98d1dab..26f74210464b 100644 --- a/builtin/logical/pki/path_issue_sign.go +++ b/builtin/logical/pki/path_issue_sign.go @@ -269,11 +269,11 @@ func (b *backend) pathIssueSignCert( if !role.NoStore { err = req.Storage.Put(&logical.StorageEntry{ - Key: "certs/" + cb.SerialNumber, + Key: "certs/" + normalizeSerial(cb.SerialNumber), Value: parsedBundle.CertificateBytes, }) if err != nil { - return nil, fmt.Errorf("Unable to store certificate locally: %v", err) + return nil, fmt.Errorf("unable to store certificate locally: %v", err) } } diff --git a/builtin/logical/pki/path_root.go b/builtin/logical/pki/path_root.go index c9a8cf297e78..d02953133cb5 100644 --- a/builtin/logical/pki/path_root.go +++ b/builtin/logical/pki/path_root.go @@ -145,7 +145,7 @@ func (b *backend) pathCAGenerateRoot( // Also store it as just the certificate identified by serial number, so it // can be revoked err = req.Storage.Put(&logical.StorageEntry{ - Key: "certs/" + cb.SerialNumber, + Key: "certs/" + normalizeSerial(cb.SerialNumber), Value: parsedBundle.CertificateBytes, }) if err != nil { @@ -277,7 +277,7 @@ func (b *backend) pathCASignIntermediate( } err = req.Storage.Put(&logical.StorageEntry{ - Key: "certs/" + cb.SerialNumber, + Key: "certs/" + normalizeSerial(cb.SerialNumber), Value: parsedBundle.CertificateBytes, }) if err != nil { diff --git a/builtin/logical/pki/secret_certs.go b/builtin/logical/pki/secret_certs.go index fbc653d1daab..32f6f4296cc0 100644 --- a/builtin/logical/pki/secret_certs.go +++ b/builtin/logical/pki/secret_certs.go @@ -2,7 +2,6 @@ package pki import ( "fmt" - "strings" "github.com/hashicorp/vault/logical" "github.com/hashicorp/vault/logical/framework" @@ -46,10 +45,8 @@ func (b *backend) secretCredsRevoke( return nil, fmt.Errorf("could not find serial in internal secret data") } - serial := strings.Replace(strings.ToLower(serialInt.(string)), "-", ":", -1) - b.revokeStorageLock.Lock() defer b.revokeStorageLock.Unlock() - return revokeCert(b, req, serial, true) + return revokeCert(b, req, serialInt.(string), true) } diff --git a/builtin/logical/pki/util.go b/builtin/logical/pki/util.go new file mode 100644 index 000000000000..3dffb536bd85 --- /dev/null +++ b/builtin/logical/pki/util.go @@ -0,0 +1,7 @@ +package pki + +import "strings" + +func normalizeSerial(serial string) string { + return strings.Replace(strings.ToLower(serial), ":", "-", -1) +}