Cache ciphertext for secret properties. (#3183)

This caching is enabled by wrapping the `secrets.Manager` returned by `DefaultSecretsProvider.OfType` in an outer `secrets.Manager` that cooperates with `stack.{Serialize,Deserialize}PropertyValue`. Ciphertext is cached on a per-secret-instance basis (i.e. not a per-plaintext-value basis). Cached ciphertext is only reused if the plaintext for the secret value has not changed. Entries are inserted into the cache upon both encryption and decryption so that values that originated from ciphertext and that have not changed can aoid re-encryption. Contributes to #3178.
2019-09-18 15:52:31 -07:00 · 2019-09-18 15:52:31 -07:00 · 40b0f8cbab
parent c53c0b6c15
commit 40b0f8cbab
8 changed files with 344 additions and 24 deletions
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@ -9,6 +9,9 @@ CHANGELOG
  [#3239](https://github.com/pulumi/pulumi/pull/3239)
 - `pulumi refresh` can now be scoped to refresh a subset of resources by adding a `--target urn` or
  `-t urn` argument.  Multiple resources can be specified using `-t urn1 -t urn2`.
+- Avoid re-encrypting secret values on each checkpoint write. These changes should improve update times for stacks
+  that contain secret values.
+  [#3183](https://github.com/pulumi/pulumi/pull/3183)

 ## 1.1.0 (2019-09-11)

--- a/cmd/crypto.go
+++ b/cmd/crypto.go
@ -22,6 +22,7 @@ import (
 	"github.com/pulumi/pulumi/pkg/backend/filestate"
 	"github.com/pulumi/pulumi/pkg/backend/httpstate"
 	"github.com/pulumi/pulumi/pkg/resource/config"
+	"github.com/pulumi/pulumi/pkg/resource/stack"
 	"github.com/pulumi/pulumi/pkg/secrets"
 	"github.com/pulumi/pulumi/pkg/secrets/passphrase"
 )
@ -50,22 +51,28 @@ func getStackSecretsManager(s backend.Stack) (secrets.Manager, error) {
 		return nil, err
 	}

-	if ps.SecretsProvider != passphrase.Type && ps.SecretsProvider != "default" && ps.SecretsProvider != "" {
-		return newCloudSecretsManager(s.Ref().Name(), stackConfigFile, ps.SecretsProvider)
-	}
+	sm, err := func() (secrets.Manager, error) {
+		if ps.SecretsProvider != passphrase.Type && ps.SecretsProvider != "default" && ps.SecretsProvider != "" {
+			return newCloudSecretsManager(s.Ref().Name(), stackConfigFile, ps.SecretsProvider)
+		}

-	if ps.EncryptionSalt != "" {
-		return newPassphraseSecretsManager(s.Ref().Name(), stackConfigFile)
-	}
+		if ps.EncryptionSalt != "" {
+			return newPassphraseSecretsManager(s.Ref().Name(), stackConfigFile)
+		}

-	switch stack := s.(type) {
-	case httpstate.Stack:
-		return newServiceSecretsManager(stack)
-	case filestate.Stack:
-		return newPassphraseSecretsManager(s.Ref().Name(), stackConfigFile)
-	}
+		switch stack := s.(type) {
+		case httpstate.Stack:
+			return newServiceSecretsManager(stack)
+		case filestate.Stack:
+			return newPassphraseSecretsManager(s.Ref().Name(), stackConfigFile)
+		}

-	return nil, errors.Errorf("unknown stack type %s", reflect.TypeOf(s))
+		return nil, errors.Errorf("unknown stack type %s", reflect.TypeOf(s))
+	}()
+	if err != nil {
+		return nil, err
+	}
+	return stack.NewCachingSecretsManager(sm), nil
 }

 func validateSecretsProvider(typ string) error {
--- a/pkg/backend/httpstate/backend.go
+++ b/pkg/backend/httpstate/backend.go
@ -30,7 +30,7 @@ import (
 	"strings"
 	"time"

-	"github.com/opentracing/opentracing-go"
+	opentracing "github.com/opentracing/opentracing-go"
 	"github.com/pkg/errors"
 	"github.com/skratchdot/open-golang/open"

@ -46,6 +46,7 @@ import (
 	"github.com/pulumi/pulumi/pkg/resource"
 	"github.com/pulumi/pulumi/pkg/resource/config"
 	"github.com/pulumi/pulumi/pkg/resource/deploy"
+	"github.com/pulumi/pulumi/pkg/secrets"
 	"github.com/pulumi/pulumi/pkg/tokens"
 	"github.com/pulumi/pulumi/pkg/util/cmdutil"
 	"github.com/pulumi/pulumi/pkg/util/contract"
@ -881,8 +882,13 @@ func (b *cloudBackend) runEngineAction(
 	}()

 	// The backend.SnapshotManager and backend.SnapshotPersister will keep track of any changes to
-	// the Snapshot (checkpoint file) in the HTTP backend.
-	persister := b.newSnapshotPersister(ctx, u.update, u.tokenSource, op.SecretsManager)
+	// the Snapshot (checkpoint file) in the HTTP backend. We will reuse the snapshot's secrets manager when possible
+	// to ensure that secrets are not re-encrypted on each update.
+	sm := op.SecretsManager
+	if secrets.AreCompatible(sm, u.GetTarget().Snapshot.SecretsManager) {
+		sm = u.GetTarget().Snapshot.SecretsManager
+	}
+	persister := b.newSnapshotPersister(ctx, u.update, u.tokenSource, sm)
 	snapshotManager := backend.NewSnapshotManager(persister, u.GetTarget().Snapshot)

 	// Depending on the action, kick off the relevant engine activity.  Note that we don't immediately check and
--- a/pkg/resource/properties.go
+++ b/pkg/resource/properties.go
@ -87,6 +87,10 @@ type Output struct {
 }

 // Secret indicates that the underlying value should be persisted securely.
+//
+// In order to facilitate the ability to distinguish secrets with identical plaintext in downstream code that may
+// want to cache a secret's ciphertext, secret PropertyValues hold the address of the Secret. If a secret must be
+// copied, its value--not its address--should be copied.
 type Secret struct {
 	Element PropertyValue
 }
@ -183,7 +187,7 @@ func NewArchiveProperty(v *Archive) PropertyValue      { return PropertyValue{v}
 func NewObjectProperty(v PropertyMap) PropertyValue    { return PropertyValue{v} }
 func NewComputedProperty(v Computed) PropertyValue     { return PropertyValue{v} }
 func NewOutputProperty(v Output) PropertyValue         { return PropertyValue{v} }
-func NewSecretProperty(v Secret) PropertyValue         { return PropertyValue{v} }
+func NewSecretProperty(v *Secret) PropertyValue        { return PropertyValue{v} }

 func MakeComputed(v PropertyValue) PropertyValue {
 	return NewComputedProperty(Computed{Element: v})
@ -194,7 +198,7 @@ func MakeOutput(v PropertyValue) PropertyValue {
 }

 func MakeSecret(v PropertyValue) PropertyValue {
-	return NewSecretProperty(Secret{Element: v})
+	return NewSecretProperty(&Secret{Element: v})
 }

 // NewPropertyValue turns a value into a property value, provided it is of a legal "JSON-like" kind.
@ -248,7 +252,7 @@ func NewPropertyValueRepl(v interface{},
 		return NewComputedProperty(t)
 	case Output:
 		return NewOutputProperty(t)
-	case Secret:
+	case *Secret:
 		return NewSecretProperty(t)
 	}

@ -373,7 +377,7 @@ func (v PropertyValue) Input() Computed { return v.V.(Computed) }
 func (v PropertyValue) OutputValue() Output { return v.V.(Output) }

 // SecretValue fetches the underlying secret value (panicking if it isn't a secret).
-func (v PropertyValue) SecretValue() Secret { return v.V.(Secret) }
+func (v PropertyValue) SecretValue() *Secret { return v.V.(*Secret) }

 // IsNull returns true if the underlying value is a null.
 func (v PropertyValue) IsNull() bool {
@ -436,7 +440,7 @@ func (v PropertyValue) IsOutput() bool {

 // IsSecret returns true if the underlying value is a secret value.
 func (v PropertyValue) IsSecret() bool {
-	_, is := v.V.(Secret)
+	_, is := v.V.(*Secret)
 	return is
 }

--- a/pkg/resource/stack/deployment.go
+++ b/pkg/resource/stack/deployment.go
@ -361,7 +361,16 @@ func SerializePropertyValue(prop resource.PropertyValue, enc config.Encrypter) (
 		if err != nil {
 			return nil, errors.Wrap(err, "encoding serialized property value")
 		}
-		ciphertext, err := enc.EncryptValue(string(bytes))
+		plaintext := string(bytes)
+
+		// If the encrypter is a cachingCrypter, call through its encryptSecret method, which will look for a matching
+		// *resource.Secret + plaintext in its cache in order to avoid re-encrypting the value.
+		var ciphertext string
+		if cachingCrypter, ok := enc.(*cachingCrypter); ok {
+			ciphertext, err = cachingCrypter.encryptSecret(prop.SecretValue(), plaintext)
+		} else {
+			ciphertext, err = enc.EncryptValue(plaintext)
+		}
 		if err != nil {
 			return nil, errors.Wrap(err, "failed to encrypt secret value")
 		}
@ -476,7 +485,13 @@ func DeserializePropertyValue(v interface{}, dec config.Decrypter) (resource.Pro
 					if err != nil {
 						return resource.PropertyValue{}, err
 					}
-					return resource.MakeSecret(ev), nil
+					prop := resource.MakeSecret(ev)
+					// If the decrypter is a cachingCrypter, insert the plain- and ciphertext into the cache with the
+					// new *resource.Secret as the key.
+					if cachingCrypter, ok := dec.(*cachingCrypter); ok {
+						cachingCrypter.insert(prop.SecretValue(), plaintext, ciphertext)
+					}
+					return prop, nil
 				default:
 					return resource.PropertyValue{}, errors.Errorf("unrecognized signature '%v' in property map", sig)
 				}
--- a/pkg/resource/stack/secrets.go
+++ b/pkg/resource/stack/secrets.go
@ -19,6 +19,8 @@ import (

 	"github.com/pkg/errors"

+	"github.com/pulumi/pulumi/pkg/resource"
+	"github.com/pulumi/pulumi/pkg/resource/config"
 	"github.com/pulumi/pulumi/pkg/secrets"
 	"github.com/pulumi/pulumi/pkg/secrets/b64"
 	"github.com/pulumi/pulumi/pkg/secrets/cloud"
@ -61,5 +63,91 @@ func (defaultSecretsProvider) OfType(ty string, state json.RawMessage) (secrets.
 		return nil, errors.Wrapf(err, "constructing secrets manager of type %q", ty)
 	}

-	return sm, nil
+	return NewCachingSecretsManager(sm), nil
+}
+
+type cacheEntry struct {
+	plaintext  string
+	ciphertext string
+}
+
+type cachingSecretsManager struct {
+	manager secrets.Manager
+	cache   map[*resource.Secret]cacheEntry
+}
+
+// NewCachingSecretsManager returns a new secrets.Manager that caches the ciphertext for secret property values. A
+// secrets.Manager that will be used to encrypt and decrypt values stored in a serialized deployment can be wrapped
+// in a caching secrets manager in order to avoid re-encrypting secrets each time the deployment is serialized.
+func NewCachingSecretsManager(manager secrets.Manager) secrets.Manager {
+	return &cachingSecretsManager{
+		manager: manager,
+		cache:   make(map[*resource.Secret]cacheEntry),
+	}
+}
+
+func (csm *cachingSecretsManager) Type() string {
+	return csm.manager.Type()
+}
+
+func (csm *cachingSecretsManager) State() interface{} {
+	return csm.manager.State()
+}
+
+func (csm *cachingSecretsManager) Encrypter() (config.Encrypter, error) {
+	enc, err := csm.manager.Encrypter()
+	if err != nil {
+		return nil, err
+	}
+	return &cachingCrypter{
+		encrypter: enc,
+		cache:     csm.cache,
+	}, nil
+}
+
+func (csm *cachingSecretsManager) Decrypter() (config.Decrypter, error) {
+	dec, err := csm.manager.Decrypter()
+	if err != nil {
+		return nil, err
+	}
+	return &cachingCrypter{
+		decrypter: dec,
+		cache:     csm.cache,
+	}, nil
+}
+
+type cachingCrypter struct {
+	encrypter config.Encrypter
+	decrypter config.Decrypter
+	cache     map[*resource.Secret]cacheEntry
+}
+
+func (c *cachingCrypter) EncryptValue(plaintext string) (string, error) {
+	return c.encrypter.EncryptValue(plaintext)
+}
+
+func (c *cachingCrypter) DecryptValue(ciphertext string) (string, error) {
+	return c.decrypter.DecryptValue(ciphertext)
+}
+
+// encryptSecret encrypts the plaintext associated with the given secret value.
+func (c *cachingCrypter) encryptSecret(secret *resource.Secret, plaintext string) (string, error) {
+	// If the cache has an entry for this secret and the plaintext has not changed, re-use the ciphertext.
+	//
+	// Otherwise, re-encrypt the plaintext and update the cache.
+	entry, ok := c.cache[secret]
+	if ok && entry.plaintext == plaintext {
+		return entry.ciphertext, nil
+	}
+	ciphertext, err := c.encrypter.EncryptValue(plaintext)
+	if err != nil {
+		return "", err
+	}
+	c.insert(secret, plaintext, ciphertext)
+	return ciphertext, nil
+}
+
+// insert associates the given secret with the given plain- and ciphertext in the cache.
+func (c *cachingCrypter) insert(secret *resource.Secret, plaintext, ciphertext string) {
+	c.cache[secret] = cacheEntry{plaintext, ciphertext}
 }
--- a/pkg/resource/stack/secrets_test.go
+++ b/pkg/resource/stack/secrets_test.go
@ -0,0 +1,174 @@
+package stack
+
+import (
+	"encoding/json"
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/pkg/errors"
+	"github.com/pulumi/pulumi/pkg/resource"
+	"github.com/pulumi/pulumi/pkg/resource/config"
+	"github.com/stretchr/testify/assert"
+)
+
+type testSecretsManager struct {
+	encryptCalls int
+	decryptCalls int
+}
+
+func (t *testSecretsManager) Type() string { return "test" }
+
+func (t *testSecretsManager) State() interface{} { return nil }
+
+func (t *testSecretsManager) Encrypter() (config.Encrypter, error) {
+	return t, nil
+}
+
+func (t *testSecretsManager) Decrypter() (config.Decrypter, error) {
+	return t, nil
+}
+
+func (t *testSecretsManager) EncryptValue(plaintext string) (string, error) {
+	t.encryptCalls++
+	return fmt.Sprintf("%v:%v", t.encryptCalls, plaintext), nil
+}
+
+func (t *testSecretsManager) DecryptValue(ciphertext string) (string, error) {
+	t.decryptCalls++
+	i := strings.Index(ciphertext, ":")
+	if i == -1 {
+		return "", errors.New("invalid ciphertext format")
+	}
+	return ciphertext[i+1:], nil
+}
+
+func deserializeProperty(v interface{}, dec config.Decrypter) (resource.PropertyValue, error) {
+	b, err := json.Marshal(v)
+	if err != nil {
+		return resource.PropertyValue{}, err
+	}
+	if err := json.Unmarshal(b, &v); err != nil {
+		return resource.PropertyValue{}, err
+	}
+	return DeserializePropertyValue(v, dec)
+}
+
+func TestCachingCrypter(t *testing.T) {
+	sm := &testSecretsManager{}
+	csm := NewCachingSecretsManager(sm)
+
+	foo1 := resource.MakeSecret(resource.NewStringProperty("foo"))
+	foo2 := resource.MakeSecret(resource.NewStringProperty("foo"))
+	bar := resource.MakeSecret(resource.NewStringProperty("bar"))
+
+	enc, err := csm.Encrypter()
+	assert.NoError(t, err)
+
+	// Serialize the first copy of "foo". Encrypt should be called once, as this value has not yet been encrypted.
+	foo1Ser, err := SerializePropertyValue(foo1, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 1, sm.encryptCalls)
+
+	// Serialize the second copy of "foo". Because this is a different secret instance, Encrypt should be called
+	// a second time even though the plaintext is the same as the last value we encrypted.
+	foo2Ser, err := SerializePropertyValue(foo2, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 2, sm.encryptCalls)
+	assert.NotEqual(t, foo1Ser, foo2Ser)
+
+	// Serialize "bar". Encrypt should be called once, as this value has not yet been encrypted.
+	barSer, err := SerializePropertyValue(bar, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, sm.encryptCalls)
+
+	// Serialize the first copy of "foo" again. Encrypt should not be called, as this value has already been
+	// encrypted.
+	foo1Ser2, err := SerializePropertyValue(foo1, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, sm.encryptCalls)
+	assert.Equal(t, foo1Ser, foo1Ser2)
+
+	// Serialize the second copy of "foo" again. Encrypt should not be called, as this value has already been
+	// encrypted.
+	foo2Ser2, err := SerializePropertyValue(foo2, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, sm.encryptCalls)
+	assert.Equal(t, foo2Ser, foo2Ser2)
+
+	// Serialize "bar" again. Encrypt should not be called, as this value has already been encrypted.
+	barSer2, err := SerializePropertyValue(bar, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, sm.encryptCalls)
+	assert.Equal(t, barSer, barSer2)
+
+	dec, err := csm.Decrypter()
+	assert.NoError(t, err)
+
+	// Decrypt foo1Ser. Decrypt should be called.
+	foo1Dec, err := deserializeProperty(foo1Ser, dec)
+	assert.NoError(t, err)
+	assert.True(t, foo1.DeepEquals(foo1Dec))
+	assert.Equal(t, 1, sm.decryptCalls)
+
+	// Decrypt foo2Ser. Decrypt should be called.
+	foo2Dec, err := deserializeProperty(foo2Ser, dec)
+	assert.NoError(t, err)
+	assert.True(t, foo2.DeepEquals(foo2Dec))
+	assert.Equal(t, 2, sm.decryptCalls)
+
+	// Decrypt barSer. Decrypt should be called.
+	barDec, err := deserializeProperty(barSer, dec)
+	assert.NoError(t, err)
+	assert.True(t, bar.DeepEquals(barDec))
+	assert.Equal(t, 3, sm.decryptCalls)
+
+	// Create a new CachingSecretsManager and re-run the decrypts. Each decrypt should insert the plain- and
+	// ciphertext into the cache with the associated secret.
+	csm = NewCachingSecretsManager(sm)
+
+	dec, err = csm.Decrypter()
+	assert.NoError(t, err)
+
+	// Decrypt foo1Ser. Decrypt should be called.
+	foo1Dec, err = deserializeProperty(foo1Ser, dec)
+	assert.NoError(t, err)
+	assert.True(t, foo1.DeepEquals(foo1Dec))
+	assert.Equal(t, 4, sm.decryptCalls)
+
+	// Decrypt foo2Ser. Decrypt should be called.
+	foo2Dec, err = deserializeProperty(foo2Ser, dec)
+	assert.NoError(t, err)
+	assert.True(t, foo2.DeepEquals(foo2Dec))
+	assert.Equal(t, 5, sm.decryptCalls)
+
+	// Decrypt barSer. Decrypt should be called.
+	barDec, err = deserializeProperty(barSer, dec)
+	assert.NoError(t, err)
+	assert.True(t, bar.DeepEquals(barDec))
+	assert.Equal(t, 6, sm.decryptCalls)
+
+	enc, err = csm.Encrypter()
+	assert.NoError(t, err)
+
+	// Serialize the first copy of "foo" again. Encrypt should not be called, as this value has already been
+	// cached by the earlier calls to Decrypt.
+	foo1Ser2, err = SerializePropertyValue(foo1Dec, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, sm.encryptCalls)
+	assert.Equal(t, foo1Ser, foo1Ser2)
+
+	// Serialize the second copy of "foo" again. Encrypt should not be called, as this value has already been
+	// cached by the earlier calls to Decrypt.
+	foo2Ser2, err = SerializePropertyValue(foo2Dec, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, sm.encryptCalls)
+	assert.Equal(t, foo2Ser, foo2Ser2)
+
+	// Serialize "bar" again. Encrypt should not be called, as this value has already been cached by the
+	// earlier calls to Decrypt.
+	barSer2, err = SerializePropertyValue(barDec, enc)
+	assert.NoError(t, err)
+	assert.Equal(t, 3, sm.encryptCalls)
+	assert.Equal(t, barSer, barSer2)
+}
--- a/pkg/secrets/manager.go
+++ b/pkg/secrets/manager.go
@ -14,6 +14,8 @@
 package secrets

 import (
+	"encoding/json"
+
 	"github.com/pulumi/pulumi/pkg/resource/config"
 )

@ -33,3 +35,24 @@ type Manager interface {
 	// deployment, or an error if one can not be constructed.
 	Decrypter() (config.Decrypter, error)
 }
+
+// AreCompatible returns true if the two Managers are of the same type and have the same state.
+func AreCompatible(a, b Manager) bool {
+	if a == nil || b == nil {
+		return a == nil && b == nil
+	}
+
+	if a.Type() != b.Type() {
+		return false
+	}
+
+	as, err := json.Marshal(a.State())
+	if err != nil {
+		return false
+	}
+	bs, err := json.Marshal(b.State())
+	if err != nil {
+		return false
+	}
+	return string(as) == string(bs)
+}