stegololz commented on issue #75560: URL: https://github.com/apache/airflow/issues/75560#issuecomment-6416845320
Thanks @baptistegh, I like the overall shape: the API server does the Keycloak exchange, airflowctl stays IdP-agnostic, the secret stays server-side, `password` stays the default grant, both flows are off by default, and the provider PR comes first. **What I'd like to see settled before the provider PR:** - **Discovery instead of 404s.** As I read it, in the browser flow the browser makes the first request, so airflowctl never sees a 404 and just waits for the timeout. Other auth managers also answer with 200, 400 or 422 rather than 404. I'd add a small metadata route under the provider, e.g. `GET /auth/cli/metadata`, that airflowctl calls first. I think it can reuse the RFC 8414 field names (`grant_types_supported`, `code_challenge_methods_supported`, `authorization_endpoint`, `device_authorization_endpoint`, `token_endpoint`), but I don't think it should be a real .well-known. - **Token lifetime.** As far as I can tell, the CLI token carries the Keycloak access token, which only gets refreshed for browser sessions. After about 5 minutes (Keycloak's default), requests start returning 403. Password login has the same problem today, but I think repeating an SSO or MFA login every few minutes would kill the feature. I'd either add a `refresh_token` grant on `/auth/token/cli`, or document raising the access token lifespan. - **Keep `--no-browser` and device code separate.** If an admin enables only the browser flow, which I think is the safer setup, every SSH or container user is pushed to a disabled flow. - **PKCE is enforced by Airflow, not Keycloak.** I think turning on S256 for the existing Keycloak client would break the web login and the device flow. I'd like `/auth/login/cli` to require an S256 `code_challenge`, and the code exchange to require a `code_verifier`. **What I'd like the spec to cover:** - I'd give `/token/cli` its own body model (password | authorization_code | device_code), defaulting to password, and keep client_credentials and jwt-bearer off it. I'd use the URN `urn:ietf:params:oauth:grant-type:device_code` as the grant name. - I'd create the token the way the password grant does (CLI lifetime, refresh token kept), not the way the web callback does. Otherwise I think it breaks for clients that only send the Authorization header. - I'd pass `scope="openid"` in both new Keycloak calls. - As far as I can see, the PKCE parameters need python-keycloak >= 6.0, and the provider currently allows 5.x. **On the open questions:** 1. *Generic or Keycloak-only?* I'd consider a light `BaseAuthManager` hook: an optional `get_cli_login_methods()` (empty by default) behind a core metadata route, the same way `/auth/login` already delegates to `get_url_login()`. Every server could then answer discovery and airflowctl would get the models from the generated client, while the flows themselves stay in the Keycloak provider under neutral paths. 2. *SSO by default?* I wouldn't at first. Once the metadata route exists, I think a bare `auth login` on a TTY could use SSO when the server offers it and doesn't offer password login. I'd never switch automatically without a TTY. -- 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]
