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)
|
vincbeck
left a comment
There was a problem hiding this comment.
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 <brouberol@wikimedia.org>
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_userrelies 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
gand not in the session.This patch allows the current user to be pulled either from
gor 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.rstor{issue_number}.significant.rst, in newsfragments.