Skip to content
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

Add HTTP-POST binding support for IdP SingleSignOnService #116

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

waynewoodfield
Copy link

Created a simple self-submitting form to POST a SAMLRequest to the IdP when HTTP-POST binding is required.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.4%) to 94.685% when pulling 8bfe16c on waynewoodfield:post-binding into 9274e6b on onelogin:master.

@pitbulk
Copy link
Contributor

pitbulk commented Jul 17, 2017

Support HTTP-POST binding is more than just send the SAML message using POST instead GET, for example, signatures need to be embedded.

@waynewoodfield
Copy link
Author

waynewoodfield commented Jul 17, 2017 via email

@docwarems
Copy link

Hi, we are also evaluating this Java toolkit for adding SAML support into our product. We would definitely need signed auth requests with POST binding.
Is there any schedule regarding adding POST binding support?

Please don't be offended by this question. In know this is free software developed eventually by persons in their spare time. It's just that we need it and have to check out other toolkits in case. We would also pay for a commercial product.

@pitbulk
Copy link
Contributor

pitbulk commented Oct 25, 2017

There is no plan to implement it.

Based on SAML specification (See reference at the bottom), all IdPs MUST support the HTTP-Redirect binding so that is the selected way we implemented to manage AuthNRequests, LogoutRequests and LogoutResponses.

Adding other binding will make the toolkit more complex and is not our goal.

There are other alternatives that support those bindings that you should review:

Reference: Conformance Requirements for the OASIS Security Assertion Markup Language (SAML) V2.0 Page 9

@docwarems
Copy link

Thanks a lot for this clear statement and pointing me to other toolkits.
I know that the size limitation of GET requests could become real problem in certain situations, eventually depending on the browser. As signed requests might exceed this limitation, I see a practical problem in using the Redirect binding. I also found this statement at other places. Without having any practical SAML experience right now, I expect a problem here.

@pitbulk
Copy link
Contributor

pitbulk commented Oct 25, 2017

I know that the size limitation of GET requests could become real problem

Only SAMLResponses can be that big since them can contain Assertions. In that scenario we support the HTTP-POST binding.

On AuthNRequest, LogoutRequest and LogoutResponse, the size of the Message isn't that big, and can be perfectly managed (take in mind also that the SAML protocol defines a gzip method to be used on SAML Messages to compress them) so the size limitation of GET requests is just an excuse of those IdPs that only implemented the POST binding (since they required it for process the SAMLResponse) .

@waynewoodfield
Copy link
Author

waynewoodfield commented Oct 25, 2017 via email

@docwarems
Copy link

Thanks a lot you both. First of all I will ask the IdP about this strict POST requirement. I also was of the opinion that the IdP should handle it dynamically, also because the SP metadata generated by the toolkit doesn't contain information about SSO Auth binding support (but metadata XML validation fails - I have no further details here currently!).

@docwarems
Copy link

This link is probably not the worst source for reliable SAML information:
https://www.oasis-open.org/committees/download.php/27819/sstc-saml-tech-overview-2.0-cd-02.pdf. I quote from chapter 5.1.3
"The HTTP POST binding may be necessary for an message in cases where its length precludes the use of the HTTP Redirect binding (which is typical)".
My english is eventually not good enough, but I understood that length problem are not that uncommon.

@pitbulk
Copy link
Contributor

pitbulk commented Oct 25, 2017

Th document that you mentioned is just a Draft, if you review the 10th version of this document:
https://www.oasis-open.org/committees/download.php/20645/sstc-saml-tech-overview-2%200-draft-10.pdf

you will see that the sentence was modified and the "(which is typical)" disappeared, but again, just a draft, not official doc.

I can assure you that the AuthNRequest messages that you can build with this toolkit never will experience the GET size parameter limitation.

@docwarems
Copy link

Hm, strange that your document from 2006 is newer than mine from 2008...

Other question; This should be the current binding spec:
https://www.oasis-open.org/committees/download.php/35387/sstc-saml-bindings-errata-2.0-wd-05-diff.pdf
I could not find a statement which bindings MUST be implemented and which are optional. I found a mandatory support for Redirect Binding at other sources, but not in this spec. Can you point me there?

@pitbulk
Copy link
Contributor

pitbulk commented Oct 25, 2017

I already did in a previous message:

Reference: Conformance Requirements for the OASIS Security Assertion Markup Language (SAML) V2.0 Page 9

Official docs are listed here:
http://docs.oasis-open.org/security/saml/v2.0/

@docwarems
Copy link

Thank you!

@pitbulk pitbulk mentioned this pull request Oct 26, 2017
@docwarems
Copy link

docwarems commented Oct 26, 2017

Hi again, forgive me if this discussion becomes a little off topic. If you prefer I could start a more specific thread.
Further investigation revealed the IdP product "F5" supports Redirect binding for signed auth requests only with (quote) "enveloped signatures". I understand this means the signature shall be included in the auth XML (and the missing signature was also the error reason the IdP admin told me).
Does the spec leaves open, whether the signature comes with extra GET parameter (as for the OneLogin toolkit), or shall be included in the XML?
If this is the only problem and the spec allows this, I should be able to modify the toolset code to support this. What do you mean?

…ied, browsers might interpret arbitrarily as plain text.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 94.556% when pulling 94eb327 on waynewoodfield:post-binding into 9274e6b on onelogin:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.6%) to 94.556% when pulling 94eb327 on waynewoodfield:post-binding into 9274e6b on onelogin:master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants