potiuk commented on issue #23:
URL: https://github.com/apache/tooling-agents/issues/23#issuecomment-4397844720
Thanks @andrewmusselman for the L3 follow-up — much wider net (200 findings
vs. 16 on the L1 run). I went through the new report at `5872e6c`. Triage below.
Two preliminary notes:
- The 5 redacted Critical findings forwarded to PMC private will be handled
there; nothing in this comment touches them.
- The L3 numbering doesn't continue from the L1 numbering, so I matched by
theme rather than by ID.
## Theme overlap with prior L1 triage
These were already addressed by the PRs from the previous round; the L3
report re-surfaces them under new IDs. No further action.
| Theme | Prior PR | New L3 IDs | Status |
|---|---|---|---|
| Password gen `random` → `secrets` | #66500 | F-009 | ✅ merged / in flight |
| `_collect_teams_to_check` JSONDecode fail-open | #66504 | F-008 (related),
F-060 (extension — see below) | ✅ partial; extension below |
| `/auth` and `/pluginsv2` not in RESERVED_URL_PREFIXES | #66501 | F-061,
F-094 | ✅ |
| SimpleAuthManager login cookie missing `samesite` | #66502 | F-088 | ✅ |
| CORS `allow_credentials` configurable | #66503 | F-090 | ✅ |
| Default-deny via router-level `Depends(get_user)` | #66505 | F-007
(partial), F-133 (extension — see below) | ✅ partial; extension below |
## New actionable findings — fixing in this round
These are real code-level concerns not covered by the prior PRs. Drafting
fixes:
| # | Finding | Plan |
|---|---|---|
| **F-008** | `services/login.py:60` uses `==` to compare passwords — timing
leak | Switch to `hmac.compare_digest()`. SimpleAuthManager is dev-only, but
it's a one-line defense-in-depth win and the threat ("timing analysis from
local network") is plausible even in dev. |
| **F-060** | Authorization dependency reads `dag_id` / `team_name` from raw
JSON body before Pydantic validates type — could pass non-string to authz logic
| Extend the JSONDecode fail-closed from #66504 with explicit type validation,
returning 400 before authz runs on malformed input. |
| **F-039** | Auto-generated SimpleAuthManager passwords printed to
stdout/logs, no production guardrail | Add a startup warning when the
deployment shape looks production-like (multi-worker, non-localhost API bind,
Postgres/MySQL backend). Loud, not blocking. |
| **F-133** | `get_user()` accepts `request.state.user` from any plugin
middleware without verification | Add a trust-tag (set only by
`SimpleAllAdminMiddleware` and the JWT pathway) so untrusted plugin middleware
can't inject pre-built users. The audit's specific concern that "any FastAPI
plugin middleware can inject a user object, bypassing JWT signature validation"
is a fair defense-in-depth point even though plugins are trusted code in our
model. |
| **F-175** | Documentation default for Kerberos ccache is
`/tmp/airflow_krb5_ccache` (world-accessible) | Docs PR — recommend a
non-world-accessible path and matching `chmod 600`-style guidance, mirroring
what we already say for the keytab. |
I'll open separate PRs for each and link them back to this comment.
## Acknowledged but not actionable
| # | Finding | Why |
|---|---|---|
| F-152 | WSGI request-smuggling via conflicting `Content-Length` +
`Transfer-Encoding` | The finding itself notes this is mitigated upstream —
Uvicorn (httptools / h11) already rejects conflicting CL/TE before the
WSGIMiddleware ever sees the request. Adding redundant middleware is
duplicative. |
| F-141 | `jti` is `uuid4()` and "doesn't meet 128-bit entropy" |
`uuid.uuid4()` provides 122 bits of entropy from a CSPRNG (`os.urandom(16)`).
The ASVS "128 bits literal" interpretation is debatable; the practical security
property holds. Not changing. |
| F-019 | Missing `HTTPSRedirectMiddleware` on API endpoints | TLS
termination is deployment-manager / reverse-proxy responsibility per
[`security_model.rst`](https://github.com/apache/airflow/blob/main/airflow-core/docs/security/security_model.rst).
Adding redirect middleware in the app is environment-dependent and conflicts
with the documented threat model. |
## By design — per Airflow's security model
The remainder cluster into a small set of categories whose rationale lives
in
[`security_model.rst`](https://github.com/apache/airflow/blob/main/airflow-core/docs/security/security_model.rst)
and
[`jwt_token_authentication.rst`](https://github.com/apache/airflow/blob/main/airflow-core/docs/security/jwt_token_authentication.rst).
One sentence each:
| Category | Findings | Why |
|---|---|---|
| InProcess Execution API auth bypass | F-153, F-154, F-201 | Documented
design — per-team DFP/Triggerer separation is the deployment guidance for
multi-team. |
| MFA / session view-terminate / re-auth / step-up | F-011, F-012, F-013,
F-014, F-017, F-041, F-042, F-043, F-044 | SimpleAuthManager is dev-only;
production deployments use FAB / Keycloak / external IdP, where these belong. |
| Field-level authorization | F-015, F-016, F-049, F-053 | Authz model is
intentionally resource-level via `BaseAuthManager.is_authorized_*()`.
Field-level would be an AIP. |
| Multi-team / multi-tenant gaps | F-018, F-056, F-057, F-058, F-149, F-194,
F-195 | Multi-team is documented as not enforcing strict task-level isolation
in the "What is NOT considered a security vulnerability" section. |
| TLS / mTLS / cipher / OCSP / ECH | F-028, F-063, F-064, F-067, F-091,
F-101, F-102, F-167, F-168, F-177, F-178 | Reverse-proxy / deployment-manager
responsibility. |
| Security-headers (CSP / HSTS / X-Frame-Options / Referrer-Policy / COOP) |
F-024, F-025, F-026, F-027, F-092, F-093, F-150, F-167, F-168, F-169, F-170,
F-172 | Reverse-proxy / deployment-manager responsibility (same as F-010/F-014
from the L1 run). |
| OAuth/OIDC abstract-contract gaps in `BaseAuthManager` | F-031, F-098,
F-099, F-125, F-127, F-128, F-129, F-130, F-131, F-132, F-137, F-138, F-196,
F-199, F-200 | `BaseAuthManager` is an interface; concrete managers (FAB,
Keycloak) implement OAuth/OIDC details where supported. The interface
deliberately does not constrain them. |
| Dependency-CVE policy | F-182, F-183, F-185, F-186 | Documented stance in
[`vulnerabilities-in-3rd-party-dependencies.rst`](https://github.com/apache/airflow/blob/main/airflow-core/docs/security/vulnerabilities-in-3rd-party-dependencies.rst).
|
| Audit-log schema gaps | F-020, F-021, F-022, F-023, F-072 through F-081,
F-161, F-162, F-163 | Substantive doc improvements; not security bugs. Worth
filing as separate doc-improvement issues if any are blocking your audit. |
| SimpleAuthManager password lifecycle | F-006, F-007, F-010, F-035, F-036,
F-037, F-040, F-146, F-205 | Dev-only auth manager; production deployments do
not use it. |
| Crypto policy / key-management docs | F-118, F-119, F-120, F-121, F-192,
F-193 | Meta-policy docs; the underlying crypto operations are correct (and
explicitly called out as a positive control in your own consolidated summary). |
## Worth a follow-up but not in this batch
A couple of findings have substance but need more design work than a quick
PR:
- **F-202** — `RevokedToken.is_revoked()` skipped when `jti` claim absent.
This is by-design for Execution-API tokens (no revocation lookup per request,
see prior F-005 triage), but the conditional makes the *user-session*
revocation path subtly fragile. Worth a defense-in-depth assertion that `jti`
is present on tokens that flow through that path.
- **F-061 / F-094** — Flask plugin mount on same origin as FastAPI app.
Partly addressed by `RESERVED_URL_PREFIXES` (PR #66501) and router-level
`Depends(get_user)` (PR #66505), but a long-term migration off the Flask bridge
is the structural answer. Tracking issue rather than a fix.
## Summary
- 5 PRs from the L1 round still apply and resolve their L3 equivalents.
- 5 new PRs incoming for F-008, F-060, F-039, F-133, F-175.
- ~125 of the remaining findings are by design per the documented security
model — happy to dig deeper on any specific one if the rationale isn't matching
what you're seeing.
---
This response was generated by AI and may contain mistakes. After you've had
a chance to look at the PRs and respond, a real Airflow maintainer (human) will
follow up.
---
Drafted-by: Claude Opus 4.7 (1M context); 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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
