From 0777ced17fb246bdc72feb5513a3af60feacbae3 Mon Sep 17 00:00:00 2001 From: Aditya Kalia <32119652+akalia25@users.noreply.github.com> Date: Thu, 27 Jun 2024 15:22:58 -0400 Subject: [PATCH] V2-telemetry: Simplify sensitive metadata allowlist to accept feature only (#63325) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This PR adds another option to the sensitive metadata allowlist by simplifying the input requirements. to accept `feature` and the allowlisted `privateMetadata` keys for that feature. This change is particularly beneficial when a `feature` has multiple associated `action`(s), and the `privateMetadata` key needs to be allowed for all events related to that feature. Building upon this initial PR: - https://github.com/sourcegraph/sourcegraph/pull/62830/files ## Test plan CI and unit tests ## Changelog --- .../sensitivemetadataallowlist.go | 114 ++++++++++++++++-- .../sensitivemetadataallowlist_test.go | 67 ++++++++-- 2 files changed, 158 insertions(+), 23 deletions(-) diff --git a/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go b/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go index a05461b7741..183a395442f 100644 --- a/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go +++ b/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist.go @@ -37,39 +37,123 @@ func AllowedEventTypes() EventTypes { "testField", }, }, - // The 'languageId' key is included for feature:'cody.completions' action:suggested/accepted events to provide + // The 'languageId' key is included for feature:'cody.completions' events to provide // customers with valuable language-specific insights from the analytics we offer. // This information helps them better understand code completion usage patterns. EventType{ Feature: "cody.completion", - Action: "suggested", + Action: "*", AllowedPrivateMetadataKeys: []string{ "languageId", }, }, - // The 'languageId' key is included for feature:'cody.completions' action:suggested/accepted events to provide + // The 'languageId' key is included for feature:'cody.hoverCommands' action: 'visible' events to provide // customers with valuable language-specific insights from the analytics we offer. // This information helps them better understand code completion usage patterns. EventType{ - Feature: "cody.completion", - Action: "accepted", + Feature: "cody.hoverCommands", + Action: "visible", AllowedPrivateMetadataKeys: []string{ "languageId", }, }, + // The 'languageId' key is included for feature:'blob.codeintel' events to provide + // customers with valuable language-specific insights from the analytics we offer. + // This information helps them better understand code completion usage patterns. + EventType{ + Feature: "blob.codeintel", + Action: "*", + AllowedPrivateMetadataKeys: []string{ + "languageId", + }, + }, + // The 'chatModel' key is included for feature:'cody.chat-question' action:'submitted' events to provide + // customers with valuable chat-model specific insights from the analytics we offer. + // Access to this information can help customers determine which models best suit their specific use cases + EventType{ + Feature: "cody.chat-question", + Action: "submitted", + AllowedPrivateMetadataKeys: []string{ + "chatModel", + }, + }, + // The 'chatModel' key is included for feature:'cody.chatResponse' events to provide + // customers with valuable chat-model specific insights from the analytics we offer. + // Access to this information can help customers determine which models best suit their specific use cases + EventType{ + Feature: "cody.chatResponse", + Action: "*", + AllowedPrivateMetadataKeys: []string{ + "chatModel", + }, + }, + // The 'chatModel' key is included for feature:'cody.chatResponseNew' events to provide + // customers with valuable chat-model specific insights from the analytics we offer. + // Access to this information can help customers determine which models best suit their specific use cases + EventType{ + Feature: "cody.chatResponseNew", + Action: "*", + AllowedPrivateMetadataKeys: []string{ + "chatModel", + }, + }, + // The 'model' key is included for feature:'cody.command.doc' action: 'executed' events to provide + // customers with valuable chat-model specific insights from the analytics we offer. + // Access to this information can help customers determine which models best suit their specific use cases + EventType{ + Feature: "cody.command.doc", + Action: "executed", + AllowedPrivateMetadataKeys: []string{ + "model", + }, + }, + // The 'model' key is included for feature:'cody.command.edit' action: 'executed' events to provide + // customers with valuable chat-model specific insights from the analytics we offer. + // Access to this information can help customers determine which models best suit their specific use cases + EventType{ + Feature: "cody.command.edit", + Action: "executed", + AllowedPrivateMetadataKeys: []string{ + "model", + }, + }, + // The 'model' key is included for feature:'cody.command.fix' action: 'executed' events to provide + // customers with valuable chat-model specific insights from the analytics we offer. + // Access to this information can help customers determine which models best suit their specific use cases + EventType{ + Feature: "cody.command.fix", + Action: "executed", + AllowedPrivateMetadataKeys: []string{ + "model", + }, + }, + // The 'model' key is included for feature:'cody.command.test' action: 'executed' events to provide + // customers with valuable chat-model specific insights from the analytics we offer. + // Access to this information can help customers determine which models best suit their specific use cases + EventType{ + Feature: "cody.command.test", + Action: "executed", + AllowedPrivateMetadataKeys: []string{ + "model", + }, + }, )...) } type EventTypes struct { types []EventType - // index of '{feature}.{action}:{allowedfields}' for checking + // index of '{feature}/{action}:{allowedfields}' for checking index map[string][]string } func eventTypes(types ...EventType) EventTypes { index := make(map[string][]string, len(types)) for _, t := range types { - index[fmt.Sprintf("%s.%s", t.Feature, t.Action)] = t.AllowedPrivateMetadataKeys + key := fmt.Sprintf("%s/%s", t.Feature, t.Action) + if t.Action == "*" { + key = t.Feature + } + index[key] = t.AllowedPrivateMetadataKeys } return EventTypes{types: types, index: index} } @@ -90,9 +174,19 @@ func (e EventTypes) Redact(event *telemetrygatewayv1.Event) redactMode { // IsAllowed indicates an event is on the sensitive telemetry allowlist, and the fields that // are allowed. func (e EventTypes) IsAllowed(event *telemetrygatewayv1.Event) ([]string, bool) { - key := fmt.Sprintf("%s.%s", event.GetFeature(), event.GetAction()) - allowedKeys, allowed := e.index[key] - return allowedKeys, allowed + // Check for the specific feature/action combination + key := fmt.Sprintf("%s/%s", event.GetFeature(), event.GetAction()) + if allowedKeys, allowed := e.index[key]; allowed { + return allowedKeys, true + } + + // Check for the feature-wide entry + if allowedKeys, allowed := e.index[event.GetFeature()]; allowed { + return allowedKeys, true + } + + // If neither specific nor feature-wide entry is found, return false + return nil, false } func (e EventTypes) validate() error { diff --git a/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go b/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go index b3d71ac8f88..ea2f20c3755 100644 --- a/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go +++ b/internal/telemetry/sensitivemetadataallowlist/sensitivemetadataallowlist_test.go @@ -41,7 +41,8 @@ func TestIsAllowed(t *testing.T) { expectAllowed: false, }, { - name: "disallowed event with additional allowed event type", + name: "event with private metadata, that has fields it is allowed to export (feature only)", + // This feature's action is defined as a wildcard(*) in the "IsAllowed" check's list of known events event: &v1.Event{ Feature: "cody.completion", Action: "accepted", @@ -51,6 +52,17 @@ func TestIsAllowed(t *testing.T) { "languageId", }, }, + { + name: "event with private metadata, that has fields it is allowed to export (feature and action)", + event: &v1.Event{ + Feature: "cody.hoverCommands", + Action: "visible", + }, + expectAllowed: true, + expectAllowlist: []string{ + "languageId", + }, + }, } { t.Run(tc.name, func(t *testing.T) { allowedKeys, allowed := allowedTypes.IsAllowed(tc.event) @@ -82,30 +94,27 @@ func TestParseAdditionalAllowedEventTypes(t *testing.T) { name: "1 type", config: "foo::bar::field", expect: autogold.Expect([]EventType{{ - Feature: "foo", - Action: "bar", - AllowedPrivateMetadataKeys: []string{ - "field", - }, + Feature: "foo", + Action: "bar", + AllowedPrivateMetadataKeys: []string{"field"}, }}), }, { name: "multiple types", - config: "foo::bar::field::field2,baz.bar::bar.baz::field", + config: "foo::bar::field::field2,baz.bar::*::field", expect: autogold.Expect([]EventType{ { Feature: "foo", Action: "bar", AllowedPrivateMetadataKeys: []string{ - "field", "field2", + "field", + "field2", }, }, { - Feature: "baz.bar", - Action: "bar.baz", - AllowedPrivateMetadataKeys: []string{ - "field", - }, + Feature: "baz.bar", + Action: "*", + AllowedPrivateMetadataKeys: []string{"field"}, }, }), }, @@ -186,5 +195,37 @@ func TestEventTypesRedact(t *testing.T) { assert.Equal(t, redactAllSensitive, mode) assert.Nil(t, ev.Parameters.PrivateMetadata) }) + t.Run("allowlisted with wildcard(*) action", func(t *testing.T) { + allowedTypes := eventTypes(EventType{ + Feature: "example", + Action: "*", + AllowedPrivateMetadataKeys: []string{"foo"}, + }) + ev := &v1.Event{ + Feature: "example", + Action: "randomAction", + Parameters: &v1.EventParameters{ + PrivateMetadata: &structpb.Struct{ + Fields: map[string]*structpb.Value{ + "foo": { + Kind: &structpb.Value_StringValue{ + StringValue: "allowed", + }, + }, + "bar": { + Kind: &structpb.Value_StringValue{ + StringValue: "redacted", + }, + }, + }, + }, + }} + mode := allowedTypes.Redact(ev) + assert.Equal(t, redactMarketingAndUnallowedPrivateMetadataKeys, mode) + + // assert that only the allowlisted privateMetadata key (foo) has a value + assert.Equal(t, "allowed", ev.Parameters.PrivateMetadata.Fields["foo"].GetStringValue()) + assert.Nil(t, ev.Parameters.PrivateMetadata.Fields["bar"]) + }) }) }