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

AuthnRequest with <samlp:Scoping> element chokes ADFS IdP #498

Closed
dnmvisser opened this Issue Oct 18, 2016 · 25 comments

Comments

Projects
None yet
8 participants
@dnmvisser
Contributor

dnmvisser commented Oct 18, 2016

SimpleSAMLphp in proxy mode will send authentication requests to upstream IdPs that contain the <samlp:Scoping> element:

Sending SAML 2 AuthnRequest to 'https://remote.idp/login'
Sending message:
<samlp:AuthnRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol"
xmlns:saml="urn:oasis:names:tc:SAML:2.0:assertion"
ID="_7eb7fe0f9dfe79092d41eb90ef63a7f5d0f851001e" Version="2.0"
IssueInstant="2016-10-14T21:32:38Z"
Destination="https://remote.idp/adfs/ls/"
AssertionConsumerServiceURL="https://login.terena.org/wayf/module.php/saml/sp/saml2-acs.php/default-sp"
ProtocolBinding="urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST">
  <saml:Issuer>https://terena.org/sp</saml:Issuer>
  <samlp:NameIDPolicy
Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient"
AllowCreate="true"/>
  <samlp:Scoping>
    <samlp:RequesterID>https://eventr.geant.org/mellon/metadata</samlp:RequesterID>
  </samlp:Scoping>
</samlp:AuthnRequest>

IdPs that can't handle this element should ignore it, but some of them (notably Microsoft ADFS 2.0 and ADFS 3.0) choke upon receiving it, causing the authentication to fail.
Affected IdPs have been reported to log errors such as: MSIS1001: The SAML authentication request element 'Scoping' is not supported.

SimpleSAMLphp logs will show:

WARNING [891ed47572] Returning error to sp: 'https://eventr.geant.org/mellon/metadata'
WARNING [891ed47572] sspmod_saml_Error: Responder
WARNING [891ed47572] Backtrace:
WARNING [891ed47572] 3 /opt/simplesamlphp/wayf/modules/saml/lib/Message.php:392 (sspmod_saml_Message::getResponseError)
WARNING [891ed47572] 2 /opt/simplesamlphp/wayf/modules/saml/lib/Message.php:499 (sspmod_saml_Message::processResponse)
WARNING [891ed47572] 1 /opt/simplesamlphp/wayf/modules/saml/www/sp/saml2-acs.php:120 (require)
WARNING [891ed47572] 0 /opt/simplesamlphp/wayf/www/module.php:137 (N/A)
@mapgrady

This comment has been minimized.

mapgrady commented Feb 23, 2017

We will be submitting a patch for this once I finish testing it out. It involves, as OpenConext did, adding a new optional setting for a saml:SP authentication source, "disable_scoping", which if set to TRUE (defaults to FALSE if not present) will keep the Scoping element info getting populated before you get to AuthnMessage.php. The changes will be in:

modules/saml/lib/Auth/Source/SP.php

and will keep any of ProxyCount, IdPList, or requesterID from being set to anything but null, so that AuthRequest.php won't find any info that would cause it to add a Scoping element to the Authn Request.

@ghalse

This comment has been minimized.

Contributor

ghalse commented May 15, 2017

@mapgrady did you make any progress with a patch for this? I have the same issue and was hoping to avoid reinventing a wheel.

@mapgrady

This comment has been minimized.

mapgrady commented May 15, 2017

Yes, I have a patch, but I need to get it submitted in the accepted way. Here is a very short description:

This would update:

modules/saml/lib/Auth/Source/SP.php

And the flag to trigger it it can go anywhere in the "saml:SP" section of a given authsources.php entry, it is at the same "level" as the 'idp', 'entityID' , etc. options. You 'd just add:

'disable_scoping' => TRUE, // For ADFS-based IdPs, we can't send the Scoping element

The diff is here:

37a38,45

  • Flag to indicate whether to disable sending the Scoping element
  • @var boolean|FALSE
    */
    private $disable_scoping;

/**
60a69
$this->disable_scoping = $this->metadata->getBoolean('disable_scoping', FALSE);
222,225c231,254
< if (isset($state['saml:IDPList'])) {
< $IDPList = $state['saml:IDPList'];
< } else {
< $IDPList = array();


  $IDPList = array();
  $requesterID = array();
  
  /* Only check for real info for Scoping element if we are going to send Scoping element */
  if ($this->disable_scoping != TRUE) {
  	if (isset($state['saml:IDPList'])) {
  		$IDPList = $state['saml:IDPList'];
  	} 
  	
  	if (isset($state['saml:ProxyCount']) && $state['saml:ProxyCount'] !== null) {
  		$ar->setProxyCount($state['saml:ProxyCount']);
  	} elseif ($idpMetadata->getInteger('ProxyCount', null) !== null) {
  		$ar->setProxyCount($idpMetadata->getInteger('ProxyCount', null));
  	} elseif ($this->metadata->getInteger('ProxyCount', null) !== null) {
  		$ar->setProxyCount($this->metadata->getInteger('ProxyCount', null));
  	}
  	
  	if (isset($state['saml:RequesterID'])) {
  		$requesterID = $state['saml:RequesterID'];
  	}

  	if (isset($state['core:SP'])) {
  		$requesterID[] = $state['core:SP'];
  	}

232,248d260
< if (isset($state['saml:ProxyCount']) && $state['saml:ProxyCount'] !== null) {
< $ar->setProxyCount($state['saml:ProxyCount']);
< } elseif ($idpMetadata->getInteger('ProxyCount', null) !== null) {
< $ar->setProxyCount($idpMetadata->getInteger('ProxyCount', null));
< } elseif ($this->metadata->getInteger('ProxyCount', null) !== null) {
< $ar->setProxyCount($this->metadata->getInteger('ProxyCount', null));
< }
<
< $requesterID = array();
< if (isset($state['saml:RequesterID'])) {
< $requesterID = $state['saml:RequesterID'];
< }
<
< if (isset($state['core:SP'])) {
< $requesterID[] = $state['core:SP'];
< }
<

@thijskh

This comment has been minimized.

Member

thijskh commented May 16, 2017

Thanks @mapgrady! Please send your patch as a pull request. That will help us review and merge it.

ghalse added a commit to ghalse/simplesamlphp that referenced this issue May 16, 2017

ghalse added a commit to ghalse/simplesamlphp that referenced this issue May 16, 2017

Extend @mapgrady's patch for simplesamlphp#498 to allow Scoping to be…
… disabled on a per-idp basis as well as globally

ghalse added a commit to ghalse/simplesamlphp that referenced this issue May 16, 2017

@ghalse

This comment has been minimized.

Contributor

ghalse commented May 16, 2017

I can convert this to a pull request, but @mapgrady should get the credit for the work if they want it.

@mapgrady

This comment has been minimized.

mapgrady commented May 16, 2017

@thijskh

This comment has been minimized.

Member

thijskh commented May 16, 2017

As for the tests, of course that would be ideal. There's already some work for testing this file in https://github.com/simplesamlphp/simplesamlphp/blob/master/tests/modules/saml/lib/Auth/Source/Auth_Source_SP_Test.php. Perhaps that is a helpful start to create a test also for this functionality. We can of course assist if required.

@lehmayr

This comment has been minimized.

lehmayr commented May 23, 2017

I'm confused: since a SP can authenticate via multiple IdPs - why would the option to skip Scoping be in the configuration of the SP? Shouldn't it be in the settings of the IdP?

@ghalse

This comment has been minimized.

Contributor

ghalse commented May 23, 2017

@lehmayr I personally think it should be in both, which is what my enhancement to @mapgrady 's patch does. In the SP for the circumstance where you've got a lot of ADFS IdPs and you don't want to configure them individually, and in the IdP for the circumstance where there's just one. Having it in both is also consistent with other settings.

@tvdijen

This comment has been minimized.

Member

tvdijen commented May 23, 2017

@ghalse: Although there are situations where it would be convenient, I think it is inherently wrong to have this settings globally or in the SP-configuration.
The only reason we want this kind of 'fix' is because ADFS simply breaks if we send the scoping-element, eventhough it is perfectly valid for an SP to send it...

The way I read the specs on this matter, it is basically the SP's right to control the authentication flow using the scoping-element:
The requester can influence proxy behavior by including a <Scoping> element where the provider sets a desired ProxyCount value and/or indicates a list of preferred identity providers which may be proxied by including an ordered <IDPList> of preferred providers.

Long story short: we should never suppress the scoping-element unless we know for sure the IdP chokes on it. And even then, we should be complaining to Microsoft instead of fixing this in SSP, but that's something I can live with since Microsoft basically stopped developing ADFS..

@mapgrady

This comment has been minimized.

mapgrady commented May 23, 2017

@thijskh

This comment has been minimized.

Member

thijskh commented May 26, 2017

It does not make sense to configure anything related to the interaction in the direction of IdP's on the SP side, that can only be confusing. So I'd rather prefer a global setting than a per-SP setting.

But ideally you'd just configure this per IdP. If you want to add it to each IdP that you import automatically, you can simply add a foreach() to your config? Or is that too crude? (Inside the foreach you could even put a simple heuristic on the SSO-location to 'detect' that it is ADFS.)

@mapgrady

This comment has been minimized.

mapgrady commented May 26, 2017

@thijskh

This comment has been minimized.

Member

thijskh commented May 26, 2017

If you disable it on the SP side, you still have the problem that non-ADFS IdPs for that SP do not get the RequesterId, right?

@mapgrady

This comment has been minimized.

mapgrady commented May 26, 2017

@ghalse

This comment has been minimized.

Contributor

ghalse commented May 29, 2017

@tvdijen I'd agree it's inherently wrong - ideally, we should not need to do this at all. In an ideal world, ADFS would get fixed... Accepting that's not going to happen anytime soon, I think we should provide maximum flexibility for different use cases. Other settings (e.g. nameid.encryption) can already be specified in both the saml:SP and idp-remote configs, because there are different circumstances where one or the other may be more convenient. @mapgrady's patch does the former, and my extension of it provides for the latter.

(As an aside, we're now using the version of the patch I posted in production.)

@hannahshort

This comment has been minimized.

hannahshort commented May 30, 2018

Hi @ghalse and @mapgrady - we've hit this bump a few times now and it would be really great to get the patch pushed out. I've seen at least 3 SSP instances having to implement this in the last year (and I'm just one IdP). Has there been any movement?

@thijskh

This comment has been minimized.

Member

thijskh commented May 30, 2018

As a side note, in our experience it helps to upgrade to ADFS 4.0.

@ghalse

This comment has been minimized.

Contributor

ghalse commented May 31, 2018

My version of the patch has been running in production for over a year without problems for us.

@thijskh and in our experience that's a slow painful process for institutions, meaning that older versions of ADFS have a long tail.

@thijskh thijskh added this to the 1.17 milestone May 31, 2018

@thijskh

This comment has been minimized.

Member

thijskh commented May 31, 2018

We'll need to sort this out and merge for 1.17 then.

@thijskh

This comment has been minimized.

Member

thijskh commented May 31, 2018

Do you have a clear reference for your version of the patch? Would be helpful I think.

@tvdijen tvdijen modified the milestones: 1.17, 1.18 Oct 11, 2018

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 13, 2018

@ghalse Could you provide the patch you're using for this please?

ghalse added a commit to ghalse/simplesamlphp that referenced this issue Nov 14, 2018

ghalse added a commit to ghalse/simplesamlphp that referenced this issue Nov 14, 2018

Extend @mapgrady's patch for simplesamlphp#498 to allow Scoping to be…
… disabled on a per-idp basis as well as globally

ghalse added a commit to ghalse/simplesamlphp that referenced this issue Nov 14, 2018

@ghalse

This comment has been minimized.

Contributor

ghalse commented Nov 14, 2018

@thijskh I've rebased my branch and checked that the results match what we've currently got running in production. We're running it against SSP1.16 and the only minor difference is namespacing on \SimpleSAML\Logger.

I can create it as a pull request if you like.

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 14, 2018

I can create it as a pull request if you like.

Yes, please!

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 15, 2018

Thanks all, this will be part of 1.17.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment