feat: cache the frontier authn token call in redis - #23
rohilsurana wants to merge 23 commits into
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1cf7c34 to
c037ab6
Compare
| end | ||
|
|
||
| for pair in cookie_header:gmatch("[^;]+") do | ||
| local name, value = pair:match("^%s*([^=%s]+)%s*=%s*(.-)%s*$") |
There was a problem hiding this comment.
Frontier parses cookies with Go's net/http, which keeps a space after = as part of the
value, and the session decoder then rejects that value. This pattern trims that space, so
sid= X and sid=X share one key while Frontier treats them differently. Can we drop the
%s* after the = so the value is kept exactly as Frontier sees it?
There was a problem hiding this comment.
Good catch, and worse than it looked. Fixed in 46b8128 by dropping the %s* after the =.
While checking your claim against Go's source I found a second divergence: parseCookieValue strips a surrounding pair of double quotes, so sid="abc" and sid=abc are one session to Frontier but were two keys here. And validCookieValueByte rejects control bytes, DEL, high bytes, a bare " and a backslash, in which case Go drops the cookie entirely while we kept it.
Rather than patch the three cases by hand I built a differential test: the same headers through real net/http and through parse_cookies, compared. 25 cases, all matching now. The interesting ones are in 04-cache_spec.
On severity, for the record: exploiting the original needs you to already know a valid sid, and if you know it you can just send it, so it is not an escalation. What it broke is the invariant the key exists for, that the cache never turns a request the auth server would reject into one it accepts.
| end | ||
|
|
||
| -- hashed, so no session sits in redis as a plaintext key | ||
| return hash(concat(parts, "\0")) |
There was a problem hiding this comment.
Every pod shares these keys, and during a rollout old and new plugin versions read each
other's entries. The value is the raw token today, so a later change to the stored shape
or to the key parts would break the older pods for the length of the ttl. Can we add a
version constant to the key, bumped whenever the value shape or the key recipe changes,
so two versions never read each other's entries?
There was a problem hiding this comment.
Agreed in principle, and it is cheap. Holding it for the next round along with the double decode, so this one stays reviewable. Tracking both.
There was a problem hiding this comment.
Done in cdb5525, kept in this PR after all.
ENTRY_FORMAT_VERSION is now the first part of the key. Since a constant only helps if someone remembers to bump it, there is also a test pinning the recipe to one exact hash, so any change to the key parts, the separator or the hash fails and forces a deliberate call on the version.
| end | ||
| end | ||
|
|
||
| if #conf.token_claims_to_append_as_headers > 0 then |
There was a problem hiding this comment.
The token is decoded at line 219 for the claim headers and again at line 249 for the
org id check, so a request with both on decodes the same string twice. Can we decode
once in run(), after check_request_identity returns, and pass the claims table to
both header steps?
There was a problem hiding this comment.
Confirmed, access.lua:219 and access.lua:249 decode the same string. Holding it for the next round with the key version.
Slight qualifier for when we do it: verify_request_organization_id_header defaults to false, so the second decode only happens when an operator turns that on. Still worth doing, just less often than it reads.
There was a problem hiding this comment.
Confirmed and fixed, but in #24 rather than here so this PR stays reviewable. It is based on this branch, so it needs this one to go in first.
run() now decodes once and passes the claims to both steps. Only the decode moved: each function keeps its own handling of an unreadable payload, since merging those into one guard would have turned the org check's dropped header into a 401.
The counting test fails on the old code with expected 1 got 2, and only in the case where both steps run and the client actually sent the org header, which matches the qualifier above.
| request_uri = function() | ||
| return { | ||
| status = 200, | ||
| headers = {}, | ||
| body = '{"' .. conf.token_response_field .. '":"' .. token .. '"}' | ||
| }, nil |
There was a problem hiding this comment.
The stub here always answers 200 with a token, so the four failure exits in
check_request_identity are not driven by any spec, including the 200-with-no-token
path the description calls the one behaviour change. Can we add cases where the stub
answers 200 with an empty body and a plain 401, and assert the 401 and the
x-upstream-status header?
There was a problem hiding this comment.
Fair hit, and it found a real bug. Fixed in 162ec08, seven cases covering all four exits.
The stub now takes either a token or a raw reply, so the failure paths can be driven. Writing the empty body case turned up a 500: cjson.decode raises on malformed input and returns only one value on success, so local bodyJson, err = json.decode(res.body) leaves err permanently nil and an empty 200 body took down the request. It is on main too, at both the authn and the authz call sites. Both are now pcalled in f813aad.
|
|
||
| local token, err | ||
|
|
||
| if conf.cache_ttl > 0 then |
There was a problem hiding this comment.
cache_ttl defaults to 5, so a deployment without redis_host still parses the cookies and hashes a key on every request, then drops it at cache.lua:84. Can we check redis.enabled(conf) here alongside the ttl, so the key is only built when there is somewhere to store it?
There was a problem hiding this comment.
Right, and it is the default path, since cache_ttl is 5 and redis_host is unset out of the box. Fixed in e766d48.
Added cache.enabled(conf), which is cache_ttl > 0 and redis.enabled(conf), and access.lua now checks that before building the key. cache.get reuses the same function as its own guard so the rule lives in one place.
| @@ -0,0 +1,171 @@ | |||
| local _M = {} | |||
There was a problem hiding this comment.
This file has no spec of its own, and 04-cache_spec swaps the whole module for a table,
so the pause starting on a failed GET, the pause skipping both commands, the not-found
reply read as a miss, and login running only on a fresh connection are covered only by
the end to end suite that is not in the repo. Can we add a spec with a stubbed
resty.redis client that drives those four paths?
There was a problem hiding this comment.
Agreed, and I had flagged the same gap to myself. I had a driver suite for exactly those four paths but it only ran against a real Redis in Docker, which is no use to you or to CI. Replaced with 4c7af14, a spec that stubs resty.redis, so it needs no Redis and no network.
Twenty two cases: the pause starting on a failed GET and on a refused password, the pause skipping both commands, the pause staying per instance, a not-found reply and an empty value read as misses, login and SELECT happening only when get_reused_times is 0, and the millisecond expiry.
One thing worth knowing: 04-cache_spec replaces this whole module in package.loaded, so the new spec clears that entry before requiring the real one. Without it the new tests silently exercise the cache spec's table.
There was a problem hiding this comment.
Done in 2fb8220, 0.1.1 to 0.2.0, with the file renamed to match.
|
There is no CI workflow, so were 3.4 and 3.9 spec runs in the description done by hand? Can we add a workflow that runs pongo against both versions on every push? |
| accepted, err = red:auth(conf.redis_password) | ||
| end | ||
|
|
||
| if not accepted then |
There was a problem hiding this comment.
A refused password or database index is logged and skipped on every request, so a
mistyped password costs a fresh dial, a login round trip and a warning line per
request, at traffic rate, and a full TLS handshake each time when redis_ssl is on.
The warning text already tells a refusal apart from an outage, so pausing hides
nothing. Can we start the pause on a refusal too, so a bad config costs one attempt
and one warning per worker per redis_breaker_seconds?
There was a problem hiding this comment.
You are right and your reasoning beats mine. Changed in 898b2d3, both the auth and the SELECT refusal now start the pause.
My original thinking was that a config mistake should not be hidden, and as you say the pause hides nothing because the warning still names which failure it was. Meanwhile the cost you describe is real: a fresh dial, a login round trip and a warning per request at traffic rate, plus a TLS handshake each time with redis_ssl on.
One consequence to be aware of. Since the pool key is host, port, database, user and ssl, and deliberately not the password, two configs pointing at the same instance as the same user with different passwords now share a breaker, so the wrong one pauses the right one for redis_breaker_seconds. That needs a fairly specific misconfiguration and the log says exactly what happened, so I left it. Happy to put the password back in the key if you would rather not have that.
| }, schema_def)) | ||
|
|
||
| assert.same({ "sid", "other_session" }, ok.config.cache_cookie_names) | ||
| end) |
There was a problem hiding this comment.
The 300 ceiling on cache_ttl is now the only thing keeping the cache under the token
lifetime, and no case here proves Kong enforces it. Can we add a case that a
cache_ttl of 301 is rejected, and one that a redis_timeout of 0 is rejected?
There was a problem hiding this comment.
Added in ddf1341. A cache_ttl of 300 passes and 301 is rejected, redis_timeout of 1 passes and 0 is rejected, plus a case that cache_exp_skew, redis_keepalive_ms and redis_pool_size are no longer accepted at all.
Worth saying plainly: this spec needs spec.helpers, so I cannot run it without pongo. I verified every one of those assertions against a real gateway with kong config parse instead:
cache_ttl: 300 parse successful
cache_ttl: 301 value should be between 0 and 300
redis_timeout: 1 parse successful
redis_timeout: 0 value should be between 1 and 10000
cache_exp_skew unknown field
redis_keepalive_ms unknown field
redis_pool_size unknown field
Which is a decent argument for your CI point.
|
On the CI question: yes, by hand. I ran them in Docker against Worth knowing before we do it: the specs have never run under real pongo, only under that shim. I already know I would rather do it as its own PR than grow this one further. Shout if you would prefer it here. The rest of your comments are addressed and pushed. The two I am holding for the next round are the cache key version and the double decode, both replied to inline. |
The problem
Every request through this plugin makes an HTTP call to Frontier to exchange the
cookie or bearer for a user token. Two hundred requests from one browser tab
means two hundred identical calls.
What this does
It caches that exchange in Redis. The lookup is Redis first, then Frontier on a
miss. Redis is shared by every pod, so a token is fetched once for the fleet
rather than once per pod.
The default TTL is 5 seconds, and it is deliberately short. A cached token means
a change to someone's access is not noticed until the entry expires.
Caching needs Redis. Set
redis_hostand it works. Leave it unset andcache_ttldoes nothing on its own, so every request goes to Frontier exactlyas it does today. There is no deployment step beyond pointing it at a Redis.
The cache is two files:
cache.luais the whole chain at 110 lines, andredis.luais the driver and its breaker.jwt_decoder.luagains a guardagainst a malformed token, and
access.luachanges only in where it asks for atoken and how it ends a failed request.
Config
redis_hostcache_ttl50turns caching off. Max300cache_cookie_names["sid"]redis_timeout100redis_key_prefixfrontier:authn:redis_breaker_seconds10Plus
redis_port,redis_username,redis_password,redis_database,redis_ssl,redis_ssl_verifyandredis_server_name. Full table in theREADME. Connection pooling is OpenResty's own and is not configurable, the same
way it is not in the bundled rate limiting plugin.
An entry costs about 1KB in Redis, so size it as users active within the TTL
window, times 1KB.
What is cached, and what is not
the config that decides what an entry means. Sessions are never stored as
plaintext keys, and two routes that would resolve a credential differently
cannot share an entry.
consent cookies that change constantly, so keying on the whole cookie header
would miss on nearly every request.
Go's
net/http, which keeps a space after the=, strips a surrounding pairof quotes, and drops a value holding a byte it does not allow. A differential
test runs the same 25 headers through
net/httpand through this plugin andasserts they agree, because a disagreement would let two different sessions
share one entry.
share an entry.
locked out for the length of the TTL.
cache_ttl. The token is never parsed, socache_ttlhas to stay well under the auth server's token lifetime. Frontiermints a fresh token on every call and its
token.validitydefaults to anhour, so the default of 5 seconds leaves a very wide margin. The ceiling of
300 is there so a careless value cannot get close.
authz_urlpermission check still runson every request.
credential will each fetch a token. They all write an equivalent entry, and
everything after that is served from Redis.
string. It is only read where it was already being read before this PR, for
the claims that become headers, and a token that is valid JSON but not shaped
like a JWT is refused there rather than half applied.
How Redis behaves
It never fails a request. Redis is a cache, not an authority. A connect
error, a timeout, a bad reply, even a raise, gets logged and the plugin carries
on to Frontier. When anything fails, that worker stops trying that instance for
redis_breaker_seconds, so an outage cannot make every request pay the timeout.The pause is per instance, so one bad Redis does not stop the worker using
another. A refused password or database index starts the pause too, otherwise a
mistyped password would cost a fresh connection, a login round trip and a
warning line on every request, and a TLS handshake as well when
redis_sslison. The warning names which it was, so the pause hides nothing.
Guard write access to this Redis. The plugin does not check the token
signature, with or without Redis. It trusts what Frontier hands back. So
anything that can write these keys can put a token of its choosing in front of
your upstream. The entries also hold live user tokens, which is more sensitive
than something like rate limit counters. Turn on auth and SSL if the instance is
shared or reachable from outside the cluster.
One behaviour change
If Frontier answers 200 but the plugin cannot find a token in the response, it
now returns 401. It used to pass nil into
set_headerand fail with a 500:This had to change, because nil cannot pass through a cache cleanly. 401 is also
the right answer. Frontier returns a proper error status for every real auth
failure, so this state only means the plugin is pointed at the wrong endpoint,
or
token_response_fielddoes not match what the server returns. The 401 cannothide a successful auth.
Performance
Measured against a real Frontier, a real Redis and Kong in DB-less mode.
Absolute numbers come from Docker on macOS, so read the gaps rather than the
values.
One session making requests as fast as it can for 20 seconds,
cache_ttlat 5:Four calls in a 20 second window is what a 5 second TTL should give. The same
client also got through 1.7 times as many requests, because it was not waiting
on an auth call every time.
Kong's own CPU per request, from the container's cgroup accounting, and latency
with the plain and cached routes interleaved:
So the Redis hop adds about 0.6ms and saves about 32ms.
Testing
The unit tests cover the key builder, the cache behaviour, the token handling,
every exit of the auth server call, and
redis.luaitself through a stubbedclient that drives the pause, the login, and a not-found reply. They need no
Redis and no network. The end to end suite runs against a real Frontier, with postgres and
SpiceDB, using a real session cookie from the mailotp flow, and counts actual
AuthToken calls from Frontier's own metrics.
The end to end suite adds a full outage of each dependency:
pods, and a session the cache has never seen gets a 401 rather than a 500.
Everything recovers when it comes back.
Redis is picked up again once the pause expires.
Two more things only a real gateway shows:
of them logs a warning about a missing one.
directions, which matters if the fleet is ever mid-upgrade.
Works on Kong 3.4 and later, using only modules that ship with Kong and
OpenResty.
Why not a node cache as well
An earlier version of this kept a shared memory cache on each pod in front of
Redis, so the common path would pay no network at all. It is not here because
it did not earn its keep. It needed a
lua_shared_dictadded tokong.confonevery gateway, it brought
mlcacheand a single-flight lock, and it neededextra machinery to stop a token going stale twice over on its way through two
caches. Three adversarial reviews found sixteen defects on this branch, and by
the end every open one lived in that layer and none lived in Redis.
The measurements say it was not buying much. The Redis round trip it would have
saved costs 0.6ms of latency and no measurable CPU: 0.330ms per request against
0.34ms for the node memory hit it replaced.