-
Notifications
You must be signed in to change notification settings - Fork 14.2k
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 authentication to lineage endpoint for experimental API #13870
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 Contribution Guide (https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst)
|
Thanks @iangcarroll for this PR. However please note this experimental API is already deprecated (https://github.com/apache/airflow/blob/master/airflow/config_templates/default_airflow.cfg#L374), and we favour the stable REST API. Currently experimental API is disabled by default (https://github.com/apache/airflow/blob/master/UPDATING.md#the-experimental-rest-api-is-disabled-by-default), and I would suggest migrating away from it, instead of making any further change on it. |
Hi @XD-DENG, sure, I agree with that. However, it still exists, and if anyone has it enabled, they would be vulnerable to a security issue, as I do not see any other authorization check on this endpoint. This would be unexpected by anyone who has authentication configured for the experimental API. And, if anyone has set the |
Yep. Agree - if that is a regression, we should fix it - while the stable API is deprecated, we still support it in 2.0 and regressions should be fixed. |
@bolkedebruin -> I think that API was added by you, is there any reason why it should be kept unauthenticated? Can you please confirm this was an accidental removal of "require_authentication" ? |
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease. |
Awesome work, congrats on your first merged pull request! |
(cherry picked from commit 24a5424)
(cherry picked from commit 24a5424)
I couldn't find a good reason why this endpoint was missing the authentication decorator. This would likely break any clients calling this endpoint in an unauthenticated manner, but given it's not documented that it should be unauthenticated, I would imagine this is fine.
This might deserve a low-impact CVE. It looks like it went wrong in fbd994a, during a refactor of the stable API which seems unrelated to this endpoint.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code change, 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 UPDATING.md.