-
Notifications
You must be signed in to change notification settings - Fork 16.3k
Fix infinite redirect caused by mistakenly setting token cookie as secure #49721
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
Conversation
…cure `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. I have also updated the Simple security manager to look at X-Forwarded-Proto so that if there is a reverse proxy in front (and if Airflow is configured to trust that) then it sets the secure flag
|
I can confirm that the change to the simple auth manager works with docker! |
… token cookie as secure `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. Provider version of apache#49721
…cookie as secure (#49721) `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. I have also updated the Simple security manager to look at X-Forwarded-Proto so that if there is a reverse proxy in front (and if Airflow is configured to trust that) then it sets the secure flag (cherry picked from commit 76edd92) Co-authored-by: Ash Berlin-Taylor <ash@apache.org>
…cookie as secure (#49721) (#49733) * [v3-0-test] Fix infinite redirect caused by mistakenly setting token cookie as secure (#49721) `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. I have also updated the Simple security manager to look at X-Forwarded-Proto so that if there is a reverse proxy in front (and if Airflow is configured to trust that) then it sets the secure flag (cherry picked from commit 76edd92) Co-authored-by: Ash Berlin-Taylor <ash@apache.org> * Update airflow-core/docs/core-concepts/auth-manager/index.rst --------- Co-authored-by: Ash Berlin-Taylor <ash@apache.org>
|
can this be manually applied on a 3.0.0 with docker-compose? |
@brechtvb Yes it can. One has to edit the image (spin up a container, access it via the cli, apply the diff, exit the container, commit), then modify docker-compose if the image name was changed and re-run. |
|
@Dev-iL i tried to do so, i can modify the py file, but i have no idea where to find the .rst file. when only modifying the .py file, the |
|
@brechtvb disregard that one, it's just docs. I only modified the py, though I'm using the simple auth manager. Perhaps you edited the py file incorrectly (e.g. add tab instead of spaces making it an invalid script)? |
|
very odd - not working 2 times from scratch. I'm familiar with python and docker. when running |
|
@brechtvb does this happen with a clean, or a migrated db? Did you prune volumes in between attempts? |
|
yes, so i used the docker-compose setup and i do this:
i make sure all previous containers are gone. |
|
@brechtvb which auth manager are you using? Did you try the other one? |
…cure (#49721) (#49733) `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. I have also updated the Simple security manager to look at X-Forwarded-Proto so that if there is a reverse proxy in front (and if Airflow is configured to trust that) then it sets the secure flag (cherry picked from commit 76edd92) Co-authored-by: Ash Berlin-Taylor <ash@apache.org> * Update airflow-core/docs/core-concepts/auth-manager/index.rst --------- Co-authored-by: Ash Berlin-Taylor <ash@apache.org>
|
i just use this: https://airflow.apache.org/docs/apache-airflow/3.0.0/docker-compose.yaml and modified the airflow container as the diff describes, then committed that to (local) tag: 3.0.0-fixed and updated that in the docker-compose.yaml |
Looking at the error message you need to set the config |
|
@vincbeck that would be odd - it is nowhere mentioned in the deployment docs. All i did was fixing the infinite loop redirect in the docker container. |
|
A possible work around for anyone eager to try out 3.0.0 before we get it released in 3.0.1: FROM apache/airflow:3.0.0
USER root
RUN apt update && apt install -y patch patchutils
RUN set -ex; \
cd /home/airflow/.local/lib/python3.12/site-packages/airflow; \
curl https://patch-diff.githubusercontent.com/raw/apache/airflow/pull/49721.patch \
| filterdiff -p1 -i 'airflow-core/src/airflow/*' | patch -p4 -u --verbose
USER airflow |
|
This does not fix it for me - i guess i'm doing something wrong. Still in a loop when logging in with airflow:airflow new container listed & updated in the docker-compose.yaml (to use the new one)
|
|
@brechtvb What Auth Manager are you using? What are the URLs you get redirected through please? |
|
If you are using the FAB auth manger, then you will need the patch for #49724 too: FROM apache/airflow:3.0.0
USER root
RUN apt update && apt install -y patch patchutils
RUN set -ex; \
cd /home/airflow/.local/lib/python3.12/site-packages/airflow; \
curl https://patch-diff.githubusercontent.com/raw/apache/airflow/pull/49721.patch \
| filterdiff -p1 -i 'airflow-core/src/airflow/*' | patch -p4 -u --verbose; \
curl https://patch-diff.githubusercontent.com/raw/apache/airflow/pull/49724.patch \
| patch -p5 -u --verbose; \
curl https://patch-diff.githubusercontent.com/raw/apache/airflow/pull/49581.patch \
| filterdiff -p1 -i 'providers/fab/src/airflow/*' | patch -p5 -u --verbose
USER airflow |
|
@ashb it'w working! thank you very much. i believe there is small typo, the double
|
…cure (apache#49721) `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. I have also updated the Simple security manager to look at X-Forwarded-Proto so that if there is a reverse proxy in front (and if Airflow is configured to trust that) then it sets the secure flag
… token cookie as secure (apache#49724) `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. Provider version of apache#49721
… token cookie as secure (apache#49724) `api/ssl_cert` has a default value of "" so `has_option` is always returning true, so we have to find in a slightly more complex way of telling if this setting is turned on. Provider version of apache#49721
|
@ashb With your solution for FAB auth manager, I am finding this error applying the second patch: |
|
@diegoscarabelli can you try 3.0.1-rc1 and see if the problem is fixed? |
I tried this version in my docker compose file and confirmed it worked! |
Yes, 3.0.1rc1 works. Hopefully becomes official soon. Thanks! |
Yup, next Monday (12 May) is a likely date |
|
Maybe off topic: Is the docker compose deployment deemed fit for production environments? |
Nope, check https://airflow.apache.org/docs/apache-airflow/stable/howto/docker-compose/index.html#:~:text=This%20procedure%20can,Airflow%20in%20production. to know why :) |
|
Trying to use FYR #49721 (comment) has patch files that don't exist |
But this link talks about production docker images |


api/ssl_certhas a default value of "" sohas_optionis always returningtrue, so we have to find in a slightly more complex way of telling if this
setting is turned on.
I have also updated the Simple security manager to look at X-Forwarded-Proto
so that if there is a reverse proxy in front (and if Airflow is configured to
trust that) then it sets the secure flag.
This PR only touches airflow core -- there will be another PR for fixing FAB
manager.
Fixes #49495
^ 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 airflow-core/newsfragments.