Merge pull request #2575 from hashicorp/pki-colons-to-hyphens

Change storage of PKI entries from colons to hyphens

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
 }
 

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

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

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 {

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

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 {

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

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