IdentityPython / djangosaml2

Django SAML2 Service Provider based on pySAML2
Apache License 2.0
254 stars 143 forks source link

Make sure djangosaml2 works in csp-enabled applications too (fix #391) #392

Closed prauscher closed 6 months ago

prauscher commented 6 months ago

See #391: Iff Django-applications use Content-Security-Policy to increase security (or just to tick boxes of automated code checkers), SAML2 with POST-Bindings tend to have a hard time: POST-Bindings typically render a simple HTML-Page, consisting of a form with hidden fields, a submit-button and an onload to trigger submitting.

Currently, users can either bend their application-wide Policies to allow unsafe-inline code and form actions to any https-url, but for security reasons this should be avoided. This PR detects if django-csp is installed and uses its decorators to add specific allowances for the POST-Bindings.

Note that this requires the IdP to use https, but imho this should not break any legitimate use cases.

peppelinux commented 6 months ago

Good catch

we need other three things in this PR:

  1. requirements update here: https://github.com/IdentityPython/djangosaml2/blob/master/setup.py#L66
  2. version set to 1.9.0 here: https://github.com/IdentityPython/djangosaml2/blob/master/setup.py#L30
  3. security considerations in the docs, decide to resuse a preexisting section or create a new one for this scope, here: https://github.com/IdentityPython/djangosaml2/tree/master/docs/source/contents
prauscher commented 6 months ago

I drafted a security considerations page, but more help is greatly appreciated regarding format and content. I also bumped to Version 1.9.0 as instructed, but did not add the requirement: In my opinion, django-csp is no requirement, as we will simply ignore if it is not found (hence the try-block).

Imho it is more an extension, but adding it as an optional dependency would not be good either: People could be tricked into installing djangosaml2[csp] and believing we would send Content-Security-Policy Headers, but django-csp requires configuration in the application first, so all we do is being compliant with django-csp if it is found.

peppelinux commented 6 months ago

@prauscher I made suggestion to help this PR to get merged soon

please accept my suggestion with the github UI, then request to me again the review.

Can I ask you to include a setup instruction in the documentation about django-csp? I want to guide the users in the best setup

prauscher commented 6 months ago

@peppelinux Sorry, only now realized that I forgot to push my latest changes, which include the warning.

This also introduced links to the django-csp install and config page - I do not feel confident enough to provide own explanations, but I think that should work :)