From c0ffbb21ac83da93017413dc979c593e9eeb9aca Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Wed, 17 May 2023 11:32:26 -0500 Subject: [PATCH 1/7] 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. --- builtin/logical/pki/fields.go | 1 + builtin/logical/pki/path_fetch_keys.go | 26 ++++++++++++++++++++++++++ sdk/helper/certutil/helpers.go | 6 +++--- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/builtin/logical/pki/fields.go b/builtin/logical/pki/fields.go index a8c793c365e2..47df98ac9cdd 100644 --- a/builtin/logical/pki/fields.go +++ b/builtin/logical/pki/fields.go @@ -16,6 +16,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 4cd5d884df9c..701203680cf5 100644 --- a/builtin/logical/pki/path_fetch_keys.go +++ b/builtin/logical/pki/path_fetch_keys.go @@ -8,6 +8,7 @@ import ( "fmt" "net/http" + "github.com/hashicorp/vault/sdk/helper/certutil" "github.com/hashicorp/vault/sdk/helper/errutil" "github.com/hashicorp/vault/sdk/framework" @@ -144,6 +145,11 @@ func buildPathKey(b *backend, pattern string, displayAttrs *framework.DisplayAtt Description: `Key Type`, Required: true, }, + "subject_key_id": { + Type: framework.TypeString, + Description: `Subject key identifier`, + Required: false, + }, "managed_key_id": { Type: framework.TypeString, Description: `Managed Key Id`, @@ -241,10 +247,20 @@ func (b *backend) pathGetKeyHandler(ctx context.Context, req *logical.Request, d return nil, err } + pkForSkid, err := getPublicKeyFromBytes([]byte(key.PrivateKey)) + if err != nil { + return nil, err + } + skid, err := certutil.GetSubjectKeyID(pkForSkid) + if err != nil { + return nil, err + } + respData := map[string]interface{}{ keyIdParam: key.ID, keyNameParam: key.Name, keyTypeParam: string(key.PrivateKeyType), + skidParam: certutil.GetHexFormatted([]byte(skid), ":"), } if key.isManagedPrivateKey() { @@ -258,6 +274,16 @@ 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)} } + pk, err := getManagedKeyPublicKey(sc.Context, sc.Backend, managedKeyUUID) + if err != nil { + return nil, err + } + skid, err := certutil.GetSubjectKeyID(pk) + if err != nil { + return nil, err + } + respData[skidParam] = skid + // 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) diff --git a/sdk/helper/certutil/helpers.go b/sdk/helper/certutil/helpers.go index ab1aecd5a0a4..c0128b0e62ec 100644 --- a/sdk/helper/certutil/helpers.go +++ b/sdk/helper/certutil/helpers.go @@ -126,7 +126,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 @@ -136,10 +136,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: From f34ece1bd0f7028e3c7ac02247f55b616e932137 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Wed, 17 May 2023 16:56:52 -0500 Subject: [PATCH 2/7] add changelog --- changelog/20642.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/20642.txt 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 +``` From 0f4445141cb7fe96cb00e81453a2f5dd70363eac Mon Sep 17 00:00:00 2001 From: John-Michael Faircloth Date: Thu, 18 May 2023 09:03:41 -0500 Subject: [PATCH 3/7] Update builtin/logical/pki/path_fetch_keys.go Co-authored-by: Alexander Scheel --- builtin/logical/pki/path_fetch_keys.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/logical/pki/path_fetch_keys.go b/builtin/logical/pki/path_fetch_keys.go index 701203680cf5..bbc41d3f0302 100644 --- a/builtin/logical/pki/path_fetch_keys.go +++ b/builtin/logical/pki/path_fetch_keys.go @@ -147,7 +147,7 @@ func buildPathKey(b *backend, pattern string, displayAttrs *framework.DisplayAtt }, "subject_key_id": { Type: framework.TypeString, - Description: `Subject key identifier`, + Description: `RFC 5280 Subject Key Identifier of the public counterpart`, Required: false, }, "managed_key_id": { From 9365c084c36b1cfe31ae7e688c80b1c412f6f1d6 Mon Sep 17 00:00:00 2001 From: JM Faircloth Date: Thu, 18 May 2023 09:56:40 -0500 Subject: [PATCH 4/7] check for managed key first --- builtin/logical/pki/path_fetch_keys.go | 30 ++++++++++++-------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/builtin/logical/pki/path_fetch_keys.go b/builtin/logical/pki/path_fetch_keys.go index bbc41d3f0302..23b3bf58dcb2 100644 --- a/builtin/logical/pki/path_fetch_keys.go +++ b/builtin/logical/pki/path_fetch_keys.go @@ -5,6 +5,7 @@ package pki import ( "context" + "crypto" "fmt" "net/http" @@ -247,22 +248,13 @@ func (b *backend) pathGetKeyHandler(ctx context.Context, req *logical.Request, d return nil, err } - pkForSkid, err := getPublicKeyFromBytes([]byte(key.PrivateKey)) - if err != nil { - return nil, err - } - skid, err := certutil.GetSubjectKeyID(pkForSkid) - if err != nil { - return nil, err - } - respData := map[string]interface{}{ keyIdParam: key.ID, keyNameParam: key.Name, keyTypeParam: string(key.PrivateKeyType), - skidParam: certutil.GetHexFormatted([]byte(skid), ":"), } + var pkForSkid crypto.PublicKey if key.isManagedPrivateKey() { managedKeyUUID, err := key.getManagedKeyUUID() if err != nil { @@ -274,22 +266,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)} } - pk, err := getManagedKeyPublicKey(sc.Context, sc.Backend, managedKeyUUID) + pkForSkid, err = getManagedKeyPublicKey(sc.Context, sc.Backend, managedKeyUUID) if err != nil { return nil, err } - skid, err := certutil.GetSubjectKeyID(pk) - if err != nil { - return nil, err - } - respData[skidParam] = skid // 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 } From 7afe9f7a16bd3d7da55cd374af850157f29f0128 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Thu, 18 May 2023 11:32:38 -0400 Subject: [PATCH 5/7] Validate the SKID matches on root CAs Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 3f98edab62ab..80d010e0ce2b 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2400,6 +2400,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) @@ -2414,6 +2422,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) From c81ccd12eddde1903ecbd59a6b841e564c48ddad Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Thu, 18 May 2023 11:40:37 -0400 Subject: [PATCH 6/7] Validate SKID matches on int CAs Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 80d010e0ce2b..544c5c2904e8 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2559,13 +2559,19 @@ func TestBackend_SignIntermediate_AllowedPastCA(t *testing.T) { "common_name": "myint.com", }) schema.ValidateResponse(t, schema.GetResponseSchema(t, b_root.Route("intermediate/generate/internal"), logical.UpdateOperation), resp, true) + 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, @@ -2600,6 +2606,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) { From 88c26c0d56e7de6d4d51e62f4eabae5576597b01 Mon Sep 17 00:00:00 2001 From: Alexander Scheel Date: Thu, 18 May 2023 12:17:35 -0400 Subject: [PATCH 7/7] Fix formatting of tests Signed-off-by: Alexander Scheel --- builtin/logical/pki/backend_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 544c5c2904e8..ec65ab237bf2 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -2404,7 +2404,7 @@ func TestBackend_Root_Idempotency(t *testing.T) { 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()) + 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) @@ -2426,7 +2426,7 @@ func TestBackend_Root_Idempotency(t *testing.T) { 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()) + 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) @@ -2563,7 +2563,7 @@ func TestBackend_SignIntermediate_AllowedPastCA(t *testing.T) { intKeyId := resp.Data["key_id"].(keyID) csr := resp.Data["csr"] - resp, err = CBRead(b_int, s_int, "key/" + intKeyId.String()) + 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)