Skip to content

Conversation

liquidpele
Copy link
Contributor

Previously, when using a custom template, we would parse the html given by pysaml2 with an xml parser to get the request xml. This broke when the SSO url had an & in it though, which was fixed by #41 ... however, other characters can cause similar xml parsing errors. The real issue is that we are using an xml parser for generated html, which could change to be invalid xml with any pysaml2 update.

To avoid this, I've redone the logic so that in the case of a http-post binding with a custom template, instead of asking pysaml2 for the full html we only ask for the request xml and utilize. This removes the need for any xml parsing there at all.

@liquidpele
Copy link
Contributor Author

@knaperek I fubar'd the last PR so opening this new one, sorry :p

I changed it so that it doesn't get the html AND the xml in that case, as you pointed out was unnecessary.

@liquidpele
Copy link
Contributor Author

@knaperek I actually got signing of the http-redirect bound authnrequest to work. pysaml2 does handle it just fine, but it's params are not documented very well. Essentially, the "sign" kwarg means to sign the xml and include the signature within the xml... the "sigalg" kwarg tells it to use that signing method to sign the redirect data and include the signature as a url param. I chose the rsa1 signature option as the spec says it is required to support that. (line 615 from https://docs.oasis-open.org/security/saml/v2.0/saml-bindings-2.0-os.pdf).

This change allows us to use http-redirect binding with an IDP that required the signature param even though we were telling it not to require signatures -_-

@knaperek knaperek merged commit ed4661b into IdentityPython:master Apr 13, 2017
@knaperek
Copy link
Collaborator

Thank you @liquidpele for digging this hidden feature out, this can be quite useful.

@liquidpele
Copy link
Contributor Author

@knaperek any chance of bumping the version now that my changes are merged?

@knaperek
Copy link
Collaborator

I already did :-)

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

Successfully merging this pull request may close these issues.

2 participants