-
Notifications
You must be signed in to change notification settings - Fork 135
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
Cannot verify peer certificate when receiving HTTPArtifact #309
Comments
Are you using this library stand-alone? As you may have read in the README;
|
@tvdijen I am using it as a dependency of SimpleSamlPHP. I described just the problematic part of my authentication process to keep the description possibly simple. The problem occurs when a request for the http artifact is made - as the certificate verification fails, I am receiving an empty response from the Soap Client. This is my stack trace for that final call:
The verification of the certificate fails for Soap Client, because it needs a private CA (root) certificate to be verified correctly - as it has been signed with one. So the standard verification procedure with a public CA (known by default by any system) doesn't work for me. I would need to load a certain private CA file to the context of the Soap Client. This is what the code https://github.com/simplesamlphp/saml2/blob/v4.6.3/src/SAML2/SOAPClient.php#L85-L105 would do, only it requires the destination metadata $dstMetadata to be configured with the certificate, and this variable isn't even provided in the method call https://github.com/simplesamlphp/saml2/blob/v4.6.3/src/SAML2/HTTPArtifact.php#L149 I hope these details will help you help me :) |
I think this one-liner fix should solve your problem; 6c48095 |
@tvdijen do I understand your fix correctly, it would require my IdP metadata to contain the CA certificate, so that it would get loaded? I've checked, and the IdP metadata of my provider unfortunatelly doesn't contain it. It only contains one certificate of the IdP subject itself (and I know that subject cert requires a chain of 3 higher order certificates up to the private root). If I understand your fix correctly, then it means it will not work, as my IdP metadata doesn't include the whole chain of certificates needed to verify their cert. |
Hi @marek-binkowski-sim ! You would have to add the context manually to the converted metadata, yes. Our metadata-array in If you want this to work out-of-the-box, you would have to trust the certificate chain on OS-level, by importing it into your OS trust store. |
Hi @tvdijen , Here's exactly what I did My IdProvider uses a private certificate which requires a chain of three higher order certificates for verification. The metadata of my IdP only contains the final subject certificate, it doesn't contain the chain of three higher order certificates. I used the simplesaml metadata parser to convert the xml IdP metadata to a flat php array representation and I uploaded it to the metadata/saml20-idp-remote.php (The metadata/ directory in my case is not located inside the simplesaml library structure, because it wouldn't work for my application which is Symfony and composer based. But I configured SSP config.php file so that it actually loads my metadata/saml20-idp-remote.php file and recognizes the IdP provider configured this way) I've added all three chain certificates in the 'keys' section of the saml20-idp-remote configuration for my IdP entityId. For each additional certifcate entry I've set the I've applied the code modification you proposed in the commit 6c48095. I've confirmed by additional logging that now the SOAPClient code collects the peer certificates and puts them in a temporary file, where I can find all four signing certificates (the subject plus 3 chain certificates) which then gets set as SSL context in $ctxOpts['ssl']['cafile'] So I would say, if the goal would be to set the chain certificates in place - it would be achieved.
If you have any other hint for me, please share :) |
Hi @marek-binkowski-sim !
That's a valid use-case that we support. You can set an environment-variable (SIMPLESAMLPHP_CONFIG_DIR) in your webserver to point to your See: https://simplesamlphp.org/docs/1.19/simplesamlphp-install.html#location-of-configuration-files Now back on-topic: I think you are mixing up a few things.. The certificates used in IdP metadata are to verify signed SAML responses and to encrypt SAML requests.. That's not the certificate the exception is complaining about. HTTP-Artifact binding implies back-channel communications between your SP and the IdP. This means that your SP tries to directly connect to https:// to deliver the AuthnRequest and during that direct connection is where the certificate is not trusted. Depending on your platform, you could verify this. If Linux-based you could try The order of certificates is important in the file: start with the root CA, then the intermediates. A chain-file should not contain the subject certificate, just the CA+intermediates |
Hi @tvdijen , Eventually, with a private review and suggestion of my local friend, I got it working, yay! The steps I took, described in my previous post were quite close to the working solution. The only thing that was stopping it from working is a hardcoded value in this line #309 (comment) It limits the certificate verification to one level deep. In my case, the CA chain consists of 3 other certificates, so this setting So, yes, I confirm your fix is good, please apply and tag it for a bugfix release. Only I need to ask you to add this further change to it - to raise the verify_depth setting from 1 to 4 in https://github.com/simplesamlphp/saml2/blob/v4.6.3/src/SAML2/SOAPClient.php#L103, so that a multilevel CA verification would be also possible. Thank you! |
If this should be another issue let me know. I seem to be facing a similar issue with verification of the peer certificate and I'm wondering wouldn't it make more sense to use peer_fingerprint in the case where the certificate is used from metadata rather than verifying the certificate chain with depth 1 which seem to assume a self signed certificate? |
The artifact binding is sub-optimal right now. You should be verifying the certificate and the entire chain. I would stay away from fingerprints since there could be collisions depending on the algorithm used. |
To sum up:
|
I think the reasoning behind this is that you're communicating with a trusted peer, so chain validation doesn't really make sense and doesn't add to the security. I'm fine with making this configurable though. |
@tvdijen please excuse me, I can see now I've mistyped my question above (corrected now). I wrote verify_peer, whereas I meant verify_depth, which is statically set to 1 and in my opinion should either have a higher static value (like 4, to allow at least 3 intermediary certs in CA chain), or be configurable. And to explain, peer cert validation is exactly reasoned by that - you want to be sure you are really communicating with the trusted peer, avoiding a man in the middle attack. So, again, a corrected question: |
I'm not aware of any reason. We could make it configurable. |
Hi there, has the abovementioned setting ($ctxOpts['ssl']['verify_depth']) been made configurable yet? I wasn't able to find anything in the source code. It would be great if this could be done because I'm running into the same issue as @marek-binkowski-sim . |
No, sorry, this isn't really on our priority-list right now, because we're in the proces of rewriting pretty much anything, from this library to SSP in general. Your contributions are much appreciated if this is a priority to you. |
Hi @tvdijen I was able to resolve the issue differently, I will also sponsor the project, so contributing, but financially :-) |
Hi,
When receiving a HTTP Artifact, I need to use a certificate which is signed with a private certificate authority.
I found a section of code in SAML2\SOAPClient::send method, which theoretically is there to do exactly what I need - load a private CA file to the context of the SoapClient, so that it could be used to verify the certificate: https://github.com/simplesamlphp/saml2/blob/v4.6.3/src/SAML2/SOAPClient.php#L85-L105
This code section depends on the $dstMetadata variable, which is the third parameter of the SOAPClient::send method, is supposed to be a SimpleSAML\Configuration, and is optional.
Unfortunatelly, the SAML2\HTTPArtifact object which uses SOAPClient::send when receiving the artifact, doesn't specify this third Configuration argument at all, leaving this $dstMetadata variable empty and causing the peer certificate verification to be skipped:
https://github.com/simplesamlphp/saml2/blob/v4.6.3/src/SAML2/HTTPArtifact.php#L149
which is by the way the only use of SAML2\SOAPClient::send method I've found in the simplesamlphp/saml2 package, which would make that verification section of the code never to be used.
Is it intentional? Is something forgotten? Wrongly removed? Is this third Configuration parameter of SOAPClient::send method used in some other package only?
Do you plan to add this possibility to SAML2\HTTPArtifact in any near future?
Is there any way I could currently use this feature to actually load a private CA file and perform the certificate verification against it?
The text was updated successfully, but these errors were encountered: