[757] https tls cert chains #758
Conversation
|
It it possible to add tests for it? |
tests for what? |
It makes sense from the point of documentation. OpenSSL is overcomplicated, especially for the beginners. A test with some hints on how to create tls chains and how the file is named would be helpful. It also makes sense as a smoke test - the openssl function may require some other functions to be called before setting the chain... a test would prevent breaking the functionality during refactorings. Please, add a smoke test to https://github.com/userver-framework/userver/blob/develop/core/src/engine/io/tls_wrapper_test.cpp |
aklyuchev
changed the title
There was a problem hiding this comment.
it took some time for me to realize the drawback of this PR. SSL_CTX_use_certificate_chain_file does blocking IO operations (reads data from disk). We try hard to do the blocking operations not in the main task processor for CPU bound tasks. So the file reading should not be done in TlsWrapper::StartTlsServer.
It looks like the certificates could be extracted from chain file https://unix.stackexchange.com/questions/368123/how-to-extract-the-root-ca-and-subordinate-ca-from-a-certificate-chain-in-linux . After that the certificates could be passed via cert, key and extra_cert_authorities parameters of the TlsWrapper::StartTlsServer function.
The PR could be changed to do the following:
- Make a function
crypto::blocking::LoadFromChainFilethat returns crypto::Certificate's - Call that function in core/src/server/net/listener_config.cpp and the certificates would make their way to core/src/server/net/listener_impl.cpp
aklyuchev
force-pushed
the
us757-https-tls-chains_http_cert
branch
from
eba1ffc to
0b6e98c
Compare
| @@ -63,7 +63,7 @@ namespace components { | |||
| /// task_processor | task processor to process incoming requests | - | |||
| /// backlog | max count of new connections pending acceptance | 1024 | |||
| /// tls.ca | paths to TLS CAs for client authentication | - | |||
| /// tls.cert | path to TLS server certificate | - | |||
| /// tls.cert | path to TLS server certificate chain | - | |||
There was a problem hiding this comment.
How about path to TLS server certificate or certificate chain
| @@ -44,6 +45,15 @@ class Certificate { | |||
| std::shared_ptr<NativeType> cert_; | |||
| }; | |||
|
|
|||
| using CertificatesChain = std::list<Certificate>; | |||
There was a problem hiding this comment.
number of certificates in chain is not known in advance
universal/src/crypto/certificate.cpp
Outdated
| CertificatesChain LoadCertficatesChainFromString(std::string_view chain) | ||
| { | ||
| CertificatesChain certificates; | ||
| const std::string beginMarker = "-----BEGIN CERTIFICATE-----"; |
universal/src/crypto/certificate.cpp
Outdated
| { | ||
| CertificatesChain certificates; | ||
| const std::string beginMarker = "-----BEGIN CERTIFICATE-----"; | ||
| const std::string endMarker = "-----END CERTIFICATE-----"; |
| /// list of 'Certificate's. | ||
| /// | ||
| /// @throw crypto::KeyParseError if failed to load the certificate. | ||
| CertificatesChain LoadCertficatesChainFromString(std::string_view certificatesChain); |
There was a problem hiding this comment.
std::string_view chain to suppress annoying warnings about missmatch of parameter names
|
addressed comments and created for another MR for sure |
|
@aklyuchev many thanks for the PR! It is a good thing that you've added the test. Our debug build noted an issue with certificate ownership, |
Note: by creating a PR or an issue you automatically agree to the CLA. See CONTRIBUTING.md. Feel free to remove this note, the agreement holds.