Information Disclosure / Secret leakage

PoC details: 1) Set up a Resource storage with an inline secure value provider. 2) Trigger a GuaranteedUpdate that creates an inline secret during the attempted update and then returns a Conflict (HTTP 409) to force a retry. 3) Without the fix, the failed attempt leaves the inline secret orphaned and it could be read by subsequent access to the inline secrets store, leading to leakage of the secret value (e.g., a token). 4) With the fix, the cleanup (DeleteWhenOwnedByResource) runs before the retry, ensuring the orphaned inline secret is removed and not leaked. Go-like PoC sketch (conceptual, simplified): package main // Setup mocks/stubs for the secure value provider secureStore := secret.NewMockInlineSecureValueSupport(t) secureStore.On("CreateInline", mock.Anything, mock.Anything, mock.Anything, mock.Anything). Return("inline-1", nil).Once() secureStore.On("DeleteWhenOwnedByResource", mock.Anything, mock.Anything, "inline-1"). Return(nil).Once() s := &Storage{ codec: unstructured.UnstructuredJSONScheme, newFunc: func() runtime.Object { return &unstructured.Unstructured{} }, versioner: &storage.APIObjectVersioner{}, store: &conflictOnceClient{prev: raw.Bytes()}, getKey: func(string) (*resourcepb.ResourceKey, error) { return &resourcepb.ResourceKey{Namespace: "default", Group: "example.grafana.app", Resource: "examples", Name: "test"}, nil }, opts: StorageOptions{SecureValues: secureStore}, } tryUpdate := func(input runtime.Object, _ storage.ResponseMeta) (runtime.Object, *uint64, error) { out := input.(*unstructured.Unstructured).DeepCopy() out.Object["secure"] = map[string]any{"token": map[string]any{"create": "s3cr3t"}} return out, nil, nil } requester := &identity.StaticRequester{Type: claims.TypeUser, UserID: 1, OrgRole: identity.RoleAdmin, IsGrafanaAdmin: true} ctx := identity.WithRequester(context.Background(), requester) err := s.GuaranteedUpdate(ctx, "example/test", &unstructured.Unstructured{}, false, &storage.Preconditions{}, tryUpdate, nil) require.NoError(t, err) secureStore.AssertExpectations(t)

diff --git a/pkg/storage/unified/apistore/secure_test.go b/pkg/storage/unified/apistore/secure_test.go index cc2f889b9b3b7..b6b239e77b5b0 100644 --- a/pkg/storage/unified/apistore/secure_test.go +++ b/pkg/storage/unified/apistore/secure_test.go @@ -1,19 +1,28 @@ package apistore import ( + "bytes" "context" "encoding/json" "fmt" + "net/http" "slices" "testing" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" + "google.golang.org/grpc" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/storage" + claims "github.com/grafana/authlib/types" common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/registry/apis/secret" + "github.com/grafana/grafana/pkg/storage/unified/resource" + "github.com/grafana/grafana/pkg/storage/unified/resourcepb" ) func TestSecureLifecycle(t *testing.T) { @@ -362,6 +371,72 @@ func TestSecureLifecycle(t *testing.T) { err = handleSecureValuesDelete(context.Background(), nil, objWithCreateSecret) require.Error(t, err, "should error when secure value storage is not configured") }) + + // A conflicting update attempt already persisted an inline secure value; it must + // be deleted before we retry, otherwise every retry leaks an orphaned secret. + t.Run("guaranteed update cleans up secure values created on a conflicting attempt", func(t *testing.T) { + prev := &unstructured.Unstructured{Object: map[string]any{ + "apiVersion": "example.grafana.app/v1", + "kind": "Example", + "metadata": map[string]any{ + "namespace": "default", + "name": "test", + "uid": "u1", + }, + }} + var raw bytes.Buffer + require.NoError(t, unstructured.UnstructuredJSONScheme.Encode(prev, &raw)) + + secureStore := secret.NewMockInlineSecureValueSupport(t) + secureStore.On("CreateInline", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return("inline-1", nil).Once() + secureStore.On("CreateInline", mock.Anything, mock.Anything, mock.Anything, mock.Anything). + Return("inline-2", nil).Once() + secureStore.On("DeleteWhenOwnedByResource", mock.Anything, mock.Anything, "inline-1"). + Return(nil).Once() + + s := &Storage{ + codec: unstructured.UnstructuredJSONScheme, + newFunc: func() runtime.Object { return &unstructured.Unstructured{} }, + versioner: &storage.APIObjectVersioner{}, + store: &conflictOnceClient{prev: raw.Bytes()}, + getKey: func(string) (*resourcepb.ResourceKey, error) { + return &resourcepb.ResourceKey{Namespace: "default", Group: "example.grafana.app", Resource: "examples", Name: "test"}, nil + }, + opts: StorageOptions{SecureValues: secureStore}, + } + + tryUpdate := func(input runtime.Object, _ storage.ResponseMeta) (runtime.Object, *uint64, error) { + out := input.(*unstructured.Unstructured).DeepCopy() + out.Object["secure"] = map[string]any{"token": map[string]any{"create": "s3cr3t"}} + return out, nil, nil + } + + requester := &identity.StaticRequester{Type: claims.TypeUser, UserID: 1, OrgRole: identity.RoleAdmin, IsGrafanaAdmin: true} + ctx := identity.WithRequester(context.Background(), requester) + + err := s.GuaranteedUpdate(ctx, "example/test", &unstructured.Unstructured{}, false, &storage.Preconditions{}, tryUpdate, nil) + require.NoError(t, err) + secureStore.AssertExpectations(t) + }) +} + +type conflictOnceClient struct { + resource.ResourceClient + prev []byte + updates int +} + +func (c *conflictOnceClient) Read(context.Context, *resourcepb.ReadRequest, ...grpc.CallOption) (*resourcepb.ReadResponse, error) { + return &resourcepb.ReadResponse{Value: c.prev, ResourceVersion: 1}, nil +} + +func (c *conflictOnceClient) Update(context.Context, *resourcepb.UpdateRequest, ...grpc.CallOption) (*resourcepb.UpdateResponse, error) { + c.updates++ + if c.updates == 1 { + return &resourcepb.UpdateResponse{Error: &resourcepb.ErrorResult{Code: http.StatusConflict}}, nil + } + return &resourcepb.UpdateResponse{ResourceVersion: 2}, nil } func asJSON(v any, pretty bool) string { diff --git a/pkg/storage/unified/apistore/store.go b/pkg/storage/unified/apistore/store.go index e8a5129370e56..c89bae93d6674 100644 --- a/pkg/storage/unified/apistore/store.go +++ b/pkg/storage/unified/apistore/store.go @@ -748,6 +748,10 @@ func (s *Storage) GuaranteedUpdate( err = resource.GetError(resource.AsErrorResult(err)) } else if updateResponse.Error != nil { if attempt < MaxUpdateAttempts && updateResponse.Error.Code == http.StatusConflict { + // Delete the secure values this attempt created; the next attempt recreates them. + // finish only echoes the conflict back and logs any cleanup failure itself, so we + // discard its return and retry instead of surfacing it. + _ = v.finish(ctx, resource.GetError(updateResponse.Error), s.opts.SecureValues) continue // try the read again } err = resource.GetError(updateResponse.Error)