Codecov Report

Merging #1303 into master will not change coverage by %.
The diff coverage is 50.00%.

@@            Coverage Diff            @@
##             master    #1303   +/-   ##
=========================================
  Coverage     37.87%   37.87%           
  Complexity     3430     3430           
=========================================
  Files           129      129           
  Lines          9735     9735           
=========================================
  Hits           3687     3687           
  Misses         6048     6048           

If in IdP metadata, allows the SP that would normally sign authn requests/logout requests [..] to not do so if the IdP doesn't want it.

I'm sorry, but that's just not how it works.. An IDP can demand signed requests, but it can't demand unsigned requests

Perhaps I'm missing something then? In the IdP metadata, the IdpSSODescriptor can have the "WantAuthnRequestsSigned" attribute which tells the SP if they want the AuthnRequest signed or not...

https://docs.oasis-open.org/security/saml/v2.0/saml-metadata-2.0-os.pdf

Paragraph 2.4.3:

WantAuthnRequestsSigned [Optional]Optional attribute that indicates a requirement for the samlp:AuthnRequest messages received by this identity provider to be signed. If omitted, the value is assumed to be false.

False here meaning 'not a requirement'.. Not 'unwanted'

https://docs.oasis-open.org/security/saml/v2.0/saml-metadata-2.0-os.pdf

Paragraph 2.4.3:

WantAuthnRequestsSigned [Optional]Optional attribute that indicates a requirement for the samlp:AuthnRequest messages received by this identity provider to be signed. If omitted, the value is assumed to be false.

False here meaning 'not a requirement'.. Not 'unwanted'

The spec is unclear about this so it would seem that this is up for interpretation by implementers. The problem I'm trying to solve is where IdPs allow "anonymous" relying parties (something that can be enabled by Shibboleth) and have no metadata of an SP but receive an SP AuthnRequest that is signed and they have no way to verify the signature so the request fails. If the SP was "smart enough" to not send a signed request in the first place, this wouldn't be a problem. But since SSP acting as an SP checks its own metadata first before checking the IdP, this comes up as a problem integrating with those IdPs. Its worth noting that this is the only case where SSP fails to check the destination metadata prior to checking its own to determine if a request should be signed.

I don't think it's unclear at all... The SP will indicate in it's metadata (2.4.4) whether requests are signed or not.. the idp should just act accordingly... Maybe the others think differently

Maybe I'm misunderstanding the spec, but I fail to see how this is any different from the "WantAssertionsSigned" attribute in SP metadata. If this set to true (corresponding SSP metadata config 'saml20.sign.assertion'), the IdP either signs the assertion or not. It doesn't do whatever it wants to unless the attribute isn't present in the SP metadata. So it makes sense for the reverse to be true. If the IdP doesn't want authn requests signed, the SP shouldn't sign them.
In fact SSP already works in this same manner if neither the SP nor IdP have 'sign.authnrequest' defined but the IdP does have the 'redirect.sign' set to false. It checks the destination metadata first, then its own (lines 110-115 of modules/saml/lib/Message.php). All I'm looking for is the same behavior if both have 'sign.authnrequest' set....use the value that the destination wants...

Can you make the usecase a bit more specific so I can understand it better? Some concrete example of such IdPs that do this?

Obviously use cases all depend on how the IdPs and SPs are configured, but internal to my organization we have a Shibboleth IdP that allows anonymous or unverified relying parties.
https://wiki.shibboleth.net/confluence/display/IDP30/RelyingPartyConfiguration

To the IdP, these are SPs that it has no metadata for. Its useful for us internally because we don't have to manage metadata for these parties. Or they don't have a common endpoint (the reason for my other pull request). The IdP simply receives the authn requests, authenticates the user and sends the assertion in the response - it does absolutely no verification of whether or not it should interoperate with the SP - it just does. When SSP sends an AuthnRequest to one of these IdPs it signs the requests (even though the IdP metadata has 'sign.authnrequests' to false). However when the Shibboleth IdP sees the request is signed, it tries to validate it but it has no metadata (certificate/public key) to use for validation. So it immediately fails the request.
Again - what I'm I'm asking for SSP already does if the 'sign.authnrequest' option isn't set in either party's metadata (as defined by how it uses the 'redirect.sign' option), I just want it to use the same logic for the more detailed scenario (sign.authnrequest and sign.logout).

To the IdP, these are SPs that it has no metadata for. Its useful for us internally because we don't have to manage metadata for these parties.

This just raises red flags to me.. You should never want this, not even internally.. Security 101.. It's just not safe.. And so are unsolicited responses..

When SSP sends an AuthnRequest to one of these IdPs it signs the requests (even though the IdP metadata has 'sign.authnrequests' to false). However when the Shibboleth IdP sees the request is signed, it tries to validate it but it has no metadata (certificate/public key) to use for validation. So it immediately fails the request.

That's up to the IDP.. They are free to ignore the signature if they like to.. Remember this is XML, and the base-rule in XML is; if you don't know it, or don't want to know it > ignore it!

@thijskh What's your opinion on this, because I'm not eager at all to merge this?

To the IdP, these are SPs that it has no metadata for. Its useful for us internally because we don't have to manage metadata for these parties.

This just raises red flags to me.. You should never want this, not even internally.. Security 101.. It's just not safe.. And so are unsolicited responses..

When SSP sends an AuthnRequest to one of these IdPs it signs the requests (even though the IdP metadata has 'sign.authnrequests' to false). However when the Shibboleth IdP sees the request is signed, it tries to validate it but it has no metadata (certificate/public key) to use for validation. So it immediately fails the request.

That's up to the IDP.. They are free to ignore the signature if they like to.. Remember this is XML, and the base-rule in XML is; if you don't know it, or don't want to know it > ignore it!

@thijskh What's your opinion on this, because I'm not eager at all to merge this?

Please do not get confused by my particular use case as it makes no difference for this PR. The fact is that I simply want a way for SSP to NOT to sign an AuthnRequest if the metadata for the IdP requests that it shouldn't be signed.

As I mentioned before, the behavior I'm FIXING in this PR is the exact behavior SSP performs when it has no "sign.logout" or "sign.authnrequest" config option in metadata and therefore checks for the "redirect.sign" option. Regardless of whether or not you guys want to check the source metadata first or the destination metadata, it needs to be consistent. Please at least review the method in question for behavior consistency before you make a decision about whether or not to merge my PR.

I'll also mention that this PR addresses behavior inconsistencies in documentation (referenced in my original issue #687 )

Per the current documentation for saml:SP authentication source and its "sign.authnrequest" config option:
"Note that this option also exists in the IdP-remote metadata, and any value in the IdP-remote metadata overrides the one configured in the SP configuration."

The current implementation of the 'addRedirectSign' does not conform to the documented behavior as the IdP-remote metadata does NOT override the SP configuration. It should - hence my PR.

I'm not confusing anything, I'm just disagreeing with you on the specs (which is an issue with the specs, because specs should by definition be non-debatable).. We should obviously fix any inconsistencies in the documentation..

I'd also like to point out the core-specs that says: The <AuthnRequest> message SHOULD be signed or otherwise authenticated and integrity protected by the protocol binding used to deliver the message.. That indicates to me that even if WantAuthnRequestsSigned = false, it still should be signed by default and that non-signing should be an agreement between IDP/SP rather than automated behaviour ..
I also believe that SPSSODescriptor::AuthnRequestsSigned prevails over IDPSSODescriptor::WantAuthnRequestsSigned, because in the SAML SSO paradigm it's up to the SP to request whatever it wants and the IDP should follow this (or not use the service at all)..

I'd be happy to hear from my colleages on this matter. We can always put up our questions on the Oasis maling list..

Fair enough - however our disagreement over the spec is a result of the inconsistent method behavior and the documentation discrepancies. Are you also disagreeing that the current method behavior is inconsistent with regards to 'sign.authnrequests/sign.logoutrequests' and 'redirect.sign' options? Because its clear that in one case the method favors the source metadata and the destination metadata in the other.
It also sounds like you would rather change the documentation to match the current implementation of the method rather than pull in my change, is that right? This seems like more work to me, not to mention something I would think would be undesirable as it currently prevents SSP acting as an SP to selectively sign authn requests...

Because its clear that in one case the method favors the source metadata and the destination metadata in the other

This is undesirable and as stated should be taken care of, one way or another.

It also sounds like you would rather change the documentation to match the current implementation of the method rather than pull in my change, is that right?

To me personally, yes. But I'm no authority here, so I'd like to agree on something with the other devs.. Even if it's against my personal preference. I just want to make clear and weighted decisions and document them to avoid having the same discussions next year.

This seems like more work to me, not to mention something I would think would be undesirable as it currently prevents SSP acting as an SP to selectively sign authn requests...

