potiuk commented on PR #72647: URL: https://github.com/apache/airflow/pull/72647#issuecomment-5767914469
@vincbeck @o-nikolas — answering the "is that really an issue then?" directly, because I think two separate claims are getting merged. "IdP-initiated SSO is used" and "accepting unsolicited assertions is safe" are different statements. The first is true and I'm not disputing it. The problem is that without `InResponseTo`, the SP cannot tell apart: - a genuine access-portal login, and - an attacker POSTing *their own* legitimately-obtained, correctly-signed assertion into a victim's browser. Those look identical on the wire. There is no field `login_callback()` could consult to distinguish them. Nothing is forged, and signature validation works exactly as intended — the assertion authenticates *the identity in the response*, it just never establishes *which browser asked*. The consequence is a forced login: the victim lands in the attacker's Airflow session, and whatever they do next — edit a connection, paste a token, upload something — goes somewhere the attacker can read later. So I don't think it's a non-issue. But the usability objection is right, and it killed option 1. I've dropped it and implemented **option 2**, now pushed: - the default binds the response to the login this browser started; - `[aws_auth_manager] allow_idp_initiated_login` (default `False`) accepts unsolicited assertions for deployments that need the access portal tile. Deployments that never use IdP-initiated SSO get the fix for free; the ones that do keep working after a one-line config change, having made that choice knowingly. With the option on, a response carrying `InResponseTo` is still refused — an unsolicited assertion answers no request, so one naming a request is a solicited assertion being replayed, and python3-saml skips that comparison entirely when handed no request id. **Correcting something I wrote in the description.** I said the state should move server-side keyed by an opaque nonce, and that this would close the cookie-injection path where signing would not. That's wrong: an attacker who can write cookies for this origin can start a real login, obtain genuine server-side state, and plant *that* nonce just as easily. Cookie-write access defeats both — it's a different threat. What server-side state actually buys is single-use enforcement and concurrent logins, and neither needs a store. So the state is now a signed, multi-entry cookie: each login gets a nonce that rides in `RelayState` and names which pending login a response answers. Two tabs no longer overwrite each other, the matched entry is consumed on use, and entries can't be forged. That also keeps shared mutable state out of the API server, which has nowhere to put it — `[api] workers` defaults to 1 and HA is documented as multiple API servers, so an in-process store would have been wrong. **One thing worth flagging separately.** Wiring up the tests, I found that `TestLoginRouter` has never been collected — by anyone, including CI. `mock_plugin_manager` is a `contextmanager`, and a `ContextDecorator` applied to a class replaces the class with a function, so pytest found nothing and reported success. That is what the "collects zero items" note in the description was; it wasn't my environment. Moving it to an autouse fixture ran the module for the first time and immediately turned up two assertions that had never executed: a missing `[api] base_url`, and a `SameSite` check comparing against the wrong case. There are 27 tests in the file now and they pass. `check_contextmanager_class_decorators` already exists for exactly this failure mode but didn't list `mock_plugin_manager` — it does now, with tests. This file was the only one in the repo affected. Rebased on current `main`, static checks green. I'll take it out of draft once you're happy with option 2 as the shape. If you'd still rather close it, I'll close it — but I'd want that recorded as a deliberate accept of the risk rather than "not an issue". --- Drafted-by: Claude Opus 5; reviewed by @potiuk before posting -- 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]
