Update settings so that SSL upgrade is not done, because it happens at the CDN/LB #148
Conversation
|
@bkochendorfer Am i right that the CDN-level HTTP->HTTPS redirect we get in bedrock is also how things work for birdbox? If so, does this all seem 👍 to you? |
stevejalim
force-pushed
the
adjust-https-redirect
branch
from
21c4652 to
db28d5a
Compare
stevejalim
force-pushed
the
adjust-https-redirect
branch
from
db28d5a to
28914d6
Compare
birdbox/birdbox/settings/base.py
Outdated
| ) # so default: "True" | ||
| if config("USE_SECURE_PROXY_HEADER", default=str(SECURE_SSL_REDIRECT), parser=bool): | ||
| ) | ||
| if config("USE_SECURE_PROXY_HEADER", default="True", parser=bool): |
There was a problem hiding this comment.
We shouldn't need this anymore either, right? Or it should at least default to False?
There was a problem hiding this comment.
Ah, I assumed we did need it, cos the CDN/LB infra was terminating HTTPS and forwarding on.
My thought process was more or less: Because we know it'll all be over HTTPS maybe we don't need it, but if it's not used then the docs ready like HttpRequest.is_secure() may return False, which could break other things (eg CSRF for the Wagtail side).
You could be totally right, though! I'll think on it a bit more
There was a problem hiding this comment.
On bedrock we short-circuit that was some trickery in the wsgi file. https://github.com/mozilla/bedrock/blob/d6d71498b4278fa3e6c5bdec2fe41b8861d2c2f9/wsgi/app.py#L16-L21
There was a problem hiding this comment.
due to it already being HTTPS, as guaranteed by the CDN, someone sending a request with x-forwarded-proto: http could poison the cache because that would cause Django to return a redirect to https which would then loop because the redirect response would be cached.
There was a problem hiding this comment.
unless GCP CDN is sending an accurate x-forwarded-proto header. it seems cloudfront is not.
There was a problem hiding this comment.
hmm so https://cloud.google.com/load-balancing/docs/https#target-proxies says x-forwarded-proto is set by different load balancer options but it's hard to determine whether Cloud CDN nixes that. @bkochendorfer Are we using the classic LB or the Global external Application Load Balancer please?
There was a problem hiding this comment.
GKE Ingress which is a Classic Application Load Balancer
There was a problem hiding this comment.
I've set this to default to False, but still be tweakable via env vars. Will merge it and see how it is on dev
There was a problem hiding this comment.
Ugh so disabling the SSL_REDIRECT upgrade breaks the redirect URI generated by for django-oicd, breaking SSO. I've just reverted it on main to bring dev back in line. I think I'll try the wsgi trick Pmac mentions above in case that gives us the best of both worlds: no HTTP->S upgrade and a signal that we're on HTTPS (because effectively we will be)
Doing that work over in #154
…t the CDN/LB The CDN redirects non-https to https, so we don't need to do it in the app. This approach means we avoid risk of a cache-poisoning attack for that http->https upgrade.
stevejalim
force-pushed
the
adjust-https-redirect
branch
from
28914d6 to
d1ecbe8
Compare
The CDN redirects non-https to https, so we don't need to do it in the app.
This approach means we avoid risk of a cache-poisoning attack for that http->https upgrade.
Resolves #143