Skip to content
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

Feature/add token authentication to internal api #40899

Merged

Conversation

jscheffl
Copy link
Contributor

@jscheffl jscheffl commented Jul 20, 2024

During the implementation of authentication protection for AIP-69 I realized that Internal API does not carry support for authentication access.

Due to review of #40897 - alternative PR using token authentication on internal API

Noteworthy: Compared to other use I just added the token to the JSON request body dict, thought this is "simpler" than adding a POST/request parameter to the call, API is internal anyway.

@potiuk
Copy link
Member

potiuk commented Jul 20, 2024

I looked in detail, and (sorry for that I was just quickly looking it up - but the concept is similar).

The actual code I think we should model it from is similar but adds a bit more protection is the one we use to retrieve logs (because It also adds expiry-date for the request) - and it uses JWTSigner Instead of itsdangerous.

Here is the server side:

@flask_app.before_request

And here is the client side:

https://github.com/apache/airflow/blob/main/airflow/utils/log/file_task_handler.py#L94

Conceptually it's very similar to signer - you can sign the whole payload of the request - and pass it as "Authorization:" header.

This way:

  • the signature cannot be re-used in another request
  • it has expiry date
  • also it can have salt added (the audience is effectively salt)

@potiuk
Copy link
Member

potiuk commented Jul 20, 2024

(and I am quickly fixing the selective check error)

@potiuk
Copy link
Member

potiuk commented Jul 20, 2024

Fix to selective checks failing here: #40904

@jscheffl jscheffl force-pushed the feature/add-token-authentication-to-internal-api branch from 70a98a0 to 132e8df Compare July 20, 2024 20:59
@jscheffl
Copy link
Contributor Author

@potiuk Thanks for the fix in selective checks. Have re-worked now with JWT approach - this is how you wanted to have it?

Note I am off on a cycling tour through switzerland tomorrow, will check probably in the evening, will not be online frequently.

@potiuk
Copy link
Member

potiuk commented Jul 21, 2024

I hope you have a good tour!

Yes that's what I thought about.

@jscheffl jscheffl force-pushed the feature/add-token-authentication-to-internal-api branch from 132e8df to 44c1a0e Compare July 21, 2024 20:42
@potiuk
Copy link
Member

potiuk commented Jul 21, 2024

One small comment on that. I think the mechanism is cool. But we should use different secret key - not the webserver one. Otherwise DAG authors will have access to webserver key which is somewhat sensitive. the Internal API key should be different.

@jscheffl
Copy link
Contributor Author

One small comment on that. I think the mechanism is cool. But we should use different secret key - not the webserver one. Otherwise DAG authors will have access to webserver key which is somewhat sensitive. the Internal API key should be different.

Yes, DAG authors could have access to the secret key. But all is internal API components need to have a shared secret. Even if we use any other means (adding another different key, using a specific keyfile, even if public/private keypair is used) the DAG code has access to these details and could grab the key.

This might be a very more complex mechanism needed if the key should be secured and this might impose a more complex implementation. I'd propose to take this into consideration when we go to Airflow 3, as ash also described a few mechanisms in his AIP-72 that might be used.

So, WDYT:
a) okay, LGTM
b) please use one additional key for internal API, replace webserver-key - gut a static shared key in Airflow conf is okay in general
c) We need to re-think the key distribution also in Airflow 2.10 for this

@potiuk
Copy link
Member

potiuk commented Jul 22, 2024

b)

@jscheffl jscheffl force-pushed the feature/add-token-authentication-to-internal-api branch from 44c1a0e to a614996 Compare July 22, 2024 19:22
airflow/config_templates/config.yml Outdated Show resolved Hide resolved
Co-authored-by: Vincent <97131062+vincbeck@users.noreply.github.com>
@jscheffl jscheffl merged commit 5b28933 into apache:main Jul 22, 2024
48 checks passed
@ephraimbuddy ephraimbuddy added the type:new-feature Changelog: New Features label Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:new-feature Changelog: New Features
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants