From 736e6ec3ee81dfc1b9fbba5a1820d0a474be7588 Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Wed, 8 Dec 2021 10:33:22 -0500 Subject: [PATCH 1/3] Warn user supplying nonce values in FIPS mode for transit encryption requests - Send back a warning within the response if an end-user supplies nonce values that we use within the various transit encrypt apis. - We do not send a warning if an end-user supplies a nonce value but we don't use it. - Affected api methods are encrypt, rewrap and datakey - The warning is only sent when we are operating in FIPS mode. --- builtin/logical/transit/path_datakey.go | 6 +- builtin/logical/transit/path_encrypt.go | 38 ++++++++++ builtin/logical/transit/path_encrypt_test.go | 80 ++++++++++++++++++++ builtin/logical/transit/path_rewrap.go | 11 ++- sdk/helper/consts/fips.go | 9 +++ 5 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 sdk/helper/consts/fips.go diff --git a/builtin/logical/transit/path_datakey.go b/builtin/logical/transit/path_datakey.go index 9e9ef2c17340..13759dd77e7b 100644 --- a/builtin/logical/transit/path_datakey.go +++ b/builtin/logical/transit/path_datakey.go @@ -5,8 +5,8 @@ import ( "crypto/rand" "encoding/base64" "fmt" - "github.com/hashicorp/vault/sdk/framework" + "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/helper/errutil" "github.com/hashicorp/vault/sdk/helper/keysutil" "github.com/hashicorp/vault/sdk/logical" @@ -159,6 +159,10 @@ func (b *backend) pathDatakeyWrite(ctx context.Context, req *logical.Request, d }, } + if consts.IsFIPS() && shouldWarnAboutNonceUsage(p, nonce) { + resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS standards.") + } + if plaintextAllowed { resp.Data["plaintext"] = base64.StdEncoding.EncodeToString(newKey) } diff --git a/builtin/logical/transit/path_encrypt.go b/builtin/logical/transit/path_encrypt.go index deb564b12926..23f3bd5a7594 100644 --- a/builtin/logical/transit/path_encrypt.go +++ b/builtin/logical/transit/path_encrypt.go @@ -5,6 +5,7 @@ import ( "encoding/base64" "encoding/json" "fmt" + "github.com/hashicorp/vault/sdk/helper/consts" "net/http" "reflect" @@ -357,11 +358,16 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d // Process batch request items. If encryption of any request // item fails, respectively mark the error in the response // collection and continue to process other items. + warnAboutNonceUsage := false for i, item := range batchInputItems { if batchResponseItems[i].Error != "" { continue } + if !warnAboutNonceUsage && shouldWarnAboutNonceUsage(p, item.DecodedNonce) { + warnAboutNonceUsage = true + } + ciphertext, err := p.Encrypt(item.KeyVersion, item.DecodedContext, item.DecodedNonce, item.Plaintext) if err != nil { switch err.(type) { @@ -411,6 +417,10 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d } } + if consts.IsFIPS() && warnAboutNonceUsage { + resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS standards.") + } + if req.Operation == logical.CreateOperation && !upserted { resp.AddWarning("Attempted creation of the key during the encrypt operation, but it was created beforehand") } @@ -431,6 +441,34 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d return resp, nil } +// shouldWarnAboutNonceUsage attempts to determine if we will use a provided nonce or not. Ideally this +// would be information returned through p.Encrypt but that would require an SDK api change and this is +// transit specific +func shouldWarnAboutNonceUsage(p *keysutil.Policy, userSuppliedNonce []byte) bool { + if len(userSuppliedNonce) == 0 { + return false + } + + var supportedKeyType bool + switch p.Type { + case keysutil.KeyType_AES128_GCM96, keysutil.KeyType_AES256_GCM96, keysutil.KeyType_ChaCha20_Poly1305: + supportedKeyType = true + default: + supportedKeyType = false + } + + if supportedKeyType && p.ConvergentEncryption && p.ConvergentVersion == 1 { + // We only use the user supplied nonce for v1 convergent encryption keys + return true + } + + if supportedKeyType && !p.ConvergentEncryption { + return true + } + + return false +} + const pathEncryptHelpSyn = `Encrypt a plaintext value or a batch of plaintext blocks using a named key` diff --git a/builtin/logical/transit/path_encrypt_test.go b/builtin/logical/transit/path_encrypt_test.go index 1842e069a513..227ff4cf49ca 100644 --- a/builtin/logical/transit/path_encrypt_test.go +++ b/builtin/logical/transit/path_encrypt_test.go @@ -3,6 +3,7 @@ package transit import ( "context" "encoding/json" + "github.com/hashicorp/vault/sdk/helper/keysutil" "reflect" "strings" "testing" @@ -729,3 +730,82 @@ func TestTransit_decodeBatchRequestItems(t *testing.T) { }) } } + +func TestShouldWarnAboutNonceUsage(t *testing.T) { + tests := []struct { + name string + keyTypes []keysutil.KeyType + nonce []byte + convergentEncryption bool + convergentVersion int + expected bool + }{ + { + name: "-NoConvergent-WithNonce", + keyTypes: []keysutil.KeyType{keysutil.KeyType_AES256_GCM96, keysutil.KeyType_AES128_GCM96, keysutil.KeyType_ChaCha20_Poly1305}, + nonce: []byte("testnonce"), + convergentEncryption: false, + convergentVersion: -1, + expected: true, + }, + { + name: "-NoConvergent-NoNonce", + keyTypes: []keysutil.KeyType{keysutil.KeyType_AES256_GCM96, keysutil.KeyType_AES128_GCM96, keysutil.KeyType_ChaCha20_Poly1305}, + nonce: []byte{}, + convergentEncryption: false, + convergentVersion: -1, + expected: false, + }, + { + name: "-Convergentv1-WithNonce", + keyTypes: []keysutil.KeyType{keysutil.KeyType_AES256_GCM96, keysutil.KeyType_AES128_GCM96, keysutil.KeyType_ChaCha20_Poly1305}, + nonce: []byte("testnonce"), + convergentEncryption: true, + convergentVersion: 1, + expected: true, + }, + { + name: "-Convergentv2-WithNonce", + keyTypes: []keysutil.KeyType{keysutil.KeyType_AES256_GCM96, keysutil.KeyType_AES128_GCM96, keysutil.KeyType_ChaCha20_Poly1305}, + nonce: []byte("testnonce"), + convergentEncryption: true, + convergentVersion: 2, + expected: false, + }, + { + name: "-Convergentv3-WithNonce", + keyTypes: []keysutil.KeyType{keysutil.KeyType_AES256_GCM96, keysutil.KeyType_AES128_GCM96, keysutil.KeyType_ChaCha20_Poly1305}, + nonce: []byte("testnonce"), + convergentEncryption: true, + convergentVersion: 3, + expected: false, + }, + { + name: "-NoConvergent-WithNonce", + keyTypes: []keysutil.KeyType{keysutil.KeyType_RSA2048, keysutil.KeyType_RSA4096}, + nonce: []byte("testnonce"), + convergentEncryption: false, + convergentVersion: -1, + expected: false, + }, + } + + for _, tt := range tests { + for _, keyType := range tt.keyTypes { + testName := keyType.String() + tt.name + t.Run(testName, func(t *testing.T) { + p := keysutil.Policy{ + ConvergentEncryption: tt.convergentEncryption, + ConvergentVersion: tt.convergentVersion, + Type: keyType, + } + + actual := shouldWarnAboutNonceUsage(&p, tt.nonce) + + if actual != tt.expected { + t.Errorf("Expected actual '%v' but got '%v'", tt.expected, actual) + } + }) + } + } +} diff --git a/builtin/logical/transit/path_rewrap.go b/builtin/logical/transit/path_rewrap.go index c32fddc99976..894fed1da69f 100644 --- a/builtin/logical/transit/path_rewrap.go +++ b/builtin/logical/transit/path_rewrap.go @@ -4,8 +4,8 @@ import ( "context" "encoding/base64" "fmt" - "github.com/hashicorp/vault/sdk/framework" + "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/helper/errutil" "github.com/hashicorp/vault/sdk/helper/keysutil" "github.com/hashicorp/vault/sdk/logical" @@ -128,6 +128,7 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * p.Lock(false) } + warnAboutNonceUsage := false for i, item := range batchInputItems { if batchResponseItems[i].Error != "" { continue @@ -145,6 +146,10 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * } } + if !warnAboutNonceUsage && shouldWarnAboutNonceUsage(p, item.DecodedNonce) { + warnAboutNonceUsage = true + } + ciphertext, err := p.Encrypt(item.KeyVersion, item.DecodedContext, item.DecodedNonce, plaintext) if err != nil { switch err.(type) { @@ -190,6 +195,10 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * } } + if consts.IsFIPS() && warnAboutNonceUsage { + resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS standards.") + } + p.Unlock() return resp, nil } diff --git a/sdk/helper/consts/fips.go b/sdk/helper/consts/fips.go new file mode 100644 index 000000000000..86ba2af2fa6e --- /dev/null +++ b/sdk/helper/consts/fips.go @@ -0,0 +1,9 @@ +//go:build !fips_140_3 +// +build !fips_140_3 + +package consts + +// IsFIPS returns true if Vault is operating in a FIPS-140-{2,3} mode. +func IsFIPS() bool { + return false +} From e106f3b9bfa0eac9924bdd50c0c89d2db896b1af Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Wed, 8 Dec 2021 13:35:51 -0500 Subject: [PATCH 2/3] Review feedback --- builtin/logical/transit/path_datakey.go | 2 +- builtin/logical/transit/path_encrypt.go | 2 +- builtin/logical/transit/path_rewrap.go | 2 +- sdk/helper/consts/fips.go | 1 - 4 files changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/logical/transit/path_datakey.go b/builtin/logical/transit/path_datakey.go index 13759dd77e7b..754468c6f3ff 100644 --- a/builtin/logical/transit/path_datakey.go +++ b/builtin/logical/transit/path_datakey.go @@ -160,7 +160,7 @@ func (b *backend) pathDatakeyWrite(ctx context.Context, req *logical.Request, d } if consts.IsFIPS() && shouldWarnAboutNonceUsage(p, nonce) { - resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS standards.") + resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } if plaintextAllowed { diff --git a/builtin/logical/transit/path_encrypt.go b/builtin/logical/transit/path_encrypt.go index 23f3bd5a7594..1af88dada3bf 100644 --- a/builtin/logical/transit/path_encrypt.go +++ b/builtin/logical/transit/path_encrypt.go @@ -418,7 +418,7 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d } if consts.IsFIPS() && warnAboutNonceUsage { - resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS standards.") + resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } if req.Operation == logical.CreateOperation && !upserted { diff --git a/builtin/logical/transit/path_rewrap.go b/builtin/logical/transit/path_rewrap.go index 894fed1da69f..5ea96ef62b24 100644 --- a/builtin/logical/transit/path_rewrap.go +++ b/builtin/logical/transit/path_rewrap.go @@ -196,7 +196,7 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * } if consts.IsFIPS() && warnAboutNonceUsage { - resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS standards.") + resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } p.Unlock() diff --git a/sdk/helper/consts/fips.go b/sdk/helper/consts/fips.go index 86ba2af2fa6e..5f436147f97b 100644 --- a/sdk/helper/consts/fips.go +++ b/sdk/helper/consts/fips.go @@ -1,4 +1,3 @@ -//go:build !fips_140_3 // +build !fips_140_3 package consts From 941986981f19bae2de12f451cb0e9613b589005c Mon Sep 17 00:00:00 2001 From: Steve Clark Date: Wed, 8 Dec 2021 14:07:25 -0500 Subject: [PATCH 3/3] Move fips helper into helper/constants outside of sdk --- builtin/logical/transit/path_datakey.go | 4 ++-- builtin/logical/transit/path_encrypt.go | 4 ++-- builtin/logical/transit/path_rewrap.go | 4 ++-- {sdk/helper/consts => helper/constants}/fips.go | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) rename {sdk/helper/consts => helper/constants}/fips.go (88%) diff --git a/builtin/logical/transit/path_datakey.go b/builtin/logical/transit/path_datakey.go index 754468c6f3ff..a9c1f426d928 100644 --- a/builtin/logical/transit/path_datakey.go +++ b/builtin/logical/transit/path_datakey.go @@ -5,8 +5,8 @@ import ( "crypto/rand" "encoding/base64" "fmt" + "github.com/hashicorp/vault/helper/constants" "github.com/hashicorp/vault/sdk/framework" - "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/helper/errutil" "github.com/hashicorp/vault/sdk/helper/keysutil" "github.com/hashicorp/vault/sdk/logical" @@ -159,7 +159,7 @@ func (b *backend) pathDatakeyWrite(ctx context.Context, req *logical.Request, d }, } - if consts.IsFIPS() && shouldWarnAboutNonceUsage(p, nonce) { + if constants.IsFIPS() && shouldWarnAboutNonceUsage(p, nonce) { resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } diff --git a/builtin/logical/transit/path_encrypt.go b/builtin/logical/transit/path_encrypt.go index 1af88dada3bf..ac683dadd17e 100644 --- a/builtin/logical/transit/path_encrypt.go +++ b/builtin/logical/transit/path_encrypt.go @@ -5,7 +5,7 @@ import ( "encoding/base64" "encoding/json" "fmt" - "github.com/hashicorp/vault/sdk/helper/consts" + "github.com/hashicorp/vault/helper/constants" "net/http" "reflect" @@ -417,7 +417,7 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d } } - if consts.IsFIPS() && warnAboutNonceUsage { + if constants.IsFIPS() && warnAboutNonceUsage { resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } diff --git a/builtin/logical/transit/path_rewrap.go b/builtin/logical/transit/path_rewrap.go index 5ea96ef62b24..04c38d2f8eb6 100644 --- a/builtin/logical/transit/path_rewrap.go +++ b/builtin/logical/transit/path_rewrap.go @@ -4,8 +4,8 @@ import ( "context" "encoding/base64" "fmt" + "github.com/hashicorp/vault/helper/constants" "github.com/hashicorp/vault/sdk/framework" - "github.com/hashicorp/vault/sdk/helper/consts" "github.com/hashicorp/vault/sdk/helper/errutil" "github.com/hashicorp/vault/sdk/helper/keysutil" "github.com/hashicorp/vault/sdk/logical" @@ -195,7 +195,7 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d * } } - if consts.IsFIPS() && warnAboutNonceUsage { + if constants.IsFIPS() && warnAboutNonceUsage { resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.") } diff --git a/sdk/helper/consts/fips.go b/helper/constants/fips.go similarity index 88% rename from sdk/helper/consts/fips.go rename to helper/constants/fips.go index 5f436147f97b..4b8a99991c7a 100644 --- a/sdk/helper/consts/fips.go +++ b/helper/constants/fips.go @@ -1,6 +1,6 @@ // +build !fips_140_3 -package consts +package constants // IsFIPS returns true if Vault is operating in a FIPS-140-{2,3} mode. func IsFIPS() bool {