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

NameIDPolicy is always sent in authentication requests #771

Closed
jaimeperez opened this Issue Jan 25, 2018 · 37 comments

Comments

Projects
None yet
@jaimeperez
Member

jaimeperez commented Jan 25, 2018

Before SimpleSAMLphp 1.15.0, it was possible (but undocumented) to set NameIDPolicy to null in order for the SP to not send a NameIDPolicy in the authentication request. Starting with the aforementioned version, setting the configuration option to null triggers an exception.

A way to skip the NameIDPolicy in the authentication request is very much desired as that element is optional in the standard, and always specifying it could cause interoperability issues.

This issue was originally reported and discussed here.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Jan 25, 2018

The main issue I see with the trick used to unset the NameIDPolicy is that in PHP, in general, not setting a variable and setting it to null is equivalent. Therefore, I think setting NameIDPolicy to null to not request a NameID format, while leaving it unset to request a transient NameID format is a bad idea, and prone to future bugs.

Since the null trick was undocumented, we have the freedom to choose how to tell SimpleSAMLphp not to request a specific NameID format. I’m inclined to use the boolean false for that. That way, we would have the following possibilities

  • Using the old, deprecated behaviour to set NameIDPolicy to a string. In that case, we use that string as the format we request.
  • Using an array to define the policy, which translates directly into what’s requested. If no options are specified inside the array (i.e. it’s empty), then we default to request a transient NameID.
  • Leaving NameIDPolicy unset or explicitly setting it to null (which should be considered equivalent). Here a transient NameID will be requested, as per the default behaviour.
  • Explicitly setting NameIDPolicy to the boolean false. In this case, a NameID is not requested at all.

@thijskh, Peter, would you two mind applying the following (updated) patch and test? As far as I’ve tried, it works as intended.

The main problem I have with this is that fixing this issue goes well beyond this simple patch. We need to:

  • Update the documentation to reflect the preferred array format for the NameIDPolicy configuration option.
  • Update the documentation to tell how to avoid sending out a NameIDPolicy (setting the option to false).
  • Make sure the SP metadata page doesn’t break when using an array instead of a string.
  • Replicate the same behaviour in the API, so that the same functionality can be achieved either by configuration or calls to requireAuth().
  • Look for any other uses of NameIDPolicy in the code and make sure nothing breaks and the behaviour is consistent.
  • Move the functionality to parse the value of the NameIDPolicy configuration option to some place (SimpleSAML\Utils\Config? SAML2\XML\samlp\NameIDPolicy?), so that we can reuse code and avoid inconsistent behaviour. Solving this is actually the first thing we should agree upon, IMO.

Regarding how to fix this on a stable version, I would assume this is a bug given that it breaks existing (even though undocumented) configurations. The same applies to the fact that the API does not allow for a consistent behaviour when passing the samlp:NameIDPolicy parameter to requireAuth(). Both ways should be completely equivalent.

I've started working on it (beyond the attached patch), but I could really use some help for this. @thijskh? @tvdijen?

diff --git a/modules/saml/lib/Message.php b/modules/saml/lib/Message.php
index b80391f..734a406 100644
--- a/modules/saml/lib/Message.php
+++ b/modules/saml/lib/Message.php
@@ -435,28 +435,37 @@ class sspmod_saml_Message
         $ar = new \SAML2\AuthnRequest();

         // get the NameIDPolicy to apply. IdP metadata has precedence.
-        $nameIdPolicy = array();
+        $nameIdPolicy = null;
         if ($idpMetadata->hasValue('NameIDPolicy')) {
             $nameIdPolicy = $idpMetadata->getValue('NameIDPolicy');
         } elseif ($spMetadata->hasValue('NameIDPolicy')) {
             $nameIdPolicy = $spMetadata->getValue('NameIDPolicy');
         }

