nic-6443 commented on code in PR #2807:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2807#discussion_r3637318813
##########
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")
Review Comment:
I don't think this branch can fire in a real cluster. The apiserver
collapses duplicate JSON object keys (last-wins) while decoding the request —
before admission webhooks are called and before storage — so
`credential.Config.Raw` never holds two `key` members.
I checked at the raw-byte level rather than trusting a round-trip through a
JSON parser (which would collapse duplicates itself and hide the answer):
registered a probe validating webhook on `consumers`, dumped the unparsed
AdmissionReview body, and POSTed the PoC through the raw REST endpoint.
```
$ kubectl create --raw ".../consumers" -f - #
{"key":123,"key":"victims-key"}
Warning: duplicate field "spec.credentials[0].config.key"
# raw bytes the webhook received:
{"credentials":[{"config":{"key":"victims-key"},"name":"c1","type":"key-auth"}],...
# occurrences of "key": -> 1 contains "key":123 -> False
```
The persisted object is that same normalized form, so the controller and
cjson downstream see it too. Which means the old struct decoder parses the PoC
fine and the duplicate check runs normally — the webhook and cjson never
disagree, so there's nothing to bypass.
Do you have a PoC that reaches the webhook with the duplicate intact? If it
was built by calling `ValidateCreate` directly in a test with hand-written
`Raw` bytes, that would explain the gap — those bytes can't come out of the
apiserver.
##########
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:
This part I think is a real regression: it turns a per-credential skip into
a hard admission failure, and the caller applies that to *other* objects too.
`validateDuplicateKeyAuthCredentials` loops over every existing Consumer
sharing the gateway and does `extractKeyAuthKeys(existing)` -> `return err`. So
one stored Consumer with e.g. `{"key":123}` now denies create/update of every
unrelated Consumer on that gateway, with an error naming a different object.
That state is reachable, unlike the duplicate-key case: `config` is
preserve-unknown-fields so the apiserver stores `{"key":123}` verbatim, and the
webhook ships `failurePolicy: Ignore`, so anything created while the controller
is down lands unvalidated.
```go
// existing: {"key":123} newcomer: {"key":"my-own-unique-key"}, same
gateway, no collision
validator := buildConsumerValidator(t, existing)
_, err := validator.ValidateCreate(context.Background(), newcomer)
require.NoError(t, err)
```
Passes on master, fails here with `invalid key-auth credential config for
Consumer default/legacy: key-auth credential "key" must be a string`.
Could the existing-consumers loop stay best-effort — skip a credential it
can't read, as today — and hard-deny only on the incoming object? That keeps
the stricter validation where it's actionable without letting one legacy object
wedge a whole gateway.
--
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]
