Input validation

HTTP PoC (requires admin/API access): Demonstrates the invalid combination being rejected by the fixed code. # Endpoint (example; actual Grafana endpoint may differ): POST https://grafana.example/api/admin/secure-values Authorization: Bearer <TOKEN> Content-Type: application/json { "secure": { "a": { "Name": "lds-sv-XXX", "Create": "someValue" } } } Expected (with fix): 400 Bad Request with a structured error detailing that only one of name, create, or remove is allowed, e.g. containing a field error for secure.a. Rationale: The PoC demonstrates that attempting to set conflicting fields for the same secure value key is rejected by the hardened input validation introduced in this commit. Prior to this fix, such conflicting configurations could be processed in a way that resulted in ambiguous or insecure handling. The added error construction ensures clear feedback and prevents inconsistent state.

diff --git a/pkg/storage/unified/apistore/secure.go b/pkg/storage/unified/apistore/secure.go index 1cce3c6565a97..a3c0077e5b32d 100644 --- a/pkg/storage/unified/apistore/secure.go +++ b/pkg/storage/unified/apistore/secure.go @@ -5,6 +5,9 @@ import ( "fmt" "strings" + apierrors "k8s.io/apimachinery/pkg/api/errors" + v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + common "github.com/grafana/grafana/pkg/apimachinery/apis/common/v0alpha1" "github.com/grafana/grafana/pkg/apimachinery/utils" secret "github.com/grafana/grafana/pkg/registry/apis/secret/contracts" @@ -67,15 +70,10 @@ func prepareSecureValues(ctx context.Context, store secret.InlineSecureValueSupp v.deleteSecureValues = append(v.deleteSecureValues, before.Name) delete(previous, k) } - if val.Remove { - delete(secure, k) - if before.Name == "" { - continue // no-op - } - v.hasChanged = true - continue - } if !val.Create.IsZero() { + if val.Remove { + return newSecureValueError("only one of create, or remove is allowed", k) + } n, err := store.CreateInline(ctx, v.ref, val.Create, val.Description) if err != nil { return err @@ -88,7 +86,19 @@ func prepareSecureValues(ctx context.Context, store secret.InlineSecureValueSupp obj.SetAnnotation(utils.AnnoKeyKubectlLastAppliedConfig, "") continue } - return fmt.Errorf("invalid secure value state: %s", k) + if val.Remove { + delete(secure, k) + if before.Name == "" { + continue // no-op + } + v.hasChanged = true + continue + } + return newSecureValueError("invalid secure value state: %s", k) + } + + if !val.Create.IsZero() || val.Remove { + return newSecureValueError("only one of name, create, or remove is allowed", k) } // The name changed from the previously stored value @@ -99,7 +109,7 @@ func prepareSecureValues(ctx context.Context, store secret.InlineSecureValueSupp } if strings.HasPrefix(val.Name, LEGACY_DATASOURCE_SECURE_VALUE_NAME_PREFIX) { - return fmt.Errorf("unable to save secure value reference with legacy datasource prefix") + return newSecureValueError("unable to save secure value reference with legacy datasource prefix", k) } delete(previous, k) @@ -160,3 +170,14 @@ func handleSecureValuesDelete(ctx context.Context, store secret.InlineSecureValu } return obj.SetSecureValues(nil) // remove them from the object } + +func newSecureValueError(msg string, key string) error { + err := apierrors.NewBadRequest(msg) + err.ErrStatus.Details = &v1.StatusDetails{ + Causes: []v1.StatusCause{{ + Type: v1.CauseTypeFieldValueInvalid, + Field: fmt.Sprintf("secure.%s", key), + }}, + } + return err +} diff --git a/pkg/storage/unified/apistore/secure_test.go b/pkg/storage/unified/apistore/secure_test.go index 41e04e20ce9de..cc2f889b9b3b7 100644 --- a/pkg/storage/unified/apistore/secure_test.go +++ b/pkg/storage/unified/apistore/secure_test.go @@ -91,6 +91,58 @@ func TestSecureLifecycle(t *testing.T) { secureStore.AssertExpectations(t) }) + t.Run("do not save datasource secure values", func(t *testing.T) { + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "lds-sv-XXX"}, // value currently saved in legacy system + }) + + info := &objectForStorage{} + secureStore := secret.NewMockInlineSecureValueSupport(t) + + err := prepareSecureValues(context.Background(), secureStore, obj, nil, info) + require.ErrorContains(t, err, "unable to save secure value reference with legacy datasource prefix") + secureStore.AssertExpectations(t) + }) + + t.Run("ensure one of", func(t *testing.T) { + t.Run("name and create", func(t *testing.T) { + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "xxx", Create: "yyy"}, + }) + + info := &objectForStorage{} + secureStore := secret.NewMockInlineSecureValueSupport(t) + + err := prepareSecureValues(context.Background(), secureStore, obj, nil, info) + require.ErrorContains(t, err, "only one of name, create, or remove is allowed") + secureStore.AssertExpectations(t) + }) + t.Run("name and remove", func(t *testing.T) { + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Name: "xxx", Remove: true}, + }) + + info := &objectForStorage{} + secureStore := secret.NewMockInlineSecureValueSupport(t) + + err := prepareSecureValues(context.Background(), secureStore, obj, nil, info) + require.ErrorContains(t, err, "only one of name, create, or remove is allowed") + secureStore.AssertExpectations(t) + }) + t.Run("create and remove", func(t *testing.T) { + obj := resourceWithSecureValues(common.InlineSecureValues{ + "a": common.InlineSecureValue{Create: "xxx", Remove: true}, + }) + + info := &objectForStorage{} + secureStore := secret.NewMockInlineSecureValueSupport(t) + + err := prepareSecureValues(context.Background(), secureStore, obj, nil, info) + require.ErrorContains(t, err, "only one of create, or remove is allowed") + secureStore.AssertExpectations(t) + }) + }) + t.Run("change name manually", func(t *testing.T) { secureStore := secret.NewMockInlineSecureValueSupport(t)