Disable scoping for broken IdPs #750
Conversation
|
Thanks Luke for your contribution, but as you have already read in #498 the question is whether we want to accomodate non SAML-compliant behaviour... |
|
@tvdijen I just want to accelerate a decision either way. 😉 Given that at least two users of SimpleSAMLphp are patching it in production to workaround non-compliance I think there's a case to be made in favour of adoption a workaround. I appreciate that it's an ugly workaround, though. Am happy to go with lowercase |
|
I understand your motivation Luke, it's just that SAML-proxying isn't supported and I'm not sure if @jaimeperez is willing to do so in future releases, given there are perfect alternatives for that. We also have to take into consideration that support for Server 2012 R2 will end on 9-10-2018 (and we're not gonna rely on LTS). My personal view on this is that we could have merged #498 when it was posted, but we shouldn't do this anymore... There are alternatives, both for SAML-proxying as for IdP's with scoping-support. |
|
I completely understand -- if this seems inappropriate in scope for this project this patch ought to be rejected. The impression I got from reading the scoping documentation was that IdP proxy behaviour was supported as a "best effort" rather than being a first-class feature. For what it's worth, SimpleSAMLphp seemed the most logical means of delivering a SAML 2 IdP proxy. We settled on this use case after looking for an easy means of providing our corporate clients with access to multiple internal applications with minimal complexity in the configuration. I don't know of any products that would better meet our requirements (bearing in mind the context of a large existing base of PHP applications and associated staffing and infrastructure resource). Should this PR be rejected we'll likely maintain a fork including this or a similar patch. |
|
First of all, there's obviously my +1 for this. We're running the patch I had in #498 in production. I've just never got round to converting it into a pull request. @tvdijen my testing suggests that the scoping problem persists in Windows Server 2016/AD FS 4.0 as well as in Azure AD so the comment about end-of-support is a straw man. It's also indicative of the fact that engaging with Microsoft on this (as CAF have done) is not going anywhere. I'll echo what @LukeCarrier says. There might be other ways to do SAML proxies, but SimpleSAMLphp is both widely used in this configuration and an obvious choice for many people. It offers a huge advantage of a relatively low learning curve for people who already have in-house PHP skills. I'll happily debate with @jaimeperez about this :). In some ways I view making AD FS talk to the R&E SAML world more as a protocol bridge than a proxy. AD FS is Microsoft's own special protocol that looks a bit like SAML, and we need to make it talk to the rest of the world in a more standards-compliant way ;). If you view it in this way, then the proposed change isn't so much of a violation of specs... :) There are many examples of software including standards-breaking options to work around deficiencies in other implementations. To my mind, this is a natural extension of the Robustness Principle. The key thing is not to violate standards by default and to always require users to explicitly configure non-standard behaviour. Which is what the patch does. |
|
@ghalse Apparently yours and @LukeCarrier's tests contradict each other then... |
|
@ghalse I'd be interested in seeing what issues you encountered with AD FS 4.0 then -- our testing echoes Microsoft's communications in the release notes. |
|
@LukeCarrier my conclusions are based on testing by one of our IdPs, and unfortunately, they're currently closed for end-of-year vacation so I can't follow up with them right now. However, as I recall it worked better than previous versions but still kicked up an exception under some circumstances. |
modules/saml/lib/Auth/Source/SP.php
Outdated
| @@ -58,7 +66,8 @@ public function __construct($info, $config) { | |||
| $this->entityId = $this->metadata->getString('entityID'); | |||
| $this->idp = $this->metadata->getString('idp', NULL); | |||
| $this->discoURL = $this->metadata->getString('discoURL', NULL); | |||
|
|
|||
| $this->disableScoping = $this->metadata->getBoolean('disable.scoping', NULL); | |||
There was a problem hiding this comment.
Would it make sense to default to false instead of NULL? Keeps the type of the var above also simpler.
modules/saml/lib/Auth/Source/SP.php
Outdated
|
|
||
| $ar->setIDPList(array_unique(array_merge($this->metadata->getArray('IDPList', array()), | ||
| $idpMetadata->getArray('IDPList', array()), | ||
| (array) $IDPList))); |
There was a problem hiding this comment.
Should this not be inside the if-statement? Because if scoping is disabled, we'd never want to setIDPList?
|
@thijskh thanks for the review, I'll aim to get amends to you by the end of the week. I'd really appreciate it if you could link me to relevant test cases as I'm not sure how to approach this. |
|
Thanks! Tests for the existing functionality start at this line: https://github.com/simplesamlphp/simplesamlphp/blob/master/tests/modules/saml/lib/Auth/Source/Auth_Source_SP_Test.php#L163 |
|
@LukeCarrier Were you able to look into this yet? |
LukeCarrier
force-pushed
the
disable-scoping
branch
from
84eff86
to
46e19f5
Compare
LukeCarrier
force-pushed
the
disable-scoping
branch
from
46e19f5
to
1bf82f6
Compare
|
Fixed in #985 |
We've recently hit an issue with older versions of AD FS failing to process
AuthnRequests containing<Scoping>elements. It appears to have been fixed as of 4.0 according to the documentation and our testing.Unfortunately a large number of our customers will not be upgrading for several years. It seems prudent that SimpleSAMLphp should at least allow users to opt-in to some degree of feature loss for a workaround. This patch introduces a new
disable.scopingoption that can be set either in the remote IdP metadata or SP configuration.Patch based on work by @ghalse in #498