Accept additional client certificates

[RemcodM]

We have a use case in which we want to use Excon to connect to a remote TLS server using client certificates part of a longer certificate chain. To make this a valid request, we not only need to send our own certificate, but also all intermediary certificates up to the root certificate.

Excon didn't provide us with the option to pass additional client certificates with the connection. This PR introduces this functionality, exposed as the client_chain (and client_chain_data) option. This option accepts one or more extra certificates that are passed to the SSL context. It tries to use OpenSSL::X509::Certificate#load to load the additional certificates when available (Ruby 3+). For older versions of Ruby, the certificate is split and converted to one or more OpenSSL::X509::Certificates using OpenSSL::X509::Certificate#new.

Hopefully this functionality can be merged into excon. Please let me know if any changes are needed before this functionality can be added.

[geemus]

@RemcodM - this looks pretty good overall. Unfortunately, I have a sick kid at home again today so there may be a little delay before I can really dig in and review in greater detail. I'll try to get to it later this week though, or update you if it would be longer. Thanks for the help, looking forward to working through this with you and getting it added.

[geemus]

Hey, thanks again for these changes and your patience.

Overall I think this looks great and it will be good to have this capability available. My only ask at this time would be if you could squash the README reversion in with the original commit to help us keep the commit history a bit cleaner. I think once that is done it should be good to bring in and I can work on a release.

Thanks!

[geemus]

@RemcodM - It's not a blocker to this merge, but I did wonder if you could share your opinion on some of the interface (I was thinking about it while looking at this). The way this is implemented is certainly inline with the existing implementation, but I wonder if it might make sense with some of these things to more directly pass values through, rather than doing in-between stuff. ie in this case, should we think about eventually providing a way to pass a value more directly to extra_chain_certs, and leave getting the right stuff in the right format to the end user? I could imagine this might simplify things a bit in terms of maintenance, but at the cost of making it a bit harder for end users (since they would need to know more about formats/context/etc). That being said, this would also allow existing documentation around those methods to be true for us too, instead of needing to document how we do it differently. The existing documentation for that stuff is also usually not good though. So maybe we should do exactly what we are doing here (or do this to make it easier for folks, but also provide the more direct methods for flexibility and expert usage). Just kind of thinking aloud here about longer term what we might want to do. I'd love your thoughts since you have been thinking and working around this already recently. Thanks!

[RemcodM]

@geemus First of all, thank you for reviewing this PR. We're happy that we can get this functionality inside upstream Excon instead of using our own fork.

I think it is quite a difficult question to answer what the interface should look like long term. As you already said, the documentation of the OpenSSL/SSLContext interfaces is also lacking on many fronts. So directly providing access to that interface will not clear up things for users. On the other hand, having a completely different interface in Excon will indeed complicate maintenance, especially since the properties Excon is using now, cert, key, and extra_chain_cert, are deprecated in favor of add_certificate.

However, one might argue that the interface that Excon is providing at this time is confusing as well. Mostly because of the deprecated options but also the different ways to pass attributes (e.g. as file with client_chain or as data with client_chain_file). So I think the truth of what Excon should provide vs. what the users provide should be somewhere in between. Maybe it should be the responsibility of the user to read the file, such that Excon only has to provide options for passing certificate data. However, maybe I am slightly biased in this case, since when we use Excon inside Ruby code, we do always pass the certificates as data, never as a file.

Although providing direct access to certain interfaces as kind of an 'expert' mode sounds nice, it also has its own disadvantages. Having Excon provide its own interface does also mean we can trust Excon to ensure changes to OpenSSL/Ruby are handled correctly. With an expert interface, you are moving much of the responsibility to the users. That might be compelling from a maintenance standpoint, but might also scare away users to alternatives that provide a more abstract interface that they can trust to work long-term.

It is a very interesting discussion though. I do not think there is one good answer or standpoint. It all depends on what Excon wants to offer the community I guess.

[geemus]

@RemcodM - thanks for your thoughts. I think we kind of organically added things as they were needed, which is how we ended up with the kind of mix of things you see today. Seeing it all laid out again working on this made me feel like it might be good to think about it more holistically. I think we are largely in alignment on the challenges hand potential problems.

I think overall that it seems like a good idea overall to provide some helpers that make using things easier here, rather than just using the underlying calls directly. Especially since the underlying calls are not particularly well documented and can be a bit confusing and fiddly. That being said, I really like the suggestion you had of basically picking ONE way for the helpers to work and trying to be consistent. To me that seems like making sure the key names are a bit more consistent as well as just handling files OR data (not both). Any change like that would of course involve deprecation and backwards compatibility issues though, so it may be a bit of a pain to get from here to there. I've created an issue to track these ideas here: #813

The other thing we may want to think about, distinct from the interface, is about moving our implementation to use add_certificate under the covers instead of the mix of different methods. I've added an issue as a reminder of this here: #812

[geemus]

Thanks for the fix, the squash, and your thoughts on the future interface!

[geemus]

Released in v0.98.0.

[geemus]

@RemcodM - I bumped into an issue related to this today and wondered if you could help.

First, some context. I try to keep an updated copy of the curl certs in the store as a fallback of last resort so people will have a good foundation even on systems where there isn't one (this is for windows predominantly, if I recall). As such, I get reminders when the curl cert changes to update and release.

When we added self-signed certs, I updated this task to also include creating new self-signed certs as well. I realize we could defer on this, but it helps me ensure that their expiry dates get pushed further out on a regular basis, so I kind of like it in that regard.

Unfortunately, the code that updates those certs doesn't update the new cert and chain that were created here, which appears to create test failures. I'd like to update the relevant script to also update these, could you help me figure out the correct invocations?

The key regeneration code I'm talking about is here:

excon/Rakefile

Lines 52 to 54 in 739e5ff

	# update self-signed certs for tests
	sh "openssl req -subj '/CN=excon/O=excon' -new -newkey rsa:2048 -sha256 -days 3650 -nodes -x509 -keyout tests/data/excon.cert.key -out tests/data/excon.cert.crt"
	sh "openssl req -subj '/CN=127.0.0.1/O=excon' -new -newkey rsa:2048 -sha256 -days 3650 -nodes -x509 -keyout tests/data/127.0.0.1.cert.key -out tests/data/127.0.0.1.cert.crt"

And you can see the related failures here, which come out of the new tests you added in this PR: https://github.com/excon/excon/actions/runs/5190608598/jobs/9357280363?pr=823

Let me know if you have any questions, it's not super urgent (given the nearly 10 years before these actually expire), but it would be good to fix up while we have it in mind).