potiuk commented on issue #23: URL: https://github.com/apache/tooling-agents/issues/23#issuecomment-4393406508
Thanks @andrewmusselman for running this — useful to have an external L1 sweep against airflow-core. I went through all 16 findings against the source at HEAD; here's the Airflow-side triage. **Real fixes worth doing — PRs in flight (5)** | # | Finding | Fix | PR | |---|---|---|---| | F-003 | `random.choices()` for password generation | Switch to `secrets.choice()` in `simple_auth_manager.py:443` | [apache/airflow#66500](https://github.com/apache/airflow/pull/66500) | | F-008 | `_collect_teams_to_check` swallows `JSONDecodeError` and returns an empty teams set, which causes the calling `requires_access_*` dependency to skip the authz callback entirely | Fail closed in `core_api/security.py:730-751` so the authz callback always runs at least once | [apache/airflow#66504](https://github.com/apache/airflow/pull/66504) | | F-009 | `/auth` and `/pluginsv2` not in `RESERVED_URL_PREFIXES` | Add them in `api_fastapi/app.py:64` | [apache/airflow#66501](https://github.com/apache/airflow/pull/66501) | | F-011 (partial) | SimpleAuthManager all-admins login cookie missing `samesite` | Add `samesite="lax"` in `simple/routes/login.py:94-100` (matches `JWTRefreshMiddleware`) | [apache/airflow#66502](https://github.com/apache/airflow/pull/66502) | | F-013 | CORS `allow_credentials` hardcoded `True` | Make configurable via `[api] access_control_allow_credentials` (default `True` for back-compat) | [apache/airflow#66503](https://github.com/apache/airflow/pull/66503) | F-008 is currently unreachable (every POST route uses Pydantic body validation that 422s before the auth dependency runs), and F-009 is defense-in-depth (plugins are trusted code in Airflow's model). Both are worth tightening because the patterns are fragile, not because they're exploitable today. **By design / out of scope per Airflow's documented security model** Documented 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): - **F-001, F-002, F-004** — `SimpleAuthManager` is dev-only ("This auth manager should not be used in production" — `simple_auth_manager.py:89-91`). Production deployments use FAB / Keycloak / external IdP, where rate-limiting, password rotation, and account lockout are the IdP's responsibility. - **F-005** — Execution-API and log-server tokens are short-lived JWTs scoped to a single task instance (per `jwt_token_authentication.rst`); the audit's claim that *"validation never checks the revocation list"* is also factually wrong — the user-session UI/API path **does** check, via `BaseAuthManager.get_user_from_token()` → `RevokedToken.is_revoked()` (`base_auth_manager.py:153`). The execution-API and log-server callers (`execution_api/security.py:125`, `execution_api/app.py:141`, `serve_logs/log_server.py:67`) intentionally skip revocation because their tokens are ephemeral and per-task; a revocation-list lookup on every worker request would be cost without threat-model benefit. - **F-006** — JWTs are short-lived; per-user invalidation on disable is the IdP's responsibility for external auth managers. - **F-010, F-012, F-014** — Security headers (CSP / HSTS / X-Frame-Options) and CSRF middleware are deployment-manager / reverse-proxy concerns. Airflow expects to be fronted by nginx / Traefik / a managed ingress in production. Worth a docs improvement; not a code fix. - **F-015, F-016** — The dependency-CVE policy in [`vulnerabilities-in-3rd-party-dependencies.rst`](https://github.com/apache/airflow/blob/main/airflow-core/docs/security/vulnerabilities-in-3rd-party-dependencies.rst) is a deliberate, communicated stance — not a doc gap. **Two findings the audit got factually wrong** - **F-005** as written ("validation never checks the revocation list") — it does, on the user-session path; see above. - **F-011** as written ("Authentication cookies lack Secure attribute") — `secure` IS set, conditional on HTTPS or configured `ssl_cert`, in both `simple/routes/login.py:94-100` and `auth/middlewares/refresh_token.py:66-74`. The actionable bit is the missing `samesite` on the SimpleAuthManager login response, which #66502 addresses. **Possibly worth a follow-up (not in this batch)** - **F-007** — auth is opt-in per route via `Depends(get_user)` rather than default-deny at the framework level. Existing routes consistently apply the dependency, but a global default-deny wrapper would be a reasonable hardening project. Not urgent. Happy to dig deeper on any of the by-design ones if the rationale isn't matching what you're seeing on your end. --- 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]
