AlinsRan commented on code in PR #2807:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2807#discussion_r3638541321
##########
internal/webhook/v1/consumer_webhook.go:
##########
@@ -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",
Review Comment:
Blocking: this error propagates out of `extractKeyAuthKeys(ctx, existing)`
in the loop over *other* Consumers (`validateDuplicateKeyAuthCredentials`), so
one pre-existing object with a bad inline config denies admission for every
other Consumer on the same gateway.
Reproduced against this branch — a `legacy` Consumer with `{"key":123}`
already in the cluster, then an unrelated create with a perfectly valid,
non-colliding key:
```
ValidateCreate(innocent) err =
invalid key-auth credential config for Consumer default/legacy: key-auth
credential "key" must be a string
```
The error even names someone else's object. The code this replaces called
this out explicitly ("existing consumers with bad config are not suddenly
denied").
If you keep any hard-deny path, it has to apply only to the object under
admission; when walking other Consumers, a parse failure should degrade to skip
+ log.
##########
internal/webhook/v1/consumer_webhook.go:
##########
@@ -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 {
Review Comment:
If the token walk stays, this whole function is
`dec.Decode(&json.RawMessage{})` — `Decode` consumes exactly one value and
keeps the stream aligned.
##########
internal/webhook/v1/consumer_webhook.go:
##########
@@ -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,
Review Comment:
`the same way downstream cjson does` — the downstream here is not cjson.
It's `json.Unmarshal(credentialSpec.Config.Raw, &authConfig)` into a
`map[string]any` at `internal/adc/translator/consumer.go:65`. Worth correcting
the comment, because it's what the whole duplicate-member branch is justified
by.
And since duplicate members can't survive the API server's unstructured
decode (see the top-level comment), `keyCount > 1` is dead in practice for
anything created through the Kubernetes API.
##########
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"}`)},
Review Comment:
This is the crux of the reachability problem: `Config.Raw` is set directly
in Go, so the bytes never pass through the API server's unstructured decode
that would have collapsed the duplicate to `{"key":"victims-key"}`. A Consumer
created via `kubectl`/client-go can't reach the validator in this state, so
this doesn't demonstrate an exploitable path.
What's missing is a case for the regression this introduces: a pre-existing
Consumer with a broken inline config, plus an unrelated new Consumer with a
valid key, asserting the new one is still admitted. That currently fails.
##########
internal/webhook/v1/consumer_webhook.go:
##########
@@ -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) {
Review Comment:
If the goal is just to match the translator's semantics (exact-case,
last-wins), `map[string]json.RawMessage` *is* that semantics — no token walk
needed:
```go
var cfg map[string]json.RawMessage
if err := json.Unmarshal(raw, &cfg); err != nil {
return "", nil // downstream fails the same way; stay lenient
}
v, ok := cfg["key"] // exact-case, last-wins, matches the translator
if !ok {
return "", nil
}
var s string
if err := json.Unmarshal(v, &s); err != nil {
return "", nil // non-string: the key-auth schema rejects it downstream
anyway
}
return s, nil
```
That's ~10 lines instead of ~90, fixes the `{"Key":"K"}` false positive, and
adds no new denial paths. Note also that `json.Valid` here parses the input a
second time — `dec.Token()` failing already covers malformed input.
Separately: the `consumerLog.V(1).Info("skipping duplicate key-auth check:
...")` line was dropped, so the lenient-skip path is now completely silent.
Worth keeping.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
