Remove NameID requirement on responses

[Fizzadar]

Currently responses will be treated as invalid if no NameID is returned. I'm not 100% but I can't find anywhere in the SAML2 spec which states NameID is required - additionally we are working with numerous IDPs which don't provide the field (currently bypassing by patching the response class).

I've not yet looked into what's involved beyond removing the exception here.

[pitbulk]

Toolkit tries to follow SAML2Int Profile

3.3.1 The Assertion
.....
The Assertion MUST contain a <saml:Subject> and the <saml:Subject> MUST contain
a <saml:NameID>.

Is true that in the 0.21 version, we can read:

The <saml2:Subject> element of the assertions issued by an Identity Provider SHOULD contain a <saml2:NameID> element. The <saml2:Subject> element MUST NOT include a <saml2:BaseID> nor a <saml2:EncryptedID>

but a lot of SAML plugins uses the php-saml toolkits and expect a nameID value, so we may keep this element as required.

[manterfield]

Apologies, I don't understand why the php-saml toolkits expecting a nameID value would be a reason to keep it as required here?

[pitbulk]

I mean python-saml

[manterfield]

Ah, that makes more sense! Could we not do this in a non breaking way? It is mightily annoying at the moment when an IDP doesn't send that through.

Do you guys accept pull requests?

[pitbulk]

What IdP are you using and why is not sending nameID?
I think that there is some setting on the IdP side to provide a NameID value.
We accept PR, but not happy with the idea of making NameID optional.

[manterfield]

It was a corporate client's IDP. I have no idea why they weren't sending it, but I have no control over that side at all. I simply had to work with it. I know they use ADFS, I also know that it is possible to configure their system to send it. Nonetheless, they hadn't done so.

Given that they are complying with the spec when doing so, I don't really have any recourse to persuade them to do otherwise. It would be awesome if this library gave me the freedom to work with it easily though.

[pitbulk]

I'm obsessed trying to maintain the toolkit as simple as possible.

I understand your issue, but making NameID optional will add a new setting parameter and affects also some core methods.

Service Providers and Identity Providers must to be aligned, I'm agree that the SAMLResponse is valid and they are complying with the SAML 2.0 spec, but you can say that the Service Provider requires the NameID value, and this is also part of the standard,Service Provider entity requiring this value.
You also can mention the SAML2Int Profile text that I previously pasted.

[manterfield]

I'm not sure attempting to convince a client that they should support an older version of a spec to accommodate me is going to take off, unfortunately. It's fine though, I'll just make the changes in my own fork.

To give my opinion, 'as simple as possible' should cover the spec as it stands, or else this library will rapidly become obsolete.

That's perfectly fine if you are only interested in supporting older versions; it's your project after all, but it would be a good thing to state what version you support up to in the readme for the sake of future devs.

[pitbulk]

why you say "support an older version" ?

Adding the NameID attribute is a common practice on SAML and 99% of environments use it.

For me makes no much sense add extra settings for the 1% of cases, that can be solved with a simple change on the settings of the Identity Provider.

[manterfield]

As in older versions of the spec. You are supporting v0.1 of SAML2int (where it is required), but not v0.2 where it is optional but recommended.

I understand your stance about it being technically better to make IDPs provide the attribute, but the change isn't simple in any useful sense of the word. The people using this library are unlikely to be able to effect change in the IDPs they are integrating with. Regardless of how simple the change is as a task, we can't do it.

That being said, thanks to the work you guys have provided so far, I can fork it and make it fit for my purpose.

I think it would be a nice addition to the readme if you added details about the spec you are supporting and to what version.

[pitbulk]

When you set a SAML trust between an Identity Provider and a Service Provider , you need to exchange metadata and configure both entities to agree what attributes are required by the Service Provider, the NameIDFormat, ...

So in order to integrate a Service Provider on ADFS, you may register Service Provider's metadata and configure some parameters.

99% of service providers will require that NameID value in their implementations, is a common practice.

If IdP and SP gonna follow v0.2 SAML2int profile, you can read in the document:

Identity Providers MUST support the urn:oasis:names:tc:SAML:2.0:nameid-format:transient name
identifier format [SAML2Core]. They SHOULD support the urn:oasis:names:tc:SAML:2.0:nameid-format:persistent
name identifier format [SAML2Core]. Support for other formats is OPTIONAL.

so if transient must be supported, the Service Provider should be able to ask for this value.

Notice that SAML2int is a profile that specifies behavior and options that deployments of the SAML V2.0 Web Browser SSO Profile.

[pitbulk]

There is another popular SAML library named pysaml2. You can see that this library also
expects NameID inside the Subject of the SAMLResponses

[manterfield]

Transient format must be supported. That doesn't state that the nameid attribute itself must be. Just that if it is, that format needs to be supported.

Regardless of anything else, the IDP are within spec and outside of my control. There's no convincing argument against that, as it's simply an observation of the facts. It is this library in this instance that doesn't meet spec, not their implementation (even if it is weird implementation-- not disagreeing with you on that part).

The code you have just linked shows them looking for it as an optional param. They do not force it as this library does. It says that

the subject may contain a name_id

(emphasis mine) then has it in a conditional. It's doing exactly what this issue is asking for in this library.

All that being said, I'm not sure this is a productive conversation for either of us anymore, as it seems we have different opinions on the approach. Thanks very much for your effort on this library, it gives me a great starting point.

All the best.

[pitbulk]

I was reviewing what other SAML libraries are doing since in the new samlint version NameID is not mandatory, and found this interesting ticket from simpleSAMLphp
simplesamlphp/simplesamlphp#19

The interesting part:

the SAML2 logout profile requires a NameID to be present.
This means if no NameID was provided by the IdP
authenticating the user, logout should be disabled for it.

As you see allowing SAMLResponse with no NameID will only carry issues.

[jaimeperez]

Indeed, not sending a NameID is something that will cause issues like the one with single logout. On the other hand, almost nobody is using single logout, and even we in Feide (who've been the strongest advocates of SLO out there so far) are stopping to require it (even though we strongly recommend to support it).

In general, I think @tomdickin has a point, and that's why I fixed this exact same issue in SimpleSAMLphp. After all, our libraries are not useful if they don't allow our users what they need (provided what they need is legit, of course). If Microsoft's ADFS IdP is not providing a NameID by default, this issue will keep happening again and again, and in some cases it will be impossible to resolve (remember people in general doesn't understand SAML, and if they are using ADFS the chances are even lower). That means we either allow the NameID to be missing, or keep getting issues and complaints about it. They won't be able to do logout, true, but in 99% of the cases they won't give a f*ck anyway...

Just my two cents.

[pitbulk]

@jaimeperez, and this is how world works, Microsoft decides not adding NameID by default, saml2int in the last version does not require that field and now all SAML SP libraries needs to be adapted to this disruptive change.

spring-security library also has this problem, Its author said 1 year ago that it will be optional, but I think nowaday is required due code has big dependence on NameID.Its documentation has a note when integrating with ADFS that specifies that the NameID must be set.

At OneLogin, we implemented SAML toolkits to allow our customers to simply add SAML to their apps in order to be integrated with our DaaS solution. There were other SAML libraries, but we decided maintain our own with the goal of keep them as simple as possible. As a open source project, developers arrived and started requesting features, most of them related to compatibilities with others Identity Providers or related to satisfy specific requirements of a custom project. We have no problem to spend resources in order to make the toolkit more flexible, more complete and more compatible with others Identity Providers , but the unique rule was: not adding extra complexity to the project.

And this is my internal fight with this NameID issue, of course I understand that @tomdickin has a point.
I will have some internal discussion, take some rest this weekend, and make a decision next week.

[jaimeperez]

Just to be clear: I didn't want to convince you one way or the other! I just saw you referenced our issue here and read the (very interesting) discussion you two had, and wanted to share our experience.

I'm not entirely sure I agree wrt Microsoft. They've been doing many things wrong for many years around SAML, and most of the time nobody cared to make things work with them because they were not complying with the standard.

In this case, they are complying with the standard, though ignoring all interoperability recommendations. I consider this then an issue between complying with the standard ourselves, or getting lots of support requests because our library does not work with ADFS (or whatever other product that generates assertions without a NameID, for that matter). I definitely agree with you that ignoring interoperability recommendations is bad. But complying with the standard is good, and getting less issues and angry users is also good, so that was the reason I decided to make it optional.

Anyway, as I said, just sharing my opinion 😄

[pitbulk]

I know @jaimeperez, I appreciate your opinion.

[pitbulk]

@Fizzadar I hope is not too late.

[jc53593]

@pitbulk Thank you for the wantNameId option! I also read and appreciate the discussion that occurred in this issue. It helped me to understand the background of the issue that I just ran in to (three years later!) and how to work around it. :-)