Better error message around us choosing what binding to use

[liquidpele]

We have customer inputted IDP metadata, and one of them only allows for HTTP-Redirect in their SingleSignOnService definition. We have our saml2 config set with authn_requests_signed as True (which is the default according to pysaml2 - https://github.com/rohe/pysaml2/blob/master/doc/howto/config.rst#authn-requests-signed). The djangosaml2 code uses that param to decide if it should use a HTTP-POST or HTTP-Redirect binding, so since we specified it on, it only tries HTTP-POST, which pysaml2 then raises an exception over since the IDP metadata didn't support it.

This gives a better error message around this, but I question if this is correct at all...

1. What if someone wanted HTTP-POST binding, but without the signature? Right now this isn't possible at all.
2. I'm not clear why djangosaml2 is handling the signature in any case, so why limit this here, why not just pass things to pysaml2 and let it handle things? I tested passing authn_requests_signed=True and HTTP-Redirect to pysaml2 and it does seem to handle it, although it breaks spec and url encodes the whole signature instead of doing the extra url param... at least in our version.

Would appreciate some feedback on this, thanks!

[knaperek]

Thanks for your input @liquidpele.

Ad 1.) As you can see djangosaml2 tries to "automatically" choose the binding based on _sp_authn_requests_signed option. This was a quick and pragmatic fix as far as I remember but it is true that it could be improved. We could add a djangosaml2 specific settings option that would specify the preferred binding to use, only falling back to this automatic logic if the option is not set.

Ad 2.) I think this is best explained in the comments already:

When authn_requests_signed is turned on, HTTP Redirect binding cannot be
used the same way as without signatures; proper usage in this case involves
stripping out the signature from SAML XML message and creating a new
signature, following precise steps defined in the SAML2.0 standard.

It is not feasible to implement this since we wouldn't be able to use an
external (xmlsec1) library to handle the signatures - more (higher level)
context is needed in order to create such signature (like the value of
RelayState parameter).

Therefore it is much easier to use the HTTP POST binding in this case, as
it can relay the whole signed SAML message as is, without the need to
manipulate the signature or the XML message itself.

Read more in the official SAML2 specs (3.4.4.1):
http://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf

Since we don't have a custom implementation for the signatures yet in djangosaml2 and only rely on the external xmlsec1 lib, this is simply not supported and won't be until somebody implements it. I don't want to break the specs just to pretend we support that combination.

[liquidpele]

Since we don't have a custom implementation for the signatures yet in djangosaml2 and only rely on the external xmlsec1 lib, this is simply not supported and won't be until somebody implements it. I don't want to break the specs just to pretend we support that combination.

But djangosaml2 doesn't use xmlsec1... pysaml2 does. It seems like djangosaml2 shouldn't have to be dealing with any of the security validation... furthermore, now that I'm looking at it, djangosaml2 doesn't appear to tell pysaml2 to actually DO the response cert validation:

https://github.com/knaperek/djangosaml2/blob/master/djangosaml2/views.py#L226

The call to parse_authn_request_response takes an "outstanding_certs" option which appears to be a list of trusted certs that pysaml2 then uses later to run response.verify(cert) on the appropriate ones. Am I missing something here?

[liquidpele]

In regards to selecting the right binding, I feel like that should probably be done based off the IDP metadata specified... since that defines what's allowed. There's also a a "WantAuthnRequestsSigned" attribute used in the IDP metadata that we could automatically select whether to sign stuff or not if it's not specified...

[knaperek]

But djangosaml2 doesn't use xmlsec1... pysaml2 does. It seems like djangosaml2 shouldn't have to be dealing with any of the security validation

That's right, djangosaml2 does not perform any crypto, it uses pysaml2 which in turn uses xmlsec1. Thus, in order to support the Redirect binding with authn_requests_signed, we would need to either implement it ourselves or the support would need to be added to pysaml2. Since we don't want to do the former, we may only push for it in pysaml2 (or contribute).

furthermore, now that I'm looking at it, djangosaml2 doesn't appear to tell pysaml2 to actually DO the response cert validation

Look closer, it does. This is just a way to override the default. I admit that the pysaml2 code is not very readable though...

[liquidpele]

That's right, djangosaml2 does not perform any crypto, it uses pysaml2 which in turn uses xmlsec1. Thus, in order to support the Redirect binding with authn_requests_signed, we would need to either implement it ourselves or the support would need to be added to pysaml2. Since we don't want to do the former, we may only push for it in pysaml2 (or contribute).

Okay, so the real issue is that pysaml2 doesn't support it yet. Okay, I get it now.

Look closer it does. This is just a way to override the default. I admit that the pysaml2 code is not very readable though...

Ah okay, whew :p

[knaperek]

In regards to selecting the right binding, I feel like that should probably be done based off the IDP metadata specified... since that defines what's allowed. There's also a a "WantAuthnRequestsSigned" attribute used in the IDP metadata that we could automatically select whether to sign stuff or not if it's not specified...

If we are to change this, then I'd be more in favour of an explicit solution. Yes, I agree that the WantAuthnRequestsSigned flag is a good sign, but that should not be solely responsible for making the decision on SP side. Also, what if it's not set or if it's false? That does not mean SP does not want to sign the requests.
We could, however, implement a runtime check that produces an error log if the IdP has set this flag but current SP settings do not follow it.

[knaperek]

Okay, so the real issue is that pysaml2 doesn't support it yet. Okay, I get it now.

Yes, and the reason for that comes from xmlsec1 limitations (that's why the comments in code transitionally refer directly to xmlsec1 as the culprit). XML signatures are a beast anyway, so I'm happy we at least have something to use :-)

[liquidpele]

I've updated this to use the same logic, but then check if the selected binding is supported according the IDP's metadata and if not then try the other binding type.

[knaperek]

This is a common antipattern - please pass the arguments to the logger directly instead of pre-merging into the message. It's easier to process the logs then...

[liquidpele]

Okay will fix. I'm having merge conflicts between my two PRs though, so I'm going to make one combined one in just a sec.

[knaperek]

I'd actually prefer if you kept them separate. It's easier to discuss and review then. If that import line bothers you, just use a separate import line in each...

[liquidpele]

Okay, I'll redo the other one then and I'll reopen this one.

[liquidpele]

Okay, I fixed the logger lines you commented on and also fixed 2 issues that unit tests found with python3

[knaperek]

Thanks for your work @liquidpele, now it turned out these changes will after all be related to the other PR about parsing HTML with XML parser. Would you mind fixing this branch now? Thanks!

[liquidpele]

@knaperek merged in master and fixed conflicts.

[knaperek]

Fair enough, thanks for your work @liquidpele!