From 9932a617d28558be8d17353477538d7894a75f0b Mon Sep 17 00:00:00 2001 From: Matthias Hensler Date: Thu, 12 May 2022 17:11:16 +0200 Subject: [PATCH] try to allow certificate-chains Currently the verification of certificates uses only the first pem in the ca-file and the first pem in the certificate-file. This breaks if an intermediate certificate is needed. A simple workaround is to put the full chain into the ca-file and give the ca-file instead of the X509-structure to the VerifyCertificate() methode. There we can just do the usual business but add the full ca-file again to OpenSSLs SSL_CTX_load_verify_locations(). While this seems a little bit hackish it should at least allow the proper verification of a certificate chain without introducing any security implications for setups with just a single root-ca. The only downside currently: while the CLI "pki verify" will correctly check the supplied parameters, it still only shows the topmost certificate from the ca-file (which I guess is fine for the moment). also process certificate chains presented by the client To make this work the icinga2-master only holds to root-ca in its local ca.crt, while the icinga2-agent has the intermediate-cert in its local ca.crt (or the intermediate together with the root in the ca.crt / or the intermediate in the cert.pem - doesn't matter). --- lib/base/tlsstream.cpp | 5 +++++ lib/base/tlsstream.hpp | 1 + lib/base/tlsutility.cpp | 8 ++++++-- lib/base/tlsutility.hpp | 2 +- lib/cli/pkiverifycommand.cpp | 2 +- lib/remote/apilistener.cpp | 4 ++-- lib/remote/jsonrpcconnection-pki.cpp | 5 ++++- 7 files changed, 20 insertions(+), 7 deletions(-) diff --git a/lib/base/tlsstream.cpp b/lib/base/tlsstream.cpp index db54c919ed5..98400a93bbc 100644 --- a/lib/base/tlsstream.cpp +++ b/lib/base/tlsstream.cpp @@ -33,6 +33,11 @@ std::shared_ptr UnbufferedAsioTlsStream::GetPeerCertificate() return std::shared_ptr(SSL_get_peer_certificate(native_handle()), X509_free); } +STACK_OF(X509) *UnbufferedAsioTlsStream::GetPeerCertificateChain() +{ + return SSL_get_peer_cert_chain(native_handle()); +} + void UnbufferedAsioTlsStream::BeforeHandshake(handshake_type type) { namespace ssl = boost::asio::ssl; diff --git a/lib/base/tlsstream.hpp b/lib/base/tlsstream.hpp index f6e52097e11..c8b99b0dab0 100644 --- a/lib/base/tlsstream.hpp +++ b/lib/base/tlsstream.hpp @@ -77,6 +77,7 @@ class UnbufferedAsioTlsStream : public AsioTcpTlsStream bool IsVerifyOK() const; String GetVerifyError() const; std::shared_ptr GetPeerCertificate(); + STACK_OF(X509) *GetPeerCertificateChain(); template inline diff --git a/lib/base/tlsutility.cpp b/lib/base/tlsutility.cpp index 7e2193c8939..5263fd0db7b 100644 --- a/lib/base/tlsutility.cpp +++ b/lib/base/tlsutility.cpp @@ -948,8 +948,10 @@ String BinaryToHex(const unsigned char* data, size_t length) { return output; } -bool VerifyCertificate(const std::shared_ptr &caCertificate, const std::shared_ptr &certificate, const String& crlFile) +bool VerifyCertificate(const String& caFile, const std::shared_ptr &certificate, const String& crlFile, STACK_OF(X509) *chain) { + std::shared_ptr caCertificate = GetX509Certificate(caFile); + X509_STORE *store = X509_STORE_new(); if (!store) @@ -961,8 +963,10 @@ bool VerifyCertificate(const std::shared_ptr &caCertificate, const std::sh AddCRLToSSLContext(store, crlFile); } + X509_STORE_load_locations(store, caFile.CStr(), NULL); /* ignore any errors for the moment, since this is just the convenient way to add full chain */ + X509_STORE_CTX *csc = X509_STORE_CTX_new(); - X509_STORE_CTX_init(csc, store, certificate.get(), nullptr); + X509_STORE_CTX_init(csc, store, certificate.get(), chain); int rc = X509_verify_cert(csc); diff --git a/lib/base/tlsutility.hpp b/lib/base/tlsutility.hpp index 3b8a89c5ecf..4596708d138 100644 --- a/lib/base/tlsutility.hpp +++ b/lib/base/tlsutility.hpp @@ -69,7 +69,7 @@ String SHA256(const String& s); String RandomString(int length); String BinaryToHex(const unsigned char* data, size_t length); -bool VerifyCertificate(const std::shared_ptr& caCertificate, const std::shared_ptr& certificate, const String& crlFile); +bool VerifyCertificate(const String& caFile, const std::shared_ptr& certificate, const String& crlFile, STACK_OF(X509) *chain); bool IsCa(const std::shared_ptr& cacert); int GetCertificateVersion(const std::shared_ptr& cert); String GetSignatureAlgorithm(const std::shared_ptr& cert); diff --git a/lib/cli/pkiverifycommand.cpp b/lib/cli/pkiverifycommand.cpp index 963903a2114..0ac16b6b1ed 100644 --- a/lib/cli/pkiverifycommand.cpp +++ b/lib/cli/pkiverifycommand.cpp @@ -130,7 +130,7 @@ int PKIVerifyCommand::Run(const boost::program_options::variables_map& vm, const bool signedByCA; try { - signedByCA = VerifyCertificate(cacert, cert, crlFile); + signedByCA = VerifyCertificate(caCertFile, cert, crlFile, nullptr); } catch (const std::exception& ex) { Log logmsg (LogCritical, "cli"); logmsg << "CRITICAL: Certificate with CN '" << certCN << "' is NOT signed by CA: "; diff --git a/lib/remote/apilistener.cpp b/lib/remote/apilistener.cpp index 616c7422b66..0b8ef8aec69 100644 --- a/lib/remote/apilistener.cpp +++ b/lib/remote/apilistener.cpp @@ -185,7 +185,7 @@ std::shared_ptr ApiListener::RenewCert(const std::shared_ptr& cert) { std::shared_ptr pubkey (X509_get_pubkey(cert.get()), EVP_PKEY_free); auto subject (X509_get_subject_name(cert.get())); - auto cacert (GetX509Certificate(GetDefaultCaPath())); + auto cacert (GetDefaultCaPath()); auto newcert (CreateCertIcingaCA(pubkey.get(), subject)); /* verify that the new cert matches the CA we're using for the ApiListener; @@ -193,7 +193,7 @@ std::shared_ptr ApiListener::RenewCert(const std::shared_ptr& cert) * we're using for cluster connections (there's no point in sending a client * a certificate it wouldn't be able to use to connect to us anyway) */ try { - if (!VerifyCertificate(cacert, newcert, GetCrlPath())) { + if (!VerifyCertificate(cacert, newcert, GetCrlPath(), nullptr)) { Log(LogWarning, "ApiListener") << "The CA in '" << GetDefaultCaPath() << "' does not match the CA which Icinga uses " << "for its own cluster connections. This is most likely a configuration problem."; diff --git a/lib/remote/jsonrpcconnection-pki.cpp b/lib/remote/jsonrpcconnection-pki.cpp index d2b727b674f..ce87fc2db73 100644 --- a/lib/remote/jsonrpcconnection-pki.cpp +++ b/lib/remote/jsonrpcconnection-pki.cpp @@ -29,6 +29,7 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona String certText = params->Get("cert_request"); std::shared_ptr cert; + STACK_OF(X509) *chain; Dictionary::Ptr result = new Dictionary(); @@ -36,8 +37,10 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona if (certText.IsEmpty()) { auto stream (origin->FromClient->GetStream()); cert = stream->next_layer().GetPeerCertificate(); + chain = stream->next_layer().GetPeerCertificateChain(); } else { cert = StringToCertificate(certText); + chain = nullptr; } if (!cert) { @@ -61,7 +64,7 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona logmsg << "Received certificate request for CN '" << cn << "'"; try { - signedByCA = VerifyCertificate(cacert, cert, listener->GetCrlPath()); + signedByCA = VerifyCertificate(listener->GetDefaultCaPath(), cert, listener->GetCrlPath(), chain); if (!signedByCA) { logmsg << " not"; }