From 2941c079ee86031ae101a02eba6eb1546beb56aa Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Fri, 17 Jul 2026 15:47:01 +0545 Subject: [PATCH 1/2] fix: reject ambiguous inline key-auth config that bypasses duplicate check The Consumer webhook extracts the key-auth "key" from inline credential config with Go's encoding/json struct decoder. On {"key":123,"key":"K"} that decoder returns an error, so the webhook silently skips the duplicate check, while downstream cjson (last-wins, no type abort) resolves the same bytes to a live key "K". The only hard-deny credential check was evadable by any Consumer creator who knows a colliding key. Parse inline config aligned with cjson (exact-case, string-valued, last-wins) and reject the ambiguous shapes that cause the divergence: duplicate "key" members and a non-string "key" are hard admission errors. Genuinely malformed JSON that cjson also rejects stays lenient. --- internal/webhook/v1/consumer_webhook.go | 103 +++++++++++++++++-- internal/webhook/v1/consumer_webhook_test.go | 64 ++++++++++++ 2 files changed, 159 insertions(+), 8 deletions(-) diff --git a/internal/webhook/v1/consumer_webhook.go b/internal/webhook/v1/consumer_webhook.go index 57767520..f052e2c9 100644 --- a/internal/webhook/v1/consumer_webhook.go +++ b/internal/webhook/v1/consumer_webhook.go @@ -16,6 +16,7 @@ package v1 import ( + "bytes" "context" "encoding/json" "fmt" @@ -227,15 +228,101 @@ func (v *ConsumerCustomValidator) extractCredentialKey(ctx context.Context, cons return "", nil } - var cfg struct { - Key string `json:"key"` + key, err := parseInlineKeyAuthKey(credential.Config.Raw) + if err != nil { + return "", fmt.Errorf("invalid key-auth credential config for Consumer %s/%s: %w", + consumer.Namespace, consumer.Name, err) } - if err := json.Unmarshal(credential.Config.Raw, &cfg); err != nil { - // Malformed JSON is not a hard error: skip duplicate detection for this - // credential so existing consumers with bad config are not suddenly denied. - consumerLog.V(1).Info("skipping duplicate key-auth check: malformed credential config", - "consumer", consumer.Name, "error", err) + return key, nil +} + +// parseInlineKeyAuthKey extracts the key-auth "key" from an inline credential +// config the same way downstream cjson does: exact-case, string-valued, +// last-wins. Ambiguous configs that Go's struct decoder would silently reject +// while cjson still resolves to a live key (duplicate "key" members, or a +// non-string "key") are returned as errors so they can't bypass the duplicate +// check. Genuinely malformed JSON that cjson also rejects returns ("", nil) so +// existing consumers with broken config are skipped, not suddenly denied. +func parseInlineKeyAuthKey(raw []byte) (string, error) { + dec := json.NewDecoder(bytes.NewReader(raw)) + + // Top level must be an object, else there is no usable key. + tok, err := dec.Token() + if err != nil { return "", nil } - return cfg.Key, nil + if delim, ok := tok.(json.Delim); !ok || delim != '{' { + return "", nil + } + + var ( + key string + keyCount int + ) + for dec.More() { + nameTok, err := dec.Token() + if err != nil { + return "", nil + } + name, ok := nameTok.(string) + if !ok { + return "", nil + } + if name != "key" { + if err := skipJSONValue(dec); err != nil { + return "", nil + } + continue + } + + keyCount++ + valTok, err := dec.Token() + if err != nil { + return "", nil + } + switch val := valTok.(type) { + case string: + key = val + case nil: + // null key: no usable value, but still counts for dup detection. + default: + // number/bool/object/array: cjson would deliver a value here while + // Go's struct decoder errors and skips. Reject instead. + return "", fmt.Errorf("key-auth credential \"key\" must be a string") + } + } + + if keyCount > 1 { + return "", fmt.Errorf("key-auth credential config has duplicate \"key\" members") + } + return key, nil +} + +// skipJSONValue consumes a single JSON value (scalar or a whole object/array) +// from the decoder so the token stream stays aligned. +func skipJSONValue(dec *json.Decoder) error { + tok, err := dec.Token() + if err != nil { + return err + } + delim, ok := tok.(json.Delim) + if !ok || (delim != '{' && delim != '[') { + return nil + } + depth := 1 + for depth > 0 { + t, err := dec.Token() + if err != nil { + return err + } + if d, ok := t.(json.Delim); ok { + switch d { + case '{', '[': + depth++ + case '}', ']': + depth-- + } + } + } + return nil } diff --git a/internal/webhook/v1/consumer_webhook_test.go b/internal/webhook/v1/consumer_webhook_test.go index 4dc32b84..f667f572 100644 --- a/internal/webhook/v1/consumer_webhook_test.go +++ b/internal/webhook/v1/consumer_webhook_test.go @@ -192,3 +192,67 @@ func TestConsumerValidator_DenyDuplicateKeyAuthCredential(t *testing.T) { require.Contains(t, err.Error(), `duplicate key-auth credential key "shared-key"`) require.Contains(t, err.Error(), "default/existing") } + +// A duplicate-key inline config ({"key":123,"key":"K"}) is unreadable to Go's +// struct decoder but resolves to "K" downstream via cjson. The webhook must +// reject it instead of silently skipping the duplicate check. +func TestConsumerValidator_DenyDuplicateKeyAuthCredential_ParserDivergence(t *testing.T) { + existing := &apisixv1alpha1.Consumer{ + ObjectMeta: metav1.ObjectMeta{Name: "existing", Namespace: "default"}, + Spec: apisixv1alpha1.ConsumerSpec{ + GatewayRef: apisixv1alpha1.GatewayRef{Name: "test-gateway"}, + Credentials: []apisixv1alpha1.Credential{{ + Type: "key-auth", + Config: apiextensionsv1.JSON{Raw: []byte(`{"key":"victims-key"}`)}, + }}, + }, + } + consumer := &apisixv1alpha1.Consumer{ + ObjectMeta: metav1.ObjectMeta{Name: "demo", Namespace: "default"}, + Spec: apisixv1alpha1.ConsumerSpec{ + GatewayRef: apisixv1alpha1.GatewayRef{Name: "test-gateway"}, + Credentials: []apisixv1alpha1.Credential{{ + Type: "key-auth", + Config: apiextensionsv1.JSON{Raw: []byte(`{"key":123,"key":"victims-key"}`)}, + }}, + }, + } + + validator := buildConsumerValidator(t, existing) + + _, err := validator.ValidateCreate(context.Background(), consumer) + require.Error(t, err) + require.Contains(t, err.Error(), "invalid key-auth credential config") +} + +func TestParseInlineKeyAuthKey(t *testing.T) { + tests := []struct { + name string + raw string + wantKey string + wantErr bool + }{ + {name: "plain string key", raw: `{"key":"K"}`, wantKey: "K"}, + {name: "extra fields ignored", raw: `{"key":"K","foo":{"a":1}}`, wantKey: "K"}, + {name: "duplicate key members", raw: `{"key":"a","key":"b"}`, wantErr: true}, + {name: "number then string (divergence PoC)", raw: `{"key":123,"key":"K"}`, wantErr: true}, + {name: "non-string key", raw: `{"key":123}`, wantErr: true}, + {name: "object key", raw: `{"key":{"nested":1}}`, wantErr: true}, + {name: "null key skipped", raw: `{"key":null}`, wantKey: ""}, + {name: "no key member", raw: `{"foo":"bar"}`, wantKey: ""}, + {name: "exact-case only, Key ignored", raw: `{"Key":"K"}`, wantKey: ""}, + {name: "malformed json skipped", raw: `{"key":`, wantKey: ""}, + {name: "non-object skipped", raw: `["key","K"]`, wantKey: ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + key, err := parseInlineKeyAuthKey([]byte(tt.raw)) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + require.Equal(t, tt.wantKey, key) + }) + } +} From 737cc0810e56cede5f026a0badd02753f930c399 Mon Sep 17 00:00:00 2001 From: Abhishek Choudhary Date: Fri, 17 Jul 2026 16:33:00 +0545 Subject: [PATCH 2/2] fix: skip malformed inline config (truncation, trailing data, multi-value) Address review: parseInlineKeyAuthKey extracted a key from truncated or multi-top-level-value input instead of skipping it, contradicting the documented cjson-aligned leniency. Guard with json.Valid so only exactly one well-formed JSON value is parsed; duplicate keys stay valid and are still caught by the token walk. Add malformed-shape test cases. --- internal/webhook/v1/consumer_webhook.go | 8 ++++++++ internal/webhook/v1/consumer_webhook_test.go | 3 +++ 2 files changed, 11 insertions(+) diff --git a/internal/webhook/v1/consumer_webhook.go b/internal/webhook/v1/consumer_webhook.go index f052e2c9..7eaa8551 100644 --- a/internal/webhook/v1/consumer_webhook.go +++ b/internal/webhook/v1/consumer_webhook.go @@ -244,6 +244,14 @@ func (v *ConsumerCustomValidator) extractCredentialKey(ctx context.Context, cons // check. Genuinely malformed JSON that cjson also rejects returns ("", nil) so // existing consumers with broken config are skipped, not suddenly denied. func parseInlineKeyAuthKey(raw []byte) (string, error) { + // cjson rejects malformed input (truncation, trailing data, multiple + // top-level values); mirror that by skipping anything that is not exactly + // one well-formed JSON value. Duplicate keys stay valid here and are caught + // by the token walk below. + if !json.Valid(raw) { + return "", nil + } + dec := json.NewDecoder(bytes.NewReader(raw)) // Top level must be an object, else there is no usable key. diff --git a/internal/webhook/v1/consumer_webhook_test.go b/internal/webhook/v1/consumer_webhook_test.go index f667f572..59319139 100644 --- a/internal/webhook/v1/consumer_webhook_test.go +++ b/internal/webhook/v1/consumer_webhook_test.go @@ -242,6 +242,9 @@ func TestParseInlineKeyAuthKey(t *testing.T) { {name: "no key member", raw: `{"foo":"bar"}`, wantKey: ""}, {name: "exact-case only, Key ignored", raw: `{"Key":"K"}`, wantKey: ""}, {name: "malformed json skipped", raw: `{"key":`, wantKey: ""}, + {name: "truncated object skipped", raw: `{"key":"K"`, wantKey: ""}, + {name: "multiple top-level values skipped", raw: `{"key":"K"}{"key":"X"}`, wantKey: ""}, + {name: "trailing garbage skipped", raw: `{"key":"K"} junk`, wantKey: ""}, {name: "non-object skipped", raw: `["key","K"]`, wantKey: ""}, } for _, tt := range tests {