Information Disclosure

PoC: Demonstrates potential data disclosure when reading at a deletion RV (pre-fix behavior) and the mitigated behavior after the fix. - Scenario (pre-fix vulnerability): a resource is created, then deleted. Reading the resource at the exact deletion RV could return the last known data (Value) from before deletion, leaking sensitive information. Go-like pseudocode illustrating the vulnerability before the fix: // Setup backend and resource key resKey := &resourcepb.ResourceKey{Name: "leak", Namespace: "default", Group: "grafana.org", Resource: "secret"} ctx := context.Background() b, _ := setupBackendTest(t) // Create resource with data rvAdd, _ := WriteEvent(ctx, b, "leak", resourcepb.WatchEvent_ADDED, WithNamespace("default"), WithValue("secret-data")) // Delete resource (at a later RV) rvDel, _ := WriteEvent(ctx, b, "leak", resourcepb.WatchEvent_DELETED, WithNamespace("default"), WithValue("secret-data")) // Read at the deletion RV. Before the fix, this would leak the last known state ("secret-data"). resp := b.ReadResource(ctx, &resourcepb.ReadRequest{Key: resKey, ResourceVersion: rvDel}) if resp.Error != nil { t.Fatalf("unexpected error: %v", resp.Error) } // Vulnerable behavior would reveal the leaked data in resp.Value fmt.Printf("Leaked value at deleted RV: %s\n", string(resp.Value)) // After the fix (the actual production behavior now): reading at the deletion RV returns NotFound respAfterFix := b.ReadResource(ctx, &resourcepb.ReadRequest{Key: resKey, ResourceVersion: rvDel}) if respAfterFix.Error == nil || respAfterFix.Error.Code != 404 { t.Fatalf("expected NotFound after fix, got: %#v", respAfterFix) } Notes: - The vulnerable path would show the Value field containing the pre-delete data when querying at the deletion RV. The fix ensures a NotFound error is returned instead of leaking data. - This PoC is conceptual; exact API calls depend on the test harness for grafana/grafana storage tests.

diff --git a/pkg/storage/unified/sql/backend.go b/pkg/storage/unified/sql/backend.go index a95e385c4afe9..d4eabb3ba1bdf 100644 --- a/pkg/storage/unified/sql/backend.go +++ b/pkg/storage/unified/sql/backend.go @@ -1263,10 +1263,10 @@ func (b *backend) readHistory(ctx context.Context, key *resourcepb.ResourceKey, Key: key, ResourceVersion: rv, }, - Response: NewReadResponse(), + Response: NewHistoryReadResponse(), } - var res *resource.BackendReadResponse + var res *historyReadResponse err := b.db.WithTx(ctx, ReadCommittedRO, func(ctx context.Context, tx db.Tx) error { var err error res, err = dbutil.QueryRow(ctx, tx, sqlResourceHistoryRead, readReq) @@ -1279,8 +1279,11 @@ func (b *backend) readHistory(ctx context.Context, key *resourcepb.ResourceKey, if err != nil { return &resource.BackendReadResponse{Error: resource.AsErrorResult(err)} } + if res.Action == int(resourcepb.WatchEvent_DELETED) { + return &resource.BackendReadResponse{Error: resource.NewNotFoundError(key)} + } - return res + return res.ReadResponse() } // getHistory fetches the resource history from the resource_history table. diff --git a/pkg/storage/unified/sql/backend_test.go b/pkg/storage/unified/sql/backend_test.go index 63bf6592428d4..7d2467a150e2e 100644 --- a/pkg/storage/unified/sql/backend_test.go +++ b/pkg/storage/unified/sql/backend_test.go @@ -477,6 +477,7 @@ type readHistoryRow struct { name string folder string resource_version string + action int value string } @@ -552,10 +553,11 @@ func TestBackend_ReadResource(t *testing.T) { name: "nm", folder: "folder", resource_version: "300", + action: int(resourcepb.WatchEvent_ADDED), value: "rv-300", } - readHistoryColumns := []string{"guid", "namespace", "group", "resource", "name", "folder", "resource_version", "value"} + readHistoryColumns := []string{"guid", "namespace", "group", "resource", "name", "folder", "resource_version", "action", "value"} b.SQLMock.ExpectBegin() b.SQLMock.ExpectQuery("SELECT .* FROM resource_history"). WillReturnRows(sqlmock.NewRows(readHistoryColumns). @@ -567,6 +569,7 @@ func TestBackend_ReadResource(t *testing.T) { expectedReadRow.name, expectedReadRow.folder, expectedReadRow.resource_version, + expectedReadRow.action, expectedReadRow.value, )) b.SQLMock.ExpectCommit() @@ -582,6 +585,49 @@ func TestBackend_ReadResource(t *testing.T) { require.Equal(t, "folder", rps.Folder) }) + t.Run("with resource version at deleted row", func(t *testing.T) { + t.Parallel() + b, ctx := setupBackendTest(t) + + expectedReadRow := readHistoryRow{ + guid: "guid", + namespace: "ns", + group: "gr", + resource: "rs", + name: "nm", + folder: "folder", + resource_version: "400", + action: int(resourcepb.WatchEvent_DELETED), + value: "rv-400", + } + + readHistoryColumns := []string{"guid", "namespace", "group", "resource", "name", "folder", "resource_version", "action", "value"} + b.SQLMock.ExpectBegin() + b.SQLMock.ExpectQuery("SELECT .* FROM resource_history"). + WillReturnRows(sqlmock.NewRows(readHistoryColumns). + AddRow( + expectedReadRow.guid, + expectedReadRow.namespace, + expectedReadRow.group, + expectedReadRow.resource, + expectedReadRow.name, + expectedReadRow.folder, + expectedReadRow.resource_version, + expectedReadRow.action, + expectedReadRow.value, + )) + b.SQLMock.ExpectCommit() + + req := &resourcepb.ReadRequest{ + Key: resKey, + ResourceVersion: 400, + } + rps := b.ReadResource(ctx, req) + require.NotNil(t, rps) + require.NotNil(t, rps.Error) + require.Equal(t, int32(404), rps.Error.Code) + }) + t.Run("error reading resource", func(t *testing.T) { t.Parallel() b, ctx := setupBackendTest(t) diff --git a/pkg/storage/unified/sql/data/resource_history_read.sql b/pkg/storage/unified/sql/data/resource_history_read.sql index e9bf729da9834..7f4afb7b6db6d 100644 --- a/pkg/storage/unified/sql/data/resource_history_read.sql +++ b/pkg/storage/unified/sql/data/resource_history_read.sql @@ -6,6 +6,7 @@ SELECT {{ .Ident "name" | .Into .Response.Key.Name }}, {{ .Ident "folder" | .Into .Response.Folder }}, {{ .Ident "resource_version" | .Into .Response.ResourceVersion }}, + {{ .Ident "action" | .Into .Response.Action }}, {{ .Ident "value" | .Into .Response.Value }} FROM {{ .Ident "resource_history" }} diff --git a/pkg/storage/unified/sql/queries.go b/pkg/storage/unified/sql/queries.go index 01e9712a02fb7..54b56d951ed45 100644 --- a/pkg/storage/unified/sql/queries.go +++ b/pkg/storage/unified/sql/queries.go @@ -268,17 +268,42 @@ type historyReadRequest struct { ResourceVersion int64 } +type historyReadResponse struct { + Key *resourcepb.ResourceKey + Folder string + GUID string + ResourceVersion int64 + Value []byte + Action int +} + +func NewHistoryReadResponse() *historyReadResponse { + return &historyReadResponse{ + Key: &resourcepb.ResourceKey{}, + } +} + +func (r *historyReadResponse) ReadResponse() *resource.BackendReadResponse { + return &resource.BackendReadResponse{ + Key: r.Key, + Folder: r.Folder, + GUID: r.GUID, + ResourceVersion: r.ResourceVersion, + Value: r.Value, + } +} + type sqlResourceHistoryReadRequest struct { sqltemplate.SQLTemplate Request *historyReadRequest - Response *resource.BackendReadResponse + Response *historyReadResponse } func (r sqlResourceHistoryReadRequest) Validate() error { return nil // TODO } -func (r sqlResourceHistoryReadRequest) Results() (*resource.BackendReadResponse, error) { +func (r sqlResourceHistoryReadRequest) Results() (*historyReadResponse, error) { return r.Response, nil } diff --git a/pkg/storage/unified/sql/queries_test.go b/pkg/storage/unified/sql/queries_test.go index 9f5844ef3f081..c9ac4080f82ff 100644 --- a/pkg/storage/unified/sql/queries_test.go +++ b/pkg/storage/unified/sql/queries_test.go @@ -277,7 +277,7 @@ func TestUnifiedStorageQueries(t *testing.T) { Name: "nm", }, }, - Response: NewReadResponse(), + Response: NewHistoryReadResponse(), }, }, }, diff --git a/pkg/storage/unified/sql/testdata/mysql--resource_history_read-single path.sql b/pkg/storage/unified/sql/testdata/mysql--resource_history_read-single path.sql index 5c6c991ed6763..683f9a9ec629b 100755 --- a/pkg/storage/unified/sql/testdata/mysql--resource_history_read-single path.sql +++ b/pkg/storage/unified/sql/testdata/mysql--resource_history_read-single path.sql @@ -6,6 +6,7 @@ SELECT `name`, `folder`, `resource_version`, + `action`, `value` FROM `resource_history` WHERE 1 = 1 diff --git a/pkg/storage/unified/sql/testdata/postgres--resource_history_read-single path.sql b/pkg/storage/unified/sql/testdata/postgres--resource_history_read-single path.sql index 9a423e4302604..836d9d91229ad 100755 --- a/pkg/storage/unified/sql/testdata/postgres--resource_history_read-single path.sql +++ b/pkg/storage/unified/sql/testdata/postgres--resource_history_read-single path.sql @@ -6,6 +6,7 @@ SELECT "name", "folder", "resource_version", + "action", "value" FROM "resource_history" WHERE 1 = 1 diff --git a/pkg/storage/unified/sql/testdata/sqlite--resource_history_read-single path.sql b/pkg/storage/unified/sql/testdata/sqlite--resource_history_read-single path.sql index 9a423e4302604..836d9d91229ad 100755 --- a/pkg/storage/unified/sql/testdata/sqlite--resource_history_read-single path.sql +++ b/pkg/storage/unified/sql/testdata/sqlite--resource_history_read-single path.sql @@ -6,6 +6,7 @@ SELECT "name", "folder", "resource_version", + "action", "value" FROM "resource_history" WHERE 1 = 1 diff --git a/pkg/storage/unified/testing/storage_backend.go b/pkg/storage/unified/testing/storage_backend.go index c49ba96695a4e..f856d30ad1f93 100644 --- a/pkg/storage/unified/testing/storage_backend.go +++ b/pkg/storage/unified/testing/storage_backend.go @@ -45,7 +45,7 @@ const ( TestOptimisticLocking = "optimistic locking on concurrent writes" TestClusterScopedResources = "cluster scoped resources" TestErrorResponses = "error responses" - TestReadAtRVBeforeDelete = "read at RV before delete" + TestReadAtRVBeforeDelete = "read at RV edge cases" ) type NewBackendFunc func(ctx context.Context) resource.StorageBackend @@ -96,7 +96,7 @@ func RunStorageBackendTest(t *testing.T, newBackend NewBackendFunc, opts *TestOp {TestOptimisticLocking, runTestIntegrationBackendOptimisticLocking}, {TestClusterScopedResources, runTestIntegrationBackendClusterScopedResources}, {TestErrorResponses, runTestIntegrationBackendErrorResponses}, - {TestReadAtRVBeforeDelete, runTestIntegrationBackendReadAtRVBeforeDelete}, + {TestReadAtRVBeforeDelete, runTestIntegrationBackendReadAtRVEdgeCases}, } for _, tc := range cases { @@ -1934,59 +1934,92 @@ func runTestIntegrationBackendClusterScopedResources(t *testing.T, backend resou }) } -// runTestIntegrationBackendReadAtRVBeforeDelete pins down the read-at-RV -// behaviour the dashboard restore-from-trash flow depends on: the trash listing -// surfaces each item's delete-event RV, and the restore flow reads the resource -// at deleteRV-1 to fetch the live pre-delete state. The sequence has multiple -// modifications, unrelated traffic on other resources, and a re-add after the -// delete, so a passing assertion really exercises "find the highest non-deleted -// RV ≤ requested for this resource" rather than "give me the only live event". -func runTestIntegrationBackendReadAtRVBeforeDelete(t *testing.T, backend resource.StorageBackend, nsPrefix string) { +// runTestIntegrationBackendReadAtRVEdgeCases pins down ReadResource behavior +// at exact revision boundaries for create, update, delete, and recreate events. +// This includes the trash contract the restore flow depends on: trash listings +// surface each item's delete-event RV, and reading at deleteRV-1 must return the +// live pre-delete state while reads at and after deleteRV must return not found +// until the resource is recreated. +func runTestIntegrationBackendReadAtRVEdgeCases(t *testing.T, backend resource.StorageBackend, nsPrefix string) { ctx := types.WithAuthInfo(context.Background(), authn.NewAccessTokenAuthInfo(authn.Claims[authn.AccessTokenClaims]{ Claims: jwt.Claims{Subject: "testuser"}, Rest: authn.AccessTokenClaims{}, })) - ns := nsPrefix + "-pre-del-rv" + ns := nsPrefix + "-read-rv" + key := &resourcepb.ResourceKey{ + Name: "target", + Namespace: ns, + Group: "group", + Resource: "resource", + } - rvAdd, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_ADDED, WithNamespace(ns)) + rvCreate, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_ADDED, + WithNamespace(ns), WithValue("target-create")) require.NoError(t, err) - // Unrelated traffic so the target's RVs are not contiguous in the global stream. _, err = WriteEvent(ctx, backend, "noise-a", resourcepb.WatchEvent_ADDED, WithNamespace(ns)) require.NoError(t, err) - rvMod1, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_MODIFIED, WithNamespaceAndRV(ns, rvAdd)) + rvUpdate, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_MODIFIED, + WithNamespaceAndRV(ns, rvCreate), WithValue("target-update")) require.NoError(t, err) _, err = WriteEvent(ctx, backend, "noise-b", resourcepb.WatchEvent_ADDED, WithNamespace(ns)) require.NoError(t, err) - rvMod2, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_MODIFIED, WithNamespaceAndRV(ns, rvMod1)) + rvDelete, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_DELETED, + WithNamespaceAndRV(ns, rvUpdate), WithValue("target-delete")) require.NoError(t, err) - rvDel, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_DELETED, WithNamespaceAndRV(ns, rvMod2)) + _, err = WriteEvent(ctx, backend, "noise-c", resourcepb.WatchEvent_ADDED, WithNamespace(ns)) require.NoError(t, err) - require.Greater(t, rvDel, rvMod2) - // Activity after the delete: other resources, plus a re-add of the same - // name. None of this should influence the lookup at rvDel-1. - _, err = WriteEvent(ctx, backend, "noise-c", resourcepb.WatchEvent_ADDED, WithNamespace(ns)) + rvRecreate, err := WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_ADDED, + WithNamespace(ns), WithValue("target-recreate")) require.NoError(t, err) - _, err = WriteEvent(ctx, backend, "target", resourcepb.WatchEvent_ADDED, WithNamespace(ns)) + + _, err = WriteEvent(ctx, backend, "noise-d", resourcepb.WatchEvent_ADDED, WithNamespace(ns)) require.NoError(t, err) - resp := backend.ReadResource(ctx, &resourcepb.ReadRequest{ - Key: &resourcepb.ResourceKey{ - Name: "target", - Namespace: ns, - Group: "group", - Resource: "resource", - }, - ResourceVersion: rvDel - 1, - }) - require.Nil(t, resp.Error) - require.Equal(t, rvMod2, resp.ResourceVersion) - require.Contains(t, string(resp.Value), "target MODIFIED") + type readAtRVCase struct { + name string + requestRV int64 + expectedStatus int32 + expectedRV int64 + expectedSubstring string + } + + cases := []readAtRVCase{ + {name: "create rv-1", requestRV: rvCreate - 1, expectedStatus: http.StatusNotFound}, + {name: "create rv", requestRV: rvCreate, expectedRV: rvCreate, expectedSubstring: "target-create"}, + {name: "create rv+1", requestRV: rvCreate + 1, expectedRV: rvCreate, expectedSubstring: "target-create"}, + {name: "update rv-1", requestRV: rvUpdate - 1, expectedRV: rvCreate, expectedSubstring: "target-create"}, + {name: "update rv", requestRV: rvUpdate, expectedRV: rvUpdate, expectedSubstring: "target-update"}, + {name: "update rv+1", requestRV: rvUpdate + 1, expectedRV: rvUpdate, expectedSubstring: "target-update"}, + {name: "delete rv-1", requestRV: rvDelete - 1, expectedRV: rvUpdate, expectedSubstring: "target-update"}, + {name: "delete rv", requestRV: rvDelete, expectedStatus: http.StatusNotFound}, + {name: "delete rv+1", requestRV: rvDelete + 1, expectedStatus: http.StatusNotFound}, + {name: "recreate rv-1", requestRV: rvRecreate - 1, expectedStatus: http.StatusNotFound}, + {name: "recreate rv", requestRV: rvRecreate, expectedRV: rvRecreate, expectedSubstring: "target-recreate"}, + {name: "recreate rv+1", requestRV: rvRecreate + 1, expectedRV: rvRecreate, expectedSubstring: "target-recreate"}, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + resp := backend.ReadResource(ctx, &resourcepb.ReadR ... [truncated]