grnet / djnro

DjNRO hits the decks of eduroam database management
http://djnro.grnet.gr/
Other
10 stars 21 forks source link

Two minor security fixes: construct secure URLs and mark cookies as secure #8

Closed vladimir-mencl-eresearch closed 8 years ago

vladimir-mencl-eresearch commented 8 years ago
vladimir-mencl-eresearch commented 8 years ago

Hi, FYI, I just found that once I set the Apache header (the change to Apache SSL VirtualHost configuration), Django starts interpreting the request as having arrived over SSL even without setting settings.SECURE_PROXY_SSL_HEADER. But, this behavior of uwsgi is undocumented, so I'd still rather explicitly tell Django which header to look for (the change in settings.py).

Cheers, Vlad

zmousm commented 8 years ago

Good catch. I agree we should enable SESSION_COOKIE_SECURE by default. As for SECURE_PROXY_SSL_HEADER, however, I see two issues:

What do you think? If you agree, would you mind updating the patch accordingly?

vladimir-mencl-eresearch commented 8 years ago

Hi,

Our deployment uses just mod_proxy to pass the requests to uwsgi via HTTP. And we pass the requests from both 443 and 80. And in our case, uwsgi picks up the X-Forwarded-SSL: on header and marks the request as secure, without having to actually configure the header in Django (the Django setting was ment more as a back up). And uwsgi ignores the X-Forwarded-Protocol: https.

But I can work with that - I can set the header in our customized local_settings.py.

So the way forward I see is that I would:

Does that sound alright to you?

Cheers, Vlad

PS: In our own deployment, I'd then also clear the same header in the plain-http VirtualHost - thanks for the Django link!

vladimir-mencl-eresearch commented 8 years ago

Hi Zenon, would you be happy with this revision?

Cheers, Vlad

zmousm commented 8 years ago

Thanks! I am rebasing this on rewritten master (just to spare you the trouble) and merging it.