Protect against key and issuer name re-use (#15481)

* Protect against key and issuer name re-use - While importing keys and issuers verify that the provided name if any has not been used by another key that we did not match against. - Validate an assumption within the key import api, that we were provided a single key - Add additional tests on the new key generation and key import handlers. * Protect key import api end-users from using "default" as a name - Do not allow end-users to provide the value of default as a name for key imports as that would lead to weird and wonderful behaviors to the end-user. * Add missing api-docs for PKI key import
2022-05-18 10:31:39 -04:00 · 2022-05-18 10:31:39 -04:00 · 7bc9cd2867
parent 5ca7065bda
commit 7bc9cd2867
5 changed files with 318 additions and 11 deletions
--- a/builtin/logical/pki/path_manage_issuers.go
+++ b/builtin/logical/pki/path_manage_issuers.go
@ -8,7 +8,6 @@ import (
 	"strings"

 	"github.com/hashicorp/vault/sdk/framework"
-	"github.com/hashicorp/vault/sdk/helper/errutil"
 	"github.com/hashicorp/vault/sdk/logical"
 )

@ -166,7 +165,7 @@ func (b *backend) pathImportIssuers(ctx context.Context, req *logical.Request, d
 	for len(bytes.TrimSpace(pemBytes)) > 0 {
 		pemBlock, pemBytes = pem.Decode(pemBytes)
 		if pemBlock == nil {
-			return nil, errutil.UserError{Err: "no data found in PEM block"}
+			return logical.ErrorResponse("provided PEM block contained no data"), nil
 		}

 		pemBlockString := string(pem.EncodeToMemory(pemBlock))
--- a/builtin/logical/pki/path_manage_keys.go
+++ b/builtin/logical/pki/path_manage_keys.go
@ -1,7 +1,9 @@
 package pki

 import (
+	"bytes"
 	"context"
+	"encoding/pem"
 	"strings"

 	"github.com/hashicorp/vault/sdk/framework"
@ -80,7 +82,7 @@ func (b *backend) pathGenerateKeyHandler(ctx context.Context, req *logical.Reque

 	keyName, err := getKeyName(ctx, req.Storage, data)
 	if err != nil { // Fail Immediately if Key Name is in Use, etc...
-		return nil, err
+		return logical.ErrorResponse(err.Error()), nil
 	}

 	exportPrivateKey := false
@ -173,7 +175,7 @@ func pathImportKey(b *backend) *framework.Path {
 const (
 	pathImportKeyHelpSyn  = `Import the specified key.`
 	pathImportKeyHelpDesc = `This endpoint allows importing a specified issuer key from a pem bundle.
-If name is set, that will be set on the key.`
+If key_name is set, that will be set on the key, assuming the key did not exist previously.`
 )

 func (b *backend) pathImportKeyHandler(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
@ -186,14 +188,31 @@ func (b *backend) pathImportKeyHandler(ctx context.Context, req *logical.Request
 		return logical.ErrorResponse("Cannot import keys until migration has completed"), nil
 	}

-	keyValueInterface, isOk := data.GetOk("pem_bundle")
-	if !isOk {
-		return logical.ErrorResponse("keyValue must be set"), nil
+	pemBundle := data.Get("pem_bundle").(string)
+	keyName, err := getKeyName(ctx, req.Storage, data)
+	if err != nil {
+		return logical.ErrorResponse(err.Error()), nil
 	}
-	keyValue := keyValueInterface.(string)
-	keyName := data.Get(keyNameParam).(string)

-	key, existed, err := importKeyFromBytes(ctx, b, req.Storage, keyValue, keyName)
+	pemBytes := []byte(pemBundle)
+	var pemBlock *pem.Block
+
+	var keys []string
+	for len(bytes.TrimSpace(pemBytes)) > 0 {
+		pemBlock, pemBytes = pem.Decode(pemBytes)
+		if pemBlock == nil {
+			return logical.ErrorResponse("provided PEM block contained no data"), nil
+		}
+
+		pemBlockString := string(pem.EncodeToMemory(pemBlock))
+		keys = append(keys, pemBlockString)
+	}
+
+	if len(keys) != 1 {
+		return logical.ErrorResponse("only a single key can be present within the pem_bundle for importing"), nil
+	}
+
+	key, existed, err := importKeyFromBytes(ctx, b, req.Storage, keys[0], keyName)
 	if err != nil {
 		return logical.ErrorResponse(err.Error()), nil
 	}
--- a/builtin/logical/pki/path_manage_keys_test.go
+++ b/builtin/logical/pki/path_manage_keys_test.go
@ -2,14 +2,20 @@ package pki

 import (
 	"context"
+	"crypto/elliptic"
+	"crypto/rand"
+	"crypto/x509"
+	"encoding/pem"
 	"fmt"
 	"testing"

+	"github.com/hashicorp/vault/sdk/helper/certutil"
+
 	"github.com/hashicorp/vault/sdk/logical"
 	"github.com/stretchr/testify/require"
 )

-func TestPKI_PathManageKeys_GenerateKeys(t *testing.T) {
+func TestPKI_PathManageKeys_GenerateInternalKeys(t *testing.T) {
 	b, s := createBackendWithStorage(t)

 	tests := []struct {
@ -37,6 +43,8 @@ func TestPKI_PathManageKeys_GenerateKeys(t *testing.T) {
 				if keyBitParam != 0 {
 					data["key_bits"] = keyBitParam
 				}
+				keyName = genUuid() + "-" + tt.keyType + "-key-name"
+				data["key_name"] = keyName
 				resp, err := b.HandleRequest(context.Background(), &logical.Request{
 					Operation:  logical.UpdateOperation,
 					Path:       "keys/generate/internal",
@ -54,8 +62,216 @@ func TestPKI_PathManageKeys_GenerateKeys(t *testing.T) {
 					require.False(t, resp.IsError(),
 						"Got logical error response when not expecting one, "+
 							"generating key with values key_type:%s key_bits:%d key_name:%s\n%s", tt.keyType, keyBitParam, keyName, resp.Error())
+
+					// Special case our all-defaults
+					if tt.keyType == "" {
+						tt.keyType = "rsa"
+					}
+
+					require.Equal(t, tt.keyType, resp.Data["key_type"], "key_type field contained an invalid type")
+					require.NotEmpty(t, resp.Data["key_id"], "returned an empty key_id field, should never happen")
+					require.Equal(t, keyName, resp.Data["key_name"], "key name was not processed correctly")
+					require.Nil(t, resp.Data["private_key"], "private_key field should not appear in internal generation type.")
 				}
 			})
 		}
 	}
 }
+
+func TestPKI_PathManageKeys_GenerateExportedKeys(t *testing.T) {
+	// We tested a lot of the logic above within the internal test, so just make sure we honor the exported contract
+	b, s := createBackendWithStorage(t)
+
+	resp, err := b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/generate/exported",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"key_type": "ec",
+			"key_bits": 224,
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "Failed generating exported key")
+	require.NotNil(t, resp, "Got nil response generating exported key")
+	require.Equal(t, "ec", resp.Data["key_type"], "key_type field contained an invalid type")
+	require.NotEmpty(t, resp.Data["key_id"], "returned an empty key_id field, should never happen")
+	require.Empty(t, resp.Data["key_name"], "key name should have been empty but was not")
+	require.NotEmpty(t, resp.Data["private_key"], "private_key field should not be empty in exported generation type.")
+
+	// Make sure we can decode our private key as expected
+	keyData := resp.Data["private_key"].(string)
+	block, rest := pem.Decode([]byte(keyData))
+	require.Empty(t, rest, "should not have had any trailing data")
+	require.NotEmpty(t, block, "failed decoding pem block")
+
+	key, err := x509.ParseECPrivateKey(block.Bytes)
+	require.NoError(t, err, "failed parsing pem block as ec private key")
+	require.Equal(t, elliptic.P224(), key.Curve, "got unexpected curve value in returned private key")
+}
+
+func TestPKI_PathManageKeys_ImportKeyBundle(t *testing.T) {
+	b, s := createBackendWithStorage(t)
+
+	bundle1, err := certutil.CreateKeyBundle("ec", 224, rand.Reader)
+	require.NoError(t, err, "failed generating an ec key bundle")
+	bundle2, err := certutil.CreateKeyBundle("rsa", 2048, rand.Reader)
+	require.NoError(t, err, "failed generating an rsa key bundle")
+	pem1, err := bundle1.ToPrivateKeyPemString()
+	require.NoError(t, err, "failed converting ec key to pem")
+	pem2, err := bundle2.ToPrivateKeyPemString()
+	require.NoError(t, err, "failed converting rsa key to pem")
+
+	resp, err := b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/import",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"key_name":   "my-ec-key",
+			"pem_bundle": pem1,
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "Failed importing ec key")
+	require.NotNil(t, resp, "Got nil response importing ec key")
+	require.False(t, resp.IsError(), "received an error response: %v", resp.Error())
+	require.NotEmpty(t, resp.Data["key_id"], "key id for ec import response was empty")
+	require.Equal(t, "my-ec-key", resp.Data["key_name"], "key_name was incorrect for ec key")
+	require.Equal(t, certutil.ECPrivateKey, resp.Data["key_type"])
+	keyId1 := resp.Data["key_id"]
+
+	resp, err = b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/import",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"key_name":   "my-rsa-key",
+			"pem_bundle": pem2,
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "Failed importing rsa key")
+	require.NotNil(t, resp, "Got nil response importing rsa key")
+	require.False(t, resp.IsError(), "received an error response: %v", resp.Error())
+	require.NotEmpty(t, resp.Data["key_id"], "key id for rsa import response was empty")
+	require.Equal(t, "my-rsa-key", resp.Data["key_name"], "key_name was incorrect for ec key")
+	require.Equal(t, certutil.RSAPrivateKey, resp.Data["key_type"])
+	keyId2 := resp.Data["key_id"]
+
+	require.NotEqual(t, keyId1, keyId2)
+
+	// Attempt to reimport the same key with a different name.
+	resp, err = b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/import",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"key_name":   "my-new-ec-key",
+			"pem_bundle": pem1,
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "Failed importing the same ec key")
+	require.NotNil(t, resp, "Got nil response importing the same ec key")
+	require.False(t, resp.IsError(), "received an error response: %v", resp.Error())
+	require.NotEmpty(t, resp.Data["key_id"], "key id for ec import response was empty")
+	// Note we should receive back the original name, not the new updated name.
+	require.Equal(t, "my-ec-key", resp.Data["key_name"], "key_name was incorrect for ec key")
+	require.Equal(t, certutil.ECPrivateKey, resp.Data["key_type"])
+	keyIdReimport := resp.Data["key_id"]
+	require.Equal(t, keyId1, keyIdReimport, "the re-imported key did not return the same key id")
+
+	// Make sure we can not reuse an existing name across different keys.
+	bundle3, err := certutil.CreateKeyBundle("ec", 224, rand.Reader)
+	require.NoError(t, err, "failed generating an ec key bundle")
+	pem3, err := bundle3.ToPrivateKeyPemString()
+	require.NoError(t, err, "failed converting rsa key to pem")
+	resp, err = b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/import",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"key_name":   "my-ec-key",
+			"pem_bundle": pem3,
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "Failed importing the same ec key")
+	require.NotNil(t, resp, "Got nil response importing the same ec key")
+	require.True(t, resp.IsError(), "should have received an error response importing a key with a re-used name")
+
+	keys, _ := listKeys(context.Background(), s)
+	for _, keyId := range keys {
+		key, _ := fetchKeyById(context.Background(), s, keyId)
+		t.Logf("%s:%s", key.ID, key.Name)
+	}
+}
+
+func TestPKI_PathManageKeys_ImportKeyBundleBadData(t *testing.T) {
+	b, s := createBackendWithStorage(t)
+
+	resp, err := b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/import",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"key_name":   "my-ec-key",
+			"pem_bundle": "this-is-not-a-pem-bundle",
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "got a 500 error type response from a bad pem bundle")
+	require.NotNil(t, resp, "Got nil response importing a bad pem bundle")
+	require.True(t, resp.IsError(), "should have received an error response importing a bad pem bundle")
+
+	// Make sure we also bomb on a proper certificate
+	bundle := genCertBundle(t, b, s)
+	resp, err = b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/import",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"pem_bundle": bundle.Certificate,
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "got a 500 error type response from a certificate pem bundle")
+	require.NotNil(t, resp, "Got nil response importing a certificate bundle")
+	require.True(t, resp.IsError(), "should have received an error response importing a certificate pem bundle")
+}
+
+func TestPKI_PathManageKeys_ImportKeyRejectsMultipleKeys(t *testing.T) {
+	b, s := createBackendWithStorage(t)
+
+	bundle1, err := certutil.CreateKeyBundle("ec", 224, rand.Reader)
+	require.NoError(t, err, "failed generating an ec key bundle")
+	bundle2, err := certutil.CreateKeyBundle("rsa", 2048, rand.Reader)
+	require.NoError(t, err, "failed generating an rsa key bundle")
+	pem1, err := bundle1.ToPrivateKeyPemString()
+	require.NoError(t, err, "failed converting ec key to pem")
+	pem2, err := bundle2.ToPrivateKeyPemString()
+	require.NoError(t, err, "failed converting rsa key to pem")
+
+	importPem := pem1 + "\n" + pem2
+
+	resp, err := b.HandleRequest(context.Background(), &logical.Request{
+		Operation: logical.UpdateOperation,
+		Path:      "keys/import",
+		Storage:   s,
+		Data: map[string]interface{}{
+			"key_name":   "my-ec-key",
+			"pem_bundle": importPem,
+		},
+		MountPoint: "pki/",
+	})
+	require.NoError(t, err, "got a 500 error type response from a bad pem bundle")
+	require.NotNil(t, resp, "Got nil response importing a bad pem bundle")
+	require.True(t, resp.IsError(), "should have received an error response importing a pem bundle with more than 1 key")
+
+	ctx := context.Background()
+	keys, _ := listKeys(ctx, s)
+	for _, keyId := range keys {
+		id, _ := fetchKeyById(ctx, s, keyId)
+		t.Logf("%s:%s", id.ID, id.Name)
+	}
+}
--- a/builtin/logical/pki/storage.go
+++ b/builtin/logical/pki/storage.go
@ -260,6 +260,7 @@ func importKey(ctx context.Context, b *backend, s logical.Storage, keyValue stri
 		}
 	}

+	foundExistingKeyWithName := false
 	for _, identifier := range knownKeys {
 		existingKey, err := fetchKeyById(ctx, s, identifier)
 		if err != nil {
@ -276,6 +277,16 @@ func importKey(ctx context.Context, b *backend, s logical.Storage, keyValue stri
 			// importing an issuer).
 			return existingKey, true, nil
 		}
+
+		// Allow us to find an existing matching key with a different name before erroring out
+		if keyName != "" && existingKey.Name == keyName {
+			foundExistingKeyWithName = true
+		}
+	}
+
+	// Another key with a different value is using the keyName so reject this request.
+	if foundExistingKeyWithName {
+		return nil, false, errutil.UserError{Err: fmt.Sprintf("an existing key is using the requested key name value: %s", keyName)}
 	}

 	// Haven't found a key, so we've gotta create it and write it into storage.
@ -538,6 +549,7 @@ func importIssuer(ctx context.Context, b *backend, s logical.Storage, certValue
 		return nil, false, err
 	}

+	foundExistingIssuerWithName := false
 	for _, identifier := range knownIssuers {
 		existingIssuer, err := fetchIssuerById(ctx, s, identifier)
 		if err != nil {
@ -553,6 +565,15 @@ func importIssuer(ctx context.Context, b *backend, s logical.Storage, certValue
 			// importing a key).
 			return existingIssuer, true, nil
 		}
+
+		// Allow us to find an existing matching issuer with a different name before erroring out
+		if issuerName != "" && existingIssuer.Name == issuerName {
+			foundExistingIssuerWithName = true
+		}
+	}
+
+	if foundExistingIssuerWithName {
+		return nil, false, errutil.UserError{Err: fmt.Sprintf("another issuer is using the requested name: %s", issuerName)}
 	}

 	// Haven't found an issuer, so we've gotta create it and write it into
--- a/website/content/api-docs/secret/pki.mdx
+++ b/website/content/api-docs/secret/pki.mdx
@ -42,6 +42,7 @@ update your API calls accordingly.
  - [Read Issuer](#read-issuer)
  - [Update Issuer](#update-issuer)
  - [Delete Issuer](#delete-issuer)
+  - [Import Key](#import-key)
  - [Read Key](#read-key)
  - [Update Key](#update-key)
  - [Delete Key](#delete-key)
@ -1781,6 +1782,57 @@ $ curl \
    http://127.0.0.1:8200/v1/pki/issuer/root-x1
 ```

+### Import Key
+
+This endpoint allows an operator to import a single pem encoded `rsa`, `ec`, or `ed25519`
+key.
+
+~> **Note**: This API does not protect against importing keys using insecure combinations of
+  algorithm and key length.
+
+| Method | Path               |
+|:-------|:-------------------|
+| `POST` | `/pki/keys/import` |
+
+#### Parameters
+
+- `pem_bundle` `(string: <required>)` - Specifies the unencrypted private key in PEM format.
+
+- `key_name` `(string: "")` - Provides a name to the specified key. The
+  name must be unique across all keys and not be the reserved value
+  `default`.
+
+#### Sample Payload
+
+```json
+{
+  "key_name": "my-imported-key",
+  "pem_bundle": "-----BEGIN RSA PRIVATE KEY-----\n...\n-----END CERTIFICATE-----"
+}
+```
+
+#### Sample Request
+
+```shell-session
+$ curl \
+    --header "X-Vault-Token: ..." \
+    --request POST \
+    --data @payload.json \
+    http://127.0.0.1:8200/v1/pki/keys/import
+```
+
+#### Sample Response
+
+```text
+{
+  "data": {
+    "key_id": "2cf03991-b052-1dc3-393e-374b41f8dcd8",
+    "key_name": "my-imported-key",
+    "key_type": "rsa"
+  },
+}
+```
+
 ### Read Key

 This endpoint allows an operator to fetch information about an existing key.