-
Notifications
You must be signed in to change notification settings - Fork 14.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fab_auth_manager: allow get_user method to return the user authenticated via Kerberos #43662
Conversation
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the catch and the fix!
Some static check failures, shoud be easy to fix :) See documentation |
…ted via Kerberos The issue this PR fixes was initially discussed in apache#39683. @jijoj-hmetrix and I noticed that, starting from Airflow 2.8.0, Kerberos authentication does not seem to work with the stable API. Even when a user provides a valid Kerberos ticket, that the whole gssapi authentication dance is successful, and that the user has the required permissions, the API returns a 403 response. ```console $ curl --negotiate -u: -s --service-name airflow https://airflow-test.xxxx.com/api/v1/pools | jq . { "detail": null, "status": 403, "title": "Forbidden", "type": "https://airflow.apache.org/docs/apache-airflow/2.10.2/stable-rest-api-ref.html#section/Errors/PermissionDenied" } ``` I found that [`airflow.providers.fab.auth_manager.fab_auth_manager.FabAuthManager.get_user`](https://github.com/apache/airflow/blob/baf2b3cb4453d44ff00598a3b0c42d432a7203f9/providers/src/airflow/providers/fab/auth_manager/fab_auth_manager.py#L185-L189) relies on flask-login's [current_user](https://github.com/maxcountryman/flask-login/blob/main/src/flask_login/utils.py#L25) to get the currently logged in user from the session. However, the Kerberos auth backend stores the authenticated user [in `g`](https://github.com/brouberol/airflow/blob/main/providers/src/airflow/providers/fab/auth_manager/api/auth/backend/kerberos_auth.py#L136) and not in the session. This patch allows the current user to be pulled either from `g` or the session, which allows the API to detect the user authenticated via Kerberos, and then link it to Fab permissions. Here's an examle from an instance running with the patch, with a admin user associated with a User account with Admin permissions: ```console $ curl --negotiate -u: -s --service-name airflow https://airflow-test.xxx.com/api/v1/pools { "pools": [ { "deferred_slots": 0, "description": "Default pool", "include_deferred": false, "name": "default_pool", "occupied_slots": 0, "open_slots": 128, "queued_slots": 0, "running_slots": 0, "scheduled_slots": 0, "slots": 128 } ], "total_entries": 1 } ``` I accompany the change with a small unit test. Signed-off-by: Balthazar Rouberol <[email protected]>
416fa1a
to
c89676d
Compare
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
Thank you!
…On Tue, Nov 5, 2024, at 5:07 PM, boring-cyborg[bot] wrote:
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker <https://github.com/apache/airflow/issues> for additional contributions.
—
Reply to this email directly, view it on GitHub <#43662 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADVHA7TOAYVZUBJKKX5FJLZ7DUMBAVCNFSM6AAAAABRFDVPQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDINJXGU4DONZQGY>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
@brouberol do you know why the code in your PR has been removed in the latest Airflow image? say 2.10.3 |
@nicolasge if that's indeed the case, I wasn't aware of it, sorry. Edit: it seems that the fix is indeed missing https://github.com/apache/airflow/blob/2.10.3/airflow/providers/fab/auth_manager/fab_auth_manager.py#L170-L174 |
@brouberol -> see the comment in #44943 -> providers are always released from main. You need to see which provider version it has been released in and have that provider. If it was not released in 2.10.3 - look at 2.10.4rc1 that is just being voted - maybe it contains newer provider version with the fix. Look at https://airflow.apache.org/docs/apache-airflow-providers/index.html to learn how providers vs. core work. |
Understood, thanks!
…On Sun, Dec 15, 2024, at 7:31 PM, Jarek Potiuk wrote:
@brouberol <https://github.com/brouberol> -> see the comment in #44943 <#44943> -> providers are always released from main. You need to see which provider version it has been released in and have that provider. If it was not released in 2.10.3 - look at 2.10.4rc1 that is just being voted - maybe it contains newer provider version with the fix.
Look at https://airflow.apache.org/docs/apache-airflow-providers/index.html to learn how providers vs. core work.
—
Reply to this email directly, view it on GitHub <#43662 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADVHA6H7GS4LNQFJVWOESL2FXDG5AVCNFSM6AAAAABRFDVPQ2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDKNBTHE4TEMZXGM>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
The issue this PR fixes was initially discussed in #39683.
@jijoj-hmetrix and I noticed that, starting from Airflow 2.8.0, Kerberos authentication does not seem to work with the stable API. Even when a user provides a valid Kerberos ticket, that the whole gssapi authentication dance is successful, and that the user has the required permissions, the API returns a 403 response.
I found that
airflow.providers.fab.auth_manager.fab_auth_manager.FabAuthManager.get_user
relies on flask-login's current_user to get the currently logged in user from the session.However, the Kerberos auth backend stores the authenticated user in
g
and not in the session.This patch allows the current user to be pulled either from
g
or the session, which allows the API to detect the user authenticated via Kerberos, and then link it to Fab permissions.Here's an example from an instance running with the patch, with a admin user associated with a User account with Admin permissions:
I accompany the change with a small unit test.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rst
or{issue_number}.significant.rst
, in newsfragments.