grpc proxy serializable Range cache leaks protected keys across auth identities

[goingforstudying-ctrl]

Was poking at the grpc-proxy caching code while debugging an authz mystery in a staging setup (clients behind the proxy occasionally saw keys their roles shouldn't allow) and it turned out the proxy cache basically doesn't care who's asking. This is the same thing reported in #22065.

The problem: kvProxy.Range serves serializable reads out of an LRU keyed only on the marshalled RangeRequest. The caller's auth token never enters the key. So once any client (say root) does a serializable Range on /secret, the proxy stuffs the response into the cache, and the next client asking for the exact same serializable range gets it replayed straight from the LRU. The backend never sees the second request, so its auth check never runs. A user with zero permissions — or an unauthenticated client — can read anything a previous caller happened to cache. Linearizable reads are also cached "as serializable", so the leak isn't limited to clients that explicitly ask for stale reads. And since permission changes never touch the cache, revoking access doesn't help while the entry is still warm either.

I went with option (a) from the issue: scope cache entries by the caller identity, plus flush on auth mutations. Concretely:

• The cache key is now the marshalled request plus a SHA-256 of the caller's token. Two different tokens never share an entry, so a response cached for one principal can't be replayed to another. Hashing it instead of storing the raw token keeps the LRU from holding credentials in cleartext. Requests without a token share one entry, which is the same behavior as today for auth-disabled setups.
• Auth-mutating RPCs forwarded through the proxy (RoleGrant/RevokePermission, UserAdd/Delete/ChangePassword, UserGrant/RevokeRole, RoleAdd/Delete, AuthEnable/Disable) now flush the whole KV cache on success. That's intentionally blunt — the cache is small (2048 entries) and refills on demand, and it closes the stale-permission window for simple tokens that don't embed the auth revision.
• NewAuthProxy takes the *KvProxy so it can reach the shared cache; the one call site in etcdmain and the test-framework call site are updated.

One trade-off worth mentioning: two tokens belonging to the same user (e.g. re-authenticated) get separate entries, so per-user hit rates drop a bit. Seemed fine — correctness beats a slightly colder cache, and entries for the same user converge on the next read anyway.

Regression coverage:

• server/proxy/grpcproxy/cache: unit tests for identity-scoped keying, Clear(), key determinism.
• tests/integration/proxy/grpcproxy/kv_auth_cache_test.go: spins up a real auth-enabled member behind a proxy wired the same way production is (token-forwarding interceptors), then
  • primes the cache as root and asserts a low-priv user and an anonymous caller get denied on the identical serializable Range (previously they'd have gotten root's data),
  • same for the linearizable-then-serializable path,
  • revokes the permission through the AuthProxy and asserts the next read is denied instead of served from the stale entry.

go test ./server/proxy/grpcproxy/... ./tests/integration/proxy/grpcproxy/... and the auth-flavored integration suites in tests/integration and tests/common -tags integration all pass locally.

Fixes #22065

[kubernetes-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: goingforstudying-ctrl
Once this PR has been reviewed and has the lgtm label, please assign jmhbnz for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[kubernetes-prow]

Hi @goingforstudying-ctrl. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work.

Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

[Deln0r]

Thanks for tackling this — the auth-bypass is real and the identity-scoped keying is the right shape. A few notes.

Correctness looks solid to me: denials are never cached (Add only runs after a successful backend Do), so the grant direction is safe by construction; the auth-disabled path is unchanged since an empty token shares one bucket as before; and the regression tests genuinely fail without the fix (low-priv/anon would be served root's primed entry, so the require.Error assertions would trip). Nice, thorough coverage.

Two things worth changing and one to document:

1. NewAuthProxy takes the whole *KvProxy just to read kvp.cache, which forces exporting the KvProxy type alias. You already added a Cache() cache.Cache method but nothing uses it — passing kvp.Cache() and taking a cache.Cache here would drop the exported concrete type, loosen the coupling, and put that method to use.

2. In TestCacheAuthKeyIsolation, the final assertion's comment ("alice and '' share the same key when authKey is empty") is inaccurate: keyFunc appends authKey, so keyFunc(req,"alice") and keyFunc(req,"") are distinct keys. That Get(req,"alice") hits because the earlier Add(req,resp,"alice") entry is still resident, not because "" collides with a named identity. Worth fixing the comment (or the assertion) so it doesn't imply "" acts as a wildcard.

3. The cache flush only fires for auth mutations that transit the proxy's AuthProxy. Permission changes applied directly against the backend won't flush the proxy cache, leaving a same-token stale-read window until eviction. Might be worth a short note on Clear()/flushCacheOnMutation so operators know the proxy only observes auth changes that pass through it.

[goingforstudying-ctrl]

Thanks for the review. I updated NewAuthProxy to take a cache.Cache interface directly instead of the concrete *KvProxy, which drops the exported type coupling and uses the existing Cache() method. I also fixed the inaccurate comment in TestCacheAuthKeyIsolation and documented the proxy-only flush limitation on Clear(). Pushed as 0424bf6.

[Hakai-Shin]

Thanks for tackling this — the auth-bypass is real and the identity-scoped keying is the right shape. A few notes.

Correctness looks solid to me: denials are never cached (Add only runs after a successful backend Do), so the grant direction is safe by construction; the auth-disabled path is unchanged since an empty token shares one bucket as before; and the regression tests genuinely fail without the fix (low-priv/anon would be served root's primed entry, so the require.Error assertions would trip). Nice, thorough coverage.

Two things worth changing and one to document:

1. NewAuthProxy takes the whole *KvProxy just to read kvp.cache, which forces exporting the KvProxy type alias. You already added a Cache() cache.Cache method but nothing uses it — passing kvp.Cache() and taking a cache.Cache here would drop the exported concrete type, loosen the coupling, and put that method to use.
2. In TestCacheAuthKeyIsolation, the final assertion's comment ("alice and '' share the same key when authKey is empty") is inaccurate: keyFunc appends authKey, so keyFunc(req,"alice") and keyFunc(req,"") are distinct keys. That Get(req,"alice") hits because the earlier Add(req,resp,"alice") entry is still resident, not because "" collides with a named identity. Worth fixing the comment (or the assertion) so it doesn't imply "" acts as a wildcard.
3. The cache flush only fires for auth mutations that transit the proxy's AuthProxy. Permission changes applied directly against the backend won't flush the proxy cache, leaving a same-token stale-read window until eviction. Might be worth a short note on Clear()/flushCacheOnMutation so operators know the proxy only observes auth changes that pass through it.

@goingforstudying-ctrl
On #3 — this isn't limited to direct-backend mutations. Tested this directly against your branch (single-node backend with a short --auth-token-ttl, your proxy in front of it) rather than just reasoning from the code, and confirmed plain token expiry hits the same gap, no bypass needed:
Same token T: denied directly by backend (expired) -> still served from proxy cache
Root cause is the same either way: Get never revalidates against the backend on a hit, so nothing here has a lifetime tied to the token it was cached under. That's more than a doc gap though — it's a live auth-bypass window for as long as the entry survives eviction. Might be worth capping cache-entry lifetime to the token TTL rather than just documenting it.

[goingforstudying-ctrl]

Thanks for the detailed review. On the three points:

1. NewAuthProxy decoupling — done in 2f3ab5f, it now takes cache.Cache via kvp.Cache() and the exported KvProxy alias is gone.

2. Test comment — already corrected at HEAD; the comment above the final assertion now says alice hits because her earlier entry is still resident, not because "" collides with a named identity.

3. Flush scope documentation — added a note on flushCacheOnMutation in the latest commit making clear the proxy only observes auth changes that pass through it, and that direct backend permission changes leave a same-token stale-read window until eviction.