Use system OpenSSL cert file and dir if available

[stanhu]

For users that manage their own SSL certs, it is a surprise that excon
ships its own root certificates. As a result, using an old version of
excon can cause SSL failures with third-party services, such as S3
(#473).

We now fall back to the OpenSSL defaults if available to avoid depending
on the shipped cacert.pem.

[stanhu]

Tests will be fixed with #771.

[stanhu]

Hmm, maybe the set_default_paths handles this already. I wonder if the add_file call in

excon/lib/excon/ssl_socket.rb

Line 67 in 0ac208d

ssl_context.cert_store.add_file(ca_file)

may cause issues in validation. With the recent LetsEncrypt root expiry, removal of the expired cert was important for validation to work properly: https://community.letsencrypt.org/t/production-chain-changes/150739

[geemus]

@stanhu Hey, apologies for any surprise/confusion around this. I'll do my best to share my recollection of the status of this and hopefully we can get to the bottom of it.

My understanding was that the set_default_paths call would establish a path that included the openssl cert. And the bundled cert would then be added at the end of the path, which would mean that it should only ever be used if the default paths do not include one already.

So my expectation is that it would use OpenSSL instead of the bundled if it exists. If I recall correctly we set it up this way because there were certain environments/installations where OpenSSL certs were not present and we wanted it to still work (I want to say this was a Windows thing, but it's been a long time now). It definitely wasn't supposed to be instead of OpenSSL, but just to make sure that it would "just work" in other cases.

So, if that isn't the behavior we are actually seeing, we should change it.

I suspect that because of the way we are doing path and the particular case (removal of an expired), it might mean that after failing to be found in the OpenSSL group it would try the next thing in the path (the bundled) which might then fail? Does that make sense and/or sound likely? Maybe we should change it to check in some way if set_default_paths added something (in which case no action is needed), vs nothing being added and needing to add the bundled one in. I think that would still allow things to "just work" in the original use case while avoiding the surprising edge case you ran into. What do you think?

[stanhu]

@geemus Makes sense. I think I'll close this. I think it threw me off that set_default_paths is called, and then potentially an old list of certs get added to it.

In https://gitlab.com/gitlab-org/gitlab/-/issues/342326#note_694301994, we saw that if you called set_default_paths and called add_file without parsing individual certs, validation wouldn't work quite right, at least when it came to the LetsEncrypt root certificate.

https://gitlab.com/gitlab-org/gitlab/-/merge_requests/71697/diffs fixed the problem for us.

I suspect the add_file isn't doing what you think it is doing, and perhaps only adding the first certificate in cacert.pem.

[geemus]

@stanhu Got it, thanks for the update. We could certainly add better documentation/explanation around this for our future selves and others. Otherwise I'm certainly open to improving the usage here if we need to. I looked over the issue/diffs you mentioned but fear I don't have enough context/memory of things to immediately grok what we would need to change. So let me know if you think action is required and if you have advice and I can try to find some time to dig in. Otherwise it sounds like maybe it wasn't as much of a problem as you originally expected and maybe we can let it be for now?