I don't care about the extra work.. If we need to be able to selectively sign, then so be it and we have to work towards that. I do however think it's already possible by setting the redirect.sign flag on the SP authsource?

To me it seems only relevant what the recipient wants to happen. So the IdP in the case of the authnrequest. They indicate they want it, we should sign it because they want it (and one may expect it not to work otherwise).

They indicate they don’t want it, we should not sign because:
a) Probably they will then not be checking the signature or relying on any sensitive information in the request;
b) There’s no use case for signing the request if you’re not sure that the relying party checks the signature; you as the sender cannot assume you can put something signworthy in it;
c) They will probably not do anything with the signature, so it’s a waste of cycles.

So I agree that if the destination does not want a signature there’s really no need to do it anyway. Because the signature is only relevant to the party receiving, verifying and acting on it.

Does this answer the question posed?

They indicate they don’t want it, we should not sign because:
a) Probably they will then not be checking the signature or relying on any sensitive information in the request;
b) There’s no use case for signing the request if you’re not sure that the relying party checks the signature; you as the sender cannot assume you can put something signworthy in it;
c) They will probably not do anything with the signature, so it’s a waste of cycles.

Or for my particular use case (dealing with a Shibboleth IdP that allows anonymous relying parties ie. SPs without metadata)

d) They will try to validate the signature, but have no metadata for the SP so the AuthnRequest cannot be validated and the request fails.

That last item I do not follow. Why would you check a signature and not continue when it fails, but would continue when the signature is absent? How does that combination make sense?

Either you require signatures or you don’t?

I think the flag's name is misleading, because of the want and then the spec talking about requirement. To NOT WANT something means something completely different to NOT REQUIRE.. It's semantics really..

I also agree to Thijs.. If you don't care about it, then ignore it; it's XML basic rules.. To me this feels like an issue with Shib IDP.

Well Tim, I do agree that there’s no use in sending signatures to a party that does not want them.

I indeed do not understand why the IdP then checks signatures in the first place if they do not want them.

That last item I do not follow. Why would you check a signature and not continue when it fails, but would continue when the signature is absent? How does that combination make sense?

Either you require signatures or you don’t?

The only way Anonymous relying parties only work for the Shibboleth IdP if no signatures or encryption happens because the request cannot be validated. Shibboleth always tries to validate a signature if it exists. It doesn't first check to see if it can find metadata for the SP and then ignore the signature if none exists. If no metadata exists and the request is signed, Shibboleth says the request is not valid. This may be a bug or issue that should be brought up with them, but all I really want is for SSP to respect the IdP metadata with respect to authn request signing.

@dub357 What about setting the redirect.sign flag on the SP authsource? Doesn't that resolve your issue?

Agreed that what shib does is in the end not very relevant to this question. Let me review the concrete change later and give you an update on where I stand (have to understand a bit more about the conext of the changed lines)

@dub357 What about setting the redirect.sign flag on the SP authsource? Doesn't that resolve your issue?

Not really....I want to default to signing requests to all other IdPs except this one. Without setting 'sign.authnrequests' or 'redirect.sign' to true, I have to rely on the IdP metadata to ensure it has 'sign.authnrequests=true'. The problem is, that isn't always the case...an IdP may not have that setting set in its metadata. Basically, I want to default to always signing the request unless the IdP doesn't want it.

Tell be a bit more about your setup..
Do you have one saml:SP and multiple remote IDP's? You should be able to set the redirect.sign on the IDP-metadata as well, and therefore set it to false for the one IDP..
Or are you running SSP in a bridge mode? I guess the same rule should apply as above..
I'm trying to understand your use-case a bit more, because it's more than anonymous SP's... I wouldn't be surprised if the logics are reversed and the IDP-local setting would be overruled by the saml:SP authsource..

Good discussion btw!! ::rocket::

Tell be a bit more about your setup..
Do you have one saml:SP and multiple remote IDP's? You should be able to set the redirect.sign on the IDP-metadata as well, and thus set it to false for the one IDP..
Or are you running SSP in a proxy/bridge mode? I guess the same rule should apply as above..
I'm trying to understand your use-case a bit more, because it's more than anonymous SP's...

I'm running SSP as a proxy/bridge. Like I stated to Tim, my particular use case shouldn't really matter and seems that it is just confusing everyone. All I want is SSP to prefer the IdP metadata over its own with respect to authnrequest signing. The documentation states that it does, but the behavior is actually inconsistent depending on the metadata options set.