search

simplesamlphp / saml2

SimpleSAMLphp low-level SAML2 PHP library
https://www.simplesamlphp.org
GNU Lesser General Public License v2.1
284 stars 134 forks source link

Cannot verify peer certificate when receiving HTTPArtifact #309

Open marek-binkowski-sim opened 2 years ago

marek-binkowski-sim commented 2 years ago

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?

tvdijen commented 2 years ago

Are you using this library stand-alone?

As you may have read in the README;

Note that the HTTP Artifact Binding and SOAP client do not work outside of SimpleSAMLphp.

marek-binkowski-sim commented 2 years ago

Are you using this library stand-alone?

As you may have read in the README;

Note that the HTTP Artifact Binding and SOAP client do not work outside of SimpleSAMLphp.

@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 at the final stage, where the Assertion Consumer Service url gets requested by the Identity Provider, and a SimpleSamlPHP module vendor/simplesamlphp/simplesamlphp/modules/saml/www/sp/saml2-acs.php is handling the process.

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:

SimpleSAML\Error\Error: UNHANDLEDEXCEPTION

Backtrace:
1 www/_include.php:17 (SimpleSAML_exception_handler)
0 [builtin] (N/A)
Caused by: Exception: Empty SOAP response, check peer certificate.
Backtrace:
4 /php/vendor/simplesamlphp/saml2/src/SAML2/SOAPClient.php:143 (SAML2\SOAPClient::send)
3 /php/vendor/simplesamlphp/saml2/src/SAML2/HTTPArtifact.php:149 (SAML2\HTTPArtifact::receive)
2 modules/saml/www/sp/saml2-acs.php:35 (require)
1 lib/SimpleSAML/Module.php:266 (SimpleSAML\Module::process)
0 www/module.php:10 (N/A)

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 :)

tvdijen commented 2 years ago

I think this one-liner fix should solve your problem; 6c48095e34a1edf64f30404ca191f705c5d659ef Could you confirm this? Then I will tag a new bugfix-release

marek-binkowski-sim commented 2 years ago

I think this one-liner fix should solve your problem; 6c48095 Could you confirm this? Then I will tag a new bugfix-release

Thank you @tvdijen , I will need a few days to confirm it, but I will give you an answer for sure

marek-binkowski-sim commented 2 years ago

@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. I am missing a way to load a root certificate or a chain of certificates directly from local file(s) into the Soap Client context

tvdijen commented 2 years ago

Hi @marek-binkowski-sim ! You would have to add the context manually to the converted metadata, yes. Our metadata-array in metadata/saml20-idp-remote.php can contain a lot more settings that cannot be extracted from SAML2 metadata.

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.

marek-binkowski-sim commented 1 year ago

Hi @tvdijen , I think I did what you suggested, unfortunately it didn't work for me.

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 encryption parameter to false, the signing parameter to true, and 'type' to X509Certificate. The 'X509Certificate' gets the actual value of the certificate.

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. Yet, when consuming the artifact, I am still getting:

SimpleSAML\Error\Error: UNHANDLEDEXCEPTION

Backtrace:
1 www/_include.php:17 (SimpleSAML_exception_handler)
0 [builtin] (N/A)
Caused by: Exception: Empty SOAP response, check peer certificate.
Backtrace:
4 /php/vendor/simplesamlphp/saml2/src/SAML2/SOAPClient.php:146 (SAML2\SOAPClient::send)
3 /php/vendor/simplesamlphp/saml2/src/SAML2/HTTPArtifact.php:152 (SAML2\HTTPArtifact::receive)
2 modules/saml/www/sp/saml2-acs.php:35 (require)
1 lib/SimpleSAML/Module.php:266 (SimpleSAML\Module::process)
0 www/module.php:10 (N/A)

If you have any other hint for me, please share :) Does the order of keys / certificates matter? I've appended my chain certificates at the bottom of keys array in the flattened IdP metadata

tvdijen commented 1 year ago

Hi @marek-binkowski-sim !

(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)

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 config-directory. SimpleSAMLphp will pick up on that env variable and will find the metadata/ and cert/ directories relative of the config-directory.

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 openssl s_client -host <your-id-provider> -port 443 -showcerts and you should see something like an unknown issuer warning. It will also print the certificate chain that you need.

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

marek-binkowski-sim commented 1 year ago

Hi @tvdijen , apologies that it took so long, I must admit I was pretty confused for a while about what I was wrong about and how to do it better.

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 https://github.com/simplesamlphp/saml2/issues/309#issuecomment-1285504705

It limits the certificate verification to one level deep. In my case, the CA chain consists of 3 other certificates, so this setting $ctxOpts['ssl']['verify_depth'] = 1; was actually stopping the verification before it was complete. When I changed it to $ctxOpts['ssl']['verify_depth'] = 4; additionally to all my other steps, the authentication just started working!

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!

Logiar commented 10 months ago

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?

tvdijen commented 10 months ago

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.

marek-binkowski-sim commented 5 months ago

To sum up:

tvdijen commented 5 months ago

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.

marek-binkowski-sim commented 5 months ago

@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: do you know if there is a reason for statically setting verify_depth to 1 in SOAPClient?

tvdijen commented 5 months ago

I'm not aware of any reason. We could make it configurable.

postme commented 1 month ago

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 .

tvdijen commented 1 month ago

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.

postme commented 4 weeks ago

Hi @tvdijen I was able to resolve the issue differently, I will also sponsor the project, so contributing, but financially :-)