From 4acb843f15231cd5da5e30eaac74de4b32fac9ed Mon Sep 17 00:00:00 2001 From: Ian Wahbe Date: Tue, 24 Sep 2024 13:48:21 +0200 Subject: [PATCH] Strip secret values from Configure --- pf/go.mod | 1 + pf/internal/plugin/provider_server.go | 37 +++++++ pf/internal/plugin/provider_server_test.go | 110 +++++++++++++++++++++ pf/tests/provider_configure_test.go | 87 ++++++++++++++++ pkg/tfbridge/config_encoding.go | 34 +++++-- 5 files changed, 260 insertions(+), 9 deletions(-) create mode 100644 pf/internal/plugin/provider_server_test.go diff --git a/pf/go.mod b/pf/go.mod index 7925c3c9b..8132e9296 100644 --- a/pf/go.mod +++ b/pf/go.mod @@ -19,6 +19,7 @@ require ( github.com/pulumi/pulumi-terraform-bridge/v3 v3.91.0 github.com/pulumi/pulumi-terraform-bridge/x/muxer v0.0.8 github.com/stretchr/testify v1.9.0 + pgregory.net/rapid v0.6.1 ) require ( diff --git a/pf/internal/plugin/provider_server.go b/pf/internal/plugin/provider_server.go index e0c9615b0..978415f80 100644 --- a/pf/internal/plugin/provider_server.go +++ b/pf/internal/plugin/provider_server.go @@ -17,6 +17,7 @@ package plugin import ( "context" + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" "github.com/pulumi/pulumi/sdk/v3/go/common/resource/plugin" "github.com/pulumi/pulumi/sdk/v3/go/common/util/contract" pulumirpc "github.com/pulumi/pulumi/sdk/v3/proto/go" @@ -80,5 +81,41 @@ func (p providerThunk) Configure( if ctx.Value(setupConfigureKey) != nil { return plugin.ConfigureResponse{}, nil } + req.Inputs = removeSecrets(req.Inputs) + contract.Assertf(!req.Inputs.ContainsSecrets(), + "Inputs to configure should not contain secrets") return p.GrpcProvider.Configure(ctx, req) } + +func removeSecrets(pMap resource.PropertyMap) resource.PropertyMap { + var remove func(resource.PropertyValue) resource.PropertyValue + remove = func(v resource.PropertyValue) resource.PropertyValue { + switch { + case v.IsArray(): + arr := make([]resource.PropertyValue, 0, len(v.ArrayValue())) + for _, v := range v.ArrayValue() { + arr = append(arr, remove(v)) + } + return resource.NewProperty(arr) + case v.IsObject(): + obj := make(resource.PropertyMap, len(v.ObjectValue())) + for k, v := range v.ObjectValue() { + obj[k] = remove(v) + } + return resource.NewProperty(obj) + case v.IsComputed(): + return resource.MakeComputed(remove(v.Input().Element)) + case v.IsOutput(): + o := v.OutputValue() + o.Secret = false + o.Element = remove(o.Element) + return resource.NewProperty(o) + case v.IsSecret(): + return remove(v.SecretValue().Element) + default: + return v + } + } + + return remove(resource.NewProperty(pMap)).ObjectValue() +} diff --git a/pf/internal/plugin/provider_server_test.go b/pf/internal/plugin/provider_server_test.go new file mode 100644 index 000000000..48355870f --- /dev/null +++ b/pf/internal/plugin/provider_server_test.go @@ -0,0 +1,110 @@ +// Copyright 2016-2024, Pulumi Corporation. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package plugin + +import ( + "testing" + + "github.com/pulumi/pulumi/sdk/v3/go/common/resource" + prapid "github.com/pulumi/pulumi/sdk/v3/go/property/testing" + "github.com/stretchr/testify/assert" + "pgregory.net/rapid" +) + +// TestRemoveSecrets checks that removeSecrets removes [resource.Secret] values and unsets +// [resource.Output.Secret] fields without making any other changes. +func TestRemoveSecrets(t *testing.T) { + + // These functions validate that a diff does not contain any non-secret changes. + var ( + validateObjectDiff func(assert.TestingT, resource.ObjectDiff) + validateArrayDiff func(assert.TestingT, resource.ArrayDiff) + validateValueDiff func(assert.TestingT, resource.ValueDiff) + ) + + validateValueDiff = func(t assert.TestingT, v resource.ValueDiff) { + switch { + case v.Old.IsOutput(): + oOld := v.Old.OutputValue() + oOld.Secret = false + if d := resource.NewProperty(oOld).DiffIncludeUnknowns(v.New); d != nil { + validateValueDiff(t, *d) + } + case v.Old.IsSecret(): + if d := v.Old.SecretValue().Element.DiffIncludeUnknowns(v.New); d != nil { + validateValueDiff(t, *d) + } + case v.Old.IsObject(): + validateObjectDiff(t, *v.Object) + case v.Old.IsArray(): + validateArrayDiff(t, *v.Array) + default: + assert.Failf(t, "", "unexpected Update.Old type %q", v.Old.TypeString()) + } + } + + validateArrayDiff = func(t assert.TestingT, diff resource.ArrayDiff) { + assert.Empty(t, diff.Adds) + assert.Empty(t, diff.Deletes) + + for _, v := range diff.Updates { + validateValueDiff(t, v) + } + } + + validateObjectDiff = func(t assert.TestingT, diff resource.ObjectDiff) { + assert.Empty(t, diff.Adds) + + // Diff does not distinguish from a missing key and a null property, so + // when we go from a map{k: secret(null)} to a map{k: null}, the diff + // machinery shows a delete. + // + // We have an explicit test for this behavior. + for _, v := range diff.Deletes { + assert.Equal(t, resource.MakeSecret(resource.NewNullProperty()), v) + } + + for _, v := range diff.Updates { + validateValueDiff(t, v) + } + } + + t.Run("rapid", rapid.MakeCheck(func(t *rapid.T) { + m := resource.ToResourcePropertyValue(prapid.Map(5).Draw(t, "top-level")).ObjectValue() + if m.ContainsSecrets() { + unsecreted := removeSecrets(m) + assert.False(t, unsecreted.ContainsSecrets()) + + // We need to assert that the only change between m and unsecreted + // is that secret values went to their element values. + if d := m.DiffIncludeUnknowns(unsecreted); d != nil { + validateObjectDiff(t, *d) + } + } else { + assert.Equal(t, m, removeSecrets(m)) + } + })) + + t.Run("map-null-secrets", func(t *testing.T) { + assert.Equal(t, + resource.PropertyMap{ + "null": resource.NewNullProperty(), + }, + removeSecrets(resource.PropertyMap{ + "null": resource.MakeSecret(resource.NewNullProperty()), + }), + ) + }) +} diff --git a/pf/tests/provider_configure_test.go b/pf/tests/provider_configure_test.go index 036610680..3ad882417 100644 --- a/pf/tests/provider_configure_test.go +++ b/pf/tests/provider_configure_test.go @@ -167,3 +167,90 @@ func TestJSONNestedConfigure(t *testing.T) { } }`) } + +func TestJSONNestedConfigureWithSecrets(t *testing.T) { + server, err := newProviderServer(t, testprovider.SyntheticTestBridgeProvider()) + require.NoError(t, err) + replay.ReplaySequence(t, server, ` +[ + { + "method": "/pulumirpc.ResourceProvider/Configure", + "request": { + "args": { + "stringConfigProp": "{\"4dabf18193072939515e22adb298388d\":\"1b47061264138c4ac30d75fd1eb44270\",\"value\":\"secret-example\"}", + "mapNestedProp": "{\"k1\":{\"4dabf18193072939515e22adb298388d\":\"1b47061264138c4ac30d75fd1eb44270\",\"value\":1},\"k2\":2}", + "listNestedProps": "[{\"4dabf18193072939515e22adb298388d\":\"1b47061264138c4ac30d75fd1eb44270\",\"value\":true},false]" + } + }, + "response": { + "supportsPreview": true, + "acceptResources": true + } + }, + { + "method": "/pulumirpc.ResourceProvider/Create", + "request": { + "urn": "urn:pulumi:test-stack::basicprogram::testbridge:index/testres:TestConfigRes::r1", + "preview": false + }, + "response": { + "id": "id-1", + "properties": { + "configCopy": "secret-example", + "id": "id-1" + } + } + } +]`) +} + +func TestConfigureWithSecrets(t *testing.T) { + server, err := newProviderServer(t, testprovider.SyntheticTestBridgeProvider()) + require.NoError(t, err) + replay.ReplaySequence(t, server, ` +[ + { + "method": "/pulumirpc.ResourceProvider/Configure", + "request": { + "args": { + "stringConfigProp": { + "4dabf18193072939515e22adb298388d": "1b47061264138c4ac30d75fd1eb44270", + "value": "secret-example" + }, + "mapNestedProp": { + "k1": { + "4dabf18193072939515e22adb298388d": "1b47061264138c4ac30d75fd1eb44270", + "value": 1 + }, + "k2": 2 + }, + "listNestedProps": [ + { + "4dabf18193072939515e22adb298388d": "1b47061264138c4ac30d75fd1eb44270", + "value": true + }, + false + ] + } + }, + "response": { + "supportsPreview": true, + "acceptResources": true + } + }, + { + "method": "/pulumirpc.ResourceProvider/Create", + "request": { + "urn": "urn:pulumi:test-stack::basicprogram::testbridge:index/testres:TestConfigRes::r1", + "preview": false + }, + "response": { + "id": "id-1", + "properties": { + "configCopy": "secret-example", + "id": "id-1" + } + } + } +]`) +} diff --git a/pkg/tfbridge/config_encoding.go b/pkg/tfbridge/config_encoding.go index ab6a14321..1cc38a815 100644 --- a/pkg/tfbridge/config_encoding.go +++ b/pkg/tfbridge/config_encoding.go @@ -1,4 +1,4 @@ -// Copyright 2016-2023, Pulumi Corporation. +// Copyright 2016-2024, Pulumi Corporation. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -68,19 +68,33 @@ func (*ConfigEncoding) tryUnwrapSecret(encoded any) (any, bool) { } func (enc *ConfigEncoding) convertStringToPropertyValue(s string, typ shim.ValueType) (resource.PropertyValue, error) { - // If the schema expects a string, we can just return this as-is. - if typ == shim.TypeString { - return resource.NewStringProperty(s), nil - } - - // Otherwise, we will attempt to deserialize the input string as JSON and convert the result into a Pulumi + // We attempt to deserialize the input string as JSON and convert the result into a Pulumi // property. If the input string is empty, we will return an appropriate zero value. + // + // We need to attempt JSON de-serialization for all incoming strings, even if the target type is also + // a string. This allows us to correctly handle secret or computed string values. For example, the + // provider might be sent an input like this: + // + // resource.PropertyMap{ + // "k": resource.NewProperty(`{"4dabf18193072939515e22adb298388d":"1b47061264138c4ac30d75fd1eb44270","value":"secret-value"}`), + // } + // + // Even if `k` is a property of type string and it's value is a string, it should be decoded as: + // + // resource.PropertyMap{ + // "k": resource.MakeSecret(resource.NewProperty("secret-value")), + // } + // + //nolint:lll if s == "" { return enc.zeroValue(typ), nil } var jsonValue interface{} if err := json.Unmarshal([]byte(s), &jsonValue); err != nil { + if typ == shim.TypeString { + return resource.NewProperty(s), nil + } return resource.PropertyValue{}, err } @@ -102,10 +116,12 @@ func (enc *ConfigEncoding) convertStringToPropertyValue(s string, typ shim.Value func (*ConfigEncoding) zeroValue(typ shim.ValueType) resource.PropertyValue { switch typ { + case shim.TypeString: + return resource.NewProperty("") case shim.TypeBool: - return resource.NewPropertyValue(false) + return resource.NewProperty(false) case shim.TypeInt, shim.TypeFloat: - return resource.NewPropertyValue(0) + return resource.NewProperty[float64](0) case shim.TypeList, shim.TypeSet: return resource.NewPropertyValue([]interface{}{}) default: