AlinsRan commented on PR #2900:
URL:
https://github.com/apache/apisix-ingress-controller/pull/2900#issuecomment-6078840051
Ordering by Consumer creation time lets an older Consumer take over a key
that a newer Consumer is already publishing, by editing its own Secret. The
webhook would reject the same change if it were made inline, so this is not the
webhook's first-come behavior.
Scenario:
1. `attacker` (older) has a key-auth credential with `secretRef` to its own
Secret, `data.key: x`.
2. `victim` (newer) has an inline key-auth key `K` and is published.
3. `attacker` updates its Secret to `data.key: K`. No webhook guards Secret
updates.
4. The Secret event requeues both Consumers:
- `keyAuthKeyTaken(attacker)`: `victim` is newer, so `attacker` keeps and
publishes `K`.
- `keyAuthKeyTaken(victim)`: `attacker` is older, so `victim`'s
credential is dropped.
After that, requests carrying `K` authenticate as `attacker`, and
`attacker`'s consumer plugins apply to `victim`'s clients. On master the
duplicate resolves as last-loaded-wins in APISIX; with this PR it resolves
deterministically in favor of the Consumer that claimed the key last.
Repro with the helpers from this PR (passes on the PR head):
```go
// internal/controller/consumer_controller_test.go
func TestOlderConsumerTakesKeyViaSecretEdit(t *testing.T) {
now := time.Now()
attacker := keyAuthConsumer("attacker", "gw", now.Add(-time.Hour),
secretKeyAuth("dup", "attacker-secret", nil))
victim := keyAuthConsumer("victim", "gw", now, inlineKeyAuth("cred",
sharedKey))
// Secret data.key edited to the victim's key after both Consumers were
admitted.
r := buildConsumerReconciler(t, attacker, victim,
keyAuthSecret("attacker-secret", consumerNS))
_, skippedAttacker := skipDuplicates(t, r, attacker)
_, skippedVictim := skipDuplicates(t, r, victim)
require.Zero(t, skippedAttacker)
require.Equal(t, 1, skippedVictim)
}
```
The root cause is that `olderConsumer` orders by when the Consumer object
was created, not by when the key was claimed:
```go
// internal/controller/consumer_controller.go
func olderConsumer(a, b *v1alpha1.Consumer) bool {
if !a.CreationTimestamp.Equal(&b.CreationTimestamp) {
return a.CreationTimestamp.Before(&b.CreationTimestamp)
}
return utils.NamespacedName(a).String() <
utils.NamespacedName(b).String()
}
```
The controller has no record of when a key was claimed, so "first-come"
can't be derived from object metadata. Two options:
- Fail closed: drop the colliding credential from every Consumer that shares
the key on the Gateway, and report the conflict in each Consumer's status. No
takeover is possible; the cost is that whoever already knows a key can disable
the other Consumer's credential.
- Keep the incumbent: persist which Consumer currently publishes the key
(e.g. in status) and let it win. This needs more state.
Either way, the PR description should drop "matching the webhook's
first-come behavior".
--
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]
