You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "pierrejeambrun (via GitHub)" <gi...@apache.org> on 2025/03/17 11:38:20 UTC
[PR] AIP-38 Fix safari login loop in dev mode [airflow]
pierrejeambrun opened a new pull request, #47859:
URL: https://github.com/apache/airflow/pull/47859
Safari has a stricter policy when it comes to secure cookies. I will not allow `localhost` to use such cookies (https is always required), which is not the case for other browser.
This will at least fix the case for the development mode. i.e safari will work in `--dev-mode` in breeze or by specifying the env variable and running the api_server.
> It will still not work in `non dev-mode` via breeze though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #47859:
URL: https://github.com/apache/airflow/pull/47859#discussion_r1999099855
##########
providers/fab/src/airflow/providers/fab/www/views.py:
##########
@@ -69,7 +69,9 @@ def index(self):
if g.user is not None and g.user.is_authenticated:
token = get_auth_manager().generate_jwt(g.user)
response = make_response(redirect(f"{conf.get('api', 'base_url')}", code=302))
- response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=True)
+
+ secure = bool(conf.get("api", "ssl_cert"))
+ response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=secure)
Review Comment:
Indeed, that could also be a solution. I think this brings also questions about ssl termination point and header forwarding. That's harder to test and would require a bit more investigation to be honest so I think we can go with what we have at first, just to unblock safari and later we can always improve that when we have more time to spend on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun merged PR #47859:
URL: https://github.com/apache/airflow/pull/47859
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop in dev mode [airflow]
Posted by "github-advanced-security[bot] (via GitHub)" <gi...@apache.org>.
github-advanced-security[bot] commented on code in PR #47859:
URL: https://github.com/apache/airflow/pull/47859#discussion_r1998556662
##########
providers/fab/src/airflow/providers/fab/www/views.py:
##########
@@ -69,7 +70,9 @@
if g.user is not None and g.user.is_authenticated:
token = get_auth_manager().generate_jwt(g.user)
response = make_response(redirect(f"{conf.get('api', 'base_url')}", code=302))
- response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=True)
+
+ secure = os.environ.get("DEV_MODE", True)
+ response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=secure)
Review Comment:
## Failure to use secure cookies
Cookie is added without the HttpOnly attribute properly set.
[Show more details](https://github.com/apache/airflow/security/code-scanning/437)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop in dev mode [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #47859:
URL: https://github.com/apache/airflow/pull/47859#issuecomment-2729189099
Maybe only enable the `secure` thing when there is an ssl config passed ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on code in PR #47859:
URL: https://github.com/apache/airflow/pull/47859#discussion_r1998728751
##########
providers/fab/src/airflow/providers/fab/www/views.py:
##########
@@ -69,7 +69,9 @@ def index(self):
if g.user is not None and g.user.is_authenticated:
token = get_auth_manager().generate_jwt(g.user)
response = make_response(redirect(f"{conf.get('api', 'base_url')}", code=302))
- response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=True)
+
+ secure = bool(conf.get("api", "ssl_cert"))
+ response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=secure)
Review Comment:
```suggestion
response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=request.is_secure)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #47859:
URL: https://github.com/apache/airflow/pull/47859#issuecomment-2729487918
> Maybe only enable the secure attribute when there is api.ssl_cert in the conf ? Actually that's a better idea I think, let me update that
That alone isn't enough -- it is much more common to use some kind of proxy in front to handle the TLS termination (ALB, Nginx, Kubernetes Ingress etc.) so we'll have to look at something else (at least as well)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #47859:
URL: https://github.com/apache/airflow/pull/47859#discussion_r1999346750
##########
providers/fab/src/airflow/providers/fab/www/views.py:
##########
@@ -69,7 +69,9 @@ def index(self):
if g.user is not None and g.user.is_authenticated:
token = get_auth_manager().generate_jwt(g.user)
response = make_response(redirect(f"{conf.get('api', 'base_url')}", code=302))
- response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=True)
+
+ secure = bool(conf.get("api", "ssl_cert"))
+ response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=secure)
Review Comment:
Because apparently the `secure` attribute is not always properly set when the TLS termination is handled before. (proxy forward header might be a good place though)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #47859:
URL: https://github.com/apache/airflow/pull/47859#issuecomment-2730413883
Issue here https://github.com/apache/airflow/issues/47878 not sure if this is required for 3.0 or can wait later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop in dev mode [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on PR #47859:
URL: https://github.com/apache/airflow/pull/47859#issuecomment-2729186303
If deployment do not use https safari will block such secure cookies and indeed this will not work.
Is that a popular use case ? Do we need to support safari for that ?
I don't think using insecure cookies for deployment is an option even for non https setup... so I don't think safari will work there...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #47859:
URL: https://github.com/apache/airflow/pull/47859#issuecomment-2729497405
It's possible we can look at the `request.secure` or something. We have this in our config
```
proxy_fix_x_proto:
description: |
Number of values to trust for ``X-Forwarded-Proto``.
See `Werkzeug: X-Forwarded-For Proxy Fix
<https://werkzeug.palletsprojects.com/en/2.3.x/middleware/proxy_fix/>`__ for more details.
version_added: 1.10.7
type: integer
example: ~
default: "1"
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #47859:
URL: https://github.com/apache/airflow/pull/47859#discussion_r1999092136
##########
providers/fab/src/airflow/providers/fab/www/views.py:
##########
@@ -69,7 +69,9 @@ def index(self):
if g.user is not None and g.user.is_authenticated:
token = get_auth_manager().generate_jwt(g.user)
response = make_response(redirect(f"{conf.get('api', 'base_url')}", code=302))
- response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=True)
+
+ secure = bool(conf.get("api", "ssl_cert"))
+ response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=secure)
Review Comment:
Updated, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop for non ssl [airflow]
Posted by "pierrejeambrun (via GitHub)" <gi...@apache.org>.
pierrejeambrun commented on code in PR #47859:
URL: https://github.com/apache/airflow/pull/47859#discussion_r1999092136
##########
providers/fab/src/airflow/providers/fab/www/views.py:
##########
@@ -69,7 +69,9 @@ def index(self):
if g.user is not None and g.user.is_authenticated:
token = get_auth_manager().generate_jwt(g.user)
response = make_response(redirect(f"{conf.get('api', 'base_url')}", code=302))
- response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=True)
+
+ secure = bool(conf.get("api", "ssl_cert"))
+ response.set_cookie(COOKIE_NAME_JWT_TOKEN, token, secure=secure)
Review Comment:
Updated, thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] AIP-38 Fix safari login loop in dev mode [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #47859:
URL: https://github.com/apache/airflow/pull/47859#issuecomment-2729176794
Does this mean that deployments must be using https as well?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org