Support signed assertions #45
Conversation
fumieval
force-pushed
the
pr-signed-assertion
branch
from
b72b69b
to
1afa8d7
Compare
|
I haven't had the time to review this yet, but I hope to be able to do so by the end of the coming weekend at the latest. Thank you as always for your contributions and patience! 🙇🏽 |
src/Network/Wai/SAML2/Validation.hs
Outdated
| let docMinusSignature = removeSignature responseXmlDoc | ||
| docMinusSignature <- removeSignature <$> case responseSignature samlResponse of | ||
| Just _ -> pure responseXmlDoc | ||
| -- if a response signature is not present, assume that the assertion contains the signature |
There was a problem hiding this comment.
When both response signature and an assertion signature are present, we could technically validate both, but I'm not sure if anyone really wants to do that
fumieval
force-pushed
the
pr-signed-assertion
branch
from
1afa8d7
to
c5ffca8
Compare
fumieval
force-pushed
the
pr-signed-assertion
branch
3 times, most recently
from
ba8e651
to
e43f0e4
Compare
|
@mbg Sure. I refactored the implementation for more clarity |
fumieval
force-pushed
the
pr-signed-assertion
branch
from
e43f0e4
to
39b31bc
Compare
| @@ -3,6 +3,7 @@ | |||
| ## 0.7 | |||
|
|
|||
| - Replaced `x509Certificate` with `x509Certificates` in `IDPSSODescriptor` so that it may have more than one certificate ([#65](https://github.com/mbg/wai-saml2/pull/65) by [@fumieval](https://github.com/fumieval)) | |||
| - Support signed assertions, not just signed responses by ([#45](https://github.com/mbg/wai-saml2/pull/45) by [@fumieval](https://github.com/mbg/wai-saml2)) | |||
There was a problem hiding this comment.
| - Support signed assertions, not just signed responses by ([#45](https://github.com/mbg/wai-saml2/pull/45) by [@fumieval](https://github.com/mbg/wai-saml2)) | |
| - Support signed assertions, not just signed responses ([#45](https://github.com/mbg/wai-saml2/pull/45) by [@fumieval](https://github.com/fumieval)) |
There was a problem hiding this comment.
Good point. I am happy for you to keep this as is.
| Just _ -> validateSAMLResponseSignature cfg responseXmlDoc samlResponse now | ||
| Nothing -> validateSAMLAssertionSignature cfg responseXmlDoc samlResponse now | ||
|
|
||
| validateSAMLResponseSignature :: SAML2Config |
There was a problem hiding this comment.
It would be good to add a docs comment for this function summarising what it does.
|
|
||
| validateSAMLPreliminary cfg samlResponse | ||
|
|
||
| case responseSignature samlResponse of |
There was a problem hiding this comment.
I am wondering to what extent the path that's taken here should be up to the contents of the response? Rather than picking this based on whether there's a responseSignature present, would it make sense to instead have a setting in SAML2Config that determines which path we take here?
| assertion <- case responseAssertion samlResponse of | ||
| Just a -> pure a | ||
| _ -> throwError $ InvalidResponse $ userError "Assertion is required" | ||
|
|
||
| signature <- case assertionSignature assertion of | ||
| Just a -> pure a | ||
| _ -> throwError $ InvalidResponse $ userError "Assertion signature is required" |
There was a problem hiding this comment.
I think it would be more consistent to have more constructors in the SAML2Error type for these errors than to use InvalidResponse with userError.
| , documentRoot = node | ||
| , documentEpilogue = [] | ||
| } | ||
| _ -> throwError $ InvalidResponse $ userError "Assertion is required" |
There was a problem hiding this comment.
This error message should probably say something else?
Summary
At the moment, wai-saml2 validates signed responses, but not signed assertions. This might cause an error when the identity provider signs assertions only (by default AzureAD does not sign responses).
This change adds support for signed assertions; when a signature for the response is present, it validates the response. If this is missing, it validates the signature for the assertion instead.
Checklist
@sinceannotations.