fix(request): generate correct redirect requests #93
+247
−290
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
depends on @danielcompton's #90 I'll rebase away those commits once that's done. |
https://shibboleth.atlassian.net/wiki/spaces/OSAML/overview Updates signed and encrypted SAML assertion as it wasn't valid under OpenSAML 5.
this fails only when running under cloverage with a reflection issue. Explicitly typehinting fixes it.
Previously we could not generate correctly signed AuthnRequests using HTTP-Redirect bindings. This happened for three reasons: 1. We were inserting the signature into the body of the SAML AuthnRequest. When sending an AuthnRequest using HTTP-Redirect the signature is included as a query param alongside the encoded SAML request 2. We were specifying the wrong binding type. Our requests used HTTP-Post binding. I think most SAML idps are pretty forgiving about what a request looks like but get more string when you put them into the verification mode that checks signatures Okta included here. 3. We did not specify a NameIDPolicy. Basically the same kind of thing as point number 2. As part of fixing those issues I decided to make our AuthnRequest flow use more of the OpenSAML library instead of using the hiccup-based XML generation. I plan to follow up to convert our Logout/SLO handling to work in the same way and clean up unused functions in the crypto/coerce namespaces once that is done. Closes #91
edpaget
force-pushed
the
ep-fix-redirect-generation
branch
from
efb48ab to
b7f5605
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously we could not generate correctly signed AuthnRequests using
HTTP-Redirect bindings. This happened for three reasons:
We were inserting the signature into the body of the SAML
AuthnRequest. When sending an AuthnRequest using HTTP-Redirect the
signature is included as a query param alongside the encoded SAML
request
We were specifying the wrong binding type. Our requests used
HTTP-Post binding. I think most SAML idps are pretty forgiving about
what a request looks like but get more string when you put them into the
verification mode that checks signatures Okta included here.
We did not specify a NameIDPolicy. Basically the same kind of thing
as point number 2.
As part of fixing those issues I decided to make our AuthnRequest flow
use more of the OpenSAML library instead of using the hiccup-based XML
generation. I plan to follow up to convert our Logout/SLO handling to
work in the same way and clean up unused functions in the crypto/coerce
namespaces once that is done.