Skip to content
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
13 changes: 3 additions & 10 deletions src/client_side.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2358,16 +2358,9 @@ clientNegotiateSSL(int fd, void *data)
conn->clientConnection->tlsNegotiations()->retrieveNegotiatedInfo(session);

#if USE_OPENSSL
X509 *client_cert = SSL_get_peer_certificate(session.get());

if (client_cert) {
debugs(83, 3, "FD " << fd << " client certificate: subject: " <<
Security::SubjectName(*client_cert));

debugs(83, 3, "FD " << fd << " client certificate: issuer: " <<
Security::IssuerName(*client_cert));

X509_free(client_cert);
if (const auto clientCert = Security::CertPointer(SSL_get_peer_certificate(session.get()))) {
debugs(83, 3, "FD " << fd << " client certificate: subject: " << *clientCert);
debugs(83, 3, "FD " << fd << " client certificate: issuer: " << Security::IssuerName(*clientCert));
} else {
debugs(83, 5, "FD " << fd << " has no client certificate.");
}
Expand Down
14 changes: 14 additions & 0 deletions src/security/Certificate.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,20 @@
namespace Security
{

/// Content of a X.509 file before parsing.
#if USE_OPENSSL
using CertFileRawPointer = std::unique_ptr<BIO, HardFun<void, BIO*, &BIO_vfree>>;
#elif HAVE_LIBGNUTLS
inline void gnutls_datum_free(gnutls_datum_t *p) {
if (p && p->size > 0)
gnutls_free(p->data);
delete p;
}
using CertFileRawPointer = std::unique_ptr<gnutls_datum_t, HardFun<void, gnutls_datum_t*, &gnutls_datum_free>>;
#else
using CertFileRawPointer = std::unique_ptr<std::nullptr_t>;
#endif

/// The SubjectName field of the given certificate (if found) or an empty SBuf.
SBuf SubjectName(Certificate &);

Expand Down
1 change: 0 additions & 1 deletion src/security/ErrorDetail.cc
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@
#include "sbuf/Stream.h"
#include "security/Certificate.h"
#include "security/ErrorDetail.h"
#include "security/forward.h"
#include "security/Io.h"
#include "util.h"

Expand Down
195 changes: 116 additions & 79 deletions src/security/KeyData.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9,34 +9,65 @@
#include "squid.h"
#include "anyp/PortCfg.h"
#include "fatal.h"
#include "security/Certificate.h"
#include "sbuf/Stream.h"
#include "security/KeyData.h"
#include "SquidConfig.h"
#include "ssl/bio.h"
#include "ssl/gadgets.h"

#include <algorithm>

/// load the signing certificate and its chain, if any, from certFile
/// \return true if the signing certificate was obtained
/**
* Read certificate from file.
* See also: Ssl::ReadX509Certificate function, gadgets.cc file
*/
bool
Security::KeyData::loadCertificates()
{
debugs(83, 2, "from " << certFile);
cert.reset(); // paranoid: ensure cert is unset

// Open the .PEM certificate file
#if USE_OPENSSL
const char *certFilename = certFile.c_str();
Ssl::BIO_Pointer bio(BIO_new(BIO_s_file()));
if (!bio || !BIO_read_filename(bio.get(), certFilename)) {
CertFileRawPointer bio(BIO_new(BIO_s_file()));
if (!bio || !BIO_read_filename(bio.get(), certFile.c_str())) {
const auto x = ERR_get_error();
debugs(83, DBG_IMPORTANT, "ERROR: unable to load certificate file '" << certFile << "': " << ErrorString(x));
return false;
}
#elif HAVE_LIBGNUTLS
CertFileRawPointer bio(new gnutls_datum_t({.data=nullptr, .size=0}));
Security::LibErrorCode x = gnutls_load_file(certFile.c_str(), bio.get());
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_IMPORTANT, "ERROR: unable to load certificate file '" << certFile << "': " << ErrorString(x));
return false;
}
#else
CertFileRawPointer bio; // unused
#endif

// Read the client/server/proxy leaf certificate
try {
#if USE_OPENSSL
cert = Ssl::ReadCertificate(bio);
debugs(83, DBG_PARSE_NOTE(2), "Loaded signing certificate: " << *cert);
#elif HAVE_LIBGNUTLS
gnutls_pcert_st pcrt;
x = gnutls_pcert_import_x509_raw(&pcrt, bio.get(), GNUTLS_X509_FMT_PEM, 0);
if (x != GNUTLS_E_SUCCESS)
throw TextException(ToSBuf("failed to import certificate due to ", ErrorString(x)), Here());

gnutls_x509_crt_t certificate;
x = gnutls_pcert_export_x509(&pcrt, &certificate);
if (x != GNUTLS_E_SUCCESS)
throw TextException(ToSBuf("unable to X.509 convert certificate due to ", ErrorString(x)), Here());

if (certificate) {
cert = Security::CertPointer(certificate, [](gnutls_x509_crt_t p) {
debugs(83, 5, "gnutls_x509_crt_deinit cert=" << (void*)p);
gnutls_x509_crt_deinit(p);
});
}
#endif
Assure(cert != nullptr);
}
catch (...) {
// TODO: Convert the rest of this method to throw on errors instead.
Expand All @@ -46,90 +77,96 @@ Security::KeyData::loadCertificates()
}

try {
// Squid sends `cert` (loaded above) followed by certificates in `chain`
// (formed below by loading and sorting the remaining certificates).

// load all the remaining configured certificates
CertList candidates;
while (const auto c = Ssl::ReadOptionalCertificate(bio))
candidates.emplace_back(c);

// Push certificates into `chain` in on-the-wire order, as defined by
// RFC 8446 Section 4.4.2: "Each following certificate SHOULD directly
// certify the one immediately preceding it."
while (!candidates.empty()) {
const auto precedingCert = chain.empty() ? cert : chain.back();

// We cannot chain any certificate after a self-signed certificate.
// This check also protects the IssuedBy() search below from adding
// duplicated (i.e. listed multiple times) self-signed certificates.
if (SelfSigned(*precedingCert))
break;

const auto issuerPos = std::find_if(candidates.begin(), candidates.end(), [&](const CertPointer &i) {
return IssuedBy(*precedingCert, *i);
});
if (issuerPos == candidates.end())
break;

const auto &issuer = *issuerPos;
debugs(83, DBG_PARSE_NOTE(3), "Adding CA certificate: " << *issuer);
chain.emplace_back(issuer);
candidates.erase(issuerPos);
}

for (const auto &c: candidates)
debugs(83, DBG_IMPORTANT, "WARNING: Ignoring certificate that does not extend the chain: " << *c);
}
catch (...) {
if (SelfSigned(*cert))
debugs(83, DBG_PARSE_NOTE(2), "Certificate is self-signed, will not be chained: " << *cert);
else
loadX509ChainFromFile(bio);
} catch (...) {
// TODO: Reject configs with malformed intermediate certs instead.
debugs(83, DBG_IMPORTANT, "ERROR: Failure while loading intermediate certificate(s) from '" << certFile << "':" <<
Debug::Extra << "problem: " << CurrentException);
}

#elif HAVE_LIBGNUTLS
const char *certFilename = certFile.c_str();
gnutls_datum_t data;
Security::LibErrorCode x = gnutls_load_file(certFilename, &data);
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_IMPORTANT, "ERROR: unable to load certificate file '" << certFile << "': " << ErrorString(x));
return false;
}
return bool(cert);
}

gnutls_pcert_st pcrt;
x = gnutls_pcert_import_x509_raw(&pcrt, &data, GNUTLS_X509_FMT_PEM, 0);
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_IMPORTANT, "ERROR: unable to import certificate from '" << certFile << "': " << ErrorString(x));
return false;
}
gnutls_free(data.data);
/**
* Read certificate chain from BIO object.
* See also: Ssl::ReadX509Certificate function, gadgets.cc file
*/
void
Security::KeyData::loadX509ChainFromFile(CertFileRawPointer &bio)
{
if (!bio)
return;

gnutls_x509_crt_t certificate;
x = gnutls_pcert_export_x509(&pcrt, &certificate);
if (x != GNUTLS_E_SUCCESS) {
debugs(83, DBG_IMPORTANT, "ERROR: unable to X.509 convert certificate from '" << certFile << "': " << ErrorString(x));
return false;
}
debugs(83, DBG_PARSE_NOTE(2), "Using certificate chain in " << certFile);

if (certificate) {
cert = Security::CertPointer(certificate, [](gnutls_x509_crt_t p) {
#if USE_OPENSSL && !TLS_CHAIN_NO_SELFSIGNED
const bool dropSelfSigned = false;
#else
const bool dropSelfSigned = true;
#endif

debugs(83, DBG_PARSE_NOTE(3), "NOTICE: will " << (dropSelfSigned?"drop":"send") << " self-signed CA (if any) in the chain");

// Squid sends `cert` (loaded above) followed by certificates in `chain`
// (formed below by loading and sorting the remaining certificates).

// load all the remaining configured certificates
CertList candidates;
#if USE_OPENSSL
while (const auto c = Ssl::ReadOptionalCertificate(bio))
candidates.emplace_back(c);
#elif HAVE_LIBGNUTLS
unsigned int loadedCertCount = 0;
gnutls_x509_crt_t *loadedCerts = nullptr;
const auto x = gnutls_x509_crt_list_import2(&loadedCerts, &loadedCertCount, bio.get(), GNUTLS_X509_FMT_PEM, 0);
if (x != GNUTLS_E_SUCCESS)
throw TextException(ToSBuf(ErrorString(x)), Here());

for (unsigned int i = 0; i < loadedCertCount; ++i) {
const auto ca = CertPointer(loadedCerts[i], [](const gnutls_x509_crt_t p) {
debugs(83, 5, "gnutls_x509_crt_deinit cert=" << (void*)p);
gnutls_x509_crt_deinit(p);
});

// abuse fact that gnutls_x509_crt_t is actually a pointer in libgnutls
loadedCerts[i] = nullptr; // memory this points to is managed by 'ca' now
candidates.emplace_back(ca);
}
// certs in loadedCerts are now either part of the chain, or destroyed by 'ca'
// we just have to free the loadedCerts array itself.
gnutls_free(loadedCerts);
#endif

// XXX: implement chain loading
debugs(83, 2, "Loading certificate chain from PEM files not implemented in this Squid.");
// Push certificates into `chain` in on-the-wire order, as defined by
// RFC 8446 Section 4.4.2: "Each following certificate SHOULD directly
// certify the one immediately preceding it."
while (!candidates.empty()) {
const auto precedingCert = chain.empty() ? cert : chain.back();

#else
// do nothing.
#endif
// We cannot chain any certificate after a self-signed certificate.
// This check also protects the IssuedBy() search below from adding
// duplicated (i.e. listed multiple times) self-signed certificates.
if (dropSelfSigned && SelfSigned(*precedingCert))
break;

const auto issuerPos = std::find_if(candidates.begin(), candidates.end(), [&](const CertPointer &i) {
return IssuedBy(*precedingCert, *i);
});
if (issuerPos == candidates.end())
break;

if (!cert) {
debugs(83, DBG_IMPORTANT, "ERROR: unable to load certificate from '" << certFile << "'");
const auto &issuer = *issuerPos;
debugs(83, DBG_PARSE_NOTE(3), "Adding CA certificate: " << *issuer);
chain.emplace_back(issuer);
candidates.erase(issuerPos);
}

return bool(cert);
for (const auto &c: candidates) {
debugs(83, DBG_IMPORTANT, "WARNING: Ignoring certificate that does not extend the chain: " << *c);
}
}

/**
Expand All @@ -154,11 +191,11 @@ Security::KeyData::loadX509PrivateKeyFromFile()

#elif HAVE_LIBGNUTLS
const char *keyFilename = privateKeyFile.c_str();
gnutls_datum_t data;
if (gnutls_load_file(keyFilename, &data) == GNUTLS_E_SUCCESS) {
gnutls_datum_t rawFileContent;
if (gnutls_load_file(keyFilename, &rawFileContent) == GNUTLS_E_SUCCESS) {
gnutls_privkey_t key;
(void)gnutls_privkey_init(&key);
Security::ErrorCode x = gnutls_privkey_import_x509_raw(key, &data, GNUTLS_X509_FMT_PEM, nullptr, 0);
Security::ErrorCode x = gnutls_privkey_import_x509_raw(key, &rawFileContent, GNUTLS_X509_FMT_PEM, nullptr, 0);
if (x == GNUTLS_E_SUCCESS) {
gnutls_x509_privkey_t xkey;
gnutls_privkey_export_x509(key, &xkey);
Expand All @@ -169,7 +206,7 @@ Security::KeyData::loadX509PrivateKeyFromFile()
});
}
}
gnutls_free(data.data);
gnutls_free(rawFileContent.data);

#else
// nothing to do.
Expand Down
3 changes: 2 additions & 1 deletion src/security/KeyData.h
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

#include "anyp/forward.h"
#include "sbuf/SBuf.h"
#include "security/forward.h"
#include "security/Certificate.h"

namespace Security
{
Expand All @@ -37,6 +37,7 @@ class KeyData
private:
bool loadCertificates();
bool loadX509PrivateKeyFromFile();
void loadX509ChainFromFile(CertFileRawPointer &);
};

} // namespace Security
Expand Down
8 changes: 4 additions & 4 deletions src/security/PeerConnector.cc
Original file line number Diff line number Diff line change
Expand Up @@ -646,17 +646,17 @@ Security::PeerConnector::certDownloadingDone(DownloaderAnswer &downloaderAnswer)
// be able to accept collection of certificates.
// TODO: support collection of certificates
auto raw = reinterpret_cast<const unsigned char*>(downloaderAnswer.resource.rawContent());
if (auto cert = d2i_X509(nullptr, &raw, downloaderAnswer.resource.length())) {
if (auto cert = CertPointer(d2i_X509(nullptr, &raw, downloaderAnswer.resource.length()))) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file changes result from incremental review polishing. The original change around these was split off into an already merged PR without this pointer-safety action.
TODO: split this off also.

debugs(81, 5, "Retrieved certificate: " << *cert);

if (!downloadedCerts)
downloadedCerts.reset(sk_X509_new_null());
sk_X509_push(downloadedCerts.get(), cert);
sk_X509_push(downloadedCerts.get(), cert.get());

const auto ctx = peerContext()->raw;
const auto certsList = SSL_get_peer_cert_chain(&sconn);
if (!Ssl::findIssuerCertificate(cert, certsList, ctx)) {
if (const auto issuerUri = Ssl::findIssuerUri(cert)) {
if (!Ssl::findIssuerCertificate(cert.get(), certsList, ctx)) {
if (const auto issuerUri = Ssl::findIssuerUri(cert.get())) {
debugs(81, 5, "certificate " << *cert <<
" points to its missing issuer certificate at " << issuerUri);
urlsOfMissingCerts.push(SBuf(issuerUri));
Expand Down
18 changes: 14 additions & 4 deletions src/security/ServerOptions.cc
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &)
return false;
}

for (auto cert : keys.chain) {
for (const auto &cert : keys.chain) {
if (SSL_CTX_add_extra_chain_cert(t.get(), cert.get())) {
// increase the certificate lock
X509_up_ref(cert.get());
Expand All @@ -251,9 +251,20 @@ Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &)

#elif HAVE_LIBGNUTLS
for (auto &keys : certs) {
gnutls_x509_crt_t crt = keys.cert.get();
gnutls_x509_privkey_t xkey = keys.pkey.get();
const auto x = gnutls_certificate_set_x509_key(t.get(), &crt, 1, xkey);

const auto certCount = 1 + keys.chain.size();
auto crt = new gnutls_x509_crt_t[certCount];
crt[0] = keys.cert.get();
if (certCount > 1) {
int i = 1;
for (const auto &cert : keys.chain) {
crt[i++] = cert.get(); // gnutls_x509_crt_t is a raw-pointer
}
}

const auto x = gnutls_certificate_set_x509_key(t.get(), crt, certCount, xkey);
delete[] crt;
Comment on lines +256 to +267

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please refactor to use std::vector instead of raw new/delete. Use std::vector::data() to pass the raw array to gnutls_certificate_set_x509_key(). I expect certCount to be gone after this refactoring.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not seem right to me. It assumes that std::vector is always a single consecutive memory allocation - AFAIK that is not a guarantee from std::vector, just a common compiler choice.

std::array does document that guarantee officially, so I will look into switching to that.

if (x != GNUTLS_E_SUCCESS) {
SBuf whichFile = keys.certFile;
if (keys.certFile != keys.privateKeyFile) {
Expand All @@ -263,7 +274,6 @@ Security::ServerOptions::createStaticServerContext(AnyP::PortCfg &)
debugs(83, DBG_CRITICAL, "ERROR: Failed to initialize server context with keys from " << whichFile << ": " << Security::ErrorString(x));
return false;
}
// XXX: add cert chain to the context
}
#endif

Expand Down
Loading
Loading