-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support signed assertions #45
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -83,16 +83,10 @@ decodeResponse responseData = do | |
Left err -> throwError $ InvalidResponse err | ||
Right samlResponse -> pure (responseXmlDoc, samlResponse) | ||
|
||
-- | 'validateSAMLResponse' @cfg doc response timestamp@ validates a decoded SAML2 | ||
-- response using the given @timestamp@. | ||
-- | 'validateSAMLPreliminary' @cfg samlResponse@ validates the status code, destination, and issuer of a SAML2 response. | ||
-- | ||
-- @since 0.4 | ||
validateSAMLResponse :: SAML2Config | ||
-> XML.Document | ||
-> Response | ||
-> UTCTime | ||
-> ExceptT SAML2Error IO Assertion | ||
validateSAMLResponse cfg responseXmlDoc samlResponse now = do | ||
validateSAMLPreliminary :: SAML2Config -> Response -> ExceptT SAML2Error IO () | ||
validateSAMLPreliminary cfg samlResponse = do | ||
|
||
-- check that the response indicates success | ||
case statusCodeValue $ responseStatusCode samlResponse of | ||
|
@@ -118,6 +112,29 @@ validateSAMLResponse cfg responseXmlDoc samlResponse now = do | |
| issuer /= expectedIssuer -> throwError $ InvalidIssuer issuer | ||
_ -> pure () | ||
|
||
-- | 'validateSAMLResponse' @cfg doc response timestamp@ validates a decoded SAML2 | ||
-- response using the given @timestamp@. | ||
-- | ||
-- @since 0.4 | ||
validateSAMLResponse :: SAML2Config | ||
-> XML.Document | ||
-> Response | ||
-> UTCTime | ||
-> ExceptT SAML2Error IO Assertion | ||
validateSAMLResponse cfg responseXmlDoc samlResponse now = do | ||
|
||
validateSAMLPreliminary cfg samlResponse | ||
|
||
case responseSignature samlResponse of | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
Just _ -> validateSAMLResponseSignature cfg responseXmlDoc samlResponse now | ||
Nothing -> validateSAMLAssertionSignature cfg responseXmlDoc samlResponse now | ||
|
||
validateSAMLResponseSignature :: SAML2Config | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be good to add a docs comment for this function summarising what it does. |
||
-> XML.Document | ||
-> Response | ||
-> UTCTime | ||
-> ExceptT SAML2Error IO Assertion | ||
validateSAMLResponseSignature cfg responseXmlDoc samlResponse now = do | ||
-- ***CORE VALIDATION*** | ||
-- See https://www.w3.org/TR/xmldsig-core1/#sec-CoreValidation | ||
-- | ||
|
@@ -126,19 +143,9 @@ validateSAMLResponse cfg responseXmlDoc samlResponse now = do | |
-- Signature element. This element contains | ||
signedInfo <- extractSignedInfo (XML.fromDocument responseXmlDoc) | ||
|
||
-- construct a new XML document from the SignedInfo element and render | ||
-- it into a textual representation | ||
let doc = XML.Document (XML.Prologue [] Nothing []) signedInfo [] | ||
let signedInfoXml = XML.renderLBS def doc | ||
|
||
-- canonicalise the textual representation of the SignedInfo element | ||
let prefixList = extractPrefixList (XML.fromDocument doc) | ||
signedInfoCanonResult <- liftIO $ try $ | ||
canonicalise prefixList (LBS.toStrict signedInfoXml) | ||
|
||
normalisedSignedInfo <- case signedInfoCanonResult of | ||
Left err -> throwError $ CanonicalisationFailure err | ||
Right result -> pure result | ||
signature <- case responseSignature samlResponse of | ||
Just sig -> pure sig | ||
Nothing -> throwError $ InvalidResponse $ userError "Response Signature is required" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wouldn't be necessary if you make the |
||
|
||
-- 2. At this point we should dereference all elements identified by | ||
-- Reference elements inside the SignedInfo element. However, we do | ||
|
@@ -149,8 +156,7 @@ validateSAMLResponse cfg responseXmlDoc samlResponse now = do | |
let documentId = responseId samlResponse | ||
let referenceId = referenceURI | ||
$ signedInfoReference | ||
$ signatureInfo | ||
$ responseSignature samlResponse | ||
$ signatureInfo signature | ||
|
||
if documentId /= referenceId | ||
then throwError $ UnexpectedReference referenceId | ||
|
@@ -162,6 +168,34 @@ validateSAMLResponse cfg responseXmlDoc samlResponse now = do | |
-- the Signature element present). First remove the Signature element: | ||
let docMinusSignature = removeSignature responseXmlDoc | ||
|
||
validateSAMLSignature ValidationContext{..} | ||
|
||
data ValidationContext = ValidationContext | ||
{ cfg :: !SAML2Config | ||
, docMinusSignature :: !XML.Document | ||
, now :: !UTCTime | ||
, responseXmlDoc :: !XML.Document | ||
, samlResponse :: !Response | ||
, signature :: !Signature | ||
, signedInfo :: !XML.Element | ||
} | ||
|
||
validateSAMLSignature :: ValidationContext -> ExceptT SAML2Error IO Assertion | ||
validateSAMLSignature ValidationContext{..} = do | ||
-- construct a new XML document from the SignedInfo element and render | ||
-- it into a textual representation | ||
let doc = XML.Document (XML.Prologue [] Nothing []) signedInfo [] | ||
let signedInfoXml = XML.renderLBS def doc | ||
|
||
-- canonicalise the textual representation of the SignedInfo element | ||
let prefixList = extractPrefixList (XML.fromDocument doc) | ||
signedInfoCanonResult <- liftIO $ try $ | ||
canonicalise prefixList (LBS.toStrict signedInfoXml) | ||
|
||
normalisedSignedInfo <- case signedInfoCanonResult of | ||
Left err -> throwError $ CanonicalisationFailure err | ||
Right result -> pure result | ||
|
||
-- then render the resulting document and canonicalise it | ||
let renderedXml = XML.renderLBS def docMinusSignature | ||
refCanonResult <- liftIO $ try $ canonicalise prefixList (LBS.toStrict renderedXml) | ||
|
@@ -180,8 +214,7 @@ validateSAMLResponse cfg responseXmlDoc samlResponse now = do | |
$ BS.decodeLenient | ||
$ referenceDigestValue | ||
$ signedInfoReference | ||
$ signatureInfo | ||
$ responseSignature samlResponse | ||
$ signatureInfo signature | ||
|
||
if Just documentHash /= referenceHash | ||
then throwError InvalidDigest | ||
|
@@ -191,7 +224,7 @@ validateSAMLResponse cfg responseXmlDoc samlResponse now = do | |
-- We need to check that the SignedInfo element has not been tampered | ||
-- with, which we do by checking the signature contained in the response; | ||
-- first: extract the signature data from the response | ||
let sig = BS.decodeLenient $ signatureValue $ responseSignature samlResponse | ||
let sig = BS.decodeLenient $ signatureValue signature | ||
|
||
-- using the IdP's public key and the canonicalised SignedInfo element, | ||
-- check that the signature is correct | ||
|
@@ -233,6 +266,32 @@ validateSAMLResponse cfg responseXmlDoc samlResponse now = do | |
-- all checks out, return the assertion | ||
pure assertion | ||
|
||
validateSAMLAssertionSignature :: SAML2Config -> XML.Document -> Response -> UTCTime -> ExceptT SAML2Error IO Assertion | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a docs comment for this as well? It may also make sense to move this function in the file to come after |
||
validateSAMLAssertionSignature cfg responseXmlDoc samlResponse now = do | ||
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" | ||
Comment on lines
+271
to
+277
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it would be more consistent to have more constructors in the |
||
|
||
-- Obtain the XML node of the assertion for validation | ||
assertionXml <- oneOrFail "Assertion is required" $ | ||
XML.fromDocument responseXmlDoc XML.$/ XML.element (saml2Name "Assertion") | ||
mbg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
signedInfo <- extractSignedInfo assertionXml | ||
|
||
docMinusSignature <- removeSignature <$> case XML.node assertionXml of | ||
XML.NodeElement node -> pure XML.Document | ||
{ documentPrologue = XML.Prologue [] Nothing [] | ||
, documentRoot = node | ||
, documentEpilogue = [] | ||
} | ||
_ -> throwError $ InvalidResponse $ userError "Assertion is required" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This error message should probably say something else? |
||
|
||
validateSAMLSignature ValidationContext{..} | ||
|
||
-- | `decryptAssertion` @key encryptedAssertion@ decrypts the AES key in | ||
-- @encryptedAssertion@ using `key`, then decrypts the contents using | ||
-- the AES key. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I am happy for you to keep this as is.