From 49ca810cc2a66f93f6f4cdfa7a62bf489bbc4856 Mon Sep 17 00:00:00 2001 From: "Sebastian (Tiedtke) Huckleberry" Date: Mon, 9 Dec 2024 16:14:00 -0800 Subject: [PATCH] Fix SensitiveKeys - broke when custom spec types were introduced (#714) Only return truly sensitive keys. Introduce a test case to guard the feature against breakage. The assertions were incomplete. --- internal/owl/graph.go | 41 +++++++++++++++++++++++++++++++------- internal/owl/graph_test.go | 35 ++++++++++++++++++++++++-------- internal/owl/store.go | 8 +++++--- internal/owl/store_test.go | 19 ++++++++++++++++++ 4 files changed, 85 insertions(+), 18 deletions(-) diff --git a/internal/owl/graph.go b/internal/owl/graph.go index b8b11801f..234360ee6 100644 --- a/internal/owl/graph.go +++ b/internal/owl/graph.go @@ -27,9 +27,10 @@ const ( ) type atomicType struct { - typeName string - typeObject *graphql.Object - resolveFn graphql.FieldResolveFn + typeName string + typeSensitive bool + typeObject *graphql.Object + resolveFn graphql.FieldResolveFn } var ( @@ -150,9 +151,10 @@ func registerAtomicType(name string, sensitive, mask bool, resolver graphql.Fiel }) return &atomicType{ - typeName: name, - typeObject: typ, - resolveFn: resolver, + typeName: name, + typeSensitive: sensitive && mask, + typeObject: typ, + resolveFn: resolver, } } @@ -291,6 +293,7 @@ func resolveSensitiveKeys() graphql.FieldResolveFn { return func(p graphql.ResolveParams) (interface{}, error) { sensitive := SetVarItems{} var opSet *OperationSet + var specOpSet *SpecOperationSet switch p.Source.(type) { case nil, string: @@ -299,17 +302,41 @@ func resolveSensitiveKeys() graphql.FieldResolveFn { case *OperationSet: opSet = p.Source.(*OperationSet) case *SpecOperationSet: - opSet = p.Source.(*SpecOperationSet).OperationSet + specOpSet = p.Source.(*SpecOperationSet) + opSet = specOpSet.OperationSet default: return nil, errors.New("source does not contain an OperationSet") } + specDefs, ok := p.Context.Value(OwlEnvSpecDefsKey).(SpecDefs) + if !ok { + return nil, errors.New("missing specDefs in context") + } + + sensitiveAtomics := make(map[string]bool) + for _, a := range AtomicTypes { + sensitiveAtomics[a.typeName] = a.typeSensitive + } + for _, v := range opSet.values { s, ok := opSet.specs[v.Var.Key] if !ok { return nil, fmt.Errorf("missing spec for %s", v.Var.Key) } + specName := s.Spec.Name + + if specOpSet != nil { + _, aitem, err := specOpSet.GetAtomic(s, specDefs) + if err == nil { + specName = aitem.Spec.Name + } + } + + if !sensitiveAtomics[specName] { + continue + } + item := &SetVarItem{ Var: v.Var, Spec: s.Spec, diff --git a/internal/owl/graph_test.go b/internal/owl/graph_test.go index 1334f8c36..acd88d8ff 100644 --- a/internal/owl/graph_test.go +++ b/internal/owl/graph_test.go @@ -1,6 +1,7 @@ package owl import ( + "context" "encoding/json" "fmt" "os" @@ -81,10 +82,12 @@ func (testCases fileTestCases) runAll(t *testing.T) { tc.pre(t, vars, &query) } + noSpecDefs := SpecDefs{} result := graphql.Do(graphql.Params{ Schema: Schema, RequestString: string(query), VariableValues: vars, + Context: context.WithValue(context.Background(), OwlEnvSpecDefsKey, noSpecDefs), }) require.False(t, result.HasErrors()) @@ -225,15 +228,31 @@ func TestGraph_SensitiveKeys(t *testing.T) { { name: "Sensitive keys", post: func(t *testing.T, result *graphql.Result) { - render, err := extractDataKey(result.Data, "render") + sensitiveKeys, err := extractDataKey(result.Data, "sensitiveKeys") require.NoError(t, err) - require.NotNil(t, render) - - b, err := yaml.Marshal(render) - // b, err := json.MarshalIndent(result, "", " ") - require.NoError(t, err) - _, _ = fmt.Println(string(b)) - require.NotNil(t, b) + require.NotNil(t, sensitiveKeys) + + expected := []interface{}{ + map[string]interface{}{ + "spec": map[string]interface{}{ + "name": "Password", + "required": true, + }, + "var": map[string]interface{}{ + "key": "KRAFTCLOUD_TOKEN", + }, + }, + map[string]interface{}{ + "spec": map[string]interface{}{ + "name": "Secret", + "required": true, + }, + "var": map[string]interface{}{ + "key": "OPENAI_API_KEY", + }, + }, + } + require.EqualValues(t, expected, sensitiveKeys) }, }, } diff --git a/internal/owl/store.go b/internal/owl/store.go index e872b2ddb..45742fdbd 100644 --- a/internal/owl/store.go +++ b/internal/owl/store.go @@ -178,9 +178,11 @@ func (res SetVarItems) getSensitiveKeys(data interface{}) ([]string, error) { var filtered []string for _, item := range res { - if specsSensitivity[item.Spec.Name] { - filtered = append(filtered, item.Var.Key) - } + // todo(sebastian): check sensitivity inside graph for now + // if specsSensitivity[item.Spec.Name] { + // filtered = append(filtered, item.Var.Key) + // } + filtered = append(filtered, item.Var.Key) } return filtered, nil diff --git a/internal/owl/store_test.go b/internal/owl/store_test.go index 5bb507d28..ad8b414ca 100644 --- a/internal/owl/store_test.go +++ b/internal/owl/store_test.go @@ -757,3 +757,22 @@ func TestStore_LoadEnvSpecDefs(t *testing.T) { require.Equal(t, 0, errors) }) } + +func TestStore_InsecureValues(t *testing.T) { + t.Run("Custom", func(t *testing.T) { + store, err := NewStore( + WithSpecFile(".env.example", []byte(`SLACK_CLIENT_ID="Client ID for Slack" # Slack! +SLACK_CLIENT_SECRET="Client Secret for Slack" # Slack! +SLACK_REDIRECT_URL="Redirect URL for Slack. Use a tunnel with ngrok" # Slack!`)), + WithEnvFile(".env.local", []byte(`SLACK_CLIENT_ID=abc-123-xyz +SLACK_CLIENT_SECRET=this-is-my-secret`)), + WithEnvFile(".env", []byte(`SLACK_REDIRECT_URL='https://my-redirect-url.com/slack/redirect'`)), + ) + require.NoError(t, err) + require.NotNil(t, store) + + keys, err := store.SensitiveKeys() + require.NoError(t, err) + require.EqualValues(t, []string{"SLACK_CLIENT_SECRET", "SLACK_REDIRECT_URL"}, keys) + }) +}