sawolf
commented
2 years ago This change looks fine to me - for anyone reading this in the future, the OpenSSL documentation indicates that SSL_CTX_use_certificate_chain_file() allows us to load certificates as before, but saves us the effort of calling SSL_CTX_add_extra_chain_cert(3) if we did want to create a complete certificate chain. I don't expect any situation where the new function fails where the old one would have succeeded.
To your other concerns:
- Your commit message is fine. That contributing document was written by one engineer working on a different project, then copied-and-pasted over by an intern. It's not an actual company policy.
- I agree that issues are unlikely. The change is compatible with our example documentation for enabling SSL in NRPE. If I do hear about backward-compatibility issues that are linked back to this change, implementing an extra option doesn't seem that hard.
This is required for public certificates as many servers will be unable to verify the certificate if a partial chain is presented. Changing
SSL_CTX_use_certificate_filetoSSL_CTX_use_certificate_chain_filefixes this issue since now the certificate and intermediate can be loaded from the same file. This reflects behaviour of other software including popular webservers. Furthermore the use ofSSL_CTX_use_certificate_chain_fileis suggested by the corresponding OpenSSL man page.First off, the above is the commit message of the change, however CONTRIBUTING.md states that I should "Keep commit messages as concise as possible". I did assume this applies to the first line which is usually displayed, and I also didn't keep to the 50/72 character limit from second line onward, if any of these are an issue I'm happy to edit the commit message right away. Same applies to the THANKS file, if the change is too small to be considered enough for that addition I'll gladly remove it. The mentioned CHANGES does not exist anymore, shall I add the change to a potential 3.3.1 release in CHANGELOG.md?
One implementation detail by the way; while it wouldn't exactly reflect industry best-practice if I dare say so, it would be less likely to cause any issues, though while issues due to this change are possible they are unlikely, to introduce another option for an intermediate certificate. Would that be a better option?
Onward to the matter at hand, to give some further context, although that's largely irrelevant, feel free to skip reading the rest. I'm running IPv6-only infrastructure with public certificates for every machine. As NRPE doesn't have a switch for validating the certificate subject when using client certificate authentication I had to use stunnel for the server side (which allows multiple checkHost instructions). Considering I'd have to use another stunnel on the serverside (Nagios server) for every client I thought I could get away with it if I just handed
check_nrpeits client cert, and surprisingly that works just fine, except for the missing intermediates. With this patch an instance of check_nrpe with client certificate and key can perform a full handshake against an equally equipped stunnel server instance, which then forwards the (plaintext) connection to an NRPE running with-n, the latter unaware of the TLS context, and the former happy with the handshake will proceed to run (and pass) the NRPE check in question.