pysaml2 icon indicating copy to clipboard operation
pysaml2 copied to clipboard

Turn warning about no signature verification into an error

Open thijskh opened this issue 3 years ago • 2 comments

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:

  1. Turn the warning about no signature verification being done into an error. There's really no situation in which an SP should operate without checking any signature on a receveived response or assertion. Making it an error makes this security hole obvious, instead of having to spot the warning somewhere in the logs.
  2. Assist users by defaulting the want_assertions_or_response_signed setting to True. This seems a more graceful fallback for a system that has turned off response signing than just not checking any signature at all (or, if 1 is implemented, throwing an error). Because the want_response_signed and want_assertions_signed options override this one if set to True, it should not change behaviour for any non-insecure exising SP config.

thijskh avatar Aug 08 '22 14:08 thijskh

Note to self: this will be a breaking change.

It will only be breaking things that are already broken... so is it?

thijskh avatar Aug 12 '22 07:08 thijskh

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.

Well, the case we encountered was using djangosaml2. It happened in this way:

  1. App uses djangosaml2
  2. IdP signs only assertions, not responses
  3. Deployer adds only "want_response_signed: false" to config
  4. This makes the SP end up not checking any signatures at all.

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.

thijskh avatar Jun 20 '23 11:06 thijskh