diff --git a/internal/webhook/v1/consumer_webhook.go b/internal/webhook/v1/consumer_webhook.go index 57767520..7eaa8551 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,109 @@ 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) + } + 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) { + // 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. + tok, err := dec.Token() + if err != nil { + return "", nil } - 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) + if delim, ok := tok.(json.Delim); !ok || delim != '{' { return "", nil } - return cfg.Key, 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..59319139 100644 --- a/internal/webhook/v1/consumer_webhook_test.go +++ b/internal/webhook/v1/consumer_webhook_test.go @@ -192,3 +192,70 @@ 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: "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 { + 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) + }) + } +}