-
Notifications
You must be signed in to change notification settings - Fork 673
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Check recipient/destination metadata before sender/source metadata wh… #1303
base: master
Are you sure you want to change the base?
Conversation
…en determining whether or not to sign messages (sign.authnrequest/sign.logout) in the HTTP-REDIRECT profile. If in IdP metadata, allows the SP that would normally sign authn requests/logout requests (because these options are 'true' in its own metadata) to not do so if the IdP doesn't want it. When in SP metadata, allows an IdP to that would normally want to logout requests/responses (because these options are 'true' in its own metadata) to not do so if the SP doesn't want it. The documentation seems to suggest that this should already be the case but it turns out the code doesn't respect those rules because each side checks its own metadata first before determining if the recipient/destination has the option. Fix for issue (I closed the older PR referenced in issue): simplesamlphp#687
Codecov Report
@@ Coverage Diff @@
## master #1303 +/- ##
=========================================
Coverage 37.87% 37.87%
Complexity 3430 3430
=========================================
Files 129 129
Lines 9735 9735
=========================================
Hits 3687 3687
Misses 6048 6048 |
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:
|
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. |
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. 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. |
1c686ab
to
eb20457
Compare
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..
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: 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: 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. |
This is undesirable and as stated should be taken care of, one way or another.
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.
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 |
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: 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? |
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 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. |
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) |
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.. Good discussion btw!! ::rocket:: |
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. |
29f7b69
to
1a911ce
Compare
c7c8357
to
fdbe001
Compare
3b5f5ba
to
96357ee
Compare
7587851
to
d523b31
Compare
8c90121
to
d534e3b
Compare
bc1c5c8
to
d0a5974
Compare
ccb9b02
to
120a100
Compare
6004a77
to
58bf8db
Compare
5c9fb2c
to
0970efc
Compare
Check recipient/destination metadata before sender/source metadata when determining whether or not to sign messages (sign.authnrequest/sign.logout) in the HTTP-REDIRECT profile. If in IdP metadata, allows the SP that would normally sign authn requests/logout requests (because these options are 'true' in its own metadata) to not do so if the IdP doesn't want it. When in SP metadata, allows an IdP to that would normally want to logout requests/responses (because these options are 'true' in its own metadata) to not do so if the SP doesn't want it.
The documentation seems to suggest that this should already be the case but it turns out the code doesn't respect those rules because each side checks its own metadata first before determining if the recipient/destination has the option.
Fix for issue (I closed the older PR referenced in issue):
#687