Don't require an IdP to return a NameID

[GoogleCodeExporter]

If you don't set a NameIDPolicy attribute for your SP in authsources.php, it 
defaults to sending 
Format="urn:oasis:names:tc:SAML:2.0:nameid-format:transient" in the request. By 
setting NameIDPolicy to NULL, you can force it not to set a Format attribute at 
all, but then it throws an exception "Missing <saml:NameID> or 
<saml:EncryptedID> in <saml:Subject>".

This patch against 1.9.0 allows the SP to work with an IdP that doesn't return 
a NameID.

Original issue reported on code.google.com by elliotke...@gmail.com on 18 Jul 2012 at 4:36

Attachments:

• allow-no-nameid.patch

[GoogleCodeExporter]

Just to be clear: The lack of a NameIDPolicy-element in the AuthnRequest does 
not necessarily mean that the IdP should return a response without a NameID. It 
just means that the IdP is free to choose what type of name identifier it wants 
to return. It looks like your IdP chooses to return a response with no name 
identifier in that case.

The question is whether the IdP is allowed to return a subject without a name 
identifier. Some parts of the SAML 2.0 specification require a name identifier. 
For example, you cannot support the single logout profile without including a 
name identifier in the response. But on the other hand, if your IdP does not 
support the single logout profile, I guess that it may work without a name 
identifier.

I'll have to think about this for a bit :)

Original comment by olavmrk@gmail.com on 19 Jul 2012 at 6:20

[GoogleCodeExporter]

My IdP was misconfigured, which is why it wasn't returning a NameID at all :) 
But even so, it was still working with Shibboleth's SP, and I decided to submit 
this bug in the interests of feature parity.

Original comment by elliotke...@gmail.com on 19 Jul 2012 at 4:25

[GoogleCodeExporter]

Sorry for taking this long to get back to you on this.

I think you finding an error in your IdP configuration illustrates why I'm 
slightly hesitant in adding this -- if a missing NameID is caused by an error, 
it would be best if we could actually produce an error.

However, since it is allowed by the specification, there may be IdPs that do 
not add a name identifier to the subject, where that doesn't happen as the 
result of an error. Therefore I agree that we should support that in 
simpleSAMLphp.

Unfortunately, adding support for that requires slightly more changes than you 
did in your patch:

 - $this->nameId should not be an array in that case, but rather NULL.
 - The SAML2_Assertion::addSubject()-function should support generating an Subject without a NameID.
 - I think there is some code related to logout processing that requires the presence of a NameID, that may fail in unpredictable ways if it is NULL. It would be better to refuse to do single logout in that case.

It would be nice if you have time to create a patch that handles these, but if 
not, I will just leave the issue open, and hopefully get back to it later.

Original comment by olavmrk@gmail.com on 25 Jul 2012 at 1:35

[GoogleCodeExporter]

Okay, here's a try at a revised patch. I haven't tested it thoroughly since my 
IdP no longer returns responses without a NameID.

Original comment by elliotke...@gmail.com on 1 Aug 2012 at 4:58

Attachments:

• allow-no-nameid-2.patch

[GoogleCodeExporter]

Hi,

There must always be a NameID in the LogoutRequest element. (It is mandatory in 
the specification.) Therefore, I do not think you should modify the 
LogoutRequest message. Instead, you need to modify the code that sends logout 
requests, so that it doesn't send a LogoutRequest if there is a NULL NameID. 
(This is in modules/saml/lib/Auth/Source/SP.php). I also think that the call to 
sspmod_saml_SP_LogoutStore::addSession() in modules/saml/www/sp/saml2-acs.php 
needs to be skipped if we don't have a NameID, since logout is impossible 
anyway.

For testing, I'd suggest introducing a (temproary) misspelling where it 
extracts the NameID from the XML data, so that it doesn't find the NameID.

Original comment by olavmrk@gmail.com on 3 Aug 2012 at 11:41

[GoogleCodeExporter]

Original comment by jaim...@gmail.com on 26 Feb 2014 at 2:17

• Added labels: Type-Enhancement

[GoogleCodeExporter]

Nothing has happened here for over a year, so closing it as WontFix for now. It 
can be reopened as a pull request on GitHub later if desired.

Original comment by olavmrk@gmail.com on 27 Feb 2014 at 9:56

• Changed state: WontFix