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

Add support for basic authentication on internal API client #40897

Conversation

jscheffl
Copy link
Contributor

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

This PR adds support on the client side with HTTP Basic Authentication. Alongside with the option to define a different access URL to call the back-end.

Comment on lines +94 to +95
internal_api_user = conf.get("core", "internal_api_user")
internal_api_password = conf.get("core", "internal_api_password")
Copy link
Contributor

@dstandish dstandish Jul 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

depends how official you want to get, given that this is going to be experimental. but typically airflow config params are added to config.yml. And i think internal_api wants to be its own section. lastly, i would suggest considering rebranding it as rpc_server. That's how i called it in the helm chart, with the intention (though not the certainty) that we would call it that, and the reason is because there's a sort of not well known "internal API" already that exists just for the webserver... and ultimately i think we need in 3.0 a proper internal / external API distinction for the webserver (so we can freely build things that support webserver functionality without worrying about backcompat). so that results in a bit of a collision here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this sounds reasonable.

@potiuk WDYT... as 2.10 cut is around the corner, shall we make a (separate) search&replace PR (now!)? I could make this but before making this out of a wild guess... (I would be OK with renaming)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. We can rename it separately, but I'd rather use something less generic, internal_aip_44_api should be specific enough for example, very "temporrary" name as well. It has a little ripple effect and I'd rather do it when we fix other missing issues to not add extra complexity.

  2. Rather than using basic_auth, maybe a better approach will be that we sign the request and verify them in similar way as we do with get_log() method? That was the initial idea actually and then we would not need any other configuration.

    if not token:
        metadata = {}
    else:
        try:
            metadata = URLSafeSerializer(key).loads(token)
        except BadSignature:
            raise BadRequest("Bad Signature. Please use only the tokens provided by the API.")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(BTW. I will be looking at messages and responding - but I will go back to look at hanging migrations and standalone processor in a few hours @jscheffl ).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No pressure :-D

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking though the code I am wondering a bit regarding the log endpoint where the code snipped is coming from. Log endpoint also uses the decorator @security.requires_access_dag("GET", DagAccessEntity.TASK_LOGS) on top, means the token is a kind of second factor but not the only means of authentication. You need to authenticate to get logs. I don't see any handling with the decorator ignoring an HTTP-based auth if a token is provided.

Do you mean with your feedback that if we implement auth in internal API that we need to put the token "on top" to have a second factor? Or should a generated token based on the secret key replace/substiture any decorator e.g. which I just added in AIP-69 in https://github.com/apache/airflow/pull/40224/files#diff-5fb7e8d1b04e10947ebd11fdba06820f44cac02f2f36ad97b5964cf0273b05cfR69 (@requires_access_custom_view("POST", REMOTE_WORKER_API_ROLE)) - would a pure token authentication be sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think I understood now what you wanted to propose as alternative, will make an alternative PR in a moment

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about #40899 ?

@jscheffl jscheffl added this to the Airflow 2.10.0 milestone Jul 20, 2024
@jscheffl jscheffl force-pushed the feature/add-support-for-authentication-to-internal-api branch from c7c8302 to 2847a68 Compare July 20, 2024 10:06
@jscheffl
Copy link
Contributor Author

Closing in favor of #40899

@jscheffl jscheffl closed this Jul 20, 2024
@jscheffl jscheffl deleted the feature/add-support-for-authentication-to-internal-api branch July 20, 2024 15:57
@Sabbir02 Sabbir02 mentioned this pull request Jan 22, 2025
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants