Skip to content
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

Open
marek-binkowski-sim opened this issue Sep 27, 2022 · 15 comments
Open

Comments

@marek-binkowski-sim
Copy link

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
Copy link
Member

tvdijen commented Sep 27, 2022

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
Copy link
Author

marek-binkowski-sim commented Sep 30, 2022

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
Copy link
Member

tvdijen commented Sep 30, 2022

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

@marek-binkowski-sim
Copy link
Author

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
Copy link
Author

marek-binkowski-sim commented Oct 4, 2022

@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
Copy link
Member

tvdijen commented Oct 4, 2022

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
Copy link
Author

marek-binkowski-sim commented Oct 20, 2022

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
Copy link
Member

tvdijen commented Oct 20, 2022

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
Copy link
Author

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 #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
$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
Copy link

Logiar commented Nov 17, 2023

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
Copy link
Member

tvdijen commented Nov 17, 2023

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
Copy link
Author

marek-binkowski-sim commented May 9, 2024

To sum up:

  • Thanks to commit 6c48095, since version 4.6.4 it is possible to configure your own CA chain to verify peer certificate. You can configure the CA chain in a metadata/saml20-idp-remote.php configuration for your IdP. (You need to point at the directory containing that file in metadata.sources in your config.php). This saml20-idp-remote.php file would contain the processed metadata for every remote IdP you use in your system, and in the 'keys' array of a specific IdP you can place the CA chain of certificates to be used for peer certificate verification.
  • one thing which still is not resolved is a situation, when your CA chain contains more than one certificate (not only a root certificate, but also one or more intermediary certificates). In the SOAPClient the verify_depth parameter is statically set to 1 (also in version 5: https://github.com/simplesamlphp/saml2/blob/v5.0.0-alpha.15/src/SAML2/SOAPClient.php#L114). It means that only the simplest scenarios without intermediary certificates is allowed. In my case intermediary certificate is involved, so I need to manually modify SOAPClient code, to make it configurable by a parameter in the same saml20-idp-remote.php file mentioned above. It would be nicer if I didn't have to modify SOAPClient code. @tvdijen do you know if there is a reason for statically setting verify_depth to 1 in SOAPClient?

@tvdijen
Copy link
Member

tvdijen commented May 9, 2024

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
Copy link
Author

marek-binkowski-sim commented May 9, 2024

@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
Copy link
Member

tvdijen commented May 9, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants