From 8759d95eb8dccd87e49bfc2bba4c118d1a0f84dd 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: Tue, 14 Nov 2023 10:14:47 -0500 Subject: [PATCH] Address a panic when exporting RSA public keys in transit (#24054) (#24116) * Address a panic export RSA public keys in transit - When attempting to export the public key for an RSA key that we only have a private key for, the export panics with a nil deference. - Add additional tests around Transit key exporting * Add cl Co-authored-by: Steven Clark --- builtin/logical/transit/path_export.go | 28 +++-- builtin/logical/transit/path_export_test.go | 110 +++++++++++++++++++- builtin/logical/transit/path_keys.go | 25 +---- changelog/24054.txt | 3 + 4 files changed, 135 insertions(+), 31 deletions(-) create mode 100644 changelog/24054.txt diff --git a/builtin/logical/transit/path_export.go b/builtin/logical/transit/path_export.go index 9239aee94..6cfd7d0ed 100644 --- a/builtin/logical/transit/path_export.go +++ b/builtin/logical/transit/path_export.go @@ -5,6 +5,7 @@ package transit import ( "context" + "crypto" "crypto/ecdsa" "crypto/elliptic" "crypto/x509" @@ -273,18 +274,31 @@ func encodeRSAPublicKey(key *keysutil.KeyEntry) (string, error) { return "", errors.New("nil KeyEntry provided") } - blockType := "RSA PUBLIC KEY" - derBytes, err := x509.MarshalPKIXPublicKey(key.RSAPublicKey) - if err != nil { - return "", err + var publicKey crypto.PublicKey + publicKey = key.RSAPublicKey + if key.RSAKey != nil { + // Prefer the private key if it exists + publicKey = key.RSAKey.Public() } - pemBlock := pem.Block{ - Type: blockType, + if publicKey == nil { + return "", errors.New("requested to encode an RSA public key with no RSA key present") + } + + // Encode the RSA public key in PEM format to return over the API + derBytes, err := x509.MarshalPKIXPublicKey(publicKey) + if err != nil { + return "", fmt.Errorf("error marshaling RSA public key: %w", err) + } + pemBlock := &pem.Block{ + Type: "PUBLIC KEY", Bytes: derBytes, } + pemBytes := pem.EncodeToMemory(pemBlock) + if pemBytes == nil || len(pemBytes) == 0 { + return "", fmt.Errorf("failed to PEM-encode RSA public key") + } - pemBytes := pem.EncodeToMemory(&pemBlock) return string(pemBytes), nil } diff --git a/builtin/logical/transit/path_export_test.go b/builtin/logical/transit/path_export_test.go index cd25383f2..5eb6eeaf1 100644 --- a/builtin/logical/transit/path_export_test.go +++ b/builtin/logical/transit/path_export_test.go @@ -1,5 +1,5 @@ // Copyright (c) HashiCorp, Inc. -// SPDX-License-Identifier: MPL-2.0 +// SPDX-License-Identifier: BUSL-1.1 package transit @@ -8,19 +8,64 @@ import ( "fmt" "reflect" "strconv" + "strings" "testing" "github.com/hashicorp/vault/sdk/logical" ) +func TestTransit_Export_Unknown_ExportType(t *testing.T) { + t.Parallel() + + b, storage := createBackendWithSysView(t) + keyType := "ed25519" + req := &logical.Request{ + Storage: storage, + Operation: logical.UpdateOperation, + Path: "keys/foo", + Data: map[string]interface{}{ + "exportable": true, + "type": keyType, + }, + } + _, err := b.HandleRequest(context.Background(), req) + if err != nil { + t.Fatalf("failed creating key %s: %v", keyType, err) + } + + req = &logical.Request{ + Storage: storage, + Operation: logical.ReadOperation, + Path: "export/bad-export-type/foo", + } + rsp, err := b.HandleRequest(context.Background(), req) + if err == nil { + t.Fatalf("did not error on bad export type got: %v", rsp) + } + if rsp == nil || !rsp.IsError() { + t.Fatalf("response did not contain an error on bad export type got: %v", rsp) + } + if !strings.Contains(rsp.Error().Error(), "invalid export type") { + t.Fatalf("failed with unexpected error: %v", err) + } +} + func TestTransit_Export_KeyVersion_ExportsCorrectVersion(t *testing.T) { + t.Parallel() + verifyExportsCorrectVersion(t, "encryption-key", "aes128-gcm96") verifyExportsCorrectVersion(t, "encryption-key", "aes256-gcm96") verifyExportsCorrectVersion(t, "encryption-key", "chacha20-poly1305") + verifyExportsCorrectVersion(t, "encryption-key", "rsa-2048") + verifyExportsCorrectVersion(t, "encryption-key", "rsa-3072") + verifyExportsCorrectVersion(t, "encryption-key", "rsa-4096") verifyExportsCorrectVersion(t, "signing-key", "ecdsa-p256") verifyExportsCorrectVersion(t, "signing-key", "ecdsa-p384") verifyExportsCorrectVersion(t, "signing-key", "ecdsa-p521") verifyExportsCorrectVersion(t, "signing-key", "ed25519") + verifyExportsCorrectVersion(t, "signing-key", "rsa-2048") + verifyExportsCorrectVersion(t, "signing-key", "rsa-3072") + verifyExportsCorrectVersion(t, "signing-key", "rsa-4096") verifyExportsCorrectVersion(t, "hmac-key", "aes128-gcm96") verifyExportsCorrectVersion(t, "hmac-key", "aes256-gcm96") verifyExportsCorrectVersion(t, "hmac-key", "chacha20-poly1305") @@ -29,6 +74,13 @@ func TestTransit_Export_KeyVersion_ExportsCorrectVersion(t *testing.T) { verifyExportsCorrectVersion(t, "hmac-key", "ecdsa-p521") verifyExportsCorrectVersion(t, "hmac-key", "ed25519") verifyExportsCorrectVersion(t, "hmac-key", "hmac") + verifyExportsCorrectVersion(t, "public-key", "rsa-2048") + verifyExportsCorrectVersion(t, "public-key", "rsa-3072") + verifyExportsCorrectVersion(t, "public-key", "rsa-4096") + verifyExportsCorrectVersion(t, "public-key", "ecdsa-p256") + verifyExportsCorrectVersion(t, "public-key", "ecdsa-p384") + verifyExportsCorrectVersion(t, "public-key", "ecdsa-p521") + verifyExportsCorrectVersion(t, "public-key", "ed25519") } func verifyExportsCorrectVersion(t *testing.T, exportType, keyType string) { @@ -125,6 +177,8 @@ func verifyExportsCorrectVersion(t *testing.T, exportType, keyType string) { } func TestTransit_Export_ValidVersionsOnly(t *testing.T) { + t.Parallel() + b, storage := createBackendWithSysView(t) // First create a key, v1 @@ -225,6 +279,8 @@ func TestTransit_Export_ValidVersionsOnly(t *testing.T) { } func TestTransit_Export_KeysNotMarkedExportable_ReturnsError(t *testing.T) { + t.Parallel() + b, storage := createBackendWithSysView(t) req := &logical.Request{ @@ -255,6 +311,8 @@ func TestTransit_Export_KeysNotMarkedExportable_ReturnsError(t *testing.T) { } func TestTransit_Export_SigningDoesNotSupportSigning_ReturnsError(t *testing.T) { + t.Parallel() + b, storage := createBackendWithSysView(t) req := &logical.Request{ @@ -283,6 +341,8 @@ func TestTransit_Export_SigningDoesNotSupportSigning_ReturnsError(t *testing.T) } func TestTransit_Export_EncryptionDoesNotSupportEncryption_ReturnsError(t *testing.T) { + t.Parallel() + testTransit_Export_EncryptionDoesNotSupportEncryption_ReturnsError(t, "ecdsa-p256") testTransit_Export_EncryptionDoesNotSupportEncryption_ReturnsError(t, "ecdsa-p384") testTransit_Export_EncryptionDoesNotSupportEncryption_ReturnsError(t, "ecdsa-p521") @@ -313,11 +373,55 @@ func testTransit_Export_EncryptionDoesNotSupportEncryption_ReturnsError(t *testi } _, err = b.HandleRequest(context.Background(), req) if err == nil { - t.Fatal("Key does not support encryption but was exported without error.") + t.Fatalf("Key %s does not support encryption but was exported without error.", keyType) + } +} + +func TestTransit_Export_PublicKeyDoesNotSupportEncryption_ReturnsError(t *testing.T) { + t.Parallel() + + testTransit_Export_PublicKeyNotSupported_ReturnsError(t, "chacha20-poly1305") + testTransit_Export_PublicKeyNotSupported_ReturnsError(t, "aes128-gcm96") + testTransit_Export_PublicKeyNotSupported_ReturnsError(t, "aes256-gcm96") + testTransit_Export_PublicKeyNotSupported_ReturnsError(t, "hmac") +} + +func testTransit_Export_PublicKeyNotSupported_ReturnsError(t *testing.T, keyType string) { + b, storage := createBackendWithSysView(t) + + req := &logical.Request{ + Storage: storage, + Operation: logical.UpdateOperation, + Path: "keys/foo", + Data: map[string]interface{}{ + "type": keyType, + }, + } + if keyType == "hmac" { + req.Data["key_size"] = 32 + } + _, err := b.HandleRequest(context.Background(), req) + if err != nil { + t.Fatalf("failed creating key %s: %v", keyType, err) + } + + req = &logical.Request{ + Storage: storage, + Operation: logical.ReadOperation, + Path: "export/public-key/foo", + } + _, err = b.HandleRequest(context.Background(), req) + if err == nil { + t.Fatalf("Key %s does not support public key exporting but was exported without error.", keyType) + } + if !strings.Contains(err.Error(), fmt.Sprintf("unknown key type %s for export type public-key", keyType)) { + t.Fatalf("unexpected error value for key type: %s: %v", keyType, err) } } func TestTransit_Export_KeysDoesNotExist_ReturnsNotFound(t *testing.T) { + t.Parallel() + b, storage := createBackendWithSysView(t) req := &logical.Request{ @@ -333,6 +437,8 @@ func TestTransit_Export_KeysDoesNotExist_ReturnsNotFound(t *testing.T) { } func TestTransit_Export_EncryptionKey_DoesNotExportHMACKey(t *testing.T) { + t.Parallel() + b, storage := createBackendWithSysView(t) req := &logical.Request{ diff --git a/builtin/logical/transit/path_keys.go b/builtin/logical/transit/path_keys.go index 285f32d33..89334a6f8 100644 --- a/builtin/logical/transit/path_keys.go +++ b/builtin/logical/transit/path_keys.go @@ -5,11 +5,8 @@ package transit import ( "context" - "crypto" "crypto/elliptic" - "crypto/x509" "encoding/base64" - "encoding/pem" "fmt" "strconv" "time" @@ -415,27 +412,11 @@ func (b *backend) formatKeyPolicy(p *keysutil.Policy, context []byte) (*logical. key.Name = "rsa-4096" } - var publicKey crypto.PublicKey - publicKey = v.RSAPublicKey - if !v.IsPrivateKeyMissing() { - publicKey = v.RSAKey.Public() - } - - // Encode the RSA public key in PEM format to return over the - // API - derBytes, err := x509.MarshalPKIXPublicKey(publicKey) + pubKey, err := encodeRSAPublicKey(&v) if err != nil { - return nil, fmt.Errorf("error marshaling RSA public key: %w", err) + return nil, err } - pemBlock := &pem.Block{ - Type: "PUBLIC KEY", - Bytes: derBytes, - } - pemBytes := pem.EncodeToMemory(pemBlock) - if pemBytes == nil || len(pemBytes) == 0 { - return nil, fmt.Errorf("failed to PEM-encode RSA public key") - } - key.PublicKey = string(pemBytes) + key.PublicKey = pubKey } retKeys[k] = structs.New(key).Map() diff --git a/changelog/24054.txt b/changelog/24054.txt new file mode 100644 index 000000000..2680d114c --- /dev/null +++ b/changelog/24054.txt @@ -0,0 +1,3 @@ +```release-note:bug +secrets/transit: Fix a panic when attempting to export a public RSA key +```