-        if (!is_array($nameIdPolicy)) {
+        $policy = null;
+        if (is_string($nameIdPolicy)) {
             // handle old configurations where 'NameIDPolicy' was used to specify just the format
-            $nameIdPolicy = array('Format' => $nameIdPolicy);
+            $policy = array('Format' => $nameIdPolicy);
+        } elseif (is_array($nameIdPolicy)) {
+            // handle current configurations specifying an array in the NameIDPolicy config option
+            $nameIdPolicy_cf = SimpleSAML_Configuration::loadFromArray($nameIdPolicy);
+            $policy = array(
+                'Format'      => $nameIdPolicy_cf->getString('Format', \SAML2\Constants::NAMEID_TRANSIENT),
+                'AllowCreate' => $nameIdPolicy_cf->getBoolean('AllowCreate', true),
+            );
+            $spNameQualifier = $nameIdPolicy_cf->getString('SPNameQualifier', false);
+            if ($spNameQualifier !== false) {
+                $policy['SPNameQualifier'] = $spNameQualifier;
+            }
+        } elseif ($nameIdPolicy === null) {
+            // when NameIDPolicy is unset or set to null, default to transient as before
+            $policy = array('Format' => \SAML2\Constants::NAMEID_TRANSIENT);
         }

-        $nameIdPolicy_cf = SimpleSAML_Configuration::loadFromArray($nameIdPolicy);
-        $policy = array(
-            'Format'      => $nameIdPolicy_cf->getString('Format', \SAML2\Constants::NAMEID_TRANSIENT),
-            'AllowCreate' => $nameIdPolicy_cf->getBoolean('AllowCreate', true),
-        );
-        $spNameQualifier = $nameIdPolicy_cf->getString('SPNameQualifier', false);
-        if ($spNameQualifier !== false) {
-            $policy['SPNameQualifier'] = $spNameQualifier;
+        if ($policy !== null) {
+            // either we have a policy set, or we used the transient default
+            $ar->setNameIdPolicy($policy);
         }
-        $ar->setNameIdPolicy($policy);

         $ar->setForceAuthn($spMetadata->getBoolean('ForceAuthn', false));
         $ar->setIsPassive($spMetadata->getBoolean('IsPassive', false));

@jaimeperez jaimeperez added the bug label Jan 25, 2018

@jaimeperez jaimeperez changed the title from `NameIDPolicy` is always sent in authentication requests to NameIDPolicy is always sent in authentication requests Jan 25, 2018

@tvdijen

This comment has been minimized.

Member

tvdijen commented Jan 25, 2018

Sure! Would it be an idea if you put your work so far in a feature-branch so we can work on it together?

Regarding how to fix this on a stable version, I would assume this is a bug given that it breaks existing (even though undocumented) configurations.

I wouldn't worry about it too much.. Everyone reads changelogs/release notes before upgrading, right?
🙈 | 🙉 | 🙊

@tvdijen

This comment has been minimized.

Member

tvdijen commented Jan 25, 2018

Using the old, deprecated behaviour to set NameIDPolicy to a string.

What would be the new behaviour? There is no deprecation notice anywhere, unless I'm missing something?

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Jan 26, 2018

Sure! Would it be an idea if you put your work so far in a feature-branch so we can work on it together?

I was already planning to do that 😄

What would be the new behaviour? There is no deprecation notice anywhere, unless I'm missing something?

One of the things I would like to have for 2.0 is a more comprehensive configuration, with options grouped by category and coherent names. Some time ago, we got an PR aiming to solve an issue with SPNEGO, and the way to solve that was to make the configuration option richer by allowing to set more parameters of the NameIDPolicy. Unfortunately, this was never documented, and that's my fault 😞

Additionally, such configuration options introduce problems in other parts of the code where NameIDPolicy is always expected to be a string. We need to fix both things.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Jan 26, 2018

I'll create a todo list here:

  • Update sspmod_saml_Message.
  • Update modules/saml/www/sp/metadata.php.
  • Update sspmod_saml_Auth_Source_SP.
  • Replicate the changes for the API (saml:NameIDPolicy).
  • Update the documentation for hosted SPs.
  • Update the documentation for remote IdPs.
  • Remove $state['SPMetadata']['NameIDPolicy'] from sspmod_saml_Auth_Process_SQLPersistentNameID.
  • Create a common helper where this config option should be parsed.
  • Add tests for this common code.
  • Update the upgrade notes.
  • Fix metadata-page
@tvdijen

This comment has been minimized.

Member

tvdijen commented Jan 26, 2018

We could keep handling null as if it were false for a while and log a deprecation notice? That way we can remain bw-compatible and label this as a bugfix for the 1.15/1.16 branch and remove the old null behaviour in 2.0... Or do you want to go in with a stretched leg?

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Jan 29, 2018

Well, considering null wasn't documented, I'm not worried about breaking backwards-compatibility. It's already broken after all, and even though that allows us to treat this as a bugfix, I feel it would be awkward to add a note to the upgrade notes saying that before it was possible to set it to null and now that should be false.

Regarding where to put the code... I'm not sure SimpleSAML\Utils\Config\Metadata is the right place. Part of this is used to configure metadata, true, but I'd argue the main use here is on authentication requests. I really don't have a place for it. Don't know if it should be on its own class or maybe in the Utils\Config class, or maybe just a completely different place.

@tvdijen

This comment has been minimized.

Member

tvdijen commented Jan 29, 2018

Would it be an idea to create a MetadataConfiguration subclass of SimpleSAML\Configuration and put it there, along with the other metadata-specific methods that are on the configuration-class right now (like ::loadKeys(), ::getEndpoints() and other related methods)

Same applies to authsources.. They could also be treated like a special kind of configuration.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Jan 29, 2018

That's the thing, I don't see this as something metadata-related... The config option has a direct impact on generated metadata, but it also goes way beyond that.

I was even considering creating a class to represent NameIDPolicy as an XML element inside the SAML2 library.

@tvdijen

This comment has been minimized.

Member

tvdijen commented Jan 29, 2018

Ohh I get it... It's to early :(

@malikkotob

This comment has been minimized.

malikkotob commented Mar 12, 2018

Hi @jaimeperez! I was one of the folks previously using the undocumented 'NameIDPolicy' => null, method for handling the described desired scenario. The patch above, coupled with 'NameIDPolicy' => FALSE, in my authsources.php file worked well. Would you recommend I go that route until the longer-term fix is released?

@owenbush

This comment has been minimized.

owenbush commented May 17, 2018

I applied the patch offered by jaimeperez and I set my NameIDPolicy to be false and that resolved some of my issues but now when trying to view my metadata I get the following:

SimpleSAML_Error_Error: UNHANDLEDEXCEPTION

Backtrace:
0 /var/www/html/xxxxxxxxx/docroot/simplesaml/module.php:178 (N/A)
Caused by: Exception: authsources['default-sp']: The option 'NameIDPolicy' is not a valid string value.
Backtrace:
2 lib/SimpleSAML/Configuration.php:698 (SimpleSAML_Configuration::getString)
1 modules/saml/www/sp/metadata.php:141 (require)
0 /var/www/html/xxxxxxxxx/docroot/simplesaml/module.php:135 (N/A)
@tvdijen

This comment has been minimized.

Member

tvdijen commented May 17, 2018

Thanks for reporting this @owenbush! This is something we have to take care of.
I have added this to the todo-list above, so we don't forget.

@if-owen

This comment has been minimized.

if-owen commented May 25, 2018

This issue is kind of a blocker for me so I wanted to try see if I could patch it myself. I am not sure I 100% understand all the nuances of this issue - is the null NameIDPolicy value going to disappear in favor of a false value? If so would this patch work - this would work for my use-case as I am trying to use false but I didn't know if backwards compatibility was being considered for the previously undocumented null value.

diff --git a/modules/saml/www/sp/metadata.php b/modules/saml/www/sp/metadata.php
index a0b0318e..59398e83 100644
--- a/modules/saml/www/sp/metadata.php
+++ b/modules/saml/www/sp/metadata.php
@@ -138,7 +138,7 @@ if ($certInfo !== null && array_key_exists('certData', $certInfo)) {
     $certData = null;
 }

-$format = $spconfig->getString('NameIDPolicy', null);
+$format = $spconfig->getString('NameIDPolicy', false);
 if ($format !== null) {
     $metaArray20['NameIDFormat'] = $format;
 }
@if-owen

This comment has been minimized.

if-owen commented May 25, 2018

Unfortunately, it got me one step further but then I am faced with:

SimpleSAML_Error_Error: UNHANDLEDEXCEPTION

Backtrace:
0 /var/www/html/xxxxxxxxx/docroot/simplesaml/module.php:178 (N/A)
Caused by: Exception: https://xxxxxxxxx: The option 'NameIDFormat' must be a string or an array of strings.
Backtrace:
3 lib/SimpleSAML/Configuration.php:934 (SimpleSAML_Configuration::getArrayizeString)
2 lib/SimpleSAML/Metadata/SAMLBuilder.php:500 (SimpleSAML_Metadata_SAMLBuilder::addMetadataSP20)
1 modules/saml/www/sp/metadata.php:237 (require)
0 /var/www/html/xxxxxxxxx/docroot/simplesaml/module.php:135 (N/A)
@tvdijen

This comment has been minimized.

Member

tvdijen commented May 25, 2018

I am not sure I 100% understand all the nuances of this issue - is the null NameIDPolicy value going to disappear in favor of a false value?

Yes, it is going to disappear in 2.0

If so would this patch work - this would work for my use-case as I am trying to use false but I didn't know if backwards compatibility was being considered for the previously undocumented null value.

No, that's not gonna work as you've already figured out yourself.. It's gonna take a lot more than a oneliner to fix this and we're not going to be backwards compatible.

@PradipShrestha

This comment has been minimized.

PradipShrestha commented Jul 9, 2018

Any resolution on this? @malikkotob I am facing exact scenario even after the patch.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Jul 10, 2018

Hi @PradipShrestha,

Have you applied the patch and set NameIDPolicy to false in the configuration?

@BurninLeo

This comment has been minimized.

BurninLeo commented Jul 18, 2018

Please note that this may also need a modification in the module that automatically rewrites metadata from the IdP to saml20-idp-remote.php and shib13-idp-remote.php.

If the IdP does not specify the NameIDFormat, than the metadata will not contain a key 'NameIDFormat', which casts to NULL (=transient) when it comes to the Request. Instead, this path would require "false".

@PradipShrestha

This comment has been minimized.

PradipShrestha commented Jul 23, 2018

@jaimeperez : I did but I am getting exception while visiting metadata page of SP.

SimpleSAML_Error_Error: UNHANDLEDEXCEPTION
Backtrace:
1 www/_include.php:45 (SimpleSAML_exception_handler)
0 [builtin] (N/A)
Caused by: Exception: authsources['default-sp']: The option 'NameIDPolicy' is not a valid string 
value.
Backtrace:
2 lib/SimpleSAML/Configuration.php:698 (SimpleSAML_Configuration::getString)
1 modules/saml/www/sp/metadata.php:141 (require)
0 www/module.php:135 (N/A)
@tvdijen

This comment has been minimized.

Member

tvdijen commented Jul 23, 2018

The fix for that is here:
c9af78a

u4i-lisaf added a commit to u4i-lisaf/simplesamlphp that referenced this issue Sep 12, 2018

DC-4073. Patch for SimpleSamlPHP configuration. Allow authentication …
…request to be sent with no nameId policy. See bug simplesamlphp#771 of the master simplesamlphp library.
@brooke-heaton

This comment has been minimized.

brooke-heaton commented Sep 20, 2018

I'm confused. Is there a PR on the simplesamlphp/simplesamlphp repo for this?

@tvdijen

This comment has been minimized.

Member

tvdijen commented Sep 27, 2018

@brooke-heaton No PR, but a feature-branch.. Work on it kinda stalled though

@drjayvee

This comment has been minimized.

drjayvee commented Oct 5, 2018

I ran into the same problem (ADFS as IdP) when upgrading to 1.16 for PHP 7.2 compatability. I can confirm that the workaround discussed here works.

For reference, here's what I did (edited after @kekkis pointed out a mistake)

git checkout -b NameIDPolicy-workaround v1.16.2
git cherry-pick c9af78ab c40ef46f4e

And in config/authsources.php: 'NameIDPolicy' => false.

I'd feel much more safe using a tagged version, but we need PHP 7.2 compat, so this will have to do for now.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Oct 5, 2018

Hi,

Just for the sake of clarity, SimpleSAMLphp has never had any incompatibility with PHP 7.2. Therefore, there’s no point in upgrading because of that. You should always keep up to date though, because of bug fixes but most importantly because of security fixes.

@drjayvee

This comment has been minimized.

drjayvee commented Oct 5, 2018

Hi Jaime,

We were getting errors because of the strings in assert() calls in 1.14. Our application sets error_reporting to E_ALL, so we were running into problems because of those asserts.

@jaimeperez

This comment has been minimized.

Member

jaimeperez commented Oct 5, 2018

assert() has been deprecated in PHP 7.2, but that doesn’t mean it doesn’t work or those messages were errors. Not everything in the log is an error, and those messages are just notices, mainly targeted at developers. Of course that doesn’t mean those messages wouldn’t fill the log, and that’s indeed annoying, but still I need to stress that previous versions of SimpleSAMLphp work perfectly fine with PHP 7.2. Note also that you should never run with E_ALL log level in production.

@drjayvee

This comment has been minimized.

drjayvee commented Oct 5, 2018

I absolutely agree that 1.14 works fine in PHP 7.2. However, we do use E_ALL in production and log Notices/Warnings/Errors/Exceptions to a 3rd-party service (although display_errors = 0, obviously). So the 'PHP Deprecated' warnings are very annoying/distracting and do fill the logs. Therefore, I'd like to upgrade to 1.16.

And like you said, we should be upgrading for bug fixes and security anyway. :)

Thanks a lot for your time and feedback, but this is getting a bit off-topic. I just wanted to confirm that the workaround/partial fix works.

@kekkis

This comment has been minimized.

kekkis commented Oct 26, 2018

@drjayvee wrote:

For reference, here's what I did

git checkout -b NameIDPolicy-workaround v1.16.2
git cherry-pick d8c3e595e4 797807a4a

And in config/authsources.php: 'NameIDPolicy' => false.

I tried replicating this with simplesamlphp/simplesamlphp but got the error: fatal: bad revision 'd8c3e595e4'. Which repo did you run this on?

@tvdijen

This comment has been minimized.

Member

tvdijen commented Oct 26, 2018

@kekkis Those branches are probably local branches for @drjayvee, not SimpleSAMLphp branches..
There is not fix for this right now

@kekkis

This comment has been minimized.

kekkis commented Oct 26, 2018

I see. You're probably correct.

Am I correct in assuming that downgrading to 1.14.9 is the best way forward to get the NameIDPolicy out of the request with immediate effect?

Or do you have a fix in mind that I could apply on top of 1.16.2 which would lead to a working version w/r/t both metadata and the message exchange?

@tvdijen

This comment has been minimized.

Member

tvdijen commented Oct 26, 2018

I'm not entirely sure.. The fix for this is just not done, but I have this in scope for our November hackathon..
You should be fine up to 1.14.15 (which I strongly recommend due to security issues)

@kekkis

This comment has been minimized.

kekkis commented Oct 26, 2018

The sorting of the output of git tag got me once again, I thought 1.14.9 was the latest in the 1.14 branch. Thanks for the suggestion/update!

@tvdijen tvdijen added this to the 1.18 milestone Oct 26, 2018

@drjayvee

This comment has been minimized.

drjayvee commented Oct 26, 2018

@kekkis oh, that's weird! I have no idea where those hashes come from.
The commits I cherry-picked onto 1.16.2 are c9af78a and c40ef46

That works fine with ADFS in production.

@kekkis

This comment has been minimized.

kekkis commented Oct 29, 2018

Thanks @drjayvee! The commit ids c9af78a and c40ef46 worked and I got the NameIDFormat element out of my request. Onto other problems which are quite possibly just configuration mismatches between the IdP and my SP. Oh the joys of multi-vendor cooperation and the communication delays! :)

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 13, 2018

An up to date attempt to fix this, following above approach, is now in PR #984

@thijskh thijskh modified the milestones: 1.18, 1.17 Nov 20, 2018

@thijskh

This comment has been minimized.

Member

thijskh commented Nov 20, 2018

Fixed in #984

@thijskh thijskh closed this Nov 20, 2018

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