Turn warning about no signature verification into an error #866
Conversation
Not checking any signature is completely insecure and we'd want to go through great lenghts to avoid this. Just logging a warning does not suffice, since will read logs when it doesn't work, but it does.
Given that the other signature verification options want_assertions_signed and want_response_signed will overrule this setting, defaulting it to true does not impact any configuration that has one of those settings. It does however catch the situation where someone has disabled response signature checking. This prevents that user from landing on an error about an insecure config, and rather employs this reasonable fallback scenario.
There was a problem hiding this comment.
Thanks for your suggestions.
There's really no situation in which an SP should operate without checking any signature on a received response or assertion
pysaml2 has been a flexible implementation where people can set things however they like. Maybe, when starting out, allowing for this behaviour is ok, but the checks should be turned on at some point (very soon). Restricting this is for the best. In general I agree and do hope that nobody is using a configuration without any checks.
Note to self: this will be a breaking change.
| @@ -174,7 +173,7 @@ def __init__(self, config=None, identity_cache=None, state_cache=None, | |||
| "authn_requests_signed": False, | |||
| "want_assertions_signed": False, | |||
| "want_response_signed": True, | |||
There was a problem hiding this comment.
This should change to False to allow messages with signed assertions only to be processed. This should also change on the documentation.
| "want_response_signed": True, | |
| "want_response_signed": False, |
There was a problem hiding this comment.
Yes, this is saml2int compliant to change this default. However of course there are some arguments to make that reponse signing is more secure/more defense in depth than assertion signing so there is some case also to make to by default check response signatures and let people change that if they need to.
There was a problem hiding this comment.
If your IdP signs responses it's probably more secure/defense in depth to not allow this to change. So defaulting to response signing as a requirement might make sense, so only installs that knowingly require only the assertion to be signed will change this?
…lient_base.py Co-authored-by: Ivan Kanakarakis <[email protected]>
It will only be breaking things that are already broken... so is it? |
Well, the case we encountered was using djangosaml2. It happened in this way:
So not someone just using the bare library themselves. In my opinion the insecure configuration should either not be allowed at all or at least not possible to enable implicitly, only explictly. |
We had a production SP configured with not checking any response or assertion signature at all. The warning was logged, but not noticed. This is quite undesirable, in other words, highly insecure.
It happened because the SP interfaces with an IdP that does not sign responses, only assertions. So want_response_signed was turned off in config. It was not transparant to the operators that setting just this setting implies that no signatures are checked at all. The warning was logged, but sure, people read logs only when things do not work and the SP worked. The warning was discovered later when researching another problem.
I propose to change the following: