Tell if response is SP-init or IdP-init (InResponseTo validation)

[jnardone]

This issue was closed for the wrong reason:

#284

There was a (maybe) bug but the original use case is not covered. How can I tell if an inbound response was SP-initiated or IdP-initiated, especially if I want to support both?

1. There is currently no access to whether or not the inResponseTo field is present
2. You can't use validateInResponseTo if you also want to support IdP-initiated, since the code throws an exception if it's missing

Additionally, I would think that supporting both validateInResponseTo (for SP-originated requests) as well as allowing no inResponseTo headers (for IdP requests) would be a valid use case. Also imposible due to the changes made in #302

So... how do I do this?

[jgomer2001]

Hi, I'm a SAML newbie, but from my understanding, IDP-initated responses don't have inResponseTo attribute at all.

So one way to identify whether you are in SP/IDP initiated scenario is by obtaining the assertion XML which is attached to the profile around here, and check for presence of inResponseTo. It's a hack though... does it make sense?

Regarding point 2, I'm not nodejs fluent, but, does it throw exception if missing, or if it's present and does not match?

[jnardone]

Yes, I could -but this is a library that's abstracting all this away. I shouldn't have to rip into the raw XML just for this. I should be able to tell within the context of the API if this is a reply to a request or not.

For number 2, again, I shouldn't have to code around passport-saml. The exception is thrown before there's easily access to it. And it bypasses all the other validations - I want it to decode and validate everything it can, but not treat a lack of a "inResponseTo" as a failure. But if it is there, I want it validated.

[andkrist]

Validating inReponseTo throws unhandled exception when using a IdP-initiated flow.

[wielrls]

I am observing the same failure for IdP initiated flows when validateInResponseTo is enabled.

[jgomer2001]

Validating inReponseTo throws unhandled exception when using a IdP-initiated flow.

True, the problem in saml.js file (see else):

if (this.options.validateInResponseTo) {
    if (inResponseTo) {
      return Q.ninvoke(this.cacheProvider, 'get', inResponseTo)
        .then(function(result) {
          if (!result)
            throw new Error('InResponseTo is not valid');
          return Q();
        });
    } else {
      throw new Error('InResponseTo is missing from response');
    }
  }

this was introduced in passport-saml 1.0.0

one would always like to use inresponseTo validation as long as inresponseTo exists (ie sp. initiated)

[markstos]

Project maintainer here. I just read the SAML spec about InResponseTo. Here's what it says:

InResponseTo [Optional] A reference to the identifier of the request to which the response corresponds, if any. If the response is not generated in response to a request, or if the ID attribute value of a request cannot be determined (for example, the request is malformed), then this attribute MUST NOT be present. Otherwise, it MUST be present and its value MUST match the value of the corresponding request's ID attribute.

In context, SP-init response MUST have InResponseTo, but IdP responses must not. When we released Passport-saml 1.0, we updated InResponseTo validation so that a missing InResponseTo was considered invalid. That made the SP-init case more secure, but it broke the IdP-init cases where validateInResponseTo was enabled.

If you are ONLY using IdP logins, you can set validateInResponseTo to false with no loss to security.

If you need to support a mix of SP-init and IdP-Init, then the most secure-but-viable option is to revert the old logic of only validating InResponseTo if it is present. But this offers hardly any better security than disabling InResponseTo-- if an attacker wants to forge a request, they can just omit InResponseTo. So perhaps the best approach here is to advise people to disable validateInResponseTo if they need to support IdP-init flows.

[markstos]

IdP-initiated is less secure, partly due to the above issue. The weaker security of IdP-initiated is discussed more here:

https://www.identityserver.com/articles/the-dangers-of-saml-idp-initiated-sso

The author's conclusion there is that maximize security, IdP-initiated should be supported on an as-needed basis.

Perhaps passport-saml should change the default value for validateInResponseTo from false to true and document that it should be set back to false when IdP-initiated needs to be supported.

[govindrai]

@markstos @jnardone

if an attacker wants to forge a request, they can just omit InResponseTo.

This is not possible if the assertion is signed, since inResponseTo is part of the payload. I think it's incorrect to not parse the incoming xml and validate the inResponseTo field if it's present. In fact, even with idp initated flow, requests come in which a unique assertionId. Those should also be stored in the cacheProvider until the NotOnOrAfter to avoid replay attacks. If you avoid the replay attack much "insecurity" around idp initiated flow instantly disappears.

I would implore you guys to reconsider the API a bit and offer a flag such as validateInResponseTo to accept values ["never", "alwasy", "onlyIfPresent"].

There are other limitations that the current design has brought. For example, current usage requires you to apply a strategy to an incoming request. Well how do you know what strategy to apply if you haven't yet parsed the request? Passport is assuming that the acs endpoint the request is coming to already has information on what strategy to apply (some query paramater in the url), but that is quite a big assumption. In our case, we have to look into the SAML request to see the idpIssuer, determine the strategy and then apply the correct strategy to the request. If the first thing passport-saml did was parse the request and make it available to use throughout the entire flow, it would make it quite a bit easier for passport-saml users to direct flows one way or another.

[markstos]

@govindrai You make a good case. Would you consider making a pull request for the changes?

How would you handle the fact that validateInResponseTo currently accepts true and false, but you propose that it accept some specific strings?

[hluedeke]

Hi @markstos! I went ahead and submitted a PR for this - to handle validateInResponseTo accepting true or false, I made a function that converts truthy/falsy into "always" or "never", which should make it backwards compatible. I also wrote a few tests to test the new functionality, but I'm not sure how to "live" test it.

[ghusse]

For info, the PR by @hluedeke is here: node-saml/node-saml#37

[cjbarth]

This will be available in the 4.0 release.