search

eXist-db / existdb-saml

XQuery module that implements SAML v2 single sign-on
GNU Lesser General Public License v2.1
4 stars 3 forks source link

Question about crypto-lib comments and obsolete crypto function #10

Open plutonik-a opened 1 year ago

plutonik-a commented 1 year ago

Hi @chakl, I've got a question regarding some commented-out lines in the content/exsaml.xqm module:

In your last commit 9c43d19 you commented out a condition for verifying a signature and left a comment
(: COMMENTED OUT until crypto-lib issues resolved :). Do you recall what these issues with the crypto-lib were and if they have been resolved in the meantime?

I'm also wondering what the empty "if then" expression is about, since the function commented out in it doesn't seem to exist (any more) in the crypto-lib repo. 

(: verify XML signature of a SAML response :)
declare %private function exsaml:verify-response-signature($resp as item()) {
    let $log  := exsaml:log("debug", "verify-response-signature: " || $resp)
    let $res :=
        (: if $idp-certfile is configured, use that to validate XML signature :)
        if ($exsaml:idp-certfile != "") then (
(:            crypto:validate-signature-by-certfile($resp, $exsaml:idp-certfile):)
        )
...

Is the XML signature of the SAML response no longer validated, or is there now an alternative method of doing this?

It would be great if you could help me sort this out. Thank you!

chakl commented 1 year ago

Hi @plutonik-a, long time no talk - sorry for the late reply, rather busy currently.

Do you recall what these issues with the crypto-lib were and if they have been resolved in the meantime?

The code you mentioned is about a workaround for a particular (rather uncommon) SAML deployment. Their SAML IDP signs SAML assertions, but it does not ship the public key to verify assertions with the SAML response. Instead, they provide an X.509 certificate file that contains this public key.

For this special case, I had a patch to expath-crypto-lib that added an XQuery function crypto:validate-signature-by-certfile() that would would extract the pubkey from the certificate file and use that to validate signatures. I stopped this patching during crypto-lib evolution and never bothered to send a PR. Also, this should probably be done differently (importing the certificate file into the Java keystore and fetching the pubkey from there).

This workaround is only relevant for a certain customer deployment. All other SAML deployments that I have dealt with send the pubkey for signature validation with the SAML response and do not need the workaround. This has bothered me for quite a while, and will be addressed correctly in an upcoming maintenance release.

In the meantime, I had disabled the problematic code. Even twice - one time correctly, the other one incorrectly.

I'm also wondering what the empty "if then" expression is about, since the function commented out in it doesn't seem to exist (any more) in the crypto-lib repo.

That one is the correct "fix" for this old workaround. It comments out the call to crypto:validate-signature-by-certfile() that does not exist in the standard crypto lib, leaving an empty "if then" expression. This noop is only executed if variable $exsaml:idp-certfile is set from existdb-saml config file. Semantically meaning: if this "pubkey-from-certfile" hack is needed, set attribute $exsaml:config/idp/@certfile. If not set (the default), assume the pubkey is included in the SAML response, and validate signatures "normally".

I obviously felt the need to fix it again, this time incorrectly. In commit https://github.com/eXist-db/existdb-saml/commit/9c43d19ec2eea1c26e749952f0c1681a7c889b18, all signature validation was disabled, even for sites that do not need the certfile hack.

This is wrong and clearly a bug. Thanks for bringing this up. I will revert that change.