Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pki::RequestCertificate handler: renew cert if it's not suitable for handshake #10328

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 24 additions & 6 deletions lib/base/tlsstream.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,24 @@ bool UnbufferedAsioTlsStream::IsVerifyOK()
return SSL_get_verify_result(native_handle()) == X509_V_OK;
}

/**
* Returns an error code for situations where IsVerifyOK() returns false.
*
* @return See SSL_get_verify_result(3)
*/
long UnbufferedAsioTlsStream::GetVerifyErrorCode()
{
if (!SSL_is_init_finished(native_handle())) {
return HandshakeNotCompleted;
}

if (GetPeerCertificate() == nullptr) {
return NoPeerCertificate;
}

return SSL_get_verify_result(native_handle());
}

/**
* Returns a human-readable error string for situations where IsVerifyOK() returns false.
*
Expand All @@ -50,16 +68,16 @@ bool UnbufferedAsioTlsStream::IsVerifyOK()
*/
String UnbufferedAsioTlsStream::GetVerifyError()
{
if (!SSL_is_init_finished(native_handle())) {
return "handshake not completed";
}
auto err (GetVerifyErrorCode());

if (GetPeerCertificate() == nullptr) {
return "no peer certificate provided";
switch (err) {
case HandshakeNotCompleted:
return "handshake not completed";
case NoPeerCertificate:
return "no peer certificate provided";
}

std::ostringstream buf;
long err = SSL_get_verify_result(native_handle());
buf << "code " << err << ": " << X509_verify_cert_error_string(err);
return buf.str();
}
Expand Down
6 changes: 6 additions & 0 deletions lib/base/tlsstream.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -68,13 +68,19 @@ typedef SeenStream<boost::asio::ssl::stream<boost::asio::ip::tcp::socket>> AsioT
class UnbufferedAsioTlsStream : public AsioTcpTlsStream
{
public:
// SSL_get_verify_result() doesn't return negative values YET, but it might in the future.
// Better safe with a margin than sorry with a collision.
static constexpr long HandshakeNotCompleted = -42;
static constexpr long NoPeerCertificate = -43;

inline
UnbufferedAsioTlsStream(UnbufferedAsioTlsStreamParams& init)
: AsioTcpTlsStream(init.IoContext, init.SslContext), m_Hostname(init.Hostname)
{
}

bool IsVerifyOK();
long GetVerifyErrorCode();
String GetVerifyError();
std::shared_ptr<X509> GetPeerCertificate();

Expand Down
37 changes: 37 additions & 0 deletions lib/remote/jsonrpcconnection-pki.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@
#include <openssl/asn1.h>
#include <openssl/ssl.h>
#include <openssl/x509.h>
#include <openssl/x509_vfy.h>
#include <time.h>

using namespace icinga;

Expand Down Expand Up @@ -80,6 +82,8 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona

std::shared_ptr<X509> parsedRequestorCA;
X509* requestorCA = nullptr;
constexpr long handshakeVerifyErrorUnset = X509_V_OK;
long handshakeVerifyError = handshakeVerifyErrorUnset;

if (signedByCA) {
bool uptodate = IsCertUptodate(cert);
Expand Down Expand Up @@ -121,6 +125,35 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona
}
}

if (uptodate) {
// There's a bug causing malformed certificates to be issued sometimes.
// The handshake chain verification fails with X509_V_ERR_CERT_SIGNATURE_FAILURE, but (luckily)
// the certificate sent via pki::RequestCertificate gets properly recognized by VerifyCertificate().
// The latter allows to self-heal the cluster by issuing a new certificate,
// despite the old one being "valid and uptodate".

if (cn == origin->FromClient->GetIdentity()) {
handshakeVerifyError = tlsConn.GetVerifyErrorCode();
} else {
Value hve;

if (params->Get("handshake_verify_error", &hve) && hve.IsNumber()) {
handshakeVerifyError = hve;
}
}

if (handshakeVerifyError == X509_V_ERR_CERT_SIGNATURE_FAILURE) {
// In the hypothetic case of a permanent failure to issue sane certificates,
// don't issue a new one on every re-connect:

time_t threshold = time(nullptr) - 10 * 60;

if (X509_cmp_time(X509_get_notBefore(cert.get()), &threshold) < 0) {
uptodate = false;
}
}
}

if (uptodate) {
Log(LogInformation, "JsonRpcConnection")
<< "The certificates for CN '" << cn << "' and its root CA are valid and uptodate. Skipping automated renewal.";
Expand Down Expand Up @@ -276,6 +309,10 @@ Value RequestCertificateHandler(const MessageOrigin::Ptr& origin, const Dictiona
request->Set("requestor_ca", CertificateToString(requestorCA));
}

if (handshakeVerifyError != handshakeVerifyErrorUnset) {
request->Set("handshake_verify_error", handshakeVerifyError);
}

Utility::SaveJsonFile(requestPath, 0600, request);

JsonRpcConnection::SendCertificateRequest(nullptr, origin, requestPath);
Expand Down
Loading