From e668412a5b82ae18009255ab89b88424d5848e44 Mon Sep 17 00:00:00 2001 From: Brandon Croft Date: Wed, 16 Mar 2022 22:47:06 -0600 Subject: [PATCH] fix: have `terraform output` adhere to authorization w/ cloud Normally, `terraform output` refreshes and reads the entire state in the command package before pulling output values out of it. This doesn't give Terraform Cloud the opportunity to apply the read state outputs org permission and instead applies the read state versions permission. I decided to expand the state manager interface to provide a separate GetRootOutputValues function in order to give the cloud backend a more nuanced opportunity to fetch just the outputs. This required moving state Refresh/Read code that was previously in the command into the shared backend state as well as the filesystem state packages. --- go.mod | 2 +- go.sum | 4 +- internal/backend/local/backend_local_test.go | 4 + internal/cloud/backend.go | 3 +- internal/cloud/backend_state_test.go | 2 +- internal/cloud/state.go | 118 +++++++++++++++++++ internal/cloud/state_test.go | 72 +++++++++++ internal/cloud/testing.go | 29 ++++- internal/cloud/tfe_client_mock.go | 45 +++++++ internal/command/output.go | 13 +- internal/states/remote/state.go | 13 ++ internal/states/remote/state_test.go | 28 +++++ internal/states/statemgr/filesystem.go | 14 +++ internal/states/statemgr/filesystem_test.go | 14 +++ internal/states/statemgr/lock.go | 4 + internal/states/statemgr/persistent.go | 11 ++ internal/states/statemgr/statemgr_fake.go | 8 ++ internal/states/statemgr/testing.go | 4 + 18 files changed, 372 insertions(+), 16 deletions(-) create mode 100644 internal/cloud/state.go create mode 100644 internal/cloud/state_test.go diff --git a/go.mod b/go.mod index fb14f3d9d574..8b4a34e87e06 100644 --- a/go.mod +++ b/go.mod @@ -41,7 +41,7 @@ require ( github.com/hashicorp/go-multierror v1.1.1 github.com/hashicorp/go-plugin v1.4.3 github.com/hashicorp/go-retryablehttp v0.7.0 - github.com/hashicorp/go-tfe v1.0.0 + github.com/hashicorp/go-tfe v1.1.1-0.20220329184205-bc3385badc7d github.com/hashicorp/go-uuid v1.0.2 github.com/hashicorp/go-version v1.3.0 github.com/hashicorp/hcl v0.0.0-20170504190234-a4b07c25de5f diff --git a/go.sum b/go.sum index 51ef2a5389ca..338a4de8da20 100644 --- a/go.sum +++ b/go.sum @@ -415,8 +415,8 @@ github.com/hashicorp/go-slug v0.8.0/go.mod h1:Ib+IWBYfEfJGI1ZyXMGNbu2BU+aa3Dzu41 github.com/hashicorp/go-sockaddr v1.0.0 h1:GeH6tui99pF4NJgfnhp+L6+FfobzVW3Ah46sLo0ICXs= github.com/hashicorp/go-sockaddr v1.0.0/go.mod h1:7Xibr9yA9JjQq1JpNB2Vw7kxv8xerXegt+ozgdvDeDU= github.com/hashicorp/go-syslog v1.0.0/go.mod h1:qPfqrKkXGihmCqbJM2mZgkZGvKG1dFdvsLplgctolz4= -github.com/hashicorp/go-tfe v1.0.0 h1:CmwoHrOs7WJfD/yEmVjJ65+dyKeVRrgvRHBLVSQQ6Ks= -github.com/hashicorp/go-tfe v1.0.0/go.mod h1:tJF/OlAXzVbmjiimAPLplSLgwg6kZDUOy0MzHuMwvF4= +github.com/hashicorp/go-tfe v1.1.1-0.20220329184205-bc3385badc7d h1:AXRz9uSle4xMabr79f4Uz7SrkMBISTzD5HF+dvw+ZY4= +github.com/hashicorp/go-tfe v1.1.1-0.20220329184205-bc3385badc7d/go.mod h1:tJF/OlAXzVbmjiimAPLplSLgwg6kZDUOy0MzHuMwvF4= github.com/hashicorp/go-uuid v1.0.0/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.1/go.mod h1:6SBZvOh/SIDV7/2o3Jml5SYk/TvGqwFJ/bN7x4byOro= github.com/hashicorp/go-uuid v1.0.2 h1:cfejS+Tpcp13yd5nYHWDI6qVCny6wyX2Mt5SGur2IGE= diff --git a/internal/backend/local/backend_local_test.go b/internal/backend/local/backend_local_test.go index 32675e0949e4..00af0a36324d 100644 --- a/internal/backend/local/backend_local_test.go +++ b/internal/backend/local/backend_local_test.go @@ -220,6 +220,10 @@ func (s *stateStorageThatFailsRefresh) State() *states.State { return nil } +func (s *stateStorageThatFailsRefresh) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return nil, fmt.Errorf("unimplemented") +} + func (s *stateStorageThatFailsRefresh) WriteState(*states.State) error { return fmt.Errorf("unimplemented") } diff --git a/internal/cloud/backend.go b/internal/cloud/backend.go index cd75bb99346c..b9b5b76456f0 100644 --- a/internal/cloud/backend.go +++ b/internal/cloud/backend.go @@ -19,7 +19,6 @@ import ( "github.com/hashicorp/terraform/internal/backend" "github.com/hashicorp/terraform/internal/configs/configschema" "github.com/hashicorp/terraform/internal/plans" - "github.com/hashicorp/terraform/internal/states/remote" "github.com/hashicorp/terraform/internal/states/statemgr" "github.com/hashicorp/terraform/internal/terraform" "github.com/hashicorp/terraform/internal/tfdiags" @@ -608,7 +607,7 @@ func (b *Cloud) StateMgr(name string) (statemgr.Full, error) { runID: os.Getenv("TFE_RUN_ID"), } - return &remote.State{Client: client}, nil + return NewState(client), nil } // Operation implements backend.Enhanced. diff --git a/internal/cloud/backend_state_test.go b/internal/cloud/backend_state_test.go index 63c970438a3c..0109b41d9994 100644 --- a/internal/cloud/backend_state_test.go +++ b/internal/cloud/backend_state_test.go @@ -33,7 +33,7 @@ func TestRemoteClient_stateLock(t *testing.T) { t.Fatalf("expected no error, got %v", err) } - remote.TestRemoteLocks(t, s1.(*remote.State).Client, s2.(*remote.State).Client) + remote.TestRemoteLocks(t, s1.(*State).Client, s2.(*State).Client) } func TestRemoteClient_withRunID(t *testing.T) { diff --git a/internal/cloud/state.go b/internal/cloud/state.go new file mode 100644 index 000000000000..ecea2a8a2423 --- /dev/null +++ b/internal/cloud/state.go @@ -0,0 +1,118 @@ +package cloud + +import ( + "context" + "encoding/json" + "fmt" + + tfe "github.com/hashicorp/go-tfe" + "github.com/hashicorp/terraform/internal/states" + "github.com/hashicorp/terraform/internal/states/remote" + "github.com/hashicorp/terraform/internal/states/statemgr" + "github.com/zclconf/go-cty/cty" + ctyjson "github.com/zclconf/go-cty/cty/json" +) + +// State is similar to remote State and delegates to it, except in the case of output values, +// which use a separate methodology that ensures the caller is authorized to read cloud +// workspace outputs. +type State struct { + Client *remoteClient + + delegate remote.State +} + +// Proof that cloud State is a statemgr.Persistent interface +var _ statemgr.Persistent = (*State)(nil) + +func NewState(client *remoteClient) *State { + return &State{ + Client: client, + delegate: remote.State{Client: client}, + } +} + +// State delegates calls to read State to the remote State +func (s *State) State() *states.State { + return s.delegate.State() +} + +// Lock delegates calls to lock state to the remote State +func (s *State) Lock(info *statemgr.LockInfo) (string, error) { + return s.delegate.Lock(info) +} + +// Unlock delegates calls to unlock state to the remote State +func (s *State) Unlock(id string) error { + return s.delegate.Unlock(id) +} + +// RefreshState delegates calls to refresh State to the remote State +func (s *State) RefreshState() error { + return s.delegate.RefreshState() +} + +// RefreshState delegates calls to refresh State to the remote State +func (s *State) PersistState() error { + return s.delegate.PersistState() +} + +// WriteState delegates calls to write State to the remote State +func (s *State) WriteState(state *states.State) error { + return s.delegate.WriteState(state) +} + +// GetRootOutputValues fetches output values from Terraform Cloud +func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { + ctx := context.Background() + + so, err := s.Client.client.StateVersionOutputs.ReadCurrent(ctx, s.Client.workspace.ID) + + if err != nil { + return nil, fmt.Errorf("Could not read state version outputs: %w", err) + } + + result := make(map[string]*states.OutputValue) + + for _, output := range so.Items { + value, err := s.cloudOutputToCtyValue(ctx, output) + if err != nil { + return nil, fmt.Errorf("Could not interpret output value as simple json: %w", err) + } + + result[output.Name] = &states.OutputValue{ + Value: *value, + Sensitive: output.Sensitive, + } + } + + return result, nil +} + +func (s *State) cloudOutputToCtyValue(ctx context.Context, output *tfe.StateVersionOutput) (*cty.Value, error) { + value := output.Value + // If an output is sensitive, the API requires that we fetch this output individually to get the value + // Terraform will decline to reveal the value under some circumstances, but we must provide it to callers + if output.Sensitive { + svo, err := s.Client.client.StateVersionOutputs.Read(ctx, output.ID) + if err != nil { + return nil, fmt.Errorf("Could not read sensitive output %s: %w", output.ID, err) + } + + value = svo.Value + } + + buf, err := json.Marshal(value) + if err != nil { + return nil, fmt.Errorf("Could not interpret output value: %w", err) + } + + v := ctyjson.SimpleJSONValue{} + err = v.UnmarshalJSON(buf) + + if err != nil { + return nil, fmt.Errorf("Could not interpret output value as simple json: %w", err) + } + + return &v.Value, err +} diff --git a/internal/cloud/state_test.go b/internal/cloud/state_test.go new file mode 100644 index 000000000000..85f3081380ac --- /dev/null +++ b/internal/cloud/state_test.go @@ -0,0 +1,72 @@ +package cloud + +import ( + "testing" + + "github.com/hashicorp/go-tfe" + "github.com/hashicorp/terraform/internal/states/statemgr" +) + +func TestState_impl(t *testing.T) { + var _ statemgr.Reader = new(State) + var _ statemgr.Writer = new(State) + var _ statemgr.Persister = new(State) + var _ statemgr.Refresher = new(State) + var _ statemgr.OutputReader = new(State) + var _ statemgr.Locker = new(State) +} + +type ExpectedOutput struct { + Name string + Sensitive bool + IsNull bool +} + +func TestState_GetRootOutputValues(t *testing.T) { + b, bCleanup := testBackendWithOutputs(t) + defer bCleanup() + + client := &remoteClient{ + client: b.client, + workspace: &tfe.Workspace{ + ID: "ws-abcd", + }, + } + + state := NewState(client) + outputs, err := state.GetRootOutputValues() + + if err != nil { + t.Fatalf("error returned from GetRootOutputValues: %s", err) + } + + cases := []ExpectedOutput{ + { + Name: "sensitive_output", + Sensitive: true, + IsNull: false, + }, + { + Name: "nonsensitive_output", + Sensitive: false, + IsNull: false, + }, + } + + if len(outputs) != len(cases) { + t.Errorf("Expected %d item but %d were returned", len(cases), len(outputs)) + } + + for _, testCase := range cases { + so, ok := outputs[testCase.Name] + if !ok { + t.Fatalf("Expected key %s but it was not found", testCase.Name) + } + if so.Value.IsNull() != testCase.IsNull { + t.Errorf("Key %s does not match null expectation %v", testCase.Name, testCase.IsNull) + } + if so.Sensitive != testCase.Sensitive { + t.Errorf("Key %s does not match sensitive expectation %v", testCase.Name, testCase.Sensitive) + } + } +} diff --git a/internal/cloud/testing.go b/internal/cloud/testing.go index e7be4748cef1..dcf899680c87 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -117,7 +117,33 @@ func testRemoteClient(t *testing.T) remote.Client { t.Fatalf("error: %v", err) } - return raw.(*remote.State).Client + return raw.(*State).Client +} + +func testBackendWithOutputs(t *testing.T) (*Cloud, func()) { + b, cleanup := testBackendWithName(t) + + // Get a new mock client to use for adding outputs + mc := NewMockClient() + + mc.StateVersionOutputs.create("svo-abcd", &tfe.StateVersionOutput{ + ID: "svo-abcd", + Value: "foobar", + Sensitive: true, + Type: "string", + Name: "sensitive_output", + }) + + mc.StateVersionOutputs.create("svo-zyxw", &tfe.StateVersionOutput{ + ID: "svo-zyxw", + Value: "bazqux", + Type: "string", + Name: "nonsensitive_output", + }) + + b.client.StateVersionOutputs = mc.StateVersionOutputs + + return b, cleanup } func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) { @@ -149,6 +175,7 @@ func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) { b.client.PolicyChecks = mc.PolicyChecks b.client.Runs = mc.Runs b.client.StateVersions = mc.StateVersions + b.client.StateVersionOutputs = mc.StateVersionOutputs b.client.Variables = mc.Variables b.client.Workspaces = mc.Workspaces diff --git a/internal/cloud/tfe_client_mock.go b/internal/cloud/tfe_client_mock.go index 9888599faede..ed1378171c10 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -29,6 +29,7 @@ type MockClient struct { PolicyChecks *MockPolicyChecks Runs *MockRuns StateVersions *MockStateVersions + StateVersionOutputs *MockStateVersionOutputs Variables *MockVariables Workspaces *MockWorkspaces } @@ -43,6 +44,7 @@ func NewMockClient() *MockClient { c.PolicyChecks = newMockPolicyChecks(c) c.Runs = newMockRuns(c) c.StateVersions = newMockStateVersions(c) + c.StateVersionOutputs = newMockStateVersionOutputs(c) c.Variables = newMockVariables(c) c.Workspaces = newMockWorkspaces(c) return c @@ -1025,6 +1027,49 @@ func (m *MockStateVersions) ListOutputs(ctx context.Context, svID string, option panic("not implemented") } +type MockStateVersionOutputs struct { + client *MockClient + outputs map[string]*tfe.StateVersionOutput +} + +func newMockStateVersionOutputs(client *MockClient) *MockStateVersionOutputs { + return &MockStateVersionOutputs{ + client: client, + outputs: make(map[string]*tfe.StateVersionOutput), + } +} + +// This is a helper function in order to create mocks to be read later +func (m *MockStateVersionOutputs) create(id string, svo *tfe.StateVersionOutput) { + m.outputs[id] = svo +} + +func (m *MockStateVersionOutputs) Read(ctx context.Context, outputID string) (*tfe.StateVersionOutput, error) { + result, ok := m.outputs[outputID] + if !ok { + return nil, tfe.ErrResourceNotFound + } + + return result, nil +} + +func (m *MockStateVersionOutputs) ReadCurrent(ctx context.Context, workspaceID string) (*tfe.StateVersionOutputsList, error) { + svl := &tfe.StateVersionOutputsList{} + for _, sv := range m.outputs { + svl.Items = append(svl.Items, sv) + } + + svl.Pagination = &tfe.Pagination{ + CurrentPage: 1, + NextPage: 1, + PreviousPage: 1, + TotalPages: 1, + TotalCount: len(svl.Items), + } + + return svl, nil +} + type MockVariables struct { client *MockClient workspaces map[string]*tfe.VariableList diff --git a/internal/command/output.go b/internal/command/output.go index 0f23a6109f43..0d50c1f09bf4 100644 --- a/internal/command/output.go +++ b/internal/command/output.go @@ -82,17 +82,12 @@ func (c *OutputCommand) Outputs(statePath string) (map[string]*states.OutputValu return nil, diags } - if err := stateStore.RefreshState(); err != nil { - diags = diags.Append(fmt.Errorf("Failed to load state: %s", err)) - return nil, diags - } - - state := stateStore.State() - if state == nil { - state = states.NewState() + output, err := stateStore.GetRootOutputValues() + if err != nil { + return nil, diags.Append(err) } - return state.RootModule().OutputValues, nil + return output, diags } func (c *OutputCommand) Help() string { diff --git a/internal/states/remote/state.go b/internal/states/remote/state.go index ca939a96a312..5348abb8f031 100644 --- a/internal/states/remote/state.go +++ b/internal/states/remote/state.go @@ -46,6 +46,19 @@ func (s *State) State() *states.State { return s.state.DeepCopy() } +func (s *State) GetRootOutputValues() (map[string]*states.OutputValue, error) { + if err := s.RefreshState(); err != nil { + return nil, fmt.Errorf("Failed to load state: %s", err) + } + + state := s.State() + if state == nil { + state = states.NewState() + } + + return state.RootModule().OutputValues, nil +} + // StateForMigration is part of our implementation of statemgr.Migrator. func (s *State) StateForMigration() *statefile.File { s.mu.Lock() diff --git a/internal/states/remote/state_test.go b/internal/states/remote/state_test.go index c38e0bfec2e5..acd17b8a4110 100644 --- a/internal/states/remote/state_test.go +++ b/internal/states/remote/state_test.go @@ -19,6 +19,7 @@ func TestState_impl(t *testing.T) { var _ statemgr.Writer = new(State) var _ statemgr.Persister = new(State) var _ statemgr.Refresher = new(State) + var _ statemgr.OutputReader = new(State) var _ statemgr.Locker = new(State) } @@ -274,6 +275,33 @@ func TestStatePersist(t *testing.T) { } } +func TestState_GetRootOutputValues(t *testing.T) { + // Initial setup of state with outputs already defined + mgr := &State{ + Client: &mockClient{ + current: []byte(` + { + "version": 4, + "lineage": "mock-lineage", + "serial": 1, + "terraform_version":"0.0.0", + "outputs": {"foo": {"value":"bar", "type": "string"}}, + "resources": [] + } + `), + }, + } + + outputs, err := mgr.GetRootOutputValues() + if err != nil { + t.Errorf("Expected GetRootOutputValues to not return an error, but it returned %v", err) + } + + if len(outputs) != 1 { + t.Errorf("Expected %d outputs, but received %d", 1, len(outputs)) + } +} + type migrationTestCase struct { name string // A function to generate a statefile diff --git a/internal/states/statemgr/filesystem.go b/internal/states/statemgr/filesystem.go index 7cd19e8b0b52..1406f046563b 100644 --- a/internal/states/statemgr/filesystem.go +++ b/internal/states/statemgr/filesystem.go @@ -233,6 +233,20 @@ func (s *Filesystem) RefreshState() error { return s.refreshState() } +func (s *Filesystem) GetRootOutputValues() (map[string]*states.OutputValue, error) { + err := s.RefreshState() + if err != nil { + return nil, err + } + + state := s.State() + if state == nil { + state = states.NewState() + } + + return state.RootModule().OutputValues, nil +} + func (s *Filesystem) refreshState() error { var reader io.Reader diff --git a/internal/states/statemgr/filesystem_test.go b/internal/states/statemgr/filesystem_test.go index 5a73819d60a2..ba9565b80e42 100644 --- a/internal/states/statemgr/filesystem_test.go +++ b/internal/states/statemgr/filesystem_test.go @@ -340,6 +340,7 @@ func TestFilesystem_impl(t *testing.T) { var _ Writer = new(Filesystem) var _ Persister = new(Filesystem) var _ Refresher = new(Filesystem) + var _ OutputReader = new(Filesystem) var _ Locker = new(Filesystem) } @@ -414,6 +415,19 @@ func TestFilesystem_refreshWhileLocked(t *testing.T) { } } +func TestFilesystem_GetRootOutputValues(t *testing.T) { + fs := testFilesystem(t) + + outputs, err := fs.GetRootOutputValues() + if err != nil { + t.Errorf("Expected GetRootOutputValues to not return an error, but it returned %v", err) + } + + if len(outputs) != 2 { + t.Errorf("Expected %d outputs, but received %d", 2, len(outputs)) + } +} + func testOverrideVersion(t *testing.T, v string) func() { oldVersionStr := tfversion.Version oldPrereleaseStr := tfversion.Prerelease diff --git a/internal/states/statemgr/lock.go b/internal/states/statemgr/lock.go index 190e06ea7de8..79c149fe736e 100644 --- a/internal/states/statemgr/lock.go +++ b/internal/states/statemgr/lock.go @@ -15,6 +15,10 @@ func (s *LockDisabled) State() *states.State { return s.Inner.State() } +func (s *LockDisabled) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return s.Inner.GetRootOutputValues() +} + func (s *LockDisabled) WriteState(v *states.State) error { return s.Inner.WriteState(v) } diff --git a/internal/states/statemgr/persistent.go b/internal/states/statemgr/persistent.go index c15e84af2dc3..73b2124fc000 100644 --- a/internal/states/statemgr/persistent.go +++ b/internal/states/statemgr/persistent.go @@ -2,6 +2,7 @@ package statemgr import ( version "github.com/hashicorp/go-version" + "github.com/hashicorp/terraform/internal/states" ) // Persistent is a union of the Refresher and Persistent interfaces, for types @@ -16,6 +17,16 @@ import ( type Persistent interface { Refresher Persister + OutputReader +} + +// OutputReader is the interface for managers that fetches output values from state +// or another source. This is a refinement of fetching the entire state and digging +// the output values from it because enhanced backends can apply special permissions +// to differentiate reading the state and reading the outputs within the state. +type OutputReader interface { + // GetRootOutputValues fetches the root module output values from state or another source + GetRootOutputValues() (map[string]*states.OutputValue, error) } // Refresher is the interface for managers that can read snapshots from diff --git a/internal/states/statemgr/statemgr_fake.go b/internal/states/statemgr/statemgr_fake.go index a547c1bc7c4f..8d88e4d24e7e 100644 --- a/internal/states/statemgr/statemgr_fake.go +++ b/internal/states/statemgr/statemgr_fake.go @@ -65,6 +65,10 @@ func (m *fakeFull) PersistState() error { return m.fakeP.WriteState(m.t.State()) } +func (m *fakeFull) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return m.State().RootModule().OutputValues, nil +} + func (m *fakeFull) Lock(info *LockInfo) (string, error) { m.lockLock.Lock() defer m.lockLock.Unlock() @@ -111,6 +115,10 @@ func (m *fakeErrorFull) State() *states.State { return nil } +func (m *fakeErrorFull) GetRootOutputValues() (map[string]*states.OutputValue, error) { + return nil, errors.New("fake state manager error") +} + func (m *fakeErrorFull) WriteState(s *states.State) error { return errors.New("fake state manager error") } diff --git a/internal/states/statemgr/testing.go b/internal/states/statemgr/testing.go index 82cecc0de55e..171b21ad2ea0 100644 --- a/internal/states/statemgr/testing.go +++ b/internal/states/statemgr/testing.go @@ -155,5 +155,9 @@ func TestFullInitialState() *states.State { Module: addrs.RootModule, } childMod.SetResourceProvider(rAddr, providerAddr) + + state.RootModule().SetOutputValue("sensitive_output", cty.StringVal("it's a secret"), true) + state.RootModule().SetOutputValue("nonsensitive_output", cty.StringVal("hello, world!"), false) + return state }