From 5e0cc29c117fd2dfa3fb73ebb8e4b0863e05b617 Mon Sep 17 00:00:00 2001 From: hc-github-team-secure-vault-core <82990506+hc-github-team-secure-vault-core@users.noreply.github.com> Date: Fri, 19 May 2023 08:20:27 -0400 Subject: [PATCH] pki: add subject key identifier to read key response (#20642) (#20658) * pki: add subject key identifier to read key response This will be helpful for the Terraform Vault Provider to detect migration of pre-1.11 exported keys (from CA generation) into post-1.11 Vault. * add changelog * Update builtin/logical/pki/path_fetch_keys.go * check for managed key first * Validate the SKID matches on root CAs * Validate SKID matches on int CAs * Fix formatting of tests --------- Signed-off-by: Alexander Scheel Co-authored-by: John-Michael Faircloth Co-authored-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 31 ++++++++++++++++++++++++-- builtin/logical/pki/fields.go | 1 + builtin/logical/pki/path_fetch_keys.go | 19 ++++++++++++++++ changelog/20642.txt | 3 +++ sdk/helper/certutil/helpers.go | 6 ++--- 5 files changed, 55 insertions(+), 5 deletions(-) create mode 100644 changelog/20642.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 13667df92f71..9eb706173764 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2382,6 +2382,14 @@ func TestBackend_Root_Idempotency(t *testing.T) { require.NotNil(t, resp, "expected ca info") keyId1 := resp.Data["key_id"] issuerId1 := resp.Data["issuer_id"] + cert := parseCert(t, resp.Data["certificate"].(string)) + certSkid := certutil.GetHexFormatted(cert.SubjectKeyId, ":") + + // -> Validate the SKID matches between the root cert and the key + resp, err = CBRead(b, s, "key/"+keyId1.(keyID).String()) + require.NoError(t, err) + require.NotNil(t, resp, "expected a response") + require.Equal(t, resp.Data["subject_key_id"], certSkid) resp, err = CBRead(b, s, "cert/ca_chain") require.NoError(t, err, "error reading ca_chain: %v", err) @@ -2396,6 +2404,14 @@ func TestBackend_Root_Idempotency(t *testing.T) { require.NotNil(t, resp, "expected ca info") keyId2 := resp.Data["key_id"] issuerId2 := resp.Data["issuer_id"] + cert = parseCert(t, resp.Data["certificate"].(string)) + certSkid = certutil.GetHexFormatted(cert.SubjectKeyId, ":") + + // -> Validate the SKID matches between the root cert and the key + resp, err = CBRead(b, s, "key/"+keyId2.(keyID).String()) + require.NoError(t, err) + require.NotNil(t, resp, "expected a response") + require.Equal(t, resp.Data["subject_key_id"], certSkid) // Make sure that we actually generated different issuer and key values require.NotEqual(t, keyId1, keyId2) @@ -2501,12 +2517,19 @@ func TestBackend_SignIntermediate_AllowedPastCA(t *testing.T) { resp, err := CBWrite(b_int, s_int, "intermediate/generate/internal", map[string]interface{}{ "common_name": "myint.com", }) + require.Contains(t, resp.Data, "key_id") + intKeyId := resp.Data["key_id"].(keyID) + csr := resp.Data["csr"] + + resp, err = CBRead(b_int, s_int, "key/"+intKeyId.String()) + require.NoError(t, err) + require.NotNil(t, resp, "expected a response") + intSkid := resp.Data["subject_key_id"].(string) + if err != nil { t.Fatal(err) } - csr := resp.Data["csr"] - _, err = CBWrite(b_root, s_root, "sign/test", map[string]interface{}{ "common_name": "myint.com", "csr": csr, @@ -2541,6 +2564,10 @@ func TestBackend_SignIntermediate_AllowedPastCA(t *testing.T) { if len(resp.Warnings) == 0 { t.Fatalf("expected warnings, got %#v", *resp) } + + cert := parseCert(t, resp.Data["certificate"].(string)) + certSkid := certutil.GetHexFormatted(cert.SubjectKeyId, ":") + require.Equal(t, intSkid, certSkid) } func TestBackend_ConsulSignLeafWithLegacyRole(t *testing.T) { diff --git a/builtin/logical/pki/fields.go b/builtin/logical/pki/fields.go index e806f473a3be..c5c68bd3783a 100644 --- a/builtin/logical/pki/fields.go +++ b/builtin/logical/pki/fields.go @@ -13,6 +13,7 @@ const ( keyIdParam = "key_id" keyTypeParam = "key_type" keyBitsParam = "key_bits" + skidParam = "subject_key_id" ) // addIssueAndSignCommonFields adds fields common to both CA and non-CA issuing diff --git a/builtin/logical/pki/path_fetch_keys.go b/builtin/logical/pki/path_fetch_keys.go index 2e718240dc8d..d0a01cb429b0 100644 --- a/builtin/logical/pki/path_fetch_keys.go +++ b/builtin/logical/pki/path_fetch_keys.go @@ -2,8 +2,10 @@ package pki import ( "context" + "crypto" "fmt" + "github.com/hashicorp/vault/sdk/helper/certutil" "github.com/hashicorp/vault/sdk/helper/errutil" "github.com/hashicorp/vault/sdk/framework" @@ -155,6 +157,7 @@ func (b *backend) pathGetKeyHandler(ctx context.Context, req *logical.Request, d keyTypeParam: string(key.PrivateKeyType), } + var pkForSkid crypto.PublicKey if key.isManagedPrivateKey() { managedKeyUUID, err := key.getManagedKeyUUID() if err != nil { @@ -166,12 +169,28 @@ func (b *backend) pathGetKeyHandler(ctx context.Context, req *logical.Request, d return nil, errutil.InternalError{Err: fmt.Sprintf("failed fetching managed key info from key id %s (%s): %v", key.ID, key.Name, err)} } + pkForSkid, err = getManagedKeyPublicKey(sc.Context, sc.Backend, managedKeyUUID) + if err != nil { + return nil, err + } + // To remain consistent across the api responses (mainly generate root/intermediate calls), return the actual // type of key, not that it is a managed key. respData[keyTypeParam] = string(keyInfo.keyType) respData[managedKeyIdArg] = string(keyInfo.uuid) respData[managedKeyNameArg] = string(keyInfo.name) + } else { + pkForSkid, err = getPublicKeyFromBytes([]byte(key.PrivateKey)) + if err != nil { + return nil, err + } + } + + skid, err := certutil.GetSubjectKeyID(pkForSkid) + if err != nil { + return nil, err } + respData[skidParam] = certutil.GetHexFormatted([]byte(skid), ":") return &logical.Response{Data: respData}, nil } diff --git a/changelog/20642.txt b/changelog/20642.txt new file mode 100644 index 000000000000..8b8bc40a112b --- /dev/null +++ b/changelog/20642.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/pki: add subject key identifier to read key response +``` diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index a600c124ee9c..547508f6838b 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -123,7 +123,7 @@ func GetSubjKeyID(privateKey crypto.Signer) ([]byte, error) { if privateKey == nil { return nil, errutil.InternalError{Err: "passed-in private key is nil"} } - return getSubjectKeyID(privateKey.Public()) + return GetSubjectKeyID(privateKey.Public()) } // Returns the explicit SKID when used for cross-signing, else computes a new @@ -133,10 +133,10 @@ func getSubjectKeyIDFromBundle(data *CreationBundle) ([]byte, error) { return data.Params.SKID, nil } - return getSubjectKeyID(data.CSR.PublicKey) + return GetSubjectKeyID(data.CSR.PublicKey) } -func getSubjectKeyID(pub interface{}) ([]byte, error) { +func GetSubjectKeyID(pub interface{}) ([]byte, error) { var publicKeyBytes []byte switch pub := pub.(type) { case *rsa.PublicKey: