Support redirect for Logout flows

[stavros-wb]

• Builds on #16 Inflating redirect response
• Reverts [638ce6e accepting redirect as well as post; redirect not verified(638ce6e)
• Implements
Support HTTP-Redirect binding for IdP to SP SLO

[berzniz]

I would love to see this merged - did you try it with SLO of OneLogin?

[stavros-wb]

I had (and fixed) a bug when retrieving the Issuer from the LogoutRequest. I've tested with SimpleSamlPhp and Onelogin. Although I can't find how to setup OneLogin to include a signature for the LogoutRequest to make sure the signature validation works as well.

[markstos]

To help review, you could link to the part of the SAML spec that discuss SP SLO and related topics.

I think this will be a nice feature to add, but want to be sure about our spec compliance.

Thanks.

[markstos]

Also could you say more about why the mentioned commit was reverted?

[stavros-wb]

@markstos from the spec

The message SHOULD be signed or otherwise authenticated and integrity protected
by the protocol binding used to deliver the message.
.......
The message SHOULD be signed or otherwise authenticated and integrity
protected by the protocol binding used to deliver the message.

which builds a use case for supporting redirect SLO (protocol binding) and verifying its signature, if it exists.

regarding the commit, I think it's an error on my end, I meant revert
6f2087e#diff-93caed0af903b38719faa793f7afae18

I'll amend and rebase on master

[stavros-wb]

See also the TODO on 638ce6e#diff-41fc97a3e28a2b6f8b5ec3bdae6130d5R190 the signature of the saml{request|response} in a redirect flow is passed as a query param

[markstos]

@stavros-wb Thanks! I'll look for the revised PR.

[markstos]

@stavros-wb I'm thinking of doing a "1.0" release within the next 10 days and could include this revised PR if it's ready.

[stavros-wb]

thanks @markstos , rebased and squashed on latest master

[markstos]

@stavros-wb This looks good to me. Are they are related updates to the README that need to be made for this?

@cjbarth Any other concerns here before this gets merged?

[markstos]

@stavros-wb Also, can you confirm if this is considered a breaking change?

[cjbarth]

I don't have any concerns about this, but I also don't have a lot of experience with this. I do feel that a README update is warranted though.

[stavros-wb]

@markstos

@stavros-wb Also, can you confirm if this is considered a breaking change?

Nope. this is not a breaking change. It supports redirects on Logouts, while right now passport-saml supports only POST

@cjbarth

I don't have any concerns about this, but I also don't have a lot of experience with this. I do feel that a README update is warranted though.

I don't think a README entry is required. I'm not opposed to it, though. SLO is already supported if the protocol binding is POST by providing the callback url to the IDP. changing it to redirect would only require changing the setting on the IDP, no further configuration on SP.
What do you have in mind for the docs?

[markstos]

I think it would be useful to mention in the Logout section of the docs that we support SLO and explicitly mention that we support both POST and Redirect bindings.

[stavros-wb]

@markstos I've added a small list regarding SLO in the README file