ALB: implement the authenticate-oidc listener action - #1523
Open
dhanesh wants to merge 1 commit into
Open
Conversation
ALB listener rules accepted an authenticate-oidc action's Type and silently discarded its AuthenticateOidcConfig. A listener created from a real Terraform module or create-listener call reported success and then forwarded every request to the target unauthenticated, so anything testing an OIDC-protected service against MiniStack passed while proving nothing. The action now works end to end. An unauthenticated request is redirected to the configured AuthorizationEndpoint carrying state; the callback at /oauth2/idpresponse exchanges the code on the back channel and writes the session to the client as AWSELBAuthSessionCookie shards; later requests have those shards reassembled and validated before X-Amzn-Oidc-Identity, -Accesstoken and -Data are injected for the target. SessionCookieName, SessionTimeout, Scope and all three OnUnauthenticatedRequest modes are honoured. ClientSecret is kept for the token exchange but never echoed back from Describe*, matching AWS. Shards are sized so the complete Set-Cookie pair — name, value and attributes — fits the 4096-byte per-cookie ceiling browsers enforce. Sizing to 4096 bytes of value alone yields a cookie the browser silently discards, which presents exactly like having no session at all. Two fixes the action depends on: Rule actions now run as a chain, in Order. Dispatch executed Actions[0] and ignored the rest, so the two-action shape every authentication rule has — authenticate, then forward — could not work. Authentication actions either short-circuit or pass the request to the action behind them; the first terminal action answers. ModifyListener now reaches the data plane. CreateListener copies the default actions into an auto-created default rule, and dispatch reads that rule rather than the listener. ModifyListener updated only the listener record, so it reported success while traffic carried on hitting the original actions. authenticate-cognito returns 501 rather than forwarding a request nobody authenticated. Adds tests/test_alb_authenticate_oidc.py: 18 tests covering the config round-trip, shard sizing and reassembly, and the data-plane behaviour a client observes.
|
Docker image for this PR has been published: |
Collaborator
|
@dhanesh your changelog entry and test file are outside shape, also the code and description are doing different things. We cannot merge this. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this fixes
ALB listener rules accept an
authenticate-oidcaction'sTypeand silently discard itsAuthenticateOidcConfig. A listener created from a real Terraform module orcreate-listenercall reports success, and then forwards every request to the target unauthenticated.That failure is quiet in the worst way: anyone testing an OIDC-protected service against MiniStack gets a green suite that proved nothing. I hit it building a client for ALB-authenticated APIs — my integration tests passed against MiniStack until I checked why no session cookie ever appeared.
What it does now
An unauthenticated request is redirected to the configured
AuthorizationEndpointcarryingstate. The callback at/oauth2/idpresponseexchanges the code on the back channel and writes the session to the client asAWSELBAuthSessionCookieshards. Later requests have those shards reassembled and validated, andX-Amzn-Oidc-Identity,-Accesstokenand-Dataare injected before the request reaches the target.SessionCookieName,SessionTimeout,Scopeand all threeOnUnauthenticatedRequestmodes are honoured.ClientSecretis kept for the token exchange but never echoed back fromDescribe*, matching AWS.The session lives entirely in the cookie, as it does on AWS, so no server-side lookup is needed to validate a request. AWS encrypts that cookie with a key only the load balancer holds; here it is base64-encoded JSON, because the point is to reproduce the protocol a client observes, not to keep a secret from the developer running the emulator.
Cookie sharding
Shards are sized so the complete
Set-Cookiepair — name, value and attributes — fits the 4096-byte per-cookie ceiling browsers enforce. Sizing to 4096 bytes of value alone yields a cookie the browser silently discards, which presents exactly like having no session at all. I got this wrong first time round and only caught it because a real Chrome dropped the shard.Two fixes this depends on
Both are pre-existing bugs, found because
authenticate-oidccannot work around them.Rule actions now run as a chain, in
Order. Dispatch executedActions[0]and ignored the rest, so the two-action shape every authentication rule has — authenticate, then forward — could not work. Authentication actions either short-circuit or pass the request to the action behind them; the first terminal action answers.ModifyListenernow reaches the data plane.CreateListenercopies the default actions into an auto-created default rule, and dispatch reads that rule rather than the listener.ModifyListenerupdated only the listener record, so it reported success while traffic carried on hitting the original actions. There is a regression test for this one on its own.authenticate-cognitoreturns501rather than forwarding a request nobody authenticated — failing loudly beats a target that assumes the load balancer checked.Tests
tests/test_alb_authenticate_oidc.py, 18 tests: the config round-trip through Create/Describe on both listeners and rules, shard sizing and reassembly (including a missing shard invalidating the session), and the data-plane behaviour a client observes for eachOnUnauthenticatedRequestmode, an established session, an expired one, a forged callback, and a custom cookie name.Existing
tests/test_alb.py: 35 passed, 5 pre-existing failures unrelated to this change — theinstance/ipstreaming tests, which need a host HTTP server my container runtime does not route back to. I verified those five fail identically onmainwith this branch stashed.Scope
Per CONTRIBUTING, the issue-first rule covers new services; this extends an existing one (
ministack/services/alb.py) and touches no Dockerfile, CI, pyproject or dependency. No new dependencies — the back-channel token exchange useshttp.clientthroughrun_reentrant, the same way the target proxy does.Happy to split the two data-plane fixes into their own PR if you would rather review them separately.