Simplify mutual TLS checks
bmcweb should be using the openssl primitives for these checks. There
are examples where we've known to have gotten the behavior incorrect, so
given that OpenSSL clearly should know these things better than we do,
use it.
Tested: unit tests pass.
Change-Id: I0bcd381a9e3c9a1e8e6dc39534e81fa698570689
Signed-off-by: Ed Tanous <ed@tanous.net>
diff --git a/http/mutual_tls.hpp b/http/mutual_tls.hpp
index 9cd4cde..64bcd49 100644
--- a/http/mutual_tls.hpp
+++ b/http/mutual_tls.hpp
@@ -47,10 +47,10 @@
BMCWEB_LOG_INFO("Last TLS error is: {}", ctxError);
return nullptr;
}
+
// Check that we have reached final certificate in chain
int32_t depth = X509_STORE_CTX_get_error_depth(cts);
if (depth != 0)
-
{
BMCWEB_LOG_DEBUG(
"Certificate verification in progress (depth {}), waiting to reach final depth",
@@ -60,73 +60,13 @@
BMCWEB_LOG_DEBUG("Certificate verification of final depth");
- // Verify KeyUsage
- bool isKeyUsageDigitalSignature = false;
- bool isKeyUsageKeyAgreement = false;
-
- ASN1_BIT_STRING* usage = static_cast<ASN1_BIT_STRING*>(
- X509_get_ext_d2i(peerCert, NID_key_usage, nullptr, nullptr));
-
- if ((usage == nullptr) || (usage->data == nullptr))
+ if (X509_check_purpose(peerCert, X509_PURPOSE_SSL_CLIENT, 0) != 1)
{
- BMCWEB_LOG_DEBUG("TLS usage is null");
+ BMCWEB_LOG_DEBUG(
+ "Chain does not allow certificate to be used for SSL client authentication");
return nullptr;
}
- for (auto usageChar :
- std::span(usage->data, static_cast<size_t>(usage->length)))
- {
- if (KU_DIGITAL_SIGNATURE & usageChar)
- {
- isKeyUsageDigitalSignature = true;
- }
- if (KU_KEY_AGREEMENT & usageChar)
- {
- isKeyUsageKeyAgreement = true;
- }
- }
- ASN1_BIT_STRING_free(usage);
-
- if (!isKeyUsageDigitalSignature || !isKeyUsageKeyAgreement)
- {
- BMCWEB_LOG_DEBUG("Certificate ExtendedKeyUsage does "
- "not allow provided certificate to "
- "be used for user authentication");
- return nullptr;
- }
-
- // Determine that ExtendedKeyUsage includes Client Auth
-
- stack_st_ASN1_OBJECT* extUsage = static_cast<stack_st_ASN1_OBJECT*>(
- X509_get_ext_d2i(peerCert, NID_ext_key_usage, nullptr, nullptr));
-
- if (extUsage == nullptr)
- {
- BMCWEB_LOG_DEBUG("TLS extUsage is null");
- return nullptr;
- }
-
- bool isExKeyUsageClientAuth = false;
- for (int i = 0; i < sk_ASN1_OBJECT_num(extUsage); i++)
- {
- // NOLINTNEXTLINE(cppcoreguidelines-pro-type-cstyle-cast)
- int nid = OBJ_obj2nid(sk_ASN1_OBJECT_value(extUsage, i));
- if (NID_client_auth == nid)
- {
- isExKeyUsageClientAuth = true;
- break;
- }
- }
- sk_ASN1_OBJECT_free(extUsage);
-
- // Certificate has to have proper key usages set
- if (!isExKeyUsageClientAuth)
- {
- BMCWEB_LOG_DEBUG("Certificate ExtendedKeyUsage does "
- "not allow provided certificate to "
- "be used for user authentication");
- return nullptr;
- }
std::string sslUser;
// Extract username contained in CommonName
sslUser.resize(256, '\0');
diff --git a/test/http/mutual_tls.cpp b/test/http/mutual_tls.cpp
index b1b7878..7b5cb25 100644
--- a/test/http/mutual_tls.cpp
+++ b/test/http/mutual_tls.cpp
@@ -25,6 +25,32 @@
OSSLX509(OSSLX509&&) = delete;
OSSLX509() = default;
+
+ void setSubjectName()
+ {
+ X509_NAME* name = X509_get_subject_name(ptr);
+ std::array<unsigned char, 5> user = {'u', 's', 'e', 'r', '\0'};
+ X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, user.data(), -1,
+ -1, 0);
+ }
+ void sign()
+ {
+ // Generate test key
+ EVP_PKEY* pkey = nullptr;
+ EVP_PKEY_CTX* pctx = EVP_PKEY_CTX_new_id(EVP_PKEY_EC, nullptr);
+ ASSERT_EQ(EVP_PKEY_keygen_init(pctx), 1);
+ ASSERT_EQ(
+ EVP_PKEY_CTX_set_ec_paramgen_curve_nid(pctx, NID_X9_62_prime256v1),
+ 1);
+ ASSERT_EQ(EVP_PKEY_keygen(pctx, &pkey), 1);
+ EVP_PKEY_CTX_free(pctx);
+
+ // Sign cert with key
+ ASSERT_EQ(X509_set_pubkey(ptr, pkey), 1);
+ ASSERT_GT(X509_sign(ptr, pkey, EVP_sha256()), 0);
+ EVP_PKEY_free(pkey);
+ }
+
X509* get()
{
return ptr;
@@ -61,11 +87,7 @@
{
OSSLX509 x509;
- X509_NAME* name = X509_get_subject_name(x509.get());
- std::array<unsigned char, 5> user = {'u', 's', 'e', 'r', '\0'};
- X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, user.data(), -1, -1,
- 0);
-
+ x509.setSubjectName();
X509_EXTENSION* ex = X509V3_EXT_conf_nid(nullptr, nullptr, NID_key_usage,
"digitalSignature, keyAgreement");
ASSERT_THAT(ex, NotNull());
@@ -76,6 +98,8 @@
ASSERT_EQ(X509_add_ext(x509.get(), ex, -1), 1);
X509_EXTENSION_free(ex);
+ x509.sign();
+
OSSLX509StoreCTX x509Store;
X509_STORE_CTX_set_current_cert(x509Store.get(), x509.get());
@@ -87,35 +111,13 @@
EXPECT_THAT(session->username, "user");
}
-TEST(MutualTLS, MissingSubject)
-{
- OSSLX509 x509;
-
- X509_EXTENSION* ex = X509V3_EXT_conf_nid(nullptr, nullptr, NID_key_usage,
- "digitalSignature, keyAgreement");
- ASSERT_THAT(ex, NotNull());
- ASSERT_EQ(X509_add_ext(x509.get(), ex, -1), 1);
- X509_EXTENSION_free(ex);
- ex = X509V3_EXT_conf_nid(nullptr, nullptr, NID_ext_key_usage, "clientAuth");
- ASSERT_THAT(ex, NotNull());
- ASSERT_EQ(X509_add_ext(x509.get(), ex, -1), 1);
- X509_EXTENSION_free(ex);
-
- OSSLX509StoreCTX x509Store;
- X509_STORE_CTX_set_current_cert(x509Store.get(), x509.get());
-
- boost::asio::ip::address ip;
- boost::asio::ssl::verify_context ctx(x509Store.get());
- std::shared_ptr<persistent_data::UserSession> session = verifyMtlsUser(ip,
- ctx);
- ASSERT_THAT(session, IsNull());
-}
-
TEST(MutualTLS, MissingKeyUsage)
{
- for (const char* usageString : {"digitalSignature", "keyAgreement"})
+ for (const char* usageString :
+ {"digitalSignature", "keyAgreement", "digitalSignature, keyAgreement"})
{
OSSLX509 x509;
+ x509.setSubjectName();
X509_EXTENSION* ex = X509V3_EXT_conf_nid(nullptr, nullptr,
NID_key_usage, usageString);
@@ -128,6 +130,7 @@
ASSERT_THAT(ex, NotNull());
ASSERT_EQ(X509_add_ext(x509.get(), ex, -1), 1);
X509_EXTENSION_free(ex);
+ x509.sign();
OSSLX509StoreCTX x509Store;
X509_STORE_CTX_set_current_cert(x509Store.get(), x509.get());
@@ -136,31 +139,10 @@
boost::asio::ssl::verify_context ctx(x509Store.get());
std::shared_ptr<persistent_data::UserSession> session =
verifyMtlsUser(ip, ctx);
- ASSERT_THAT(session, IsNull());
+ ASSERT_THAT(session, NotNull());
}
}
-TEST(MutualTLS, MissingExtKeyUsage)
-{
- OSSLX509 x509;
-
- X509_EXTENSION* ex = X509V3_EXT_conf_nid(nullptr, nullptr, NID_key_usage,
- "digitalSignature, keyAgreement");
-
- ASSERT_THAT(ex, NotNull());
- ASSERT_EQ(X509_add_ext(x509.get(), ex, -1), 1);
- X509_EXTENSION_free(ex);
-
- OSSLX509StoreCTX x509Store;
- X509_STORE_CTX_set_current_cert(x509Store.get(), x509.get());
-
- boost::asio::ip::address ip;
- boost::asio::ssl::verify_context ctx(x509Store.get());
- std::shared_ptr<persistent_data::UserSession> session = verifyMtlsUser(ip,
- ctx);
- ASSERT_THAT(session, IsNull());
-}
-
TEST(MutualTLS, MissingCert)
{
OSSLX509StoreCTX x509Store;