diff --git a/CHANGELOG.md b/CHANGELOG.md index 0fe3e3a4da..cad210a0c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -29,9 +29,10 @@ FEATURES: deprecates App-ID. [GH-1426] * **Convergent Encryption in `Transit`**: The `transit` backend now supports a convergent encryption mode where the same plaintext will produce the same - ciphertext. Although very useful in some situations, this has security - implications, which are mostly mitigated by requiring the use of key - derivation when convergent encryption is enabled. See [the `transit` + ciphertext. Although very useful in some situations, this has potential + security implications, which are mostly mitigated by requiring the use of + key derivation when convergent encryption is enabled. See [the `transit` + backend documentation](https://www.vaultproject.io/docs/secrets/transit/index.html) for more details. [GH-1537] * **Improved LDAP Group Filters**: The `ldap` auth backend now uses templates diff --git a/builtin/logical/transit/backend_test.go b/builtin/logical/transit/backend_test.go index 21d83aec97..14c47fd005 100644 --- a/builtin/logical/transit/backend_test.go +++ b/builtin/logical/transit/backend_test.go @@ -230,7 +230,6 @@ func testAccStepReadPolicy(t *testing.T, name string, expectNone, derived bool) KDFMode string `mapstructure:"kdf_mode"` DeletionAllowed bool `mapstructure:"deletion_allowed"` ConvergentEncryption bool `mapstructure:"convergent_encryption"` - ContextAsNonce bool `mapstructure:"context_as_nonce"` } if err := mapstructure.Decode(resp.Data, &d); err != nil { return err @@ -608,6 +607,16 @@ func TestConvergentEncryption(t *testing.T) { t.Fatalf("expected error response, got %#v", *resp) } + // Ensure we fail if we do not provide a nonce + req.Data = map[string]interface{}{ + "plaintext": "emlwIHphcA==", // "zip zap" + "context": "pWZ6t/im3AORd0lVYE0zBdKpX6Bl3/SvFtoVTPWbdkzjG788XmMAnOlxandSdd7S", + } + resp, err = b.HandleRequest(req) + if err == nil && (resp == nil || !resp.IsError()) { + t.Fatal("expected error response") + } + // Now test encrypting the same value twice req.Data = map[string]interface{}{ "plaintext": "emlwIHphcA==", // "zip zap" @@ -702,126 +711,6 @@ func TestConvergentEncryption(t *testing.T) { } } -func TestConvergentEncryptionContextAsNonce(t *testing.T) { - var b *backend - sysView := logical.TestSystemView() - storage := &logical.InmemStorage{} - - b = Backend(&logical.BackendConfig{ - StorageView: storage, - System: sysView, - }) - - req := &logical.Request{ - Storage: storage, - Operation: logical.UpdateOperation, - Path: "keys/testkeynonderived", - Data: map[string]interface{}{ - "derived": false, - "convergent_encryption": true, - "context_as_nonce": true, - }, - } - - resp, err := b.HandleRequest(req) - if err != nil { - t.Fatal(err) - } - if resp == nil { - t.Fatal("expected non-nil response") - } - if !resp.IsError() { - t.Fatalf("bad: expected error response, got %#v", *resp) - } - - req.Path = "keys/testkey" - req.Data = map[string]interface{}{ - "derived": true, - "convergent_encryption": true, - "context_as_nonce": true, - } - - resp, err = b.HandleRequest(req) - if err != nil { - t.Fatal(err) - } - if resp != nil { - t.Fatalf("bad: got resp %#v", *resp) - } - - // First, test using an invalid length of nonce - req.Path = "encrypt/testkey" - req.Data = map[string]interface{}{ - "plaintext": "emlwIHphcA==", // "zip zap" - "context": "Zm9vIGJhcg==", // "foo bar" - } - resp, err = b.HandleRequest(req) - if resp == nil { - t.Fatal("expected non-nil response") - } - if !resp.IsError() { - t.Fatalf("expected error response, got %#v", *resp) - } - - // Now test encrypting the same value twice - req.Data = map[string]interface{}{ - "plaintext": "emlwIHphcA==", // "zip zap" - "context": "b25ldHdvdGhyZWVl", // "onetwothreee" - } - resp, err = b.HandleRequest(req) - if resp == nil { - t.Fatal("expected non-nil response") - } - if resp.IsError() { - t.Fatalf("got error response: %#v", *resp) - } - ciphertext1 := resp.Data["ciphertext"].(string) - - resp, err = b.HandleRequest(req) - if resp == nil { - t.Fatal("expected non-nil response") - } - if resp.IsError() { - t.Fatalf("got error response: %#v", *resp) - } - ciphertext2 := resp.Data["ciphertext"].(string) - - if ciphertext1 != ciphertext2 { - t.Fatalf("expected the same ciphertext but got %s and %s", ciphertext1, ciphertext2) - } - - // For sanity, also check a different value - req.Data = map[string]interface{}{ - "plaintext": "emlwIHphcA==", // "zip zap" - "context": "dHdvdGhyZWVmb3Vy", // "twothreefour" - } - resp, err = b.HandleRequest(req) - if resp == nil { - t.Fatal("expected non-nil response") - } - if resp.IsError() { - t.Fatalf("got error response: %#v", *resp) - } - ciphertext3 := resp.Data["ciphertext"].(string) - - resp, err = b.HandleRequest(req) - if resp == nil { - t.Fatal("expected non-nil response") - } - if resp.IsError() { - t.Fatalf("got error response: %#v", *resp) - } - ciphertext4 := resp.Data["ciphertext"].(string) - - if ciphertext3 != ciphertext4 { - t.Fatalf("expected the same ciphertext but got %s and %s", ciphertext3, ciphertext4) - } - if ciphertext1 == ciphertext3 { - t.Fatalf("expected different ciphertexts") - } - -} - func TestPolicyFuzzing(t *testing.T) { var be *backend sysView := logical.TestSystemView() diff --git a/builtin/logical/transit/lock_manager.go b/builtin/logical/transit/lock_manager.go index a3ac90348c..a34f4ed03c 100644 --- a/builtin/logical/transit/lock_manager.go +++ b/builtin/logical/transit/lock_manager.go @@ -105,42 +105,42 @@ func (lm *lockManager) UnlockPolicy(lock *sync.RWMutex, lockType bool) { // is needed (for instance, for an upgrade/migration), give up the read lock, // call again with an exclusive lock, then swap back out for a read lock. func (lm *lockManager) GetPolicyShared(storage logical.Storage, name string) (*Policy, *sync.RWMutex, error) { - p, lock, _, err := lm.getPolicyCommon(storage, name, false, false, false, false, shared) + p, lock, _, err := lm.getPolicyCommon(storage, name, false, false, false, shared) if err == nil || (err != nil && err != errNeedExclusiveLock) { return p, lock, err } // Try again while asking for an exlusive lock - p, lock, _, err = lm.getPolicyCommon(storage, name, false, false, false, false, exclusive) + p, lock, _, err = lm.getPolicyCommon(storage, name, false, false, false, exclusive) if err != nil || p == nil || lock == nil { return p, lock, err } lock.Unlock() - p, lock, _, err = lm.getPolicyCommon(storage, name, false, false, false, false, shared) + p, lock, _, err = lm.getPolicyCommon(storage, name, false, false, false, shared) return p, lock, err } // Get the policy with an exclusive lock func (lm *lockManager) GetPolicyExclusive(storage logical.Storage, name string) (*Policy, *sync.RWMutex, error) { - p, lock, _, err := lm.getPolicyCommon(storage, name, false, false, false, false, exclusive) + p, lock, _, err := lm.getPolicyCommon(storage, name, false, false, false, exclusive) return p, lock, err } // Get the policy with a read lock; if it returns that an exclusive lock is // needed, retry. If successful, call one more time to get a read lock and // return the value. -func (lm *lockManager) GetPolicyUpsert(storage logical.Storage, name string, derived, convergent, contextAsNonce bool) (*Policy, *sync.RWMutex, bool, error) { - p, lock, _, err := lm.getPolicyCommon(storage, name, true, derived, convergent, contextAsNonce, shared) +func (lm *lockManager) GetPolicyUpsert(storage logical.Storage, name string, derived, convergent bool) (*Policy, *sync.RWMutex, bool, error) { + p, lock, _, err := lm.getPolicyCommon(storage, name, true, derived, convergent, shared) if err == nil || (err != nil && err != errNeedExclusiveLock) { return p, lock, false, err } // Try again while asking for an exlusive lock - p, lock, upserted, err := lm.getPolicyCommon(storage, name, true, derived, convergent, contextAsNonce, exclusive) + p, lock, upserted, err := lm.getPolicyCommon(storage, name, true, derived, convergent, exclusive) if err != nil || p == nil || lock == nil { return p, lock, upserted, err } @@ -148,14 +148,14 @@ func (lm *lockManager) GetPolicyUpsert(storage logical.Storage, name string, der lock.Unlock() // Now get a shared lock for the return, but preserve the value of upsert - p, lock, _, err = lm.getPolicyCommon(storage, name, true, derived, convergent, contextAsNonce, shared) + p, lock, _, err = lm.getPolicyCommon(storage, name, true, derived, convergent, shared) return p, lock, upserted, err } // When the function returns, a lock will be held on the policy if err == nil. // It is the caller's responsibility to unlock. -func (lm *lockManager) getPolicyCommon(storage logical.Storage, name string, upsert, derived, convergent, contextAsNonce, lockType bool) (*Policy, *sync.RWMutex, bool, error) { +func (lm *lockManager) getPolicyCommon(storage logical.Storage, name string, upsert, derived, convergent, lockType bool) (*Policy, *sync.RWMutex, bool, error) { lock := lm.policyLock(name, lockType) var p *Policy @@ -204,8 +204,6 @@ func (lm *lockManager) getPolicyCommon(storage logical.Storage, name string, ups if derived { p.KDFMode = kdfMode p.ConvergentEncryption = convergent - p.ContextAsNonce = new(bool) - *p.ContextAsNonce = contextAsNonce } err = p.rotate(storage) diff --git a/builtin/logical/transit/path_datakey.go b/builtin/logical/transit/path_datakey.go index fced4825e5..19367ecc36 100644 --- a/builtin/logical/transit/path_datakey.go +++ b/builtin/logical/transit/path_datakey.go @@ -30,6 +30,11 @@ ciphertext; "wrapped" will return the ciphertext only.`, Description: "Context for key derivation. Required for derived keys.", }, + "nonce": &framework.FieldSchema{ + Type: framework.TypeString, + Description: "Nonce for when convergent encryption is used", + }, + "bits": &framework.FieldSchema{ Type: framework.TypeInt, Description: `Number of bits for the key; currently 128, 256, @@ -61,17 +66,28 @@ func (b *backend) pathDatakeyWrite( return logical.ErrorResponse("Invalid path, must be 'plaintext' or 'wrapped'"), logical.ErrInvalidRequest } + var err error + // Decode the context if any contextRaw := d.Get("context").(string) var context []byte if len(contextRaw) != 0 { - var err error context, err = base64.StdEncoding.DecodeString(contextRaw) if err != nil { return logical.ErrorResponse("failed to decode context as base64"), logical.ErrInvalidRequest } } + // Decode the nonce if any + nonceRaw := d.Get("nonce").(string) + var nonce []byte + if len(nonceRaw) != 0 { + nonce, err = base64.StdEncoding.DecodeString(nonceRaw) + if err != nil { + return logical.ErrorResponse("failed to decode nonce as base64"), logical.ErrInvalidRequest + } + } + // Get the policy p, lock, err := b.lm.GetPolicyShared(req.Storage, name) if lock != nil { @@ -100,7 +116,7 @@ func (b *backend) pathDatakeyWrite( return nil, err } - ciphertext, err := p.Encrypt(context, nil, base64.StdEncoding.EncodeToString(newKey)) + ciphertext, err := p.Encrypt(context, nonce, base64.StdEncoding.EncodeToString(newKey)) if err != nil { switch err.(type) { case errutil.UserError: diff --git a/builtin/logical/transit/path_decrypt.go b/builtin/logical/transit/path_decrypt.go index 481f947bbd..4587fae836 100644 --- a/builtin/logical/transit/path_decrypt.go +++ b/builtin/logical/transit/path_decrypt.go @@ -30,7 +30,7 @@ func (b *backend) pathDecrypt() *framework.Path { "nonce": &framework.FieldSchema{ Type: framework.TypeString, - Description: "Nonce for when convergent encryption is used and the context is not used as the nonce", + Description: "Nonce for when convergent encryption is used", }, }, diff --git a/builtin/logical/transit/path_encrypt.go b/builtin/logical/transit/path_encrypt.go index 00089423e0..d27126f09d 100644 --- a/builtin/logical/transit/path_encrypt.go +++ b/builtin/logical/transit/path_encrypt.go @@ -31,7 +31,7 @@ func (b *backend) pathEncrypt() *framework.Path { "nonce": &framework.FieldSchema{ Type: framework.TypeString, - Description: "Nonce for when convergent encryption is used and the context is not used as the nonce", + Description: "Nonce for when convergent encryption is used", }, }, @@ -95,7 +95,7 @@ func (b *backend) pathEncryptWrite( var lock *sync.RWMutex var upserted bool if req.Operation == logical.CreateOperation { - p, lock, upserted, err = b.lm.GetPolicyUpsert(req.Storage, name, len(context) != 0, false, false) + p, lock, upserted, err = b.lm.GetPolicyUpsert(req.Storage, name, len(context) != 0, false) } else { p, lock, err = b.lm.GetPolicyShared(req.Storage, name) } diff --git a/builtin/logical/transit/path_keys.go b/builtin/logical/transit/path_keys.go index e59409db0f..3d61a23a45 100644 --- a/builtin/logical/transit/path_keys.go +++ b/builtin/logical/transit/path_keys.go @@ -29,31 +29,14 @@ allows for per-transaction unique keys.`, This is only supported when using a key with key derivation enabled and will require all requests to carry both a context and 96-bit -(12-byte) nonce, unless the "context_as_nonce" -feature is also enabled. The given nonce will -be used in place of a randomly generated nonce. -As a result, when the same context and nonce -(or context, if "context_as_nonce" is enabled) -are supplied, the same ciphertext is emitted -from the encryption function. It is *very -important* when using this mode that you ensure -that all nonces are unique for a given context, -or, when using "context_as_nonce", that all -contexts are unique for a given key. Failing to -do so will severely impact the ciphertext's -security.`, - }, - - "context_as_nonce": &framework.FieldSchema{ - Type: framework.TypeBool, - Description: `Whether to use the context value as the -nonce in the convergent encryption operation -mode. If set true, the user will have to -supply a 96-bit (12-byte) context value. -It is *very important* when using this -mode that you ensure that all contexts are -*globally unique*. Failing to do so will -severely impact the security of the key.`, +(12-byte) nonce. The given nonce will be used +in place of a randomly generated nonce. As a +result, when the same context and nonce are +supplied, the same ciphertext is generated. It +is *very important* when using this mode that +you ensure that all nonces are unique for a +given context. Failing to do so will severely +impact the ciphertext's security.`, }, }, @@ -73,13 +56,12 @@ func (b *backend) pathPolicyWrite( name := d.Get("name").(string) derived := d.Get("derived").(bool) convergent := d.Get("convergent_encryption").(bool) - contextAsNonce := d.Get("context_as_nonce").(bool) if !derived && convergent { return logical.ErrorResponse("convergent encryption requires derivation to be enabled"), nil } - p, lock, upserted, err := b.lm.GetPolicyUpsert(req.Storage, name, derived, convergent, contextAsNonce) + p, lock, upserted, err := b.lm.GetPolicyUpsert(req.Storage, name, derived, convergent) if lock != nil { defer lock.RUnlock() } @@ -127,9 +109,6 @@ func (b *backend) pathPolicyRead( if p.Derived { resp.Data["kdf_mode"] = p.KDFMode resp.Data["convergent_encryption"] = p.ConvergentEncryption - if p.ContextAsNonce != nil { - resp.Data["context_as_nonce"] = *p.ContextAsNonce - } } retKeys := map[string]int64{} diff --git a/builtin/logical/transit/path_rewrap.go b/builtin/logical/transit/path_rewrap.go index 1455266853..a519475758 100644 --- a/builtin/logical/transit/path_rewrap.go +++ b/builtin/logical/transit/path_rewrap.go @@ -30,7 +30,7 @@ func (b *backend) pathRewrap() *framework.Path { "nonce": &framework.FieldSchema{ Type: framework.TypeString, - Description: "Nonce for when convergent encryption is used and the context is not used as the nonce", + Description: "Nonce for when convergent encryption is used", }, }, diff --git a/builtin/logical/transit/policy.go b/builtin/logical/transit/policy.go index 7dae12fe73..a4b87e247b 100644 --- a/builtin/logical/transit/policy.go +++ b/builtin/logical/transit/policy.go @@ -72,7 +72,6 @@ type Policy struct { Derived bool `json:"derived"` KDFMode string `json:"kdf_mode"` ConvergentEncryption bool `json:"convergent_encryption"` - ContextAsNonce *bool `json:"context_as_nonce"` // The minimum version of the key allowed to be used // for decryption @@ -260,10 +259,6 @@ func (p *Policy) needsUpgrade() bool { return true } - if p.ConvergentEncryption && p.ContextAsNonce == nil { - return true - } - return false } @@ -293,14 +288,6 @@ func (p *Policy) upgrade(storage logical.Storage) error { persistNeeded = true } - // Originally the context-as-nonce mode was the only mode, so keep that - // behavior if convergent encryption is already in use - if p.ConvergentEncryption && p.ContextAsNonce == nil { - p.ContextAsNonce = new(bool) - *p.ContextAsNonce = true - persistNeeded = true - } - if persistNeeded { err := p.Persist(storage) if err != nil { @@ -377,17 +364,9 @@ func (p *Policy) Encrypt(context, nonce []byte, value string) (string, error) { } if p.ConvergentEncryption { - - if *p.ContextAsNonce { - if len(context) != gcm.NonceSize() { - return "", errutil.UserError{Err: fmt.Sprintf("base64-decoded context must be %d bytes long when using convergent encryption with context-as-nonce with this key", gcm.NonceSize())} - } - nonce = context - - } else if len(nonce) != gcm.NonceSize() { + if len(nonce) != gcm.NonceSize() { return "", errutil.UserError{Err: fmt.Sprintf("base64-decoded nonce must be %d bytes long when using convergent encryption with this key", gcm.NonceSize())} } - } else { // Compute random nonce nonce = make([]byte, gcm.NonceSize()) @@ -421,7 +400,7 @@ func (p *Policy) Decrypt(context, nonce []byte, value string) (string, error) { return "", errutil.UserError{Err: "invalid ciphertext: no prefix"} } - if p.ConvergentEncryption && !*p.ContextAsNonce && (nonce == nil || len(nonce) == 0) { + if p.ConvergentEncryption && (nonce == nil || len(nonce) == 0) { return "", errutil.UserError{Err: "invalid convergent nonce supplied"} } @@ -483,9 +462,6 @@ func (p *Policy) Decrypt(context, nonce []byte, value string) (string, error) { // Extract the nonce and ciphertext var ciphertext []byte if p.ConvergentEncryption { - if *p.ContextAsNonce { - nonce = context - } ciphertext = decoded } else { nonce = decoded[:gcm.NonceSize()] diff --git a/builtin/logical/transit/policy_test.go b/builtin/logical/transit/policy_test.go index d147b387e9..c823b94ec9 100644 --- a/builtin/logical/transit/policy_test.go +++ b/builtin/logical/transit/policy_test.go @@ -22,7 +22,7 @@ func Test_KeyUpgrade(t *testing.T) { func testKeyUpgradeCommon(t *testing.T, lm *lockManager) { storage := &logical.InmemStorage{} - p, lock, upserted, err := lm.GetPolicyUpsert(storage, "test", false, false, false) + p, lock, upserted, err := lm.GetPolicyUpsert(storage, "test", false, false) if lock != nil { defer lock.RUnlock() } @@ -68,7 +68,7 @@ func testArchivingUpgradeCommon(t *testing.T, lm *lockManager) { storage := &logical.InmemStorage{} - p, lock, _, err := lm.GetPolicyUpsert(storage, "test", false, false, false) + p, lock, _, err := lm.GetPolicyUpsert(storage, "test", false, false) if err != nil { t.Fatal(err) } @@ -198,7 +198,7 @@ func testArchivingCommon(t *testing.T, lm *lockManager) { storage := &logical.InmemStorage{} - p, lock, _, err := lm.GetPolicyUpsert(storage, "test", false, false, false) + p, lock, _, err := lm.GetPolicyUpsert(storage, "test", false, false) if lock != nil { defer lock.RUnlock() } diff --git a/website/source/docs/secrets/transit/index.html.md b/website/source/docs/secrets/transit/index.html.md index e90c3abdb6..dc2e9582c8 100644 --- a/website/source/docs/secrets/transit/index.html.md +++ b/website/source/docs/secrets/transit/index.html.md @@ -33,7 +33,7 @@ data's attack surface. Key derivation is supported, which allows the same key to be used for multiple purposes by deriving a new key based on a user-supplied context value. In this mode, convergent encryption can optionally be supported, which allows the same -context and plaintext to produce the same ciphertext. +input values to produce the same ciphertext. The backend also supports key rotation, which allows a new version of the named key to be generated. All data encrypted with the key will use the newest @@ -156,12 +156,16 @@ only encrypt or decrypt using the named keys they need access to. optional If set, the key will support convergent encryption, where the same plaintext creates the same ciphertext. This requires _derived_ to be - set to `true`. When enabled, the context value must be exactly 12 bytes - (96 bits) and will both be used to derive the key and as the nonce for - the encryption operation. Note that while this is useful for particular - situations, it also has security implications. In particular, you must - ensure that you do **not** use the same context value for more than one - plaintext value. Defaults to false. + set to `true`. When enabled, each + encryption(/decryption/rewrap/datakey) operation will require a `nonce` + value to be specified. Note that while this is useful for particular + situations, all nonce values used with a given context value **must be + unique** or it will compromise the security of your key. A common way + to use this will be to generate a unique identifier for the given data + (for instance, a SHA-512 sum), then separate the bytes so that twelve + bytes are used as the nonce and the remaining as the context, ensuring + that all bits of unique identity are used as a part of the encryption + operation. Defaults to false. @@ -347,6 +351,15 @@ only encrypt or decrypt using the named keys they need access to. The key derivation context, provided as base64 encoded. Must be provided if derivation is enabled. +