Skip to content

Commit

Permalink
Revert "Reject supplied nonces for non-convergent encryption operatio…
Browse files Browse the repository at this point in the history 
…ns (#22852)"

This reverts commit 4ff9d9a.
  • Loading branch information
@sgmiller
sgmiller committed Sep 8, 2023
1 parent 4ff9d9a commit d653525
Show file tree
Hide file tree
Showing 7 changed files with 13 additions and 142 deletions.
25 changes: 9 additions & 16 deletions builtin/logical/transit/backend_test.go
View file
Expand Up @@ -1111,12 +1111,10 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT

// Now test encrypting the same value twice
req.Data = map[string]interface{}{
"plaintext": "emlwIHphcA==", // "zip zap"
"plaintext": "emlwIHphcA==", // "zip zap"
"nonce": "b25ldHdvdGhyZWVl", // "onetwothreee"
"context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S",
}
if ver == 0 {
req.Data["nonce"] = "b25ldHdvdGhyZWVl" // "onetwothreee"
}
resp, err = b.HandleRequest(context.Background(), req)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -1147,10 +1145,11 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT

// For sanity, also check a different nonce value...
req.Data = map[string]interface{}{
"plaintext": "emlwIHphcA==", // "zip zap"
"plaintext": "emlwIHphcA==", // "zip zap"
"nonce": "dHdvdGhyZWVmb3Vy", // "twothreefour"
"context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S",
}
if ver == 0 {
if ver < 2 {
req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour"
} else {
req.Data["context"] = "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOldandSdd7S"
Expand Down Expand Up @@ -1189,12 +1188,10 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT

// ...and a different context value
req.Data = map[string]interface{}{
"plaintext": "emlwIHphcA==", // "zip zap"
"plaintext": "emlwIHphcA==", // "zip zap"
"nonce": "dHdvdGhyZWVmb3Vy", // "twothreefour"
"context": "qV4h9iQyvn+raODOer4JNAsOhkXBwdT4HZ677Ql4KLqXSU+Jk4C/fXBWbv6xkSYT",
}
if ver == 0 {
req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour"
}
resp, err = b.HandleRequest(context.Background(), req)
if err != nil {
t.Fatal(err)
Expand Down Expand Up @@ -1306,11 +1303,9 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT
// Finally, check operations on empty values
// First, check without setting a plaintext at all
req.Data = map[string]interface{}{
"nonce": "b25ldHdvdGhyZWVl", // "onetwothreee"
"context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S",
}
if ver == 0 {
req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour"
}
resp, err = b.HandleRequest(context.Background(), req)
if err == nil {
t.Fatal("expected error, got nil")
Expand All @@ -1325,11 +1320,9 @@ func testConvergentEncryptionCommon(t *testing.T, ver int, keyType keysutil.KeyT
// Now set plaintext to empty
req.Data = map[string]interface{}{
"plaintext": "",
"nonce": "b25ldHdvdGhyZWVl", // "onetwothreee"
"context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S",
}
if ver == 0 {
req.Data["nonce"] = "dHdvdGhyZWVmb3Vy" // "twothreefour"
}
resp, err = b.HandleRequest(context.Background(), req)
if err != nil {
t.Fatal(err)
Expand Down
4 changes: 0 additions & 4 deletions builtin/logical/transit/path_datakey.go
View file
Expand Up @@ -160,10 +160,6 @@ func (b *backend) pathDatakeyWrite(ctx context.Context, req *logical.Request, d
},
}

if len(nonce) > 0 && !nonceAllowed(p) {
return nil, ErrNonceNotAllowed
}

if constants.IsFIPS() && shouldWarnAboutNonceUsage(p, nonce) {
resp.AddWarning("A provided nonce value was used within FIPS mode, this violates FIPS 140 compliance.")
}
Expand Down
26 changes: 0 additions & 26 deletions builtin/logical/transit/path_encrypt.go
View file
Expand Up @@ -395,13 +395,6 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d
successesInBatch := false
for i, item := range batchInputItems {
if batchResponseItems[i].Error != "" {
userErrorInBatch = true
continue
}

if item.Nonce != "" && !nonceAllowed(p) {
userErrorInBatch = true
batchResponseItems[i].Error = ErrNonceNotAllowed.Error()
continue
}

Expand Down Expand Up @@ -472,25 +465,6 @@ func (b *backend) pathEncryptWrite(ctx context.Context, req *logical.Request, d
return batchRequestResponse(d, resp, req, successesInBatch, userErrorInBatch, internalErrorInBatch)
}

func nonceAllowed(p *keysutil.Policy) bool {
var supportedKeyType bool
switch p.Type {
case keysutil.KeyType_MANAGED_KEY:
return true
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
}

return false
}

// Depending on the errors in the batch, different status codes should be returned. User errors
// will return a 400 and precede internal errors which return a 500. The reasoning behind this is
// that user errors are non-retryable without making changes to the request, and should be surfaced
Expand Down
81 changes: 3 additions & 78 deletions builtin/logical/transit/path_encrypt_test.go
View file
Expand Up @@ -3,7 +3,6 @@ package transit
import (
"context"
"encoding/json"
"net/http"
"reflect"
"strings"
"testing"
Expand Down Expand Up @@ -646,26 +645,13 @@ func TestTransit_BatchEncryptionCase12(t *testing.T) {
}

// Case13: Incorrect input for nonce when we aren't in convergent encryption should fail the operation
func TestTransit_EncryptionCase13(t *testing.T) {
func TestTransit_BatchEncryptionCase13(t *testing.T) {
var err error

b, s := createBackendWithStorage(t)

// Non-batch first
data := map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "nonce": "R80hr9eNUIuFV52e"}
req := &logical.Request{
Operation: logical.CreateOperation,
Path: "encrypt/my-key",
Storage: s,
Data: data,
}
resp, err := b.HandleRequest(context.Background(), req)
if err == nil {
t.Fatal("expected invalid request")
}

batchInput := []interface{}{
map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "nonce": "R80hr9eNUIuFV52e"},
map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "nonce": "YmFkbm9uY2U="},
}

batchData := map[string]interface{}{
Expand All @@ -677,71 +663,10 @@ func TestTransit_EncryptionCase13(t *testing.T) {
Storage: s,
Data: batchData,
}
resp, err = b.HandleRequest(context.Background(), batchReq)
if err != nil {
t.Fatal(err)
}

if v, ok := resp.Data["http_status_code"]; !ok || v.(int) != http.StatusBadRequest {
t.Fatal("expected request error")
}
}

// Case14: Incorrect input for nonce when we are in convergent version 3 should fail
func TestTransit_EncryptionCase14(t *testing.T) {
var err error

b, s := createBackendWithStorage(t)

cReq := &logical.Request{
Operation: logical.UpdateOperation,
Path: "keys/my-key",
Storage: s,
Data: map[string]interface{}{
"convergent_encryption": "true",
"derived": "true",
},
}
resp, err := b.HandleRequest(context.Background(), cReq)
if err != nil {
t.Fatal(err)
}

// Non-batch first
data := map[string]interface{}{"plaintext": "bXkgc2VjcmV0IGRhdGE=", "context": "SGVsbG8sIFdvcmxkCg==", "nonce": "R80hr9eNUIuFV52e"}
req := &logical.Request{
Operation: logical.CreateOperation,
Path: "encrypt/my-key",
Storage: s,
Data: data,
}

resp, err = b.HandleRequest(context.Background(), req)
if err == nil {
t.Fatal("expected invalid request")
}

batchInput := []interface{}{
data,
}

batchData := map[string]interface{}{
"batch_input": batchInput,
}
batchReq := &logical.Request{
Operation: logical.CreateOperation,
Path: "encrypt/my-key",
Storage: s,
Data: batchData,
}
resp, err = b.HandleRequest(context.Background(), batchReq)
_, err = b.HandleRequest(context.Background(), batchReq)
if err != nil {
t.Fatal(err)
}

if v, ok := resp.Data["http_status_code"]; !ok || v.(int) != http.StatusBadRequest {
t.Fatal("expected request error")
}
}

// Test that the fast path function decodeBatchRequestItems behave like mapstructure.Decode() to decode []BatchRequestItem.
Expand Down
8 changes: 0 additions & 8 deletions builtin/logical/transit/path_rewrap.go
View file
Expand Up @@ -3,7 +3,6 @@ package transit
import (
"context"
"encoding/base64"
"errors"
"fmt"

"github.com/hashicorp/vault/helper/constants"
Expand All @@ -14,8 +13,6 @@ import (
"github.com/mitchellh/mapstructure"
)

var ErrNonceNotAllowed = errors.New("provided nonce not allowed for this key")

func (b *backend) pathRewrap() *framework.Path {
return &framework.Path{
Pattern: "rewrap/" + framework.GenericNameRegex("name"),
Expand Down Expand Up @@ -146,11 +143,6 @@ func (b *backend) pathRewrapWrite(ctx context.Context, req *logical.Request, d *
continue
}

if item.Nonce != "" && !nonceAllowed(p) {
batchResponseItems[i].Error = ErrNonceNotAllowed.Error()
continue
}

plaintext, err := p.Decrypt(item.DecodedContext, item.DecodedNonce, item.Ciphertext)
if err != nil {
switch err.(type) {
Expand Down
3 changes: 0 additions & 3 deletions changelog/22852.txt
View file

This file was deleted.

8 changes: 1 addition & 7 deletions sdk/helper/keysutil/policy.go
View file
Expand Up @@ -1873,15 +1873,9 @@ func (p *Policy) EncryptWithFactory(ver int, context []byte, nonce []byte, value

encBytes := 32
hmacBytes := 0
convergentVersion := p.convergentVersion(ver)
if convergentVersion > 2 {
if p.convergentVersion(ver) > 2 {
deriveHMAC = true
hmacBytes = 32
if len(nonce) > 0 {
return "", errutil.UserError{Err: "nonce provided when not allowed"}
}
} else if len(nonce) > 0 && (!p.ConvergentEncryption || convergentVersion != 1) {
return "", errutil.UserError{Err: "nonce provided when not allowed"}
}
if p.Type == KeyType_AES128_GCM96 {
encBytes = 16
Expand Down

0 comments on commit d653525

Please sign in to comment.