From c33c8b013f556e09233cc24b1d32f561cfb074e5 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. --- internal/backend/local/backend_local_test.go | 4 + internal/cloud/backend.go | 3 +- internal/cloud/backend_state_test.go | 2 +- internal/cloud/state.go | 110 +++++++++++++++++++ internal/cloud/state_test.go | 82 ++++++++++++++ internal/cloud/testing.go | 66 ++++++++++- 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 + 16 files changed, 408 insertions(+), 13 deletions(-) create mode 100644 internal/cloud/state.go create mode 100644 internal/cloud/state_test.go diff --git a/internal/backend/local/backend_local_test.go b/internal/backend/local/backend_local_test.go index 32675e0949..00af0a3632 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 989fcc26eb..fa3ed98f85 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" @@ -628,7 +627,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 d28d4d65b8..1414274000 100644 --- a/internal/cloud/backend_state_test.go +++ b/internal/cloud/backend_state_test.go @@ -78,7 +78,7 @@ func TestRemoteClient_TestRemoteLocks(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 0000000000..739d422c59 --- /dev/null +++ b/internal/cloud/state.go @@ -0,0 +1,110 @@ +package cloud + +import ( + "context" + "encoding/json" + "fmt" + + "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" + "github.com/zclconf/go-cty/cty/gocty" +) + +// 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 { + if output.Sensitive { + // Since this is a sensitive value, the output must be requested explicitly in order to + // read its value, which is assumed to be present by callers + sensitiveOutput, err := s.Client.client.StateVersionOutputs.Read(ctx, output.ID) + if err != nil { + return nil, fmt.Errorf("could not read state version output %s: %w", output.ID, err) + } + output.Value = sensitiveOutput.Value + } + + bufType, err := json.Marshal(output.DetailedType) + if err != nil { + return nil, fmt.Errorf("could not marshal output %s type: %w", output.ID, err) + } + + var ctype cty.Type + err = ctype.UnmarshalJSON(bufType) + if err != nil { + return nil, fmt.Errorf("could not interpret output %s type: %w", output.ID, err) + } + + cval, err := gocty.ToCtyValue(output.Value, ctype) + if err != nil { + return nil, fmt.Errorf("could not interpret value %v as type %s for output %s: %w", cval, ctype.FriendlyName(), output.ID, err) + } + + result[output.Name] = &states.OutputValue{ + Value: cval, + Sensitive: output.Sensitive, + } + } + + return result, nil +} diff --git a/internal/cloud/state_test.go b/internal/cloud/state_test.go new file mode 100644 index 0000000000..331daff661 --- /dev/null +++ b/internal/cloud/state_test.go @@ -0,0 +1,82 @@ +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, + }, + { + Name: "object_output", + Sensitive: false, + IsNull: false, + }, + { + Name: "list_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 4297540fca..c800178e99 100644 --- a/internal/cloud/testing.go +++ b/internal/cloud/testing.go @@ -2,6 +2,7 @@ package cloud import ( "context" + "encoding/json" "fmt" "io" "net/http" @@ -117,7 +118,69 @@ 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", + DetailedType: "string", + }) + + mc.StateVersionOutputs.create("svo-zyxw", &tfe.StateVersionOutput{ + ID: "svo-zyxw", + Value: "bazqux", + Type: "string", + Name: "nonsensitive_output", + DetailedType: "string", + }) + + var dt interface{} + var val interface{} + err := json.Unmarshal([]byte(`["object", {"foo":"string"}]`), &dt) + if err != nil { + t.Fatalf("could not unmarshal detailed type: %s", err) + } + err = json.Unmarshal([]byte(`{"foo":"bar"}`), &val) + if err != nil { + t.Fatalf("could not unmarshal value: %s", err) + } + mc.StateVersionOutputs.create("svo-efgh", &tfe.StateVersionOutput{ + ID: "svo-efgh", + Value: val, + Type: "object", + Name: "object_output", + DetailedType: dt, + }) + + err = json.Unmarshal([]byte(`["list", "bool"]`), &dt) + if err != nil { + t.Fatalf("could not unmarshal detailed type: %s", err) + } + err = json.Unmarshal([]byte(`[true, false, true, true]`), &val) + if err != nil { + t.Fatalf("could not unmarshal value: %s", err) + } + mc.StateVersionOutputs.create("svo-ijkl", &tfe.StateVersionOutput{ + ID: "svo-ijkl", + Value: val, + Type: "array", + Name: "list_output", + DetailedType: dt, + }) + + b.client.StateVersionOutputs = mc.StateVersionOutputs + + return b, cleanup } func testBackend(t *testing.T, obj cty.Value) (*Cloud, func()) { @@ -149,6 +212,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 cd27af8f40..2ac259a2eb 100644 --- a/internal/cloud/tfe_client_mock.go +++ b/internal/cloud/tfe_client_mock.go @@ -30,6 +30,7 @@ type MockClient struct { PolicyChecks *MockPolicyChecks Runs *MockRuns StateVersions *MockStateVersions + StateVersionOutputs *MockStateVersionOutputs Variables *MockVariables Workspaces *MockWorkspaces } @@ -44,6 +45,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 @@ -1029,6 +1031,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 f53f6e206a..0bcde54e8f 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 ca939a96a3..5348abb8f0 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 9687d11f1b..1089ba1aae 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) } @@ -276,6 +277,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 7cd19e8b0b..1406f04656 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 1d319920f5..2f6285fbd1 100644 --- a/internal/states/statemgr/filesystem_test.go +++ b/internal/states/statemgr/filesystem_test.go @@ -336,6 +336,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) } @@ -410,6 +411,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 190e06ea7d..79c149fe73 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 c15e84af2d..73b2124fc0 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 a547c1bc7c..8d88e4d24e 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 82cecc0de5..171b21ad2e 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 }