From f1894cf6e386b8d3478c5e52f981a601773d7060 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 29 Jan 2025 10:40:02 +0100 Subject: [PATCH 001/114] [platform] added the platform root_cert path utility function. --- include/multipass/platform.h | 1 + src/platform/platform_unix.cpp | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/include/multipass/platform.h b/include/multipass/platform.h index 28c06ee8d4..a3e3a0ba95 100644 --- a/include/multipass/platform.h +++ b/include/multipass/platform.h @@ -82,6 +82,7 @@ class Platform : public Singleton [[nodiscard]] virtual std::string bridge_nomenclature() const; virtual int get_cpus() const; virtual long long get_total_ram() const; + [[nodiscard]] virtual std::filesystem::path get_root_cert_path() const; }; QString interpret_setting(const QString& key, const QString& val); diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index dfb628154e..749a1fe31f 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -263,3 +263,8 @@ long long mp::platform::Platform::get_total_ram() const { return static_cast(sysconf(_SC_PHYS_PAGES)) * sysconf(_SC_PAGESIZE); } + +std::filesystem::path mp::platform::Platform::get_root_cert_path() const +{ + return {"/usr/local/etc/root_cert.pem"}; +} From 92ae3ebbf541fb458b8cb547a4a7593cef89626a Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 29 Jan 2025 10:58:59 +0100 Subject: [PATCH 002/114] [platform] change the default root cert location --- src/platform/platform_unix.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 749a1fe31f..5b6677b1da 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -266,5 +266,5 @@ long long mp::platform::Platform::get_total_ram() const std::filesystem::path mp::platform::Platform::get_root_cert_path() const { - return {"/usr/local/etc/root_cert.pem"}; + return {"/usr/local/share/ca-certificates/multipass_root_cert.pem"}; } From 47ef48f58285283be00619c2f6632a457346a3c2 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 11:22:29 +0100 Subject: [PATCH 003/114] [ssl cert] added root certificate generation utility function. --- src/cert/ssl_cert_provider.cpp | 58 ++++++++++++++++++++++++++++++++++ 1 file changed, 58 insertions(+) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 7699ccd437..c402cd7099 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -173,6 +173,64 @@ class X509Cert throw std::runtime_error("Failed to sign certificate"); } + explicit X509Cert(const EVPKey& key) // generate root certificate only + { + if (x509 == nullptr) + throw std::runtime_error("Failed to allocate x509 cert structure"); + + long big_num{0}; + const auto rand_bytes = MP_UTILS.random_bytes(4); + for (unsigned int i = 0; i < 4u; i++) + big_num |= rand_bytes[i] << i * 8u; + + X509_set_version(x509.get(), 2); // X.509 v3 + + ASN1_INTEGER_set(X509_get_serialNumber(x509.get()), big_num); + X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); // Start time: now + X509_gmtime_adj(X509_get_notAfter(x509.get()), 3650L * 24L * 60L * 60L); // Valid for 10 years + + constexpr int APPEND_ENTRY{-1}; + constexpr int ADD_RDN{0}; + + const auto country = as_vector("US"); + const auto org = as_vector("Canonical"); + const auto cn = as_vector("Multipass Root CA"); + + const auto name = X509_get_subject_name(x509.get()); + X509_NAME_add_entry_by_txt(name, "C", MBSTRING_ASC, country.data(), country.size(), APPEND_ENTRY, ADD_RDN); + X509_NAME_add_entry_by_txt(name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); + X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); + X509_set_issuer_name(x509.get(), name); + // set_san_name(x509.get(), server_name); + + if (!X509_set_pubkey(x509.get(), key.get())) + throw std::runtime_error("Failed to set certificate public key"); + + // Add X509v3 extensions + X509V3_CTX ctx; + X509V3_set_ctx(&ctx, x509.get(), x509.get(), NULL, NULL, 0); + + // wrap into function or struct + X509_EXTENSION* ext; + // Subject Key Identifier + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); + + // Authority Key Identifier + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, "keyid:always"); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); + + // Basic Constraints: critical, CA:TRUE + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, "critical,CA:TRUE"); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); + + if (!X509_sign(x509.get(), key.get(), EVP_sha256())) + throw std::runtime_error("Failed to sign certificate"); + } + std::string as_pem() { mp::BIOMem mem; From a7b70b980fcfdfbf358999c02d00559efd2c80f4 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 11:24:55 +0100 Subject: [PATCH 004/114] [ssl cert] added the utility function of signing server certificate by root certificate --- src/cert/ssl_cert_provider.cpp | 64 ++++++++++++++++++++++++++++++++++ 1 file changed, 64 insertions(+) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index c402cd7099..424c5551e2 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -231,6 +231,70 @@ class X509Cert throw std::runtime_error("Failed to sign certificate"); } + X509Cert(const EVPKey& root_certificate_key, + const X509Cert& root_certificate, + const EVPKey& server_key, + const std::string& server_name) + // generate signed server certificates + { + if (x509 == nullptr) + throw std::runtime_error("Failed to allocate x509 cert structure"); + + long big_num{0}; + auto rand_bytes = MP_UTILS.random_bytes(4); + for (unsigned int i = 0; i < 4u; i++) + big_num |= rand_bytes[i] << i * 8u; + + X509_set_version(x509.get(), 2); + + ASN1_INTEGER_set(X509_get_serialNumber(x509.get()), big_num); + X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); + X509_gmtime_adj(X509_get_notAfter(x509.get()), 31536000L);// Valid for 10 years + + constexpr int APPEND_ENTRY{-1}; + constexpr int ADD_RDN{0}; + + const auto country = as_vector("US"); + const auto org = as_vector("Canonical"); + const auto cn = as_vector(cn_name_from(server_name)); + + const auto server_certificate_name = X509_get_subject_name(x509.get()); + X509_NAME_add_entry_by_txt(server_certificate_name, "C", MBSTRING_ASC, country.data(), country.size(), APPEND_ENTRY, ADD_RDN); + X509_NAME_add_entry_by_txt(server_certificate_name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); + X509_NAME_add_entry_by_txt(server_certificate_name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); + // Set issuer name (from root certificate) + X509_set_issuer_name(x509.get(), X509_get_subject_name(root_certificate.x509.get())); + + set_san_name(x509.get(), server_name); + + if (!X509_set_pubkey(x509.get(), server_key.get())) + throw std::runtime_error("Failed to set certificate public key"); + + // Add X509v3 extensions + X509V3_CTX ctx; + X509V3_set_ctx(&ctx, root_certificate.x509.get(), x509.get(), NULL, NULL, 0); + + // wrap into function or struct + X509_EXTENSION* ext; + // Subject Key Identifier + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); + + // Authority Key Identifier + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, "keyid:always,issuer"); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); + + // Basic Constraints: critical, CA:FALSE + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, "critical,CA:FALSE"); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); + + if (!X509_sign(x509.get(), root_certificate_key.get(), EVP_sha256())) + throw std::runtime_error("Failed to sign certificate"); + } + std::string as_pem() { mp::BIOMem mem; From 8838aa33712936de4e692c3b4fefcd8edac8f781 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 11:28:56 +0100 Subject: [PATCH 005/114] [ssl cert] a small variable name improvement. --- src/cert/ssl_cert_provider.cpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 424c5551e2..9d5befc7cf 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -88,13 +88,13 @@ class EVPKey return mem.as_string(); } - void write(const QString& name) + void write(const QString& key_path) { - WritableFile file{name}; + WritableFile file{key_path}; if (!PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr)) - throw std::runtime_error(fmt::format("Failed writing certificate private key to file '{}'", name)); + throw std::runtime_error(fmt::format("Failed writing certificate private key to file '{}'", key_path)); - QFile::setPermissions(name, QFile::ReadOwner); + QFile::setPermissions(key_path, QFile::ReadOwner); } EVP_PKEY* get() const @@ -304,11 +304,11 @@ class X509Cert return mem.as_string(); } - void write(const QString& name) + void write(const QString& cert_path) { - WritableFile file{name}; + WritableFile file{cert_path}; if (!PEM_write_X509(file.get(), x509.get())) - throw std::runtime_error(fmt::format("Failed writing certificate to file '{}'", name)); + throw std::runtime_error(fmt::format("Failed writing certificate to file '{}'", cert_path)); } private: From 2421fb523ba547f21d33496df6fec2a4d5305df7 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 11:43:05 +0100 Subject: [PATCH 006/114] [ssl cert][client] adapted the make_cert_key_pair function so it can generated signed server certificate. Client side ssl option is also updated accordingly. Unit tests are broken temporarily due to this change. --- src/cert/ssl_cert_provider.cpp | 33 ++++++++++++++++++++++------- src/client/common/client_common.cpp | 3 ++- 2 files changed, 27 insertions(+), 9 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 9d5befc7cf..9b7b0d4b4a 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -16,6 +16,7 @@ */ #include +#include #include #include @@ -28,7 +29,6 @@ #include -#include #include #include #include @@ -327,15 +327,32 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, return {mp::utils::contents_of(cert_path), mp::utils::contents_of(priv_key_path)}; } - EVPKey key; - X509Cert cert{key, server_name}; - - key.write(priv_key_path); - cert.write(cert_path); + if (!server_name.empty()) + { + EVPKey root_cert_key; + const auto priv_root_key_path = cert_dir.filePath(prefix + "_root_key.pem"); + const std::filesystem::path root_cert_path = MP_PLATFORM.get_root_cert_path(); + + X509Cert root_cert{root_cert_key}; + root_cert_key.write(priv_root_key_path); + root_cert.write(root_cert_path.u8string().c_str()); + + EVPKey server_cert_key; + X509Cert signed_server_cert{root_cert_key, root_cert, server_cert_key, server_name}; + server_cert_key.write(priv_key_path); + signed_server_cert.write(cert_path); + return {signed_server_cert.as_pem(), server_cert_key.as_pem()}; + } + else + { + EVPKey client_cert_key; + X509Cert client_cert{client_cert_key, server_name}; + client_cert_key.write(priv_key_path); + client_cert.write(cert_path); - return {cert.as_pem(), key.as_pem()}; + return {client_cert.as_pem(), client_cert_key.as_pem()}; + } } - } // namespace mp::SSLCertProvider::SSLCertProvider(const multipass::Path& cert_dir, const std::string& server_name) diff --git a/src/client/common/client_common.cpp b/src/client/common/client_common.cpp index 2472e39528..5aa931501a 100644 --- a/src/client/common/client_common.cpp +++ b/src/client/common/client_common.cpp @@ -74,7 +74,8 @@ grpc::SslCredentialsOptions get_ssl_credentials_opts_from(const mp::CertProvider { auto opts = grpc::SslCredentialsOptions(); - opts.server_certificate_request = GRPC_SSL_REQUEST_SERVER_CERTIFICATE_BUT_DONT_VERIFY; + opts.pem_root_certs = MP_UTILS.contents_of(MP_PLATFORM.get_root_cert_path().u8string().c_str()); + opts.server_certificate_request = GRPC_SSL_REQUEST_SERVER_CERTIFICATE_AND_VERIFY; opts.pem_cert_chain = cert_provider.PEM_certificate(); opts.pem_private_key = cert_provider.PEM_signing_key(); From b8310b5aa6a6d49f3eebec620edae35a4c6b5335 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 11:49:01 +0100 Subject: [PATCH 007/114] [unit test][ssl cert] fixed the unit tests in test_ssl_cert_provider.cpp --- tests/mock_platform.h | 1 + tests/test_ssl_cert_provider.cpp | 17 +++++++++++------ 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/tests/mock_platform.h b/tests/mock_platform.h index aa95b70cbc..5d56ff5e87 100644 --- a/tests/mock_platform.h +++ b/tests/mock_platform.h @@ -59,6 +59,7 @@ class MockPlatform : public platform::Platform MOCK_METHOD(bool, is_image_url_supported, (), (const, override)); MOCK_METHOD(QString, get_username, (), (const, override)); MOCK_METHOD(std::string, bridge_nomenclature, (), (const, override)); + MOCK_METHOD(std::filesystem::path, get_root_cert_path, (), (const, override)); MP_MOCK_SINGLETON_BOILERPLATE(MockPlatform, Platform); }; diff --git a/tests/test_ssl_cert_provider.cpp b/tests/test_ssl_cert_provider.cpp index 28215f3256..bc8aa9e41d 100644 --- a/tests/test_ssl_cert_provider.cpp +++ b/tests/test_ssl_cert_provider.cpp @@ -17,6 +17,7 @@ #include "common.h" #include "file_operations.h" +#include "mock_platform.h" #include "temp_dir.h" #include @@ -27,9 +28,9 @@ namespace mpt = multipass::test; using namespace testing; -struct SSLCertProvider : public testing::Test +struct SSLCertProviderFixture : public testing::Test { - SSLCertProvider() + SSLCertProviderFixture() { cert_dir = MP_UTILS.make_dir(temp_dir.path(), "test-cert"); } @@ -37,7 +38,7 @@ struct SSLCertProvider : public testing::Test mp::Path cert_dir; }; -TEST_F(SSLCertProvider, creates_cert_and_key) +TEST_F(SSLCertProviderFixture, creates_cert_and_key) { mp::SSLCertProvider cert_provider{cert_dir}; @@ -47,7 +48,7 @@ TEST_F(SSLCertProvider, creates_cert_and_key) EXPECT_THAT(pem_key, StrNe("")); } -TEST_F(SSLCertProvider, imports_existing_cert_and_key) +TEST_F(SSLCertProviderFixture, imports_existing_cert_and_key) { constexpr auto key_data = "-----BEGIN PRIVATE KEY-----\n" "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgsSAz5ggzrLjai0I/\n" @@ -79,7 +80,7 @@ TEST_F(SSLCertProvider, imports_existing_cert_and_key) EXPECT_THAT(cert_provider.PEM_certificate(), StrEq(cert_data)); } -TEST_F(SSLCertProvider, persists_cert_and_key) +TEST_F(SSLCertProviderFixture, persists_cert_and_key) { QDir dir{cert_dir}; auto key_file = dir.filePath("multipass_cert_key.pem"); @@ -94,8 +95,12 @@ TEST_F(SSLCertProvider, persists_cert_and_key) EXPECT_TRUE(QFile::exists(cert_file)); } -TEST_F(SSLCertProvider, creates_different_certs_per_server_name) +TEST_F(SSLCertProviderFixture, creates_different_certs_per_server_name) { + const auto [mock_platform, _] = mpt::MockPlatform::inject(); + EXPECT_CALL(*mock_platform, get_root_cert_path()) + .WillRepeatedly(Return(std::filesystem::path{cert_dir.toStdU16String()} / "multipass_root_cert.pem")); + mp::SSLCertProvider cert_provider1{cert_dir, "test_server1"}; mp::SSLCertProvider cert_provider2{cert_dir, "test_server2"}; From 049f7c2eb8fbe5f01f98d916a152697ac1fa2c06 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 12:30:27 +0100 Subject: [PATCH 008/114] [unit test] fixed the unit tests in test_cli_client.cpp file Added the root certificate and signed server certificate. Note, the client_cert key and certificate are default mock_ssl_cert_provider output and as a result these two variables are used as the server certificate key pair in almost all unit tests. --- tests/mock_cert_provider.h | 37 ++++++++++++++++++++++++++----------- tests/test_cli_client.cpp | 9 ++++++--- 2 files changed, 32 insertions(+), 14 deletions(-) diff --git a/tests/mock_cert_provider.h b/tests/mock_cert_provider.h index 40d88a62e6..289a2da504 100644 --- a/tests/mock_cert_provider.h +++ b/tests/mock_cert_provider.h @@ -24,21 +24,36 @@ using namespace testing; namespace multipass::test { +constexpr auto root_cert = "-----BEGIN CERTIFICATE-----\n" + "MIIBvjCCAWWgAwIBAgIEUlzMbjAKBggqhkjOPQQDAjA9MQswCQYDVQQGEwJVUzES\n" + "MBAGA1UECgwJQ2Fub25pY2FsMRowGAYDVQQDDBFNdWx0aXBhc3MgUm9vdCBDQTAe\n" + "Fw0yNTAxMjkxMzAzNDBaFw0zNTAxMjcxMzAzNDBaMD0xCzAJBgNVBAYTAlVTMRIw\n" + "EAYDVQQKDAlDYW5vbmljYWwxGjAYBgNVBAMMEU11bHRpcGFzcyBSb290IENBMFkw\n" + "EwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAExnNTmDW0EMAg/ADMNJIc5V4BP/si6MlA\n" + "+YSHDTy+nbgXJ9q0/ORKZETjydqIkVRLzu1LR5sWpdbSzSPo3ft/9KNTMFEwHQYD\n" + "VR0OBBYEFAYlQy+QMhSOh/gACsgSdWbIH5NoMB8GA1UdIwQYMBaAFAYlQy+QMhSO\n" + "h/gACsgSdWbIH5NoMA8GA1UdEwEB/wQFMAMBAf8wCgYIKoZIzj0EAwIDRwAwRAIg\n" + "ErA3KYoWLTkQ9J6cu/bSS539veIBO7p0xvb2x0u2UA0CIEiJ0mMdATr/I8tovsZm\n" + "xgvZMY2ColjLunUiNG8H096n\n" + "-----END CERTIFICATE-----\n"; + constexpr auto client_cert = "-----BEGIN CERTIFICATE-----\n" - "MIIBUjCB+AIBKjAKBggqhkjOPQQDAjA1MQswCQYDVQQGEwJDQTESMBAGA1UECgwJ\n" - "Q2Fub25pY2FsMRIwEAYDVQQDDAlsb2NhbGhvc3QwHhcNMTgwNjIxMTM0MjI5WhcN\n" - "MTkwNjIxMTM0MjI5WjA1MQswCQYDVQQGEwJDQTESMBAGA1UECgwJQ2Fub25pY2Fs\n" - "MRIwEAYDVQQDDAlsb2NhbGhvc3QwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQA\n" - "FGNAqq7c5IMDeQ/cV4+EmogmkfpbTLSPfXgXVLHRsvL04xUAkqGpL+eyGFVE6dqa\n" - "J7sAPJJwlVj1xD0r5DX5MAoGCCqGSM49BAMCA0kAMEYCIQCvI0PYv9f201fbe4LP\n" - "BowTeYWSqMQtLNjvZgd++AAGhgIhALNPW+NRSKCXwadiIFgpbjPInLPqXPskLWSc\n" - "aXByaQyt\n" + "MIIByjCCAXCgAwIBAgIENvdePTAKBggqhkjOPQQDAjA9MQswCQYDVQQGEwJVUzES\n" + "MBAGA1UECgwJQ2Fub25pY2FsMRowGAYDVQQDDBFNdWx0aXBhc3MgUm9vdCBDQTAe\n" + "Fw0yNTAxMjkxMzAzNDBaFw0yNjAxMjkxMzAzNDBaMDUxCzAJBgNVBAYTAlVTMRIw\n" + "EAYDVQQKDAlDYW5vbmljYWwxEjAQBgNVBAMMCWxvY2FsaG9zdDBZMBMGByqGSM49\n" + "AgEGCCqGSM49AwEHA0IABGAw4mRhGqCg7uSIsVgBIzMOoGnlEFWga2dxUzA1YwNe\n" + "8SB679smyb7KVsPg4fK/P7XS4ORxSnMVnKWvTAfYKXWjZjBkMBQGA1UdEQQNMAuC\n" + "CWxvY2FsaG9zdDAdBgNVHQ4EFgQU++FdgRpFokGT+7Fdgqe4SxmSD9UwHwYDVR0j\n" + "BBgwFoAUBiVDL5AyFI6H+AAKyBJ1Zsgfk2gwDAYDVR0TAQH/BAIwADAKBggqhkjO\n" + "PQQDAgNIADBFAiAesF7z8ItZVxK6fgUwhWfgN5rUFzCO5tBGJFDHU7eIZgIhALdl\n" + "2mAn2oocQZfHohrbVUIuWDiUr0SxOkdGUISX0ElJ\n" "-----END CERTIFICATE-----\n"; constexpr auto client_key = "-----BEGIN PRIVATE KEY-----\n" - "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgsSAz5ggzrLjai0I/\n" - "F0hYg5oG/shpXJiBQtJdBCG3lUShRANCAAQAFGNAqq7c5IMDeQ/cV4+Emogmkfpb\n" - "TLSPfXgXVLHRsvL04xUAkqGpL+eyGFVE6dqaJ7sAPJJwlVj1xD0r5DX5\n" + "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgwRNA3VMqakM32i0C\n" + "PHE5i4qRNGdvgXtCWwpp0gXv+oGhRANCAARgMOJkYRqgoO7kiLFYASMzDqBp5RBV\n" + "oGtncVMwNWMDXvEgeu/bJsm+ylbD4OHyvz+10uDkcUpzFZylr0wH2Cl1\n" "-----END PRIVATE KEY-----\n"; constexpr auto daemon_cert = "-----BEGIN CERTIFICATE-----\n" diff --git a/tests/test_cli_client.cpp b/tests/test_cli_client.cpp index 033fe962fb..50226881e6 100644 --- a/tests/test_cli_client.cpp +++ b/tests/test_cli_client.cpp @@ -161,6 +161,7 @@ struct Client : public Test EXPECT_CALL(mock_settings, get(Eq(mp::mounts_key))).WillRepeatedly(Return("true")); EXPECT_CALL(mock_settings, register_handler(_)).WillRepeatedly(Return(nullptr)); EXPECT_CALL(mock_settings, unregister_handler).Times(AnyNumber()); + EXPECT_CALL(*mock_utils, contents_of(_)).WillRepeatedly(Return(multipass::test::root_cert)); EXPECT_CALL(mpt::MockStandardPaths::mock_instance(), locate(_, _, _)) .Times(AnyNumber()); // needed to allow general calls once we have added the specific expectation below @@ -379,8 +380,11 @@ struct Client : public Test }; std::unique_ptr> daemon_cert_provider{ std::make_unique>()}; - mpt::MockPlatform::GuardedMock attr{mpt::MockPlatform::inject()}; - mpt::MockPlatform* mock_platform = attr.first; + const mpt::MockPlatform::GuardedMock platform_attr{mpt::MockPlatform::inject()}; + const mpt::MockPlatform* mock_platform = platform_attr.first; + const mpt::MockUtils::GuardedMock utils_attr{mpt::MockUtils::inject()}; + mpt::MockUtils* mock_utils = utils_attr.first; + mpt::StubCertStore cert_store; StrictMock mock_daemon{server_address, *daemon_cert_provider, &cert_store}; // strict to fail on unexpected calls and play well with sharing @@ -3569,7 +3573,6 @@ struct TimeoutSuite : Client, WithParamInterface TEST_P(TimeoutSuite, command_exits_on_timeout) { - auto [mock_utils, guard] = mpt::MockUtils::inject(); EXPECT_CALL(mock_daemon, launch).Times(AtMost(1)); EXPECT_CALL(mock_daemon, start).Times(AtMost(1)); From 3aceacc26d892a02f15c92b71f5cbfd9d185b3a7 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 12:34:56 +0100 Subject: [PATCH 009/114] [unit test][daemon rpc] fixed the unt tests in test_daemon_rpc.cpp file Made the mock_cert_provider using the default certificate and key pair which is the client_cert and client_key in the file mock_cert_provider.h. Meanwhile, the make_secure_stub is also updated to make the unit tests pass. --- tests/unix/test_daemon_rpc.cpp | 27 ++++++--------------------- 1 file changed, 6 insertions(+), 21 deletions(-) diff --git a/tests/unix/test_daemon_rpc.cpp b/tests/unix/test_daemon_rpc.cpp index 7ec3b86af5..022662cee0 100644 --- a/tests/unix/test_daemon_rpc.cpp +++ b/tests/unix/test_daemon_rpc.cpp @@ -34,36 +34,21 @@ using namespace testing; namespace { -constexpr auto key_data = "-----BEGIN PRIVATE KEY-----\n" - "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgsSAz5ggzrLjai0I/\n" - "F0hYg5oG/shpXJiBQtJdBCG3lUShRANCAAQAFGNAqq7c5IMDeQ/cV4+Emogmkfpb\n" - "TLSPfXgXVLHRsvL04xUAkqGpL+eyGFVE6dqaJ7sAPJJwlVj1xD0r5DX5\n" - "-----END PRIVATE KEY-----\n"; - -constexpr auto cert_data = "-----BEGIN CERTIFICATE-----\n" - "MIIBUjCB+AIBKjAKBggqhkjOPQQDAjA1MQswCQYDVQQGEwJDQTESMBAGA1UECgwJ\n" - "Q2Fub25pY2FsMRIwEAYDVQQDDAlsb2NhbGhvc3QwHhcNMTgwNjIxMTM0MjI5WhcN\n" - "MTkwNjIxMTM0MjI5WjA1MQswCQYDVQQGEwJDQTESMBAGA1UECgwJQ2Fub25pY2Fs\n" - "MRIwEAYDVQQDDAlsb2NhbGhvc3QwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQA\n" - "FGNAqq7c5IMDeQ/cV4+EmogmkfpbTLSPfXgXVLHRsvL04xUAkqGpL+eyGFVE6dqa\n" - "J7sAPJJwlVj1xD0r5DX5MAoGCCqGSM49BAMCA0kAMEYCIQCvI0PYv9f201fbe4LP\n" - "BowTeYWSqMQtLNjvZgd++AAGhgIhALNPW+NRSKCXwadiIFgpbjPInLPqXPskLWSc\n" - "aXByaQyt\n" - "-----END CERTIFICATE-----\n"; - struct TestDaemonRpc : public mpt::DaemonTestFixture { TestDaemonRpc() { - EXPECT_CALL(*mock_cert_provider, PEM_certificate()).WillOnce(Return(cert_data)); - EXPECT_CALL(*mock_cert_provider, PEM_signing_key()).WillOnce(Return(key_data)); + EXPECT_CALL(*mock_cert_provider, PEM_certificate()).Times(1); + EXPECT_CALL(*mock_cert_provider, PEM_signing_key()).Times(1); EXPECT_CALL(*mock_platform, multipass_storage_location()).Times(AnyNumber()).WillRepeatedly(Return(QString())); + EXPECT_CALL(*mock_utils, contents_of(_)).WillRepeatedly(Return(mpt::root_cert)); } mp::Rpc::Stub make_secure_stub() { auto opts = grpc::SslCredentialsOptions(); - opts.server_certificate_request = GRPC_SSL_REQUEST_SERVER_CERTIFICATE_BUT_DONT_VERIFY; + opts.pem_root_certs = mpt::root_cert; + opts.server_certificate_request = GRPC_SSL_REQUEST_SERVER_CERTIFICATE_AND_VERIFY; opts.pem_cert_chain = mpt::client_cert; opts.pem_private_key = mpt::client_key; @@ -94,7 +79,7 @@ struct TestDaemonRpc : public mpt::DaemonTestFixture std::make_unique>()}; std::unique_ptr mock_cert_store{std::make_unique()}; - mpt::MockPlatform::GuardedMock platform_attr{mpt::MockPlatform::inject()}; + mpt::MockPlatform::GuardedMock platform_attr{mpt::MockPlatform::inject()}; mpt::MockPlatform* mock_platform = platform_attr.first; mpt::MockUtils::GuardedMock attr{mpt::MockUtils::inject()}; From dd0349e6df7561e96e25339f81e03e225b75e6b0 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 12:35:43 +0100 Subject: [PATCH 010/114] [unit tests][daemon] fixes the unit tests in test_daemon.cpp --- tests/test_daemon.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/test_daemon.cpp b/tests/test_daemon.cpp index c49ff552d6..a0e9673784 100644 --- a/tests/test_daemon.cpp +++ b/tests/test_daemon.cpp @@ -21,6 +21,7 @@ #include "dummy_ssh_key_provider.h" #include "fake_alias_config.h" #include "json_test_utils.h" +#include "mock_cert_provider.h" #include "mock_daemon.h" #include "mock_environment_helpers.h" #include "mock_file_ops.h" @@ -38,7 +39,6 @@ #include "mock_vm_image_vault.h" #include "path.h" #include "stub_virtual_machine.h" -#include "stub_vm_image_vault.h" #include "tracking_url_downloader.h" #include @@ -113,7 +113,7 @@ struct Daemon : public mpt::DaemonTestFixture ON_CALL(mock_utils, filesystem_bytes_available(_)).WillByDefault([this](const QString& data_directory) { return mock_utils.Utils::filesystem_bytes_available(data_directory); }); - + ON_CALL(mock_utils, contents_of(_)).WillByDefault(Return(multipass::test::root_cert)); EXPECT_CALL(mock_platform, get_blueprints_url_override()).WillRepeatedly([] { return QString{}; }); EXPECT_CALL(mock_platform, multipass_storage_location()).Times(AnyNumber()).WillRepeatedly(Return(QString())); EXPECT_CALL(mock_platform, create_alias_script(_, _)).WillRepeatedly(Return()); @@ -134,7 +134,7 @@ struct Daemon : public mpt::DaemonTestFixture mpt::MockUtils::GuardedMock mock_utils_injection{mpt::MockUtils::inject()}; mpt::MockUtils& mock_utils = *mock_utils_injection.first; - mpt::MockPlatform::GuardedMock mock_platform_injection{mpt::MockPlatform::inject()}; + mpt::MockPlatform::GuardedMock mock_platform_injection{mpt::MockPlatform::inject()}; mpt::MockPlatform& mock_platform = *mock_platform_injection.first; mpt::MockSettings::GuardedMock mock_settings_injection = mpt::MockSettings::inject(); From 862e62ed556af93dcfa62923211a5ba132655a79 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 12:37:33 +0100 Subject: [PATCH 011/114] [unit test][daemon find] fixed the unit tests in test_daemon_find.cpp --- tests/test_daemon_find.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/test_daemon_find.cpp b/tests/test_daemon_find.cpp index f6f54262fd..eafba0114e 100644 --- a/tests/test_daemon_find.cpp +++ b/tests/test_daemon_find.cpp @@ -17,10 +17,12 @@ #include "common.h" #include "daemon_test_fixture.h" +#include "mock_cert_provider.h" #include "mock_image_host.h" #include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_settings.h" +#include "mock_utils.h" #include "mock_vm_blueprint_provider.h" #include "mock_vm_image_vault.h" @@ -49,6 +51,7 @@ struct DaemonFind : public mpt::DaemonTestFixture EXPECT_CALL(mock_settings, register_handler).WillRepeatedly(Return(nullptr)); EXPECT_CALL(mock_settings, unregister_handler).Times(AnyNumber()); EXPECT_CALL(mock_settings, get(Eq(mp::winterm_key))).WillRepeatedly(Return("none")); + ON_CALL(mock_utils, contents_of(_)).WillByDefault(Return(multipass::test::root_cert)); } mpt::MockPlatform::GuardedMock attr{mpt::MockPlatform::inject()}; @@ -60,6 +63,9 @@ struct DaemonFind : public mpt::DaemonTestFixture const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = mpt::MockPermissionUtils::inject(); mpt::MockPermissionUtils& mock_permission_utils = *mock_permission_utils_injection.first; + + mpt::MockUtils::GuardedMock mock_utils_injection{mpt::MockUtils::inject()}; + mpt::MockUtils& mock_utils = *mock_utils_injection.first; }; TEST_F(DaemonFind, blankQueryReturnsAllData) From 0b5d95f1d3aa9f13759f5f08383d9c4aa0e643d1 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 12:38:15 +0100 Subject: [PATCH 012/114] [unit test][alias dict] fixed the unit tests in test_alias_dict.cpp --- tests/test_alias_dict.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/test_alias_dict.cpp b/tests/test_alias_dict.cpp index 1a049d0792..e0f948817d 100644 --- a/tests/test_alias_dict.cpp +++ b/tests/test_alias_dict.cpp @@ -27,10 +27,12 @@ #include "fake_alias_config.h" #include "file_operations.h" #include "json_test_utils.h" +#include "mock_cert_provider.h" #include "mock_file_ops.h" #include "mock_permission_utils.h" #include "mock_platform.h" #include "mock_settings.h" +#include "mock_utils.h" #include "mock_vm_image_vault.h" #include "stub_terminal.h" @@ -624,6 +626,9 @@ TEST_P(DaemonAliasTestsuite, purge_removes_purged_instance_aliases_and_scripts) EXPECT_CALL(*mock_image_vault, prune_expired_images()).WillRepeatedly(Return()); EXPECT_CALL(*mock_image_vault, has_record_for(_)).WillRepeatedly(Return(true)); + const auto [mock_utils, guard] = mpt::MockUtils::inject(); + EXPECT_CALL(*mock_utils, contents_of(_)).WillRepeatedly(Return(multipass::test::root_cert)); + config_builder.vault = std::move(mock_image_vault); auto mock_factory = use_a_mock_vm_factory(); From 065b134dbaf80fd7224d601024a34a22640704f7 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 30 Jan 2025 12:43:26 +0100 Subject: [PATCH 013/114] [ssl cert] fixed the format. --- src/cert/ssl_cert_provider.cpp | 34 ++++++++++++++++++++++++++-------- 1 file changed, 26 insertions(+), 8 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 9b7b0d4b4a..0c6a95c80e 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -249,7 +249,7 @@ class X509Cert ASN1_INTEGER_set(X509_get_serialNumber(x509.get()), big_num); X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); - X509_gmtime_adj(X509_get_notAfter(x509.get()), 31536000L);// Valid for 10 years + X509_gmtime_adj(X509_get_notAfter(x509.get()), 31536000L); // Valid for 10 years constexpr int APPEND_ENTRY{-1}; constexpr int ADD_RDN{0}; @@ -259,9 +259,27 @@ class X509Cert const auto cn = as_vector(cn_name_from(server_name)); const auto server_certificate_name = X509_get_subject_name(x509.get()); - X509_NAME_add_entry_by_txt(server_certificate_name, "C", MBSTRING_ASC, country.data(), country.size(), APPEND_ENTRY, ADD_RDN); - X509_NAME_add_entry_by_txt(server_certificate_name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); - X509_NAME_add_entry_by_txt(server_certificate_name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); + X509_NAME_add_entry_by_txt(server_certificate_name, + "C", + MBSTRING_ASC, + country.data(), + country.size(), + APPEND_ENTRY, + ADD_RDN); + X509_NAME_add_entry_by_txt(server_certificate_name, + "O", + MBSTRING_ASC, + org.data(), + org.size(), + APPEND_ENTRY, + ADD_RDN); + X509_NAME_add_entry_by_txt(server_certificate_name, + "CN", + MBSTRING_ASC, + cn.data(), + cn.size(), + APPEND_ENTRY, + ADD_RDN); // Set issuer name (from root certificate) X509_set_issuer_name(x509.get(), X509_get_subject_name(root_certificate.x509.get())); @@ -270,23 +288,23 @@ class X509Cert if (!X509_set_pubkey(x509.get(), server_key.get())) throw std::runtime_error("Failed to set certificate public key"); - // Add X509v3 extensions + // Add X509v3 extensions X509V3_CTX ctx; X509V3_set_ctx(&ctx, root_certificate.x509.get(), x509.get(), NULL, NULL, 0); - // wrap into function or struct + // wrap into function or struct X509_EXTENSION* ext; // Subject Key Identifier ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); X509_add_ext(x509.get(), ext, -1); X509_EXTENSION_free(ext); - // Authority Key Identifier + // Authority Key Identifier ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, "keyid:always,issuer"); X509_add_ext(x509.get(), ext, -1); X509_EXTENSION_free(ext); - // Basic Constraints: critical, CA:FALSE + // Basic Constraints: critical, CA:FALSE ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, "critical,CA:FALSE"); X509_add_ext(x509.get(), ext, -1); X509_EXTENSION_free(ext); From 7044051acb1ba46954a32dc8125582434e518ba4 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 31 Jan 2025 11:32:01 +0100 Subject: [PATCH 014/114] [unit test][client common] fixed the unit tests in test_client_common.cpp file --- tests/test_client_common.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/tests/test_client_common.cpp b/tests/test_client_common.cpp index c7eb110a9c..79682ebff2 100644 --- a/tests/test_client_common.cpp +++ b/tests/test_client_common.cpp @@ -44,6 +44,16 @@ struct TestClientCommon : public mpt::DaemonTestFixture { ON_CALL(mpt::MockStandardPaths::mock_instance(), writableLocation(mp::StandardPaths::GenericDataLocation)) .WillByDefault(Return(temp_dir.path())); + ON_CALL(*mock_utils, contents_of(_)).WillByDefault(Return(multipass::test::root_cert)); + //delegate some functions to the orignal implementation + ON_CALL(*mock_utils, make_dir(A(), A(), A())) + .WillByDefault([](const QDir& dir, const QString& name, std::filesystem::perms permissions) -> mp::Path { + return MP_UTILS.Utils::make_dir(dir, name, permissions); + }); + ON_CALL(*mock_utils, make_dir(A(), A())) + .WillByDefault([](const QDir& dir, std::filesystem::perms permissions) -> mp::Path { + return MP_UTILS.Utils::make_dir(dir, permissions); + }); } mpt::MockDaemon make_secure_server() @@ -60,6 +70,8 @@ struct TestClientCommon : public mpt::DaemonTestFixture std::unique_ptr> mock_cert_provider{ std::make_unique>()}; std::unique_ptr mock_cert_store{std::make_unique()}; + const mpt::MockUtils::GuardedMock utils_attr{mpt::MockUtils::inject()}; + mpt::MockUtils* mock_utils = utils_attr.first; const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = mpt::MockPermissionUtils::inject(); From dd4e6c15f5f581c074c8967a51d23127979c2d71 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 31 Jan 2025 11:34:24 +0100 Subject: [PATCH 015/114] [unit test] added constness to mock_utils and to corresponding functions. --- include/multipass/utils.h | 4 ++-- src/utils/utils.cpp | 4 ++-- tests/mock_utils.h | 4 ++-- tests/test_client_common.cpp | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/include/multipass/utils.h b/include/multipass/utils.h index cfd6141dad..e55de02a3b 100644 --- a/include/multipass/utils.h +++ b/include/multipass/utils.h @@ -209,8 +209,8 @@ class Utils : public Singleton const bool& overwrite = false); virtual Path make_dir(const QDir& a_dir, const QString& name, - std::filesystem::perms permissions = std::filesystem::perms::none); - virtual Path make_dir(const QDir& dir, std::filesystem::perms permissions = std::filesystem::perms::none); + std::filesystem::perms permissions = std::filesystem::perms::none) const; + virtual Path make_dir(const QDir& dir, std::filesystem::perms permissions = std::filesystem::perms::none) const; // command and process helpers virtual std::string run_cmd_for_output(const QString& cmd, const QStringList& args, diff --git a/src/utils/utils.cpp b/src/utils/utils.cpp index eaa3da921a..cf145f42cd 100644 --- a/src/utils/utils.cpp +++ b/src/utils/utils.cpp @@ -275,7 +275,7 @@ std::string mp::Utils::run_in_ssh_session(mp::SSHSession& session, const std::st return mp::utils::trim_end(output); } -mp::Path mp::Utils::make_dir(const QDir& a_dir, const QString& name, std::filesystem::perms permissions) +mp::Path mp::Utils::make_dir(const QDir& a_dir, const QString& name, std::filesystem::perms permissions) const { mp::Path dir_path; bool success{false}; @@ -304,7 +304,7 @@ mp::Path mp::Utils::make_dir(const QDir& a_dir, const QString& name, std::filesy return dir_path; } -mp::Path mp::Utils::make_dir(const QDir& dir, std::filesystem::perms permissions) +mp::Path mp::Utils::make_dir(const QDir& dir, std::filesystem::perms permissions) const { return make_dir(dir, QString(), permissions); } diff --git a/tests/mock_utils.h b/tests/mock_utils.h index 7bda4a0546..a12683b214 100644 --- a/tests/mock_utils.h +++ b/tests/mock_utils.h @@ -38,8 +38,8 @@ class MockUtils : public Utils MOCK_METHOD(std::string, contents_of, (const multipass::Path&), (const, override)); MOCK_METHOD(void, make_file_with_content, (const std::string&, const std::string&), ()); MOCK_METHOD(void, make_file_with_content, (const std::string&, const std::string&, const bool&), (override)); - MOCK_METHOD(Path, make_dir, (const QDir&, const QString&, std::filesystem::perms), (override)); - MOCK_METHOD(Path, make_dir, (const QDir&, std::filesystem::perms), (override)); + MOCK_METHOD(Path, make_dir, (const QDir&, const QString&, std::filesystem::perms), (const override)); + MOCK_METHOD(Path, make_dir, (const QDir&, std::filesystem::perms), (const override)); MOCK_METHOD(std::string, get_kernel_version, (), (const, override)); MOCK_METHOD(QString, generate_scrypt_hash_for, (const QString&), (const, override)); MOCK_METHOD(bool, client_certs_exist, (const QString&), (const)); diff --git a/tests/test_client_common.cpp b/tests/test_client_common.cpp index 79682ebff2..bd5d82beb6 100644 --- a/tests/test_client_common.cpp +++ b/tests/test_client_common.cpp @@ -71,7 +71,7 @@ struct TestClientCommon : public mpt::DaemonTestFixture std::make_unique>()}; std::unique_ptr mock_cert_store{std::make_unique()}; const mpt::MockUtils::GuardedMock utils_attr{mpt::MockUtils::inject()}; - mpt::MockUtils* mock_utils = utils_attr.first; + const mpt::MockUtils* mock_utils = utils_attr.first; const mpt::MockPermissionUtils::GuardedMock mock_permission_utils_injection = mpt::MockPermissionUtils::inject(); From 1ceadc38690dad2899ab3e2ca98bb10bba4b9da2 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 31 Jan 2025 11:41:49 +0100 Subject: [PATCH 016/114] [unit test] added constness to MockUtils and corresponding function. --- include/multipass/utils.h | 2 +- src/utils/utils.cpp | 2 +- tests/mock_utils.h | 2 +- tests/test_cli_client.cpp | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/include/multipass/utils.h b/include/multipass/utils.h index e55de02a3b..73f9936a8a 100644 --- a/include/multipass/utils.h +++ b/include/multipass/utils.h @@ -203,7 +203,7 @@ class Utils : public Singleton Utils(const Singleton::PrivatePass&) noexcept; virtual qint64 filesystem_bytes_available(const QString& data_directory) const; - virtual void exit(int code); + virtual void exit(int code) const; virtual std::string contents_of(const multipass::Path& file_path) const; virtual void make_file_with_content(const std::string& file_name, const std::string& content, const bool& overwrite = false); diff --git a/src/utils/utils.cpp b/src/utils/utils.cpp index cf145f42cd..877b6ece06 100644 --- a/src/utils/utils.cpp +++ b/src/utils/utils.cpp @@ -67,7 +67,7 @@ qint64 mp::Utils::filesystem_bytes_available(const QString& data_directory) cons return QStorageInfo(QDir(data_directory)).bytesAvailable(); } -void mp::Utils::exit(int code) +void mp::Utils::exit(int code) const { std::exit(code); } diff --git a/tests/mock_utils.h b/tests/mock_utils.h index a12683b214..c2698257e8 100644 --- a/tests/mock_utils.h +++ b/tests/mock_utils.h @@ -32,7 +32,7 @@ class MockUtils : public Utils public: using Utils::Utils; MOCK_METHOD(qint64, filesystem_bytes_available, (const QString&), (const, override)); - MOCK_METHOD(void, exit, (int), (override)); + MOCK_METHOD(void, exit, (int), (const override)); MOCK_METHOD(std::string, run_cmd_for_output, (const QString&, const QStringList&, const int), (const, override)); MOCK_METHOD(bool, run_cmd_for_status, (const QString&, const QStringList&, const int), (const, override)); MOCK_METHOD(std::string, contents_of, (const multipass::Path&), (const, override)); diff --git a/tests/test_cli_client.cpp b/tests/test_cli_client.cpp index 50226881e6..b1a580130a 100644 --- a/tests/test_cli_client.cpp +++ b/tests/test_cli_client.cpp @@ -383,7 +383,7 @@ struct Client : public Test const mpt::MockPlatform::GuardedMock platform_attr{mpt::MockPlatform::inject()}; const mpt::MockPlatform* mock_platform = platform_attr.first; const mpt::MockUtils::GuardedMock utils_attr{mpt::MockUtils::inject()}; - mpt::MockUtils* mock_utils = utils_attr.first; + const mpt::MockUtils* mock_utils = utils_attr.first; mpt::StubCertStore cert_store; StrictMock mock_daemon{server_address, *daemon_cert_provider, From 92379b2fcdf62dbba564a53b378c626e2fe2baa1 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 31 Jan 2025 12:43:45 +0100 Subject: [PATCH 017/114] [unit test] fixed the format. --- tests/test_client_common.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_common.cpp b/tests/test_client_common.cpp index bd5d82beb6..e9c7aa3458 100644 --- a/tests/test_client_common.cpp +++ b/tests/test_client_common.cpp @@ -45,7 +45,7 @@ struct TestClientCommon : public mpt::DaemonTestFixture ON_CALL(mpt::MockStandardPaths::mock_instance(), writableLocation(mp::StandardPaths::GenericDataLocation)) .WillByDefault(Return(temp_dir.path())); ON_CALL(*mock_utils, contents_of(_)).WillByDefault(Return(multipass::test::root_cert)); - //delegate some functions to the orignal implementation + // delegate some functions to the orignal implementation ON_CALL(*mock_utils, make_dir(A(), A(), A())) .WillByDefault([](const QDir& dir, const QString& name, std::filesystem::perms permissions) -> mp::Path { return MP_UTILS.Utils::make_dir(dir, name, permissions); From 54a0093b93fbbe6cedc5a390db49c0375bb10876 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 31 Jan 2025 15:10:12 +0100 Subject: [PATCH 018/114] [ssl cert] added some missed constness. --- src/cert/ssl_cert_provider.cpp | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 0c6a95c80e..1921e78693 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -41,10 +41,10 @@ namespace class WritableFile { public: - explicit WritableFile(const QString& name) : fp{fopen(name.toStdString().c_str(), "wb"), fclose} + explicit WritableFile(const QString& file_path) : fp{fopen(file_path.toStdString().c_str(), "wb"), fclose} { if (fp == nullptr) - throw std::runtime_error(fmt::format("failed to open file '{}': {}({})", name, strerror(errno), errno)); + throw std::runtime_error(fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); } FILE* get() const @@ -88,7 +88,7 @@ class EVPKey return mem.as_string(); } - void write(const QString& key_path) + void write(const QString& key_path) const { WritableFile file{key_path}; if (!PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr)) @@ -313,7 +313,7 @@ class X509Cert throw std::runtime_error("Failed to sign certificate"); } - std::string as_pem() + std::string as_pem() const { mp::BIOMem mem; auto bytes = PEM_write_bio_X509(mem.get(), x509.get()); @@ -322,7 +322,7 @@ class X509Cert return mem.as_string(); } - void write(const QString& cert_path) + void write(const QString& cert_path) const { WritableFile file{cert_path}; if (!PEM_write_X509(file.get(), x509.get())) @@ -335,10 +335,10 @@ class X509Cert mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const std::string& server_name) { - QString prefix = server_name.empty() ? "multipass_cert" : QString::fromStdString(server_name); + const QString prefix = server_name.empty() ? "multipass_cert" : QString::fromStdString(server_name); - auto priv_key_path = cert_dir.filePath(prefix + "_key.pem"); - auto cert_path = cert_dir.filePath(prefix + ".pem"); + const auto priv_key_path = cert_dir.filePath(prefix + "_key.pem"); + const auto cert_path = cert_dir.filePath(prefix + ".pem"); if (QFile::exists(priv_key_path) && QFile::exists(cert_path)) { @@ -347,24 +347,24 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, if (!server_name.empty()) { - EVPKey root_cert_key; + const EVPKey root_cert_key; const auto priv_root_key_path = cert_dir.filePath(prefix + "_root_key.pem"); const std::filesystem::path root_cert_path = MP_PLATFORM.get_root_cert_path(); - X509Cert root_cert{root_cert_key}; + const X509Cert root_cert{root_cert_key}; root_cert_key.write(priv_root_key_path); root_cert.write(root_cert_path.u8string().c_str()); - EVPKey server_cert_key; - X509Cert signed_server_cert{root_cert_key, root_cert, server_cert_key, server_name}; + const EVPKey server_cert_key; + const X509Cert signed_server_cert{root_cert_key, root_cert, server_cert_key, server_name}; server_cert_key.write(priv_key_path); signed_server_cert.write(cert_path); return {signed_server_cert.as_pem(), server_cert_key.as_pem()}; } else { - EVPKey client_cert_key; - X509Cert client_cert{client_cert_key, server_name}; + const EVPKey client_cert_key; + const X509Cert client_cert{client_cert_key, server_name}; client_cert_key.write(priv_key_path); client_cert.write(cert_path); From e0253736313cb28adb666156bb10394b24ccbff7 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 31 Jan 2025 16:42:54 +0100 Subject: [PATCH 019/114] [unit test] removed daemon_cert and daemon_key. --- src/cert/ssl_cert_provider.cpp | 3 +- tests/mock_cert_provider.h | 56 ++++++++++++---------------------- tests/test_client_common.cpp | 8 ++--- tests/unix/test_daemon_rpc.cpp | 24 +++++++-------- 4 files changed, 38 insertions(+), 53 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 1921e78693..99cf7c4e0f 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -44,7 +44,8 @@ class WritableFile explicit WritableFile(const QString& file_path) : fp{fopen(file_path.toStdString().c_str(), "wb"), fclose} { if (fp == nullptr) - throw std::runtime_error(fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); + throw std::runtime_error( + fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); } FILE* get() const diff --git a/tests/mock_cert_provider.h b/tests/mock_cert_provider.h index 289a2da504..16d4025ee1 100644 --- a/tests/mock_cert_provider.h +++ b/tests/mock_cert_provider.h @@ -37,48 +37,32 @@ constexpr auto root_cert = "-----BEGIN CERTIFICATE-----\n" "xgvZMY2ColjLunUiNG8H096n\n" "-----END CERTIFICATE-----\n"; -constexpr auto client_cert = "-----BEGIN CERTIFICATE-----\n" - "MIIByjCCAXCgAwIBAgIENvdePTAKBggqhkjOPQQDAjA9MQswCQYDVQQGEwJVUzES\n" - "MBAGA1UECgwJQ2Fub25pY2FsMRowGAYDVQQDDBFNdWx0aXBhc3MgUm9vdCBDQTAe\n" - "Fw0yNTAxMjkxMzAzNDBaFw0yNjAxMjkxMzAzNDBaMDUxCzAJBgNVBAYTAlVTMRIw\n" - "EAYDVQQKDAlDYW5vbmljYWwxEjAQBgNVBAMMCWxvY2FsaG9zdDBZMBMGByqGSM49\n" - "AgEGCCqGSM49AwEHA0IABGAw4mRhGqCg7uSIsVgBIzMOoGnlEFWga2dxUzA1YwNe\n" - "8SB679smyb7KVsPg4fK/P7XS4ORxSnMVnKWvTAfYKXWjZjBkMBQGA1UdEQQNMAuC\n" - "CWxvY2FsaG9zdDAdBgNVHQ4EFgQU++FdgRpFokGT+7Fdgqe4SxmSD9UwHwYDVR0j\n" - "BBgwFoAUBiVDL5AyFI6H+AAKyBJ1Zsgfk2gwDAYDVR0TAQH/BAIwADAKBggqhkjO\n" - "PQQDAgNIADBFAiAesF7z8ItZVxK6fgUwhWfgN5rUFzCO5tBGJFDHU7eIZgIhALdl\n" - "2mAn2oocQZfHohrbVUIuWDiUr0SxOkdGUISX0ElJ\n" - "-----END CERTIFICATE-----\n"; +// cert and key are used as both server certificate and client certificate in the unit test environment +constexpr auto cert = "-----BEGIN CERTIFICATE-----\n" + "MIIByjCCAXCgAwIBAgIENvdePTAKBggqhkjOPQQDAjA9MQswCQYDVQQGEwJVUzES\n" + "MBAGA1UECgwJQ2Fub25pY2FsMRowGAYDVQQDDBFNdWx0aXBhc3MgUm9vdCBDQTAe\n" + "Fw0yNTAxMjkxMzAzNDBaFw0yNjAxMjkxMzAzNDBaMDUxCzAJBgNVBAYTAlVTMRIw\n" + "EAYDVQQKDAlDYW5vbmljYWwxEjAQBgNVBAMMCWxvY2FsaG9zdDBZMBMGByqGSM49\n" + "AgEGCCqGSM49AwEHA0IABGAw4mRhGqCg7uSIsVgBIzMOoGnlEFWga2dxUzA1YwNe\n" + "8SB679smyb7KVsPg4fK/P7XS4ORxSnMVnKWvTAfYKXWjZjBkMBQGA1UdEQQNMAuC\n" + "CWxvY2FsaG9zdDAdBgNVHQ4EFgQU++FdgRpFokGT+7Fdgqe4SxmSD9UwHwYDVR0j\n" + "BBgwFoAUBiVDL5AyFI6H+AAKyBJ1Zsgfk2gwDAYDVR0TAQH/BAIwADAKBggqhkjO\n" + "PQQDAgNIADBFAiAesF7z8ItZVxK6fgUwhWfgN5rUFzCO5tBGJFDHU7eIZgIhALdl\n" + "2mAn2oocQZfHohrbVUIuWDiUr0SxOkdGUISX0ElJ\n" + "-----END CERTIFICATE-----\n"; -constexpr auto client_key = "-----BEGIN PRIVATE KEY-----\n" - "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgwRNA3VMqakM32i0C\n" - "PHE5i4qRNGdvgXtCWwpp0gXv+oGhRANCAARgMOJkYRqgoO7kiLFYASMzDqBp5RBV\n" - "oGtncVMwNWMDXvEgeu/bJsm+ylbD4OHyvz+10uDkcUpzFZylr0wH2Cl1\n" - "-----END PRIVATE KEY-----\n"; - -constexpr auto daemon_cert = "-----BEGIN CERTIFICATE-----\n" - "MIIBUjCB+AIBKjAKBggqhkjOPQQDAjA1MQswCQYDVQQGEwJDQTESMBAGA1UECgwJ\n" - "Q2Fub25pY2FsMRIwEAYDVQQDDAlsb2NhbGhvc3QwHhcNMTgwNjIxMTM0MjI5WhcN\n" - "MTkwNjIxMTM0MjI5WjA1MQswCQYDVQQGEwJDQTESMBAGA1UECgwJQ2Fub25pY2Fs\n" - "MRIwEAYDVQQDDAlsb2NhbGhvc3QwWTATBgcqhkjOPQIBBggqhkjOPQMBBwNCAAQA\n" - "FGNAqq7c5IMDeQ/cV4+EmogmkfpbTLSPfXgXVLHRsvL04xUAkqGpL+eyGFVE6dqa\n" - "J7sAPJJwlVj1xD0r5DX5MAoGCCqGSM49BAMCA0kAMEYCIQCvI0PYv9f201fbe4LP\n" - "BowTeYWSqMQtLNjvZgd++AAGhgIhALNPW+NRSKCXwadiIFgpbjPInLPqXPskLWSc\n" - "aXByaQyt\n" - "-----END CERTIFICATE-----\n"; - -constexpr auto daemon_key = "-----BEGIN PRIVATE KEY-----\n" - "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgsSAz5ggzrLjai0I/\n" - "F0hYg5oG/shpXJiBQtJdBCG3lUShRANCAAQAFGNAqq7c5IMDeQ/cV4+Emogmkfpb\n" - "TLSPfXgXVLHRsvL04xUAkqGpL+eyGFVE6dqaJ7sAPJJwlVj1xD0r5DX5\n" - "-----END PRIVATE KEY-----\n"; +constexpr auto key = "-----BEGIN PRIVATE KEY-----\n" + "MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgwRNA3VMqakM32i0C\n" + "PHE5i4qRNGdvgXtCWwpp0gXv+oGhRANCAARgMOJkYRqgoO7kiLFYASMzDqBp5RBV\n" + "oGtncVMwNWMDXvEgeu/bJsm+ylbD4OHyvz+10uDkcUpzFZylr0wH2Cl1\n" + "-----END PRIVATE KEY-----\n"; struct MockCertProvider : public CertProvider { MockCertProvider() { - ON_CALL(*this, PEM_certificate).WillByDefault(Return(client_cert)); - ON_CALL(*this, PEM_signing_key).WillByDefault(Return(client_key)); + ON_CALL(*this, PEM_certificate).WillByDefault(Return(cert)); + ON_CALL(*this, PEM_signing_key).WillByDefault(Return(key)); } MOCK_METHOD(std::string, PEM_certificate, (), (override, const)); diff --git a/tests/test_client_common.cpp b/tests/test_client_common.cpp index e9c7aa3458..3fd979a1b9 100644 --- a/tests/test_client_common.cpp +++ b/tests/test_client_common.cpp @@ -58,8 +58,8 @@ struct TestClientCommon : public mpt::DaemonTestFixture mpt::MockDaemon make_secure_server() { - EXPECT_CALL(*mock_cert_provider, PEM_certificate()).WillOnce(Return(mpt::daemon_cert)); - EXPECT_CALL(*mock_cert_provider, PEM_signing_key()).WillOnce(Return(mpt::daemon_key)); + EXPECT_CALL(*mock_cert_provider, PEM_certificate()).Times(1); + EXPECT_CALL(*mock_cert_provider, PEM_signing_key()).Times(1); config_builder.server_address = server_address; config_builder.cert_provider = std::move(mock_cert_provider); @@ -87,8 +87,8 @@ TEST_F(TestClientCommon, usesCommonCertWhenItExists) const auto common_client_cert_file = common_cert_dir + "/" + mp::client_cert_file; const auto common_client_key_file = common_cert_dir + "/" + mp::client_key_file; - mpt::make_file_with_content(common_client_cert_file, mpt::client_cert); - mpt::make_file_with_content(common_client_key_file, mpt::client_key); + mpt::make_file_with_content(common_client_cert_file, mpt::cert); + mpt::make_file_with_content(common_client_key_file, mpt::key); EXPECT_TRUE(mp::client::make_channel(server_address, *mp::client::get_cert_provider())); } diff --git a/tests/unix/test_daemon_rpc.cpp b/tests/unix/test_daemon_rpc.cpp index 022662cee0..33c4b3633b 100644 --- a/tests/unix/test_daemon_rpc.cpp +++ b/tests/unix/test_daemon_rpc.cpp @@ -49,8 +49,8 @@ struct TestDaemonRpc : public mpt::DaemonTestFixture auto opts = grpc::SslCredentialsOptions(); opts.pem_root_certs = mpt::root_cert; opts.server_certificate_request = GRPC_SSL_REQUEST_SERVER_CERTIFICATE_AND_VERIFY; - opts.pem_cert_chain = mpt::client_cert; - opts.pem_private_key = mpt::client_key; + opts.pem_cert_chain = mpt::cert; + opts.pem_private_key = mpt::key; auto channel = grpc::CreateChannel(server_address, grpc::SslCredentials(opts)); @@ -115,7 +115,7 @@ TEST_F(TestDaemonRpc, authenticateCompletesSuccessfully) EXPECT_CALL(*mock_platform, set_server_socket_restrictions(_, false)).Times(1); EXPECT_CALL(*mock_cert_store, empty()).WillOnce(Return(true)); - EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::client_cert))).Times(1); + EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::cert))).Times(1); mpt::MockDaemon daemon{make_secure_server()}; EXPECT_CALL(daemon, authenticate(_, _, _)).WillOnce([](auto, auto, auto* status_promise) { @@ -165,7 +165,7 @@ TEST_F(TestDaemonRpc, pingReturnsOkWhenCertIsVerified) EXPECT_CALL(*mock_platform, set_server_socket_restrictions(_, false)).Times(1); EXPECT_CALL(*mock_cert_store, empty()).WillOnce(Return(false)); - EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::client_cert))).WillOnce(Return(true)); + EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::cert))).WillOnce(Return(true)); mpt::MockDaemon daemon{make_secure_server()}; mp::Rpc::Stub stub{make_secure_stub()}; @@ -182,7 +182,7 @@ TEST_F(TestDaemonRpc, pingReturnsUnauthenticatedWhenCertIsNotVerified) EXPECT_CALL(*mock_platform, set_server_socket_restrictions(_, false)).Times(1); EXPECT_CALL(*mock_cert_store, empty()).WillOnce(Return(false)); - EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::client_cert))).WillOnce(Return(false)); + EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::cert))).WillOnce(Return(false)); mpt::MockDaemon daemon{make_secure_server()}; mp::Rpc::Stub stub{make_secure_stub()}; @@ -200,7 +200,7 @@ TEST_F(TestDaemonRpc, listCertExistsCompletesSuccessfully) EXPECT_CALL(*mock_platform, set_server_socket_restrictions(_, false)).Times(1); EXPECT_CALL(*mock_cert_store, empty()).Times(2).WillRepeatedly(Return(false)); - EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::client_cert))).WillOnce(Return(true)); + EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::cert))).WillOnce(Return(true)); mpt::MockDaemon daemon{make_secure_server()}; mock_empty_list_reply(daemon); @@ -214,7 +214,7 @@ TEST_F(TestDaemonRpc, listNoCertsExistWillVerifyAndComplete) EXPECT_CALL(*mock_platform, set_server_socket_restrictions(_, false)).Times(1); EXPECT_CALL(*mock_cert_store, empty()).Times(2).WillRepeatedly(Return(true)); - EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::client_cert))).Times(1); + EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::cert))).Times(1); mpt::MockDaemon daemon{make_secure_server()}; mock_empty_list_reply(daemon); @@ -227,7 +227,7 @@ TEST_F(TestDaemonRpc, listCertNotVerifiedHasError) EXPECT_CALL(*mock_platform, set_server_socket_restrictions(_, false)).Times(1); EXPECT_CALL(*mock_cert_store, empty()).Times(2).WillRepeatedly(Return(false)); - EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::client_cert))).WillOnce(Return(false)); + EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::cert))).WillOnce(Return(false)); mpt::MockDaemon daemon{make_secure_server()}; @@ -247,8 +247,8 @@ TEST_F(TestDaemonRpc, listTCPSocketNoCertsExistHasError) EXPECT_CALL(*mock_platform, set_server_socket_restrictions).Times(1); EXPECT_CALL(*mock_cert_store, empty()).Times(1); - EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::client_cert))).Times(0); - EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::client_cert))).WillOnce(Return(false)); + EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::cert))).Times(0); + EXPECT_CALL(*mock_cert_store, verify_cert(StrEq(mpt::cert))).WillOnce(Return(false)); mpt::MockDaemon daemon{make_secure_server()}; @@ -267,7 +267,7 @@ TEST_F(TestDaemonRpc, listAcceptCertFailsHasError) EXPECT_CALL(*mock_platform, set_server_socket_restrictions(_, true)).Times(1); EXPECT_CALL(*mock_cert_store, empty()).Times(2).WillRepeatedly(Return(true)); - EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::client_cert))).WillOnce(Throw(std::runtime_error(error_msg))); + EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::cert))).WillOnce(Throw(std::runtime_error(error_msg))); mpt::MockDaemon daemon{make_secure_server()}; @@ -287,7 +287,7 @@ TEST_F(TestDaemonRpc, listSettingServerPermissionsFailLogsErrorAndExits) .WillOnce(Throw(std::runtime_error(error_msg))); EXPECT_CALL(*mock_cert_store, empty()).Times(2).WillRepeatedly(Return(true)); - EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::client_cert))).Times(1); + EXPECT_CALL(*mock_cert_store, add_cert(StrEq(mpt::cert))).Times(1); // Detects if the daemon would actually exit EXPECT_CALL(*mock_utils, exit(EXIT_FAILURE)).Times(1); From 8ee9d14913f76f9b7e9a2219de83fb27d834f104 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 31 Jan 2025 17:35:29 +0100 Subject: [PATCH 020/114] [ssl cert] Using OpenSSL's built-in function to create SAN field. --- src/cert/ssl_cert_provider.cpp | 24 +++++------------------- 1 file changed, 5 insertions(+), 19 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 99cf7c4e0f..f25e66db5b 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -119,21 +119,6 @@ std::string cn_name_from(const std::string& server_name) return server_name; } -void set_san_name(X509* c, const std::string& server_name) -{ - std::string san_dns = server_name; - GENERAL_NAMES* gens = sk_GENERAL_NAME_new_null(); - GENERAL_NAME* gen = GENERAL_NAME_new(); - ASN1_IA5STRING* ia5 = ASN1_IA5STRING_new(); - ASN1_STRING_set(ia5, san_dns.data(), san_dns.length()); - GENERAL_NAME_set0_value(gen, GEN_DNS, ia5); - sk_GENERAL_NAME_push(gens, gen); - - X509_add1_ext_i2d(c, NID_subject_alt_name, gens, 0, X509V3_ADD_DEFAULT); - - GENERAL_NAMES_free(gens); -} - class X509Cert { public: @@ -165,7 +150,6 @@ class X509Cert X509_NAME_add_entry_by_txt(name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); X509_set_issuer_name(x509.get(), name); - set_san_name(x509.get(), server_name); if (!X509_set_pubkey(x509.get(), key.get())) throw std::runtime_error("Failed to set certificate public key"); @@ -202,7 +186,6 @@ class X509Cert X509_NAME_add_entry_by_txt(name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); X509_set_issuer_name(x509.get(), name); - // set_san_name(x509.get(), server_name); if (!X509_set_pubkey(x509.get(), key.get())) throw std::runtime_error("Failed to set certificate public key"); @@ -284,8 +267,6 @@ class X509Cert // Set issuer name (from root certificate) X509_set_issuer_name(x509.get(), X509_get_subject_name(root_certificate.x509.get())); - set_san_name(x509.get(), server_name); - if (!X509_set_pubkey(x509.get(), server_key.get())) throw std::runtime_error("Failed to set certificate public key"); @@ -294,7 +275,12 @@ class X509Cert X509V3_set_ctx(&ctx, root_certificate.x509.get(), x509.get(), NULL, NULL, 0); // wrap into function or struct + X509_EXTENSION* ext; + // Subject Alternative Name + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); // Subject Key Identifier ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); X509_add_ext(x509.get(), ext, -1); From ec7cdb2fe543a29127ae1c754c8e0e7110101c99 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Sat, 1 Feb 2025 16:15:02 +0100 Subject: [PATCH 021/114] [ssl cert] use default argument instead of function overload. --- include/multipass/ssl_cert_provider.h | 3 +-- src/cert/ssl_cert_provider.cpp | 4 ---- 2 files changed, 1 insertion(+), 6 deletions(-) diff --git a/include/multipass/ssl_cert_provider.h b/include/multipass/ssl_cert_provider.h index a038bd59ca..383befb6c2 100644 --- a/include/multipass/ssl_cert_provider.h +++ b/include/multipass/ssl_cert_provider.h @@ -34,8 +34,7 @@ class SSLCertProvider : public CertProvider std::string pem_priv_key; }; - explicit SSLCertProvider(const Path& data_dir); - SSLCertProvider(const Path& data_dir, const std::string& server_name); + SSLCertProvider(const Path& data_dir, const std::string& server_name = ""); std::string PEM_certificate() const override; std::string PEM_signing_key() const override; diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index f25e66db5b..3a4b4c41da 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -365,10 +365,6 @@ mp::SSLCertProvider::SSLCertProvider(const multipass::Path& cert_dir, const std: { } -mp::SSLCertProvider::SSLCertProvider(const multipass::Path& data_dir) : SSLCertProvider(data_dir, "") -{ -} - std::string mp::SSLCertProvider::PEM_certificate() const { return key_cert_pair.pem_cert; From 33d25a4a3796abefaac5898c0b2cec1a5abdc968 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Sun, 2 Feb 2025 11:41:47 +0100 Subject: [PATCH 022/114] [ssl cert] remove the cn_name_from function since the dispatch is done via X509Cert constructor overload. --- src/cert/ssl_cert_provider.cpp | 13 +++---------- 1 file changed, 3 insertions(+), 10 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 3a4b4c41da..a1a08d3909 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -112,17 +112,10 @@ std::vector as_vector(const std::string& v) return {v.begin(), v.end()}; } -std::string cn_name_from(const std::string& server_name) -{ - if (server_name.empty()) - return mp::utils::make_uuid().toStdString(); - return server_name; -} - class X509Cert { public: - explicit X509Cert(const EVPKey& key, const std::string& server_name) + explicit X509Cert(const EVPKey& key, const std::string& server_name) // generate client certificate only { if (x509 == nullptr) throw std::runtime_error("Failed to allocate x509 cert structure"); @@ -143,7 +136,7 @@ class X509Cert auto country = as_vector("US"); auto org = as_vector("Canonical"); - auto cn = as_vector(cn_name_from(server_name)); + auto cn = as_vector(mp::utils::make_uuid().toStdString()); auto name = X509_get_subject_name(x509.get()); X509_NAME_add_entry_by_txt(name, "C", MBSTRING_ASC, country.data(), country.size(), APPEND_ENTRY, ADD_RDN); @@ -240,7 +233,7 @@ class X509Cert const auto country = as_vector("US"); const auto org = as_vector("Canonical"); - const auto cn = as_vector(cn_name_from(server_name)); + const auto cn = as_vector(server_name); const auto server_certificate_name = X509_get_subject_name(x509.get()); X509_NAME_add_entry_by_txt(server_certificate_name, From a017c44ebcfcc29a950a64d6e5fd6e71bbb0dbc9 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Sun, 2 Feb 2025 11:43:42 +0100 Subject: [PATCH 023/114] [ssl cert] added cert type enum to facilitate the deduplication of the 3 X509Cert constructors. --- src/cert/ssl_cert_provider.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index a1a08d3909..039634339b 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -115,6 +115,13 @@ std::vector as_vector(const std::string& v) class X509Cert { public: + enum class CertType + { + Root, + Client, + Server + }; + explicit X509Cert(const EVPKey& key, const std::string& server_name) // generate client certificate only { if (x509 == nullptr) From 3795eb902bc31cd8846be72c1ca76abf2011ec8f Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Sun, 2 Feb 2025 20:45:03 +0100 Subject: [PATCH 024/114] [ssl cert] merged the root, client, signed server certificate generation x509 constructor into one. --- src/cert/ssl_cert_provider.cpp | 180 +++++++++------------------------ 1 file changed, 46 insertions(+), 134 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 039634339b..46a393caa9 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -122,43 +122,12 @@ class X509Cert Server }; - explicit X509Cert(const EVPKey& key, const std::string& server_name) // generate client certificate only - { - if (x509 == nullptr) - throw std::runtime_error("Failed to allocate x509 cert structure"); - - long big_num{0}; - auto rand_bytes = MP_UTILS.random_bytes(4); - for (unsigned int i = 0; i < 4u; i++) - big_num |= rand_bytes[i] << i * 8u; - - X509_set_version(x509.get(), 2); - - ASN1_INTEGER_set(X509_get_serialNumber(x509.get()), big_num); - X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); - X509_gmtime_adj(X509_get_notAfter(x509.get()), 31536000L); - - constexpr int APPEND_ENTRY{-1}; - constexpr int ADD_RDN{0}; - - auto country = as_vector("US"); - auto org = as_vector("Canonical"); - auto cn = as_vector(mp::utils::make_uuid().toStdString()); - - auto name = X509_get_subject_name(x509.get()); - X509_NAME_add_entry_by_txt(name, "C", MBSTRING_ASC, country.data(), country.size(), APPEND_ENTRY, ADD_RDN); - X509_NAME_add_entry_by_txt(name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); - X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); - X509_set_issuer_name(x509.get(), name); - - if (!X509_set_pubkey(x509.get(), key.get())) - throw std::runtime_error("Failed to set certificate public key"); - - if (!X509_sign(x509.get(), key.get(), EVP_sha256())) - throw std::runtime_error("Failed to sign certificate"); - } - - explicit X509Cert(const EVPKey& key) // generate root certificate only + explicit X509Cert(const EVPKey& key, + CertType cert_type, + const std::string& server_name = "", + const std::optional& root_certificate_key = std::nullopt, + const std::optional& root_certificate = std::nullopt) + // generate root, client or signed server certificate, the third one requires the last three arguments populated { if (x509 == nullptr) throw std::runtime_error("Failed to allocate x509 cert structure"); @@ -171,132 +140,71 @@ class X509Cert X509_set_version(x509.get(), 2); // X.509 v3 ASN1_INTEGER_set(X509_get_serialNumber(x509.get()), big_num); - X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); // Start time: now - X509_gmtime_adj(X509_get_notAfter(x509.get()), 3650L * 24L * 60L * 60L); // Valid for 10 years - - constexpr int APPEND_ENTRY{-1}; - constexpr int ADD_RDN{0}; - - const auto country = as_vector("US"); - const auto org = as_vector("Canonical"); - const auto cn = as_vector("Multipass Root CA"); - - const auto name = X509_get_subject_name(x509.get()); - X509_NAME_add_entry_by_txt(name, "C", MBSTRING_ASC, country.data(), country.size(), APPEND_ENTRY, ADD_RDN); - X509_NAME_add_entry_by_txt(name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); - X509_NAME_add_entry_by_txt(name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); - X509_set_issuer_name(x509.get(), name); - - if (!X509_set_pubkey(x509.get(), key.get())) - throw std::runtime_error("Failed to set certificate public key"); - - // Add X509v3 extensions - X509V3_CTX ctx; - X509V3_set_ctx(&ctx, x509.get(), x509.get(), NULL, NULL, 0); - - // wrap into function or struct - X509_EXTENSION* ext; - // Subject Key Identifier - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); - X509_add_ext(x509.get(), ext, -1); - X509_EXTENSION_free(ext); - - // Authority Key Identifier - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, "keyid:always"); - X509_add_ext(x509.get(), ext, -1); - X509_EXTENSION_free(ext); - - // Basic Constraints: critical, CA:TRUE - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, "critical,CA:TRUE"); - X509_add_ext(x509.get(), ext, -1); - X509_EXTENSION_free(ext); - - if (!X509_sign(x509.get(), key.get(), EVP_sha256())) - throw std::runtime_error("Failed to sign certificate"); - } - - X509Cert(const EVPKey& root_certificate_key, - const X509Cert& root_certificate, - const EVPKey& server_key, - const std::string& server_name) - // generate signed server certificates - { - if (x509 == nullptr) - throw std::runtime_error("Failed to allocate x509 cert structure"); - - long big_num{0}; - auto rand_bytes = MP_UTILS.random_bytes(4); - for (unsigned int i = 0; i < 4u; i++) - big_num |= rand_bytes[i] << i * 8u; - - X509_set_version(x509.get(), 2); - - ASN1_INTEGER_set(X509_get_serialNumber(x509.get()), big_num); - X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); - X509_gmtime_adj(X509_get_notAfter(x509.get()), 31536000L); // Valid for 10 years + X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); // Start time: now + const long valid_duration_sec = cert_type == CertType::Root ? 3650L * 24L * 60L * 60L : 365L * 24L * 60L * 60L; + // 10 years for root certicicate and 1 year for server and client certificate + X509_gmtime_adj(X509_get_notAfter(x509.get()), valid_duration_sec); constexpr int APPEND_ENTRY{-1}; constexpr int ADD_RDN{0}; const auto country = as_vector("US"); const auto org = as_vector("Canonical"); - const auto cn = as_vector(server_name); - - const auto server_certificate_name = X509_get_subject_name(x509.get()); - X509_NAME_add_entry_by_txt(server_certificate_name, + const auto cn = as_vector(cert_type == CertType::Root ? "Multipass Root CA" + : cert_type == CertType::Client ? mp::utils::make_uuid().toStdString() + : server_name); + const auto subject_name = X509_get_subject_name(x509.get()); + X509_NAME_add_entry_by_txt(subject_name, "C", MBSTRING_ASC, country.data(), country.size(), APPEND_ENTRY, ADD_RDN); - X509_NAME_add_entry_by_txt(server_certificate_name, - "O", - MBSTRING_ASC, - org.data(), - org.size(), - APPEND_ENTRY, - ADD_RDN); - X509_NAME_add_entry_by_txt(server_certificate_name, - "CN", - MBSTRING_ASC, - cn.data(), - cn.size(), - APPEND_ENTRY, - ADD_RDN); - // Set issuer name (from root certificate) - X509_set_issuer_name(x509.get(), X509_get_subject_name(root_certificate.x509.get())); + X509_NAME_add_entry_by_txt(subject_name, "O", MBSTRING_ASC, org.data(), org.size(), APPEND_ENTRY, ADD_RDN); + X509_NAME_add_entry_by_txt(subject_name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); - if (!X509_set_pubkey(x509.get(), server_key.get())) + const auto issuer_name = + cert_type == CertType::Server ? X509_get_subject_name(root_certificate.value().x509.get()) : subject_name; + X509_set_issuer_name(x509.get(), issuer_name); + + if (!X509_set_pubkey(x509.get(), key.get())) throw std::runtime_error("Failed to set certificate public key"); + const auto& issuer_x509 = cert_type == CertType::Server ? root_certificate.value().x509 : x509; // Add X509v3 extensions X509V3_CTX ctx; - X509V3_set_ctx(&ctx, root_certificate.x509.get(), x509.get(), NULL, NULL, 0); + X509V3_set_ctx(&ctx, issuer_x509.get(), x509.get(), NULL, NULL, 0); // wrap into function or struct - X509_EXTENSION* ext; // Subject Alternative Name - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); - X509_add_ext(x509.get(), ext, -1); - X509_EXTENSION_free(ext); + if (cert_type == CertType::Server) + { + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); + X509_add_ext(x509.get(), ext, -1); + X509_EXTENSION_free(ext); + } + // Subject Key Identifier ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); X509_add_ext(x509.get(), ext, -1); X509_EXTENSION_free(ext); // Authority Key Identifier - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, "keyid:always,issuer"); + const std::string is_from_issuer = cert_type == CertType::Server ? ",issuer" : ""; + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, ("keyid:always" + is_from_issuer).c_str()); X509_add_ext(x509.get(), ext, -1); X509_EXTENSION_free(ext); - // Basic Constraints: critical, CA:FALSE - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, "critical,CA:FALSE"); + // Basic Constraints: critical, CA:TRUE or CA:FALSE + const std::string is_ca = cert_type == CertType::Root ? "TRUE" : "FALSE"; + ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, ("critical,CA:" + is_ca).c_str()); X509_add_ext(x509.get(), ext, -1); X509_EXTENSION_free(ext); - if (!X509_sign(x509.get(), root_certificate_key.get(), EVP_sha256())) + const auto& signing_key = cert_type == CertType::Server ? *root_certificate_key : key; + if (!X509_sign(x509.get(), signing_key.get(), EVP_sha256())) throw std::runtime_error("Failed to sign certificate"); } @@ -334,16 +242,20 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, if (!server_name.empty()) { - const EVPKey root_cert_key; const auto priv_root_key_path = cert_dir.filePath(prefix + "_root_key.pem"); const std::filesystem::path root_cert_path = MP_PLATFORM.get_root_cert_path(); - const X509Cert root_cert{root_cert_key}; + EVPKey root_cert_key; + X509Cert root_cert{root_cert_key, X509Cert::CertType::Root}; root_cert_key.write(priv_root_key_path); root_cert.write(root_cert_path.u8string().c_str()); const EVPKey server_cert_key; - const X509Cert signed_server_cert{root_cert_key, root_cert, server_cert_key, server_name}; + const X509Cert signed_server_cert{server_cert_key, + X509Cert::CertType::Server, + server_name, + std::move(root_cert_key), + std::move(root_cert)}; server_cert_key.write(priv_key_path); signed_server_cert.write(cert_path); return {signed_server_cert.as_pem(), server_cert_key.as_pem()}; @@ -351,7 +263,7 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, else { const EVPKey client_cert_key; - const X509Cert client_cert{client_cert_key, server_name}; + const X509Cert client_cert{client_cert_key, X509Cert::CertType::Client}; client_cert_key.write(priv_key_path); client_cert.write(cert_path); From 192f6901a51a3522c166bb151b03bba4d812535e Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 14:17:01 +0100 Subject: [PATCH 025/114] [ssl cert] modernize the key generation by using openssl 3.0 C apis --- src/cert/ssl_cert_provider.cpp | 35 +++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 46a393caa9..64dbe7bd6d 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -22,6 +22,7 @@ #include "biomem.h" +#include #include #include #include @@ -62,22 +63,30 @@ class EVPKey public: EVPKey() { - if (key == nullptr) - throw std::runtime_error("Failed to allocate EVP_PKEY"); + std::unique_ptr ctx( + EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), + EVP_PKEY_CTX_free); - std::unique_ptr ec_key(EC_KEY_new_by_curve_name(NID_X9_62_prime256v1), - EC_KEY_free); - if (ec_key == nullptr) - throw std::runtime_error("Failed to allocate ec key structure"); + if (!ctx || EVP_PKEY_keygen_init(ctx.get()) <= 0) + { + throw std::runtime_error("Failed to initialize key generation"); + } - if (!EC_KEY_generate_key(ec_key.get())) - throw std::runtime_error("Failed to generate key"); + // Set EC curve (P-256) + OSSL_PARAM params[] = { + OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), + OSSL_PARAM_construct_end()}; + EVP_PKEY_CTX_set_params(ctx.get(), params); - if (!EVP_PKEY_assign_EC_KEY(key.get(), ec_key.get())) - throw std::runtime_error("Failed to assign key"); + // Generate the key + EVP_PKEY* raw_key = nullptr; + if (EVP_PKEY_generate(ctx.get(), &raw_key) <= 0) + { + throw std::runtime_error("Failed to generate EC key"); + } - // EVPKey has ownership now - ec_key.release(); + // Assign generated key to unique_ptr for RAII management + key.reset(raw_key); } std::string as_pem() const @@ -104,7 +113,7 @@ class EVPKey } private: - std::unique_ptr key{EVP_PKEY_new(), EVP_PKEY_free}; + std::unique_ptr key{nullptr, EVP_PKEY_free}; }; std::vector as_vector(const std::string& v) From 905a29c6380c28541fc7c8287a3672acb3b9fccb Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 14:24:41 +0100 Subject: [PATCH 026/114] [ssl cert] use decltype on function pointer directly. So it is consistent with other custom deleters. Meanwhile, it is also more explicit and preferred for clarity. --- src/cert/ssl_cert_provider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 64dbe7bd6d..f15fed516d 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -234,7 +234,7 @@ class X509Cert } private: - std::unique_ptr x509{X509_new(), X509_free}; + std::unique_ptr x509{X509_new(), X509_free}; }; mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const std::string& server_name) From a744702fe7a7a1f354a8c8034fda86a6e0c7a55d Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 14:29:14 +0100 Subject: [PATCH 027/114] [ssl cert] remove unneeded headers. --- src/cert/ssl_cert_provider.cpp | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index f15fed516d..614af6fb20 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -24,12 +24,8 @@ #include #include -#include -#include #include -#include - #include #include #include From 61a42825c0f54808d8be6e0a5d02ed00cb494413 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 14:33:28 +0100 Subject: [PATCH 028/114] [ssl cert] use std::array to replace C-style array to interface with function EVP_PKEY_CTX_set_params --- src/cert/ssl_cert_provider.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 614af6fb20..966cae2309 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -69,10 +69,11 @@ class EVPKey } // Set EC curve (P-256) - OSSL_PARAM params[] = { + const std::array params = { OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), - OSSL_PARAM_construct_end()}; - EVP_PKEY_CTX_set_params(ctx.get(), params); + OSSL_PARAM_construct_end() + }; + EVP_PKEY_CTX_set_params(ctx.get(), params.data()); // Generate the key EVP_PKEY* raw_key = nullptr; From a6e7a848390f6f0f86e34810feac30e048946b3e Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 14:43:02 +0100 Subject: [PATCH 029/114] [ssl cert] added comment on the OSSL_PARAM_construct_utf8_string function. --- src/cert/ssl_cert_provider.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 966cae2309..5d7c04472f 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -70,9 +70,9 @@ class EVPKey // Set EC curve (P-256) const std::array params = { + // the 3rd argument is length of the buffer, which is 0 in the case of static buffer like "P-256" OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), - OSSL_PARAM_construct_end() - }; + OSSL_PARAM_construct_end()}; EVP_PKEY_CTX_set_params(ctx.get(), params.data()); // Generate the key From 6f6e66b72cd361a27ee5f8e23becd0fc168d0131 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 15:39:57 +0100 Subject: [PATCH 030/114] [ssl cert] rename x509 to cert for better match with variable name EVPKey::Key. --- src/cert/ssl_cert_provider.cpp | 38 +++++++++++++++++----------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 5d7c04472f..4487e27ec3 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -135,7 +135,7 @@ class X509Cert const std::optional& root_certificate = std::nullopt) // generate root, client or signed server certificate, the third one requires the last three arguments populated { - if (x509 == nullptr) + if (cert == nullptr) throw std::runtime_error("Failed to allocate x509 cert structure"); long big_num{0}; @@ -143,13 +143,13 @@ class X509Cert for (unsigned int i = 0; i < 4u; i++) big_num |= rand_bytes[i] << i * 8u; - X509_set_version(x509.get(), 2); // X.509 v3 + X509_set_version(cert.get(), 2); // X.509 v3 - ASN1_INTEGER_set(X509_get_serialNumber(x509.get()), big_num); - X509_gmtime_adj(X509_get_notBefore(x509.get()), 0); // Start time: now + ASN1_INTEGER_set(X509_get_serialNumber(cert.get()), big_num); + X509_gmtime_adj(X509_get_notBefore(cert.get()), 0); // Start time: now const long valid_duration_sec = cert_type == CertType::Root ? 3650L * 24L * 60L * 60L : 365L * 24L * 60L * 60L; // 10 years for root certicicate and 1 year for server and client certificate - X509_gmtime_adj(X509_get_notAfter(x509.get()), valid_duration_sec); + X509_gmtime_adj(X509_get_notAfter(cert.get()), valid_duration_sec); constexpr int APPEND_ENTRY{-1}; constexpr int ADD_RDN{0}; @@ -159,7 +159,7 @@ class X509Cert const auto cn = as_vector(cert_type == CertType::Root ? "Multipass Root CA" : cert_type == CertType::Client ? mp::utils::make_uuid().toStdString() : server_name); - const auto subject_name = X509_get_subject_name(x509.get()); + const auto subject_name = X509_get_subject_name(cert.get()); X509_NAME_add_entry_by_txt(subject_name, "C", MBSTRING_ASC, @@ -171,16 +171,16 @@ class X509Cert X509_NAME_add_entry_by_txt(subject_name, "CN", MBSTRING_ASC, cn.data(), cn.size(), APPEND_ENTRY, ADD_RDN); const auto issuer_name = - cert_type == CertType::Server ? X509_get_subject_name(root_certificate.value().x509.get()) : subject_name; - X509_set_issuer_name(x509.get(), issuer_name); + cert_type == CertType::Server ? X509_get_subject_name(root_certificate.value().cert.get()) : subject_name; + X509_set_issuer_name(cert.get(), issuer_name); - if (!X509_set_pubkey(x509.get(), key.get())) + if (!X509_set_pubkey(cert.get(), key.get())) throw std::runtime_error("Failed to set certificate public key"); - const auto& issuer_x509 = cert_type == CertType::Server ? root_certificate.value().x509 : x509; + const auto& issuer_cert = cert_type == CertType::Server ? root_certificate.value().cert : cert; // Add X509v3 extensions X509V3_CTX ctx; - X509V3_set_ctx(&ctx, issuer_x509.get(), x509.get(), NULL, NULL, 0); + X509V3_set_ctx(&ctx, issuer_cert.get(), cert.get(), NULL, NULL, 0); // wrap into function or struct X509_EXTENSION* ext; @@ -188,36 +188,36 @@ class X509Cert if (cert_type == CertType::Server) { ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); - X509_add_ext(x509.get(), ext, -1); + X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); } // Subject Key Identifier ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); - X509_add_ext(x509.get(), ext, -1); + X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); // Authority Key Identifier const std::string is_from_issuer = cert_type == CertType::Server ? ",issuer" : ""; ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, ("keyid:always" + is_from_issuer).c_str()); - X509_add_ext(x509.get(), ext, -1); + X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); // Basic Constraints: critical, CA:TRUE or CA:FALSE const std::string is_ca = cert_type == CertType::Root ? "TRUE" : "FALSE"; ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, ("critical,CA:" + is_ca).c_str()); - X509_add_ext(x509.get(), ext, -1); + X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); const auto& signing_key = cert_type == CertType::Server ? *root_certificate_key : key; - if (!X509_sign(x509.get(), signing_key.get(), EVP_sha256())) + if (!X509_sign(cert.get(), signing_key.get(), EVP_sha256())) throw std::runtime_error("Failed to sign certificate"); } std::string as_pem() const { mp::BIOMem mem; - auto bytes = PEM_write_bio_X509(mem.get(), x509.get()); + auto bytes = PEM_write_bio_X509(mem.get(), cert.get()); if (bytes == 0) throw std::runtime_error("Failed to write certificate in PEM format"); return mem.as_string(); @@ -226,12 +226,12 @@ class X509Cert void write(const QString& cert_path) const { WritableFile file{cert_path}; - if (!PEM_write_X509(file.get(), x509.get())) + if (!PEM_write_X509(file.get(), cert.get())) throw std::runtime_error(fmt::format("Failed writing certificate to file '{}'", cert_path)); } private: - std::unique_ptr x509{X509_new(), X509_free}; + std::unique_ptr cert{X509_new(), X509_free}; }; mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const std::string& server_name) From 72f681c1590ec6827b8d6858aa476f8467df3b46 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 15:42:20 +0100 Subject: [PATCH 031/114] [ssl cert] use nullptr instead of NULL to comply with Modern C++ style. --- src/cert/ssl_cert_provider.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 4487e27ec3..8ff9f2498b 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -180,32 +180,32 @@ class X509Cert const auto& issuer_cert = cert_type == CertType::Server ? root_certificate.value().cert : cert; // Add X509v3 extensions X509V3_CTX ctx; - X509V3_set_ctx(&ctx, issuer_cert.get(), cert.get(), NULL, NULL, 0); + X509V3_set_ctx(&ctx, issuer_cert.get(), cert.get(), nullptr, nullptr, 0); // wrap into function or struct X509_EXTENSION* ext; // Subject Alternative Name if (cert_type == CertType::Server) { - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); + ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); } // Subject Key Identifier - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_subject_key_identifier, "hash"); + ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_subject_key_identifier, "hash"); X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); // Authority Key Identifier const std::string is_from_issuer = cert_type == CertType::Server ? ",issuer" : ""; - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_authority_key_identifier, ("keyid:always" + is_from_issuer).c_str()); + ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_authority_key_identifier, ("keyid:always" + is_from_issuer).c_str()); X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); // Basic Constraints: critical, CA:TRUE or CA:FALSE const std::string is_ca = cert_type == CertType::Root ? "TRUE" : "FALSE"; - ext = X509V3_EXT_conf_nid(NULL, &ctx, NID_basic_constraints, ("critical,CA:" + is_ca).c_str()); + ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_basic_constraints, ("critical,CA:" + is_ca).c_str()); X509_add_ext(cert.get(), ext, -1); X509_EXTENSION_free(ext); From 147b2c2e8c17e4c46f357a4e6490ebbf8ad4d5f9 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 17:56:11 +0100 Subject: [PATCH 032/114] [ssl cert] using add_extension function to deduplicate some code. --- src/cert/ssl_cert_provider.cpp | 30 ++++++++++++++---------------- 1 file changed, 14 insertions(+), 16 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 8ff9f2498b..442f1ed5e8 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -182,32 +182,24 @@ class X509Cert X509V3_CTX ctx; X509V3_set_ctx(&ctx, issuer_cert.get(), cert.get(), nullptr, nullptr, 0); - // wrap into function or struct - X509_EXTENSION* ext; // Subject Alternative Name if (cert_type == CertType::Server) { - ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); - X509_add_ext(cert.get(), ext, -1); - X509_EXTENSION_free(ext); + add_extension(ctx, NID_subject_alt_name, ("DNS:" + server_name).c_str()); } // Subject Key Identifier - ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_subject_key_identifier, "hash"); - X509_add_ext(cert.get(), ext, -1); - X509_EXTENSION_free(ext); + add_extension(ctx, NID_subject_key_identifier, "hash"); // Authority Key Identifier - const std::string is_from_issuer = cert_type == CertType::Server ? ",issuer" : ""; - ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_authority_key_identifier, ("keyid:always" + is_from_issuer).c_str()); - X509_add_ext(cert.get(), ext, -1); - X509_EXTENSION_free(ext); + add_extension(ctx, + NID_authority_key_identifier, + (std::string("keyid:always") + (cert_type == CertType::Server ? ",issuer" : "")).c_str()); // Basic Constraints: critical, CA:TRUE or CA:FALSE - const std::string is_ca = cert_type == CertType::Root ? "TRUE" : "FALSE"; - ext = X509V3_EXT_conf_nid(nullptr, &ctx, NID_basic_constraints, ("critical,CA:" + is_ca).c_str()); - X509_add_ext(cert.get(), ext, -1); - X509_EXTENSION_free(ext); + add_extension(ctx, + NID_basic_constraints, + (std::string("critical,CA:") + (cert_type == CertType::Root ? "TRUE" : "FALSE")).c_str()); const auto& signing_key = cert_type == CertType::Server ? *root_certificate_key : key; if (!X509_sign(cert.get(), signing_key.get(), EVP_sha256())) @@ -231,6 +223,12 @@ class X509Cert } private: + void add_extension(X509V3_CTX& ctx, int nid, const char* value) { + X509_EXTENSION* ext = X509V3_EXT_conf_nid(nullptr, &ctx, nid, value); + X509_add_ext(cert.get(), ext, -1); + X509_EXTENSION_free(ext); + } + std::unique_ptr cert{X509_new(), X509_free}; }; From d5ee6904c2379db1a5b1796606163d5442c8103e Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 18:00:36 +0100 Subject: [PATCH 033/114] [ssl cert] use std::unique_ptr and custom deleter to simplify the code --- src/cert/ssl_cert_provider.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 442f1ed5e8..840f64c10f 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -224,9 +224,10 @@ class X509Cert private: void add_extension(X509V3_CTX& ctx, int nid, const char* value) { - X509_EXTENSION* ext = X509V3_EXT_conf_nid(nullptr, &ctx, nid, value); - X509_add_ext(cert.get(), ext, -1); - X509_EXTENSION_free(ext); + const std::unique_ptr ext( + X509V3_EXT_conf_nid(nullptr, &ctx, nid, value), + X509_EXTENSION_free); + X509_add_ext(cert.get(), ext.get(), -1); } std::unique_ptr cert{X509_new(), X509_free}; From 243efa18bee00330524b24d1f7eaf54794a77cdc Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Feb 2025 21:52:53 +0100 Subject: [PATCH 034/114] [ssl cert] added check to the pointer and the function return code --- src/cert/ssl_cert_provider.cpp | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 840f64c10f..11f70f2b1c 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -223,11 +223,21 @@ class X509Cert } private: - void add_extension(X509V3_CTX& ctx, int nid, const char* value) { + void add_extension(X509V3_CTX& ctx, int nid, const char* value) + { const std::unique_ptr ext( X509V3_EXT_conf_nid(nullptr, &ctx, nid, value), X509_EXTENSION_free); - X509_add_ext(cert.get(), ext.get(), -1); + + if (!ext) + { + throw std::runtime_error("Failed to create X509 extension"); + } + + if (!X509_add_ext(cert.get(), ext.get(), -1)) + { + throw std::runtime_error("Failed to add X509 extension"); + } } std::unique_ptr cert{X509_new(), X509_free}; From e94fb2d062990a25ad52d553bfeaf0496c53ae2d Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Feb 2025 14:50:11 +0100 Subject: [PATCH 035/114] [ssl cert] added the root certificate existence check for server certificate migration when the user upgrades multipass to the version includes this. --- src/cert/ssl_cert_provider.cpp | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 11f70f2b1c..a67aa68996 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -250,15 +250,15 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const auto priv_key_path = cert_dir.filePath(prefix + "_key.pem"); const auto cert_path = cert_dir.filePath(prefix + ".pem"); - if (QFile::exists(priv_key_path) && QFile::exists(cert_path)) - { - return {mp::utils::contents_of(cert_path), mp::utils::contents_of(priv_key_path)}; - } - if (!server_name.empty()) { - const auto priv_root_key_path = cert_dir.filePath(prefix + "_root_key.pem"); const std::filesystem::path root_cert_path = MP_PLATFORM.get_root_cert_path(); + if (std::filesystem::exists(root_cert_path) && QFile::exists(priv_key_path) && QFile::exists(cert_path)) + { + return {mp::utils::contents_of(cert_path), mp::utils::contents_of(priv_key_path)}; + } + + const auto priv_root_key_path = cert_dir.filePath(prefix + "_root_key.pem"); EVPKey root_cert_key; X509Cert root_cert{root_cert_key, X509Cert::CertType::Root}; @@ -277,6 +277,11 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, } else { + if (QFile::exists(priv_key_path) && QFile::exists(cert_path)) + { + return {mp::utils::contents_of(cert_path), mp::utils::contents_of(priv_key_path)}; + } + const EVPKey client_cert_key; const X509Cert client_cert{client_cert_key, X509Cert::CertType::Client}; client_cert_key.write(priv_key_path); From 0825a0cfadd11834a9439612dcd4827439b1d9ec Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Feb 2025 15:05:24 +0100 Subject: [PATCH 036/114] [vcpckg][grpc client] Switch the gRPC repository to the standard one and commit to release 1.52.1, which serves as the base for our patches. Then, remove the patch-related code. --- 3rd-party/vcpkg-ports/grpc/portfile.cmake | 6 +++--- src/client/common/client_common.cpp | 1 - tests/unix/test_daemon_rpc.cpp | 1 - 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/3rd-party/vcpkg-ports/grpc/portfile.cmake b/3rd-party/vcpkg-ports/grpc/portfile.cmake index ef1f48b984..874028294e 100644 --- a/3rd-party/vcpkg-ports/grpc/portfile.cmake +++ b/3rd-party/vcpkg-ports/grpc/portfile.cmake @@ -4,9 +4,9 @@ endif() vcpkg_from_github( OUT_SOURCE_PATH SOURCE_PATH - REPO canonical/grpc - REF e3acf245a91630fe4d464091ba5446f6a638d82f - SHA512 18574197f4a5070de07c39c096ead2175c150a2b790adbb3d9639b0637641015fb91f5cffa916b50863d6ee62203ad2a6964ce87566b6ae7b41716594c445c06 + REPO grpc/grpc + REF v1.52.1 + SHA512 06c69fb817af75b2610761a3a193178b749755eb7bed58875aa251def7c0c253cdaf02cf834c31c8b2cae7b01a6081e2aece4b131a162f64bd45ff0aff4d7758 HEAD_REF master PATCHES 00002-static-linking-in-linux.patch diff --git a/src/client/common/client_common.cpp b/src/client/common/client_common.cpp index 5aa931501a..9010eef148 100644 --- a/src/client/common/client_common.cpp +++ b/src/client/common/client_common.cpp @@ -75,7 +75,6 @@ grpc::SslCredentialsOptions get_ssl_credentials_opts_from(const mp::CertProvider auto opts = grpc::SslCredentialsOptions(); opts.pem_root_certs = MP_UTILS.contents_of(MP_PLATFORM.get_root_cert_path().u8string().c_str()); - opts.server_certificate_request = GRPC_SSL_REQUEST_SERVER_CERTIFICATE_AND_VERIFY; opts.pem_cert_chain = cert_provider.PEM_certificate(); opts.pem_private_key = cert_provider.PEM_signing_key(); diff --git a/tests/unix/test_daemon_rpc.cpp b/tests/unix/test_daemon_rpc.cpp index 33c4b3633b..e366e38fce 100644 --- a/tests/unix/test_daemon_rpc.cpp +++ b/tests/unix/test_daemon_rpc.cpp @@ -48,7 +48,6 @@ struct TestDaemonRpc : public mpt::DaemonTestFixture { auto opts = grpc::SslCredentialsOptions(); opts.pem_root_certs = mpt::root_cert; - opts.server_certificate_request = GRPC_SSL_REQUEST_SERVER_CERTIFICATE_AND_VERIFY; opts.pem_cert_chain = mpt::cert; opts.pem_private_key = mpt::key; From edf1115ae00341f1197dcc156d54d26b947ef888 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Feb 2025 17:34:12 +0100 Subject: [PATCH 037/114] [platform] added snap root cert file path --- src/platform/platform_unix.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 5b6677b1da..7f8bc67d7c 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -18,6 +18,7 @@ #include #include #include +#include #include #include @@ -266,5 +267,10 @@ long long mp::platform::Platform::get_total_ram() const std::filesystem::path mp::platform::Platform::get_root_cert_path() const { - return {"/usr/local/share/ca-certificates/multipass_root_cert.pem"}; + constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; + + return mp::utils::in_multipass_snap() + ? std::filesystem::path{QString{mp::utils::snap_common_dir()}.toStdString().c_str()} / + "data/multipassd/certificates" / root_cert_file_name + : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; } From 2b69d63fa1e7494f2309c90e2ec6e9b0ebfec7d9 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Feb 2025 15:01:01 +0100 Subject: [PATCH 038/114] [ssl cert] fixes the snap version multipass can not overwrite the key file issue. This is due to the key file is read-only to the owner itself. As a result of that, the owner can not open it with "wb" write and binary. In our use case, we need to overwrite the file, so remove it first can be a good workaround of this. --- src/cert/ssl_cert_provider.cpp | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index a67aa68996..71fed15e12 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -38,11 +38,16 @@ namespace class WritableFile { public: - explicit WritableFile(const QString& file_path) : fp{fopen(file_path.toStdString().c_str(), "wb"), fclose} + explicit WritableFile(const QString& file_path) { - if (fp == nullptr) + std::filesystem::remove(file_path.toStdString().c_str()); + const auto raw_fp = fopen(file_path.toStdString().c_str(), "wb"); + + if (raw_fp == nullptr) throw std::runtime_error( fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); + + fp.reset(raw_fp); } FILE* get() const @@ -51,7 +56,7 @@ class WritableFile } private: - std::unique_ptr> fp; + std::unique_ptr> fp{nullptr, fclose}; }; class EVPKey From 305814382776f69addc233ad22828056e935d185 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 10:15:57 +0100 Subject: [PATCH 039/114] [ssl cert] convert from std::function to function pointer to avoid the construction of std::function --- src/cert/ssl_cert_provider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 71fed15e12..f30bd16cb5 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -56,7 +56,7 @@ class WritableFile } private: - std::unique_ptr> fp{nullptr, fclose}; + std::unique_ptr fp{nullptr, fclose}; }; class EVPKey From 9c5e0a7bf2f0907257ee0eb1571f4f10a706382f Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 10:19:21 +0100 Subject: [PATCH 040/114] [ssl cert] added comment for why use hard coded function pointer --- src/cert/ssl_cert_provider.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index f30bd16cb5..61472b2663 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -56,6 +56,8 @@ class WritableFile } private: + // decltype(&fclose) does not preserve these some extra function attributes of fclose, leads to warning and + // compilation error std::unique_ptr fp{nullptr, fclose}; }; From 8afa5af9cc12bc6c2f9914ceda94fae0b8fac18d Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 10:30:48 +0100 Subject: [PATCH 041/114] [ssl cert] added a comment for the WritableFile constructor change --- src/cert/ssl_cert_provider.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 61472b2663..e1be71eb46 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -41,6 +41,9 @@ class WritableFile explicit WritableFile(const QString& file_path) { std::filesystem::remove(file_path.toStdString().c_str()); + // The key files are read-only for the owner, meaning that opening them with fopen in wb mode typically fails, + // preventing the overwrite functionality. Therefore, fs::remove is called beforehand, as it depeneds on the + // parent directory's write permission rather than the file's write permission. const auto raw_fp = fopen(file_path.toStdString().c_str(), "wb"); if (raw_fp == nullptr) From dc9fdb42c83cfcd3c5b2342824f07a6c3d34fe28 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 14:45:56 +0100 Subject: [PATCH 042/114] [ssl cert] standardize the serial number format. Changed from decimal integer to 20 bytes hex values which is the standard format of x.509 serial number. Before: Serial Number: -1667044401 (-0x635d1431) After: Serial Number: 6a:5d:50:e6:23:39:4f:43:9f:59:d4:af:93:f5:14:14:11:a5:af:0e --- src/cert/ssl_cert_provider.cpp | 43 ++++++++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 7 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index e1be71eb46..24cc7caf11 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -128,6 +129,40 @@ std::vector as_vector(const std::string& v) return {v.begin(), v.end()}; } +void set_random_serial_number(X509* cert) +{ + // OpenSSL recommends a 20-byte (160-bit) serial number + std::array serial_bytes; + RAND_bytes(serial_bytes.data(), serial_bytes.size()); + + // Convert bytes to an BIGNUM, an arbitrary-precision integer type + BIGNUM* bn = BN_bin2bn(serial_bytes.data(), serial_bytes.size(), NULL); + if (!bn) + { + fprintf(stderr, "Failed to convert serial bytes to BIGNUM\n"); + return; + } + + // Ensure the serial number is positive + BN_set_bit(bn, 159); // Set the highest bit to ensure it's positive + + // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number + // ASN1 is a standard binary format for encoding data like serial numbers in X.509 certificates + ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn, NULL); + if (!serial) + { + fprintf(stderr, "Failed to convert BIGNUM to ASN1_INTEGER\n"); + BN_free(bn); + return; + } + + // Set the serial number in the certificate + X509_set_serialNumber(cert, serial); + + // Cleanup + BN_free(bn); +} + class X509Cert { public: @@ -148,14 +183,8 @@ class X509Cert if (cert == nullptr) throw std::runtime_error("Failed to allocate x509 cert structure"); - long big_num{0}; - const auto rand_bytes = MP_UTILS.random_bytes(4); - for (unsigned int i = 0; i < 4u; i++) - big_num |= rand_bytes[i] << i * 8u; - - X509_set_version(cert.get(), 2); // X.509 v3 + set_random_serial_number(cert.get()); - ASN1_INTEGER_set(X509_get_serialNumber(cert.get()), big_num); X509_gmtime_adj(X509_get_notBefore(cert.get()), 0); // Start time: now const long valid_duration_sec = cert_type == CertType::Root ? 3650L * 24L * 60L * 60L : 365L * 24L * 60L * 60L; // 10 years for root certicicate and 1 year for server and client certificate From a3a317eedc17396a37f09a01a1ffc207b471b72c Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 14:52:58 +0100 Subject: [PATCH 043/114] [ssl cert] using std::unique_ptr to automate memory management. --- src/cert/ssl_cert_provider.cpp | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 24cc7caf11..c3d389cbe0 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -136,7 +136,8 @@ void set_random_serial_number(X509* cert) RAND_bytes(serial_bytes.data(), serial_bytes.size()); // Convert bytes to an BIGNUM, an arbitrary-precision integer type - BIGNUM* bn = BN_bin2bn(serial_bytes.data(), serial_bytes.size(), NULL); + std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), NULL), BN_free); + if (!bn) { fprintf(stderr, "Failed to convert serial bytes to BIGNUM\n"); @@ -144,23 +145,19 @@ void set_random_serial_number(X509* cert) } // Ensure the serial number is positive - BN_set_bit(bn, 159); // Set the highest bit to ensure it's positive + BN_set_bit(bn.get(), 159); // Set the highest bit to ensure it's positive // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number // ASN1 is a standard binary format for encoding data like serial numbers in X.509 certificates - ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn, NULL); + ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn.get(), NULL); if (!serial) { fprintf(stderr, "Failed to convert BIGNUM to ASN1_INTEGER\n"); - BN_free(bn); return; } // Set the serial number in the certificate X509_set_serialNumber(cert, serial); - - // Cleanup - BN_free(bn); } class X509Cert From 64aeb8cea3890b7fd387ab720b9f0d6ae8629cf1 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 14:55:58 +0100 Subject: [PATCH 044/114] [ssl cert] replace fprint with throw. --- src/cert/ssl_cert_provider.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index c3d389cbe0..fb87f675bf 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -140,8 +140,7 @@ void set_random_serial_number(X509* cert) if (!bn) { - fprintf(stderr, "Failed to convert serial bytes to BIGNUM\n"); - return; + throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); } // Ensure the serial number is positive @@ -152,8 +151,7 @@ void set_random_serial_number(X509* cert) ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn.get(), NULL); if (!serial) { - fprintf(stderr, "Failed to convert BIGNUM to ASN1_INTEGER\n"); - return; + throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); } // Set the serial number in the certificate From a078ab58eab6147497e6cff6f47a7a73cd69f7a9 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 15:16:58 +0100 Subject: [PATCH 045/114] [ssl cert] use raw bitwise operation rather than calling BN_set_bit. --- src/cert/ssl_cert_provider.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index fb87f675bf..20e857d4ad 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -134,6 +134,8 @@ void set_random_serial_number(X509* cert) // OpenSSL recommends a 20-byte (160-bit) serial number std::array serial_bytes; RAND_bytes(serial_bytes.data(), serial_bytes.size()); + // Set the highest bit to 0 (unsigned) to ensure it's positive + serial_bytes[0] &= 0x7F; // Convert bytes to an BIGNUM, an arbitrary-precision integer type std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), NULL), BN_free); @@ -143,9 +145,6 @@ void set_random_serial_number(X509* cert) throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); } - // Ensure the serial number is positive - BN_set_bit(bn.get(), 159); // Set the highest bit to ensure it's positive - // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number // ASN1 is a standard binary format for encoding data like serial numbers in X.509 certificates ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn.get(), NULL); From 59cb1b327b90c00a13a7d866422830aa955899a2 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Feb 2025 17:18:51 +0100 Subject: [PATCH 046/114] [unit test][ssl cert] added a comment for creates_different_certs_per_server_name unit test. --- tests/test_ssl_cert_provider.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_ssl_cert_provider.cpp b/tests/test_ssl_cert_provider.cpp index bc8aa9e41d..3b0b741967 100644 --- a/tests/test_ssl_cert_provider.cpp +++ b/tests/test_ssl_cert_provider.cpp @@ -98,6 +98,7 @@ TEST_F(SSLCertProviderFixture, persists_cert_and_key) TEST_F(SSLCertProviderFixture, creates_different_certs_per_server_name) { const auto [mock_platform, _] = mpt::MockPlatform::inject(); + // move the multipass_root_cert.pem into the temporary directory so it will be deleted automatically later EXPECT_CALL(*mock_platform, get_root_cert_path()) .WillRepeatedly(Return(std::filesystem::path{cert_dir.toStdU16String()} / "multipass_root_cert.pem")); From a8d12f125a2f59d38351335fa4a830340ebea13f Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Feb 2025 11:56:51 +0100 Subject: [PATCH 047/114] [ssl cert] fixes the ci compilation error. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit [error]:: /root/parts/multipass/src/src/cert/ssl_cert_provider.cpp:136:54: error: ‘serial_bytes’ may be used uninitialized [-Werror=maybe-uninitialized] --- src/cert/ssl_cert_provider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 20e857d4ad..ea0e36cec3 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -132,7 +132,7 @@ std::vector as_vector(const std::string& v) void set_random_serial_number(X509* cert) { // OpenSSL recommends a 20-byte (160-bit) serial number - std::array serial_bytes; + std::array serial_bytes{}; RAND_bytes(serial_bytes.data(), serial_bytes.size()); // Set the highest bit to 0 (unsigned) to ensure it's positive serial_bytes[0] &= 0x7F; From 23e3006a2a688459515b75178e2e85ad86446e24 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Feb 2025 12:39:42 +0100 Subject: [PATCH 048/114] [ssl cert] addressed one review comment. --- include/multipass/ssl_cert_provider.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/multipass/ssl_cert_provider.h b/include/multipass/ssl_cert_provider.h index 383befb6c2..ab1243a37a 100644 --- a/include/multipass/ssl_cert_provider.h +++ b/include/multipass/ssl_cert_provider.h @@ -34,7 +34,7 @@ class SSLCertProvider : public CertProvider std::string pem_priv_key; }; - SSLCertProvider(const Path& data_dir, const std::string& server_name = ""); + explicit SSLCertProvider(const Path& data_dir, const std::string& server_name = ""); std::string PEM_certificate() const override; std::string PEM_signing_key() const override; From 84b1fc10e01493a6dcc3354dfbdfaa57c749c9a3 Mon Sep 17 00:00:00 2001 From: George Liao Date: Fri, 7 Feb 2025 12:40:31 +0100 Subject: [PATCH 049/114] Update tests/test_alias_dict.cpp Co-authored-by: Andrei Toterman Signed-off-by: George Liao --- tests/test_alias_dict.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_alias_dict.cpp b/tests/test_alias_dict.cpp index e0f948817d..8123f893e4 100644 --- a/tests/test_alias_dict.cpp +++ b/tests/test_alias_dict.cpp @@ -627,7 +627,7 @@ TEST_P(DaemonAliasTestsuite, purge_removes_purged_instance_aliases_and_scripts) EXPECT_CALL(*mock_image_vault, has_record_for(_)).WillRepeatedly(Return(true)); const auto [mock_utils, guard] = mpt::MockUtils::inject(); - EXPECT_CALL(*mock_utils, contents_of(_)).WillRepeatedly(Return(multipass::test::root_cert)); + EXPECT_CALL(*mock_utils, contents_of(_)).WillRepeatedly(Return(mpt::root_cert)); config_builder.vault = std::move(mock_image_vault); auto mock_factory = use_a_mock_vm_factory(); From ecff7ed9e8dc06d813324db073fb3df828f9eefe Mon Sep 17 00:00:00 2001 From: George Liao Date: Fri, 7 Feb 2025 12:40:51 +0100 Subject: [PATCH 050/114] Update tests/test_cli_client.cpp Co-authored-by: Andrei Toterman Signed-off-by: George Liao --- tests/test_cli_client.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_cli_client.cpp b/tests/test_cli_client.cpp index b1a580130a..303d83babe 100644 --- a/tests/test_cli_client.cpp +++ b/tests/test_cli_client.cpp @@ -161,7 +161,7 @@ struct Client : public Test EXPECT_CALL(mock_settings, get(Eq(mp::mounts_key))).WillRepeatedly(Return("true")); EXPECT_CALL(mock_settings, register_handler(_)).WillRepeatedly(Return(nullptr)); EXPECT_CALL(mock_settings, unregister_handler).Times(AnyNumber()); - EXPECT_CALL(*mock_utils, contents_of(_)).WillRepeatedly(Return(multipass::test::root_cert)); + EXPECT_CALL(*mock_utils, contents_of(_)).WillRepeatedly(Return(mpt::root_cert)); EXPECT_CALL(mpt::MockStandardPaths::mock_instance(), locate(_, _, _)) .Times(AnyNumber()); // needed to allow general calls once we have added the specific expectation below From 85f1976a13fd5dd225898df683785f4112fb12df Mon Sep 17 00:00:00 2001 From: George Liao Date: Fri, 7 Feb 2025 12:40:57 +0100 Subject: [PATCH 051/114] Update tests/test_client_common.cpp Co-authored-by: Andrei Toterman Signed-off-by: George Liao --- tests/test_client_common.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_client_common.cpp b/tests/test_client_common.cpp index 3fd979a1b9..f315ffc3bd 100644 --- a/tests/test_client_common.cpp +++ b/tests/test_client_common.cpp @@ -44,7 +44,7 @@ struct TestClientCommon : public mpt::DaemonTestFixture { ON_CALL(mpt::MockStandardPaths::mock_instance(), writableLocation(mp::StandardPaths::GenericDataLocation)) .WillByDefault(Return(temp_dir.path())); - ON_CALL(*mock_utils, contents_of(_)).WillByDefault(Return(multipass::test::root_cert)); + ON_CALL(*mock_utils, contents_of(_)).WillByDefault(Return(mpt::root_cert)); // delegate some functions to the orignal implementation ON_CALL(*mock_utils, make_dir(A(), A(), A())) .WillByDefault([](const QDir& dir, const QString& name, std::filesystem::perms permissions) -> mp::Path { From c7c7c1fb3ff45e5ddb4a798fcbdca024796895df Mon Sep 17 00:00:00 2001 From: George Liao Date: Fri, 7 Feb 2025 12:41:06 +0100 Subject: [PATCH 052/114] Update tests/test_daemon_find.cpp Co-authored-by: Andrei Toterman Signed-off-by: George Liao --- tests/test_daemon_find.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_daemon_find.cpp b/tests/test_daemon_find.cpp index eafba0114e..04dfbedc43 100644 --- a/tests/test_daemon_find.cpp +++ b/tests/test_daemon_find.cpp @@ -51,7 +51,7 @@ struct DaemonFind : public mpt::DaemonTestFixture EXPECT_CALL(mock_settings, register_handler).WillRepeatedly(Return(nullptr)); EXPECT_CALL(mock_settings, unregister_handler).Times(AnyNumber()); EXPECT_CALL(mock_settings, get(Eq(mp::winterm_key))).WillRepeatedly(Return("none")); - ON_CALL(mock_utils, contents_of(_)).WillByDefault(Return(multipass::test::root_cert)); + ON_CALL(mock_utils, contents_of(_)).WillByDefault(Return(mpt::root_cert)); } mpt::MockPlatform::GuardedMock attr{mpt::MockPlatform::inject()}; From c95b81c59410862a0e37f42f633fd849443efa70 Mon Sep 17 00:00:00 2001 From: George Liao Date: Fri, 7 Feb 2025 12:48:08 +0100 Subject: [PATCH 053/114] Update src/platform/platform_unix.cpp Co-authored-by: Andrei Toterman Signed-off-by: George Liao --- src/platform/platform_unix.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 7f8bc67d7c..fe90131c1a 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -270,7 +270,7 @@ std::filesystem::path mp::platform::Platform::get_root_cert_path() const constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; return mp::utils::in_multipass_snap() - ? std::filesystem::path{QString{mp::utils::snap_common_dir()}.toStdString().c_str()} / + ? std::filesystem::path{mp::utils::snap_common_dir().toStdString()} / "data/multipassd/certificates" / root_cert_file_name : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; } From 53fd6c240c8f4682ca71d03cec1d50b16cde83ce Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Feb 2025 12:50:10 +0100 Subject: [PATCH 054/114] [ssl cert] use cached variable to avoid double conversion. --- src/cert/ssl_cert_provider.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index ea0e36cec3..3321c65f7b 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -41,11 +41,12 @@ class WritableFile public: explicit WritableFile(const QString& file_path) { - std::filesystem::remove(file_path.toStdString().c_str()); + const std::string file_path_str = file_path.toStdString(); + std::filesystem::remove(std::filesystem::path{file_path_str}); // The key files are read-only for the owner, meaning that opening them with fopen in wb mode typically fails, // preventing the overwrite functionality. Therefore, fs::remove is called beforehand, as it depeneds on the // parent directory's write permission rather than the file's write permission. - const auto raw_fp = fopen(file_path.toStdString().c_str(), "wb"); + const auto raw_fp = fopen(file_path_str.c_str(), "wb"); if (raw_fp == nullptr) throw std::runtime_error( From 9d83a9ef258a5f52aadc9f74b26524bb522a6b57 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Feb 2025 13:07:45 +0100 Subject: [PATCH 055/114] [platform] fixes the lint. --- src/platform/platform_unix.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index fe90131c1a..01595f6c5b 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -270,7 +270,7 @@ std::filesystem::path mp::platform::Platform::get_root_cert_path() const constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; return mp::utils::in_multipass_snap() - ? std::filesystem::path{mp::utils::snap_common_dir().toStdString()} / - "data/multipassd/certificates" / root_cert_file_name + ? std::filesystem::path{mp::utils::snap_common_dir().toStdString()} / "data/multipassd/certificates" / + root_cert_file_name : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; } From 84d84ef3db214258bb73a7f1e082288de63ae926 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Feb 2025 15:20:09 +0100 Subject: [PATCH 056/114] [platform] move the get_root_cert_path function from unix to linux. --- src/platform/platform_linux.cpp | 11 +++++++++++ src/platform/platform_unix.cpp | 10 ---------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index 6b1245ffc2..69b8849683 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -470,3 +470,14 @@ std::string multipass::platform::host_version() return mpu::in_multipass_snap() ? multipass::platform::detail::read_os_release() : fmt::format("{}-{}", QSysInfo::productType(), QSysInfo::productVersion()); } + + +std::filesystem::path mp::platform::Platform::get_root_cert_path() const +{ + constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; + + return mp::utils::in_multipass_snap() + ? std::filesystem::path{mp::utils::snap_common_dir().toStdString()} / "data/multipassd/certificates" / + root_cert_file_name + : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; +} diff --git a/src/platform/platform_unix.cpp b/src/platform/platform_unix.cpp index 01595f6c5b..7ed79b1351 100644 --- a/src/platform/platform_unix.cpp +++ b/src/platform/platform_unix.cpp @@ -264,13 +264,3 @@ long long mp::platform::Platform::get_total_ram() const { return static_cast(sysconf(_SC_PHYS_PAGES)) * sysconf(_SC_PAGESIZE); } - -std::filesystem::path mp::platform::Platform::get_root_cert_path() const -{ - constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; - - return mp::utils::in_multipass_snap() - ? std::filesystem::path{mp::utils::snap_common_dir().toStdString()} / "data/multipassd/certificates" / - root_cert_file_name - : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; -} From b2d03684352e2a61dc608cb57b1b34a0b82df1e2 Mon Sep 17 00:00:00 2001 From: George Liao Date: Fri, 7 Feb 2025 15:42:48 +0100 Subject: [PATCH 057/114] Update tests/test_daemon.cpp Co-authored-by: Andrei Toterman Signed-off-by: George Liao --- tests/test_daemon.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_daemon.cpp b/tests/test_daemon.cpp index a0e9673784..ff5ee59f63 100644 --- a/tests/test_daemon.cpp +++ b/tests/test_daemon.cpp @@ -113,7 +113,7 @@ struct Daemon : public mpt::DaemonTestFixture ON_CALL(mock_utils, filesystem_bytes_available(_)).WillByDefault([this](const QString& data_directory) { return mock_utils.Utils::filesystem_bytes_available(data_directory); }); - ON_CALL(mock_utils, contents_of(_)).WillByDefault(Return(multipass::test::root_cert)); + ON_CALL(mock_utils, contents_of(_)).WillByDefault(Return(mpt::root_cert)); EXPECT_CALL(mock_platform, get_blueprints_url_override()).WillRepeatedly([] { return QString{}; }); EXPECT_CALL(mock_platform, multipass_storage_location()).Times(AnyNumber()).WillRepeatedly(Return(QString())); EXPECT_CALL(mock_platform, create_alias_script(_, _)).WillRepeatedly(Return()); From c321ebc1d21ee15eb54fc9879b33836158192f91 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Feb 2025 15:46:21 +0100 Subject: [PATCH 058/114] [platform] fix the lint. --- src/platform/platform_linux.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index 69b8849683..b80700c711 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -471,7 +471,6 @@ std::string multipass::platform::host_version() : fmt::format("{}-{}", QSysInfo::productType(), QSysInfo::productVersion()); } - std::filesystem::path mp::platform::Platform::get_root_cert_path() const { constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; From 60fe47ad390a955ce4b364e9d83b03875aa03ed0 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 10 Feb 2025 14:29:33 +0100 Subject: [PATCH 059/114] [platform][linux] used the mp::StandardPaths::AppDataLocation instead of mp::utils::snap_common_dir() to access the multipass data storage location. It also dispatched for the user defined storage location case. --- src/platform/platform_linux.cpp | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index b80700c711..63c6b840e8 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -179,6 +179,15 @@ std::string get_alias_script_path(const std::string& alias) return aliases_folder.absoluteFilePath(QString::fromStdString(alias)).toStdString(); } + +std::filesystem::path multipass_final_storage_location() +{ + const auto user_specified_mp_storage = MP_PLATFORM.multipass_storage_location(); + const auto mp_final_storage = user_specified_mp_storage.isEmpty() + ? MP_STDPATHS.writableLocation(mp::StandardPaths::AppDataLocation) + : user_specified_mp_storage; + return std::filesystem::path{mp_final_storage.toStdString()}; +} } // namespace std::unique_ptr multipass::platform::detail::find_os_release() @@ -474,9 +483,7 @@ std::string multipass::platform::host_version() std::filesystem::path mp::platform::Platform::get_root_cert_path() const { constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; - return mp::utils::in_multipass_snap() - ? std::filesystem::path{mp::utils::snap_common_dir().toStdString()} / "data/multipassd/certificates" / - root_cert_file_name + ? multipass_final_storage_location() / "certificates" / root_cert_file_name : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; } From ef8bd748261f932648dfb441829476bddda00a42 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 11 Feb 2025 11:19:30 +0100 Subject: [PATCH 060/114] [ssl cert] restore the accidentally deleted X509_set_version call. This caused slightly malformed cert format but it somehow passed the grpc c++ client check. On the other side, it failed the grpc dart client check. As a result, this change fixed the gui can not connect server issue. --- src/cert/ssl_cert_provider.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 3321c65f7b..2119fef938 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -178,8 +178,9 @@ class X509Cert if (cert == nullptr) throw std::runtime_error("Failed to allocate x509 cert structure"); - set_random_serial_number(cert.get()); + X509_set_version(cert.get(), 2); // 0 index based, 2 amounts to 3 + set_random_serial_number(cert.get()); X509_gmtime_adj(X509_get_notBefore(cert.get()), 0); // Start time: now const long valid_duration_sec = cert_type == CertType::Root ? 3650L * 24L * 60L * 60L : 365L * 24L * 60L * 60L; // 10 years for root certicicate and 1 year for server and client certificate From 82cfa6e95a7ef7a8b64cc01dc040928fb3c238bf Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 11 Feb 2025 11:46:38 +0100 Subject: [PATCH 061/114] [platform][linux] change the snap case storage location back to snap dir utility function. Can not use mp::StandardPaths::AppDataLocation because client and server process interprets this variable to different paths. --- src/platform/platform_linux.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index 63c6b840e8..bd3d907841 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -183,9 +183,8 @@ std::string get_alias_script_path(const std::string& alias) std::filesystem::path multipass_final_storage_location() { const auto user_specified_mp_storage = MP_PLATFORM.multipass_storage_location(); - const auto mp_final_storage = user_specified_mp_storage.isEmpty() - ? MP_STDPATHS.writableLocation(mp::StandardPaths::AppDataLocation) - : user_specified_mp_storage; + const auto mp_final_storage = user_specified_mp_storage.isEmpty() ? mp::utils::snap_common_dir() + "data/multipassd" + : user_specified_mp_storage; return std::filesystem::path{mp_final_storage.toStdString()}; } } // namespace From 4d374a0b378ab567967b2bccd4954434f4020de6 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 11 Feb 2025 15:14:59 +0100 Subject: [PATCH 062/114] [platform][linux] fix the snap path mistake. --- src/platform/platform_linux.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index bd3d907841..47d7f4bf1f 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -183,8 +183,9 @@ std::string get_alias_script_path(const std::string& alias) std::filesystem::path multipass_final_storage_location() { const auto user_specified_mp_storage = MP_PLATFORM.multipass_storage_location(); - const auto mp_final_storage = user_specified_mp_storage.isEmpty() ? mp::utils::snap_common_dir() + "data/multipassd" - : user_specified_mp_storage; + const auto mp_final_storage = user_specified_mp_storage.isEmpty() + ? mp::utils::snap_common_dir() + "/data/multipassd" + : user_specified_mp_storage; return std::filesystem::path{mp_final_storage.toStdString()}; } } // namespace From 07557b87432a2b49753ca5271df2dfba150ce47d Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 12 Feb 2025 14:25:43 +0100 Subject: [PATCH 063/114] [ssl cert] make sure the parent directory of key, certificate file always exist. --- src/cert/ssl_cert_provider.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 2119fef938..02041ec1bd 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -41,12 +41,14 @@ class WritableFile public: explicit WritableFile(const QString& file_path) { - const std::string file_path_str = file_path.toStdString(); - std::filesystem::remove(std::filesystem::path{file_path_str}); + const std::filesystem::path file_path_std{file_path.toStdString()}; + std::filesystem::remove(file_path_std); // The key files are read-only for the owner, meaning that opening them with fopen in wb mode typically fails, // preventing the overwrite functionality. Therefore, fs::remove is called beforehand, as it depeneds on the // parent directory's write permission rather than the file's write permission. - const auto raw_fp = fopen(file_path_str.c_str(), "wb"); + std::filesystem::create_directories(file_path_std.parent_path()); + // make sure the parent directory exist + const auto raw_fp = fopen(file_path_std.c_str(), "wb"); if (raw_fp == nullptr) throw std::runtime_error( From 21b6bc9be7e12d04c40af69943eedfd564ceedaf Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 12 Feb 2025 15:35:33 +0100 Subject: [PATCH 064/114] [ssl cert] fixed the windows build failure. --- src/cert/ssl_cert_provider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 02041ec1bd..eb217ea58a 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -48,7 +48,7 @@ class WritableFile // parent directory's write permission rather than the file's write permission. std::filesystem::create_directories(file_path_std.parent_path()); // make sure the parent directory exist - const auto raw_fp = fopen(file_path_std.c_str(), "wb"); + const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); if (raw_fp == nullptr) throw std::runtime_error( From 5f412a29c9ec005ec8527b48bbe69805b91ee6cc Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 12 Feb 2025 23:10:38 +0100 Subject: [PATCH 065/114] [ssl cert] replace the remove file with adding owner write permission to the file. This is a way to enable fopen with wb mode, the original hack only worked on unix but no on windows. --- src/cert/ssl_cert_provider.cpp | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index eb217ea58a..146cb53150 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -42,12 +42,15 @@ class WritableFile explicit WritableFile(const QString& file_path) { const std::filesystem::path file_path_std{file_path.toStdString()}; - std::filesystem::remove(file_path_std); - // The key files are read-only for the owner, meaning that opening them with fopen in wb mode typically fails, - // preventing the overwrite functionality. Therefore, fs::remove is called beforehand, as it depeneds on the - // parent directory's write permission rather than the file's write permission. std::filesystem::create_directories(file_path_std.parent_path()); // make sure the parent directory exist + if (std::filesystem::exists(file_path_std)) + { + // enable fopen with wb mode + MP_PLATFORM.set_permissions(file_path_std, + std::filesystem::perms::owner_read | std::filesystem::perms::owner_write); + } + const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); if (raw_fp == nullptr) From 0f97e5c5d26e989b56ffc21cbbbc3afbd069c494 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 13 Feb 2025 12:17:00 +0100 Subject: [PATCH 066/114] [client][cert] removed the unneeded check and create directory. The "std::filesystem::create_directories(file_path_std.parent_path());" call in the WritableFile class constructor already took care of that. --- src/client/common/client_common.cpp | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/src/client/common/client_common.cpp b/src/client/common/client_common.cpp index 9010eef148..4128dfc569 100644 --- a/src/client/common/client_common.cpp +++ b/src/client/common/client_common.cpp @@ -80,13 +80,6 @@ grpc::SslCredentialsOptions get_ssl_credentials_opts_from(const mp::CertProvider return opts; } - -bool client_certs_exist(const QString& cert_dir_path) -{ - QDir cert_dir{cert_dir_path}; - - return cert_dir.exists(mp::client_cert_file) && cert_dir.exists(mp::client_key_file); -} } // namespace mp::ReturnCode mp::cmd::standard_failure_handler_for(const std::string& command, std::ostream& cerr, @@ -157,11 +150,6 @@ std::unique_ptr mp::client::get_cert_provider() auto data_location{MP_STDPATHS.writableLocation(StandardPaths::GenericDataLocation)}; auto common_client_cert_dir_path{data_location + common_client_cert_dir}; - if (!client_certs_exist(common_client_cert_dir_path)) - { - MP_UTILS.make_dir(common_client_cert_dir_path); - } - return std::make_unique(common_client_cert_dir_path); } From e84205a177d3ef9b8f49de3b2050be2e42e7aad2 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 13 Feb 2025 12:43:23 +0100 Subject: [PATCH 067/114] [ssl cert] move the permission change from WritableFile to EVPKey::write so it only affects the key file. --- src/cert/ssl_cert_provider.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 146cb53150..ff4aa8530e 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -44,12 +44,6 @@ class WritableFile const std::filesystem::path file_path_std{file_path.toStdString()}; std::filesystem::create_directories(file_path_std.parent_path()); // make sure the parent directory exist - if (std::filesystem::exists(file_path_std)) - { - // enable fopen with wb mode - MP_PLATFORM.set_permissions(file_path_std, - std::filesystem::perms::owner_read | std::filesystem::perms::owner_write); - } const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); @@ -114,6 +108,14 @@ class EVPKey void write(const QString& key_path) const { + const std::filesystem::path key_path_std = key_path.toStdU16String(); + if (std::filesystem::exists(key_path_std)) + { + // enable fopen in WritableFile with wb mode + MP_PLATFORM.set_permissions(key_path_std, + std::filesystem::perms::owner_read | std::filesystem::perms::owner_write); + } + WritableFile file{key_path}; if (!PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr)) throw std::runtime_error(fmt::format("Failed writing certificate private key to file '{}'", key_path)); From 8de2df466770e37be6bf99815978f373f7ff18ad Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 13 Feb 2025 12:48:19 +0100 Subject: [PATCH 068/114] [daemon_config] remove the unneeded MP_UTILS.make_dir call . The call std::filesystem::create_directories(file_path_std.parent_path()); in WritableFile constructor already check the directory existence and create that if it is absent. --- src/daemon/daemon_config.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index 93679577fe..8b8032d86f 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -170,8 +170,8 @@ std::unique_ptr mp::DaemonConfigBuilder::build() if (ssh_key_provider == nullptr) ssh_key_provider = std::make_unique(data_directory); if (cert_provider == nullptr) - cert_provider = std::make_unique(MP_UTILS.make_dir(data_directory, "certificates"), - server_name_from(server_address)); + cert_provider = + std::make_unique(data_directory + "/certificates", server_name_from(server_address)); if (client_cert_store == nullptr) client_cert_store = std::make_unique(data_directory); if (ssh_username.empty()) From 9934ea41d42da892e34d2f94f5252f0896b2a896 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 13 Feb 2025 12:58:02 +0100 Subject: [PATCH 069/114] [unit test][ssl cert] removed the unneeded MP_UTILS.make_dir call. --- tests/test_ssl_cert_provider.cpp | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/tests/test_ssl_cert_provider.cpp b/tests/test_ssl_cert_provider.cpp index 3b0b741967..c9475ffd34 100644 --- a/tests/test_ssl_cert_provider.cpp +++ b/tests/test_ssl_cert_provider.cpp @@ -30,12 +30,8 @@ using namespace testing; struct SSLCertProviderFixture : public testing::Test { - SSLCertProviderFixture() - { - cert_dir = MP_UTILS.make_dir(temp_dir.path(), "test-cert"); - } mpt::TempDir temp_dir; - mp::Path cert_dir; + mp::Path cert_dir{temp_dir.path() + "/test-cert"}; }; TEST_F(SSLCertProviderFixture, creates_cert_and_key) From 5eaf6ad9cb10c766306ea46f00a3e130de138c05 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 13 Feb 2025 14:52:34 +0100 Subject: [PATCH 070/114] [unit test][platform linux] added a unit test to cover multipass_final_storage_location function code. --- tests/linux/test_platform_linux.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/tests/linux/test_platform_linux.cpp b/tests/linux/test_platform_linux.cpp index 2c61102ba3..e736950c1e 100644 --- a/tests/linux/test_platform_linux.cpp +++ b/tests/linux/test_platform_linux.cpp @@ -707,4 +707,12 @@ TEST_F(PlatformLinux, remove_alias_script_throws_if_cannot_remove_script) MP_EXPECT_THROW_THAT(MP_PLATFORM.remove_alias_script("alias_name"), std::runtime_error, mpt::match_what(StrEq("No such file or directory"))); } + +TEST_F(PlatformLinux, test_snap_multipass_storage_location) +{ + mpt::SetEnvScope env{"SNAP_NAME", "multipass"}; + mpt::SetEnvScope env2("SNAP_COMMON", "common"); + + EXPECT_EQ(MP_PLATFORM.get_root_cert_path(), "/data/multipassd/certificates/multipass_root_cert.pem"); +} } // namespace From 9a353ee30cece3dab1e6dd12773d1d7bcc5aaa0a Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 13 Feb 2025 15:18:10 +0100 Subject: [PATCH 071/114] [platform][linux] moved "/data/multipassd" out so the user defined storage location also gets the right path for root certificate. --- src/platform/platform_linux.cpp | 7 +++---- tests/linux/test_platform_linux.cpp | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index 47d7f4bf1f..b8838cfd7a 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -183,9 +183,8 @@ std::string get_alias_script_path(const std::string& alias) std::filesystem::path multipass_final_storage_location() { const auto user_specified_mp_storage = MP_PLATFORM.multipass_storage_location(); - const auto mp_final_storage = user_specified_mp_storage.isEmpty() - ? mp::utils::snap_common_dir() + "/data/multipassd" - : user_specified_mp_storage; + const auto mp_final_storage = + user_specified_mp_storage.isEmpty() ? mp::utils::snap_common_dir() : user_specified_mp_storage; return std::filesystem::path{mp_final_storage.toStdString()}; } } // namespace @@ -484,6 +483,6 @@ std::filesystem::path mp::platform::Platform::get_root_cert_path() const { constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; return mp::utils::in_multipass_snap() - ? multipass_final_storage_location() / "certificates" / root_cert_file_name + ? multipass_final_storage_location() / "data" / "multipassd" / "certificates" / root_cert_file_name : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; } diff --git a/tests/linux/test_platform_linux.cpp b/tests/linux/test_platform_linux.cpp index e736950c1e..137140629a 100644 --- a/tests/linux/test_platform_linux.cpp +++ b/tests/linux/test_platform_linux.cpp @@ -713,6 +713,6 @@ TEST_F(PlatformLinux, test_snap_multipass_storage_location) mpt::SetEnvScope env{"SNAP_NAME", "multipass"}; mpt::SetEnvScope env2("SNAP_COMMON", "common"); - EXPECT_EQ(MP_PLATFORM.get_root_cert_path(), "/data/multipassd/certificates/multipass_root_cert.pem"); + EXPECT_EQ(MP_PLATFORM.get_root_cert_path(), "data/multipassd/certificates/multipass_root_cert.pem"); } } // namespace From c0ce12e24f3c17dfde20aa35674a9bf81f26b622 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 14 Feb 2025 15:23:38 +0100 Subject: [PATCH 072/114] [ssl cert] moved the creating file pointer logic to a function, so the initialization can be done in the initializer list. --- src/cert/ssl_cert_provider.cpp | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index ff4aa8530e..faa470b23c 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -36,22 +36,27 @@ namespace mp = multipass; namespace { -class WritableFile +FILE* open_file(const QString& file_path) { -public: - explicit WritableFile(const QString& file_path) - { - const std::filesystem::path file_path_std{file_path.toStdString()}; - std::filesystem::create_directories(file_path_std.parent_path()); - // make sure the parent directory exist + const std::filesystem::path file_path_std{file_path.toStdString()}; + std::filesystem::create_directories(file_path_std.parent_path()); + // make sure the parent directory exist + + const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); + + if (raw_fp == nullptr) + throw std::runtime_error( + fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); - const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); + return raw_fp; +} - if (raw_fp == nullptr) - throw std::runtime_error( - fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); - fp.reset(raw_fp); +class WritableFile +{ +public: + explicit WritableFile(const QString& file_path) : fp{open_file(file_path), fclose} + { } FILE* get() const @@ -62,7 +67,7 @@ class WritableFile private: // decltype(&fclose) does not preserve these some extra function attributes of fclose, leads to warning and // compilation error - std::unique_ptr fp{nullptr, fclose}; + std::unique_ptr fp; }; class EVPKey From 6444ca446479283d7c936b3536d37bd7b53e9b10 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 14 Feb 2025 15:29:36 +0100 Subject: [PATCH 073/114] [ssl cert] moved the raw key pointer creation into a function so the construction of EVPKey can be done in the initializer list. --- src/cert/ssl_cert_provider.cpp | 58 +++++++++++++++++----------------- 1 file changed, 29 insertions(+), 29 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index faa470b23c..c95d5964f2 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -45,12 +45,37 @@ FILE* open_file(const QString& file_path) const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); if (raw_fp == nullptr) - throw std::runtime_error( - fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); + throw std::runtime_error(fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); return raw_fp; } +EVP_PKEY* create_raw_key_pointer() +{ + std::unique_ptr ctx(EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), + EVP_PKEY_CTX_free); + + if (!ctx || EVP_PKEY_keygen_init(ctx.get()) <= 0) + { + throw std::runtime_error("Failed to initialize key generation"); + } + + // Set EC curve (P-256) + const std::array params = { + // the 3rd argument is length of the buffer, which is 0 in the case of static buffer like "P-256" + OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), + OSSL_PARAM_construct_end()}; + EVP_PKEY_CTX_set_params(ctx.get(), params.data()); + + // Generate the key + EVP_PKEY* raw_key = nullptr; + if (EVP_PKEY_generate(ctx.get(), &raw_key) <= 0) + { + throw std::runtime_error("Failed to generate EC key"); + } + + return raw_key; +} class WritableFile { @@ -73,33 +98,8 @@ class WritableFile class EVPKey { public: - EVPKey() + EVPKey() : key{create_raw_key_pointer(), EVP_PKEY_free} { - std::unique_ptr ctx( - EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), - EVP_PKEY_CTX_free); - - if (!ctx || EVP_PKEY_keygen_init(ctx.get()) <= 0) - { - throw std::runtime_error("Failed to initialize key generation"); - } - - // Set EC curve (P-256) - const std::array params = { - // the 3rd argument is length of the buffer, which is 0 in the case of static buffer like "P-256" - OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), - OSSL_PARAM_construct_end()}; - EVP_PKEY_CTX_set_params(ctx.get(), params.data()); - - // Generate the key - EVP_PKEY* raw_key = nullptr; - if (EVP_PKEY_generate(ctx.get(), &raw_key) <= 0) - { - throw std::runtime_error("Failed to generate EC key"); - } - - // Assign generated key to unique_ptr for RAII management - key.reset(raw_key); } std::string as_pem() const @@ -134,7 +134,7 @@ class EVPKey } private: - std::unique_ptr key{nullptr, EVP_PKEY_free}; + std::unique_ptr key; }; std::vector as_vector(const std::string& v) From 56507bebe35ff315289698a086b6b467d3c72476 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 14 Feb 2025 16:03:14 +0100 Subject: [PATCH 074/114] [ssl cert] used the chrono time units to improve the readability. --- src/cert/ssl_cert_provider.cpp | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index c95d5964f2..4b4e4f9e1b 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -194,9 +194,11 @@ class X509Cert set_random_serial_number(cert.get()); X509_gmtime_adj(X509_get_notBefore(cert.get()), 0); // Start time: now - const long valid_duration_sec = cert_type == CertType::Root ? 3650L * 24L * 60L * 60L : 365L * 24L * 60L * 60L; - // 10 years for root certicicate and 1 year for server and client certificate - X509_gmtime_adj(X509_get_notAfter(cert.get()), valid_duration_sec); + + constexpr std::chrono::seconds one_year = std::chrono::hours{24} * 365; + constexpr std::chrono::seconds ten_years = one_year * 10; + const auto valid_duration = cert_type == CertType::Root ? ten_years : one_year; + X509_gmtime_adj(X509_get_notAfter(cert.get()), valid_duration.count()); constexpr int APPEND_ENTRY{-1}; constexpr int ADD_RDN{0}; From 1afb8124448a773a107c7d62711a1d5a10a456c1 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 14 Feb 2025 16:31:00 +0100 Subject: [PATCH 075/114] [ssl cert] added assertion to the end BIGNUM value. --- src/cert/ssl_cert_provider.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 4b4e4f9e1b..f823d44596 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -152,11 +152,11 @@ void set_random_serial_number(X509* cert) // Convert bytes to an BIGNUM, an arbitrary-precision integer type std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), NULL), BN_free); - if (!bn) { throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); } + assert(!BN_is_negative(bn.get())); // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number // ASN1 is a standard binary format for encoding data like serial numbers in X.509 certificates From 59f1fd557892b78a0899be4a00c9ab9569fa252e Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 14 Feb 2025 16:32:32 +0100 Subject: [PATCH 076/114] [ssl cert] changed the remaining NULL to nullptr --- src/cert/ssl_cert_provider.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index f823d44596..982a68e59c 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -151,7 +151,7 @@ void set_random_serial_number(X509* cert) serial_bytes[0] &= 0x7F; // Convert bytes to an BIGNUM, an arbitrary-precision integer type - std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), NULL), BN_free); + std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), nullptr), BN_free); if (!bn) { throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); @@ -160,7 +160,7 @@ void set_random_serial_number(X509* cert) // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number // ASN1 is a standard binary format for encoding data like serial numbers in X.509 certificates - ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn.get(), NULL); + ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn.get(), nullptr); if (!serial) { throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); From 302d7d6cc0204ce2bd2a7513ce64cfb3ed0fcd99 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 14 Feb 2025 16:40:15 +0100 Subject: [PATCH 077/114] [ssl cert] added comment for the usage of the SSLCertProvider class. --- include/multipass/ssl_cert_provider.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/include/multipass/ssl_cert_provider.h b/include/multipass/ssl_cert_provider.h index ab1243a37a..95f726bf6e 100644 --- a/include/multipass/ssl_cert_provider.h +++ b/include/multipass/ssl_cert_provider.h @@ -35,6 +35,8 @@ class SSLCertProvider : public CertProvider }; explicit SSLCertProvider(const Path& data_dir, const std::string& server_name = ""); + // leave server_name empty for clients; choose a (non-empty) name for servers. + std::string PEM_certificate() const override; std::string PEM_signing_key() const override; From 9747cbbec15b59efe43d329af5eb36788589a681 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 17 Feb 2025 10:38:20 +0100 Subject: [PATCH 078/114] [ssl cert] fixed the linter. --- src/cert/ssl_cert_provider.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 982a68e59c..c1a6cb217c 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -151,7 +151,8 @@ void set_random_serial_number(X509* cert) serial_bytes[0] &= 0x7F; // Convert bytes to an BIGNUM, an arbitrary-precision integer type - std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), nullptr), BN_free); + std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), nullptr), + BN_free); if (!bn) { throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); From 3984815dafb1b02136611a17b52dc56b44faf289 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 18 Feb 2025 15:30:21 +0100 Subject: [PATCH 079/114] [platform] for just testing snap standardpaths values. --- src/platform/platform_linux.cpp | 35 +++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index b8838cfd7a..05b61d3247 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -481,6 +481,41 @@ std::string multipass::platform::host_version() std::filesystem::path mp::platform::Platform::get_root_cert_path() const { + const auto paths = MP_STDPATHS.standardLocations(StandardPaths::AppDataLocation); + + for (const auto& path : paths) + { + mpl::log(mpl::Level::info, "general", fmt::format("path AppDataLocation value is : {}", path.toStdString())); + } + + mpl::log(mpl::Level::warning, "general", "\n"); + + const auto paths2 = MP_STDPATHS.standardLocations(StandardPaths::GenericDataLocation); + for (const auto& path : paths2) + { + mpl::log(mpl::Level::info, + "general", + fmt::format("path GenericDataLocation value is : {}", path.toStdString())); + } + mpl::log(mpl::Level::info, "general", "\n"); + + mpl::log( + mpl::Level::info, + "general", + fmt::format( + "located path in GenericDataLocation value is : {}", + MP_STDPATHS.locate(mp::StandardPaths::GenericDataLocation, "certificates/localhost.pem").toStdString())); + + mpl::log(mpl::Level::info, + "general", + fmt::format("final AppDataLocation value is : {}", + MP_STDPATHS.writableLocation(StandardPaths::AppDataLocation).toStdString())); + + mpl::log(mpl::Level::info, + "general", + fmt::format("final GenericDataLocation value is : {}", + MP_STDPATHS.writableLocation(StandardPaths::GenericDataLocation).toStdString())); + constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; return mp::utils::in_multipass_snap() ? multipass_final_storage_location() / "data" / "multipassd" / "certificates" / root_cert_file_name From d18e3cceb702dd52c47fdf5c9a9f6c225bbb023d Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 24 Feb 2025 15:32:51 +0100 Subject: [PATCH 080/114] [platform linuxj] removed the logs and use constant variable for multipassd directory name. --- src/platform/platform_linux.cpp | 37 +-------------------------------- 1 file changed, 1 insertion(+), 36 deletions(-) diff --git a/src/platform/platform_linux.cpp b/src/platform/platform_linux.cpp index 05b61d3247..c5a0e46da8 100644 --- a/src/platform/platform_linux.cpp +++ b/src/platform/platform_linux.cpp @@ -481,43 +481,8 @@ std::string multipass::platform::host_version() std::filesystem::path mp::platform::Platform::get_root_cert_path() const { - const auto paths = MP_STDPATHS.standardLocations(StandardPaths::AppDataLocation); - - for (const auto& path : paths) - { - mpl::log(mpl::Level::info, "general", fmt::format("path AppDataLocation value is : {}", path.toStdString())); - } - - mpl::log(mpl::Level::warning, "general", "\n"); - - const auto paths2 = MP_STDPATHS.standardLocations(StandardPaths::GenericDataLocation); - for (const auto& path : paths2) - { - mpl::log(mpl::Level::info, - "general", - fmt::format("path GenericDataLocation value is : {}", path.toStdString())); - } - mpl::log(mpl::Level::info, "general", "\n"); - - mpl::log( - mpl::Level::info, - "general", - fmt::format( - "located path in GenericDataLocation value is : {}", - MP_STDPATHS.locate(mp::StandardPaths::GenericDataLocation, "certificates/localhost.pem").toStdString())); - - mpl::log(mpl::Level::info, - "general", - fmt::format("final AppDataLocation value is : {}", - MP_STDPATHS.writableLocation(StandardPaths::AppDataLocation).toStdString())); - - mpl::log(mpl::Level::info, - "general", - fmt::format("final GenericDataLocation value is : {}", - MP_STDPATHS.writableLocation(StandardPaths::GenericDataLocation).toStdString())); - constexpr auto* root_cert_file_name = "multipass_root_cert.pem"; return mp::utils::in_multipass_snap() - ? multipass_final_storage_location() / "data" / "multipassd" / "certificates" / root_cert_file_name + ? multipass_final_storage_location() / "data" / daemon_name / "certificates" / root_cert_file_name : std::filesystem::path{"/usr/local/share/ca-certificates"} / root_cert_file_name; } From c58c8c7ceea1f244dd3bd2ac66ea2cc7a5f28e64 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Mar 2025 11:17:27 +0100 Subject: [PATCH 081/114] [daemon config] remove the restrict_permissions on data folder. --- src/daemon/daemon_config.cpp | 15 ++++----------- tests/test_daemon.cpp | 4 +--- 2 files changed, 5 insertions(+), 14 deletions(-) diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index 8b8032d86f..2df73fc32b 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -109,7 +109,7 @@ std::unique_ptr mp::DaemonConfigBuilder::build() auto multiplexing_logger = std::make_shared(std::move(logger)); mpl::set_logger(multiplexing_logger); - MP_PLATFORM.setup_permission_inheritance(); + MP_PLATFORM.setup_permission_inheritance(false); auto storage_path = MP_PLATFORM.multipass_storage_location(); if (!storage_path.isEmpty()) @@ -192,16 +192,9 @@ std::unique_ptr mp::DaemonConfigBuilder::build() std::make_unique(url_downloader.get(), cache_directory, manifest_ttl); } - // restrict permissions for all existing files and folders - if (!storage_path.isEmpty()) - { - MP_PERMISSIONS.restrict_permissions(storage_path.toStdU16String()); - } - else - { - MP_PERMISSIONS.restrict_permissions(data_directory.toStdU16String()); - MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String()); - } + // restrict permissions for all existing files and folders in cache directory, the data directory will have a more + // granular permissions setting + MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String()); return std::unique_ptr(new DaemonConfig{ std::move(url_downloader), std::move(factory), std::move(image_hosts), std::move(vault), diff --git a/tests/test_daemon.cpp b/tests/test_daemon.cpp index ff5ee59f63..92d5d6626a 100644 --- a/tests/test_daemon.cpp +++ b/tests/test_daemon.cpp @@ -2519,7 +2519,6 @@ TEST_F(Daemon, sets_permissions_on_provided_storage_path) const std::filesystem::path std_path{path.toStdU16String()}; EXPECT_CALL(mock_platform, multipass_storage_location()).WillOnce(Return(path)); - EXPECT_CALL(mock_permission_utils, restrict_permissions(std_path)); config_builder.build(); } @@ -2533,7 +2532,6 @@ TEST_F(Daemon, sets_permissions_on_storage_dirs) config_builder.cache_directory = "Pirate's secret cache"; const std::filesystem::path std_cache_path{config_builder.cache_directory.toStdU16String()}; - EXPECT_CALL(mock_permission_utils, restrict_permissions(std_data_path)); EXPECT_CALL(mock_permission_utils, restrict_permissions(std_cache_path)); config_builder.build(); @@ -2541,7 +2539,7 @@ TEST_F(Daemon, sets_permissions_on_storage_dirs) TEST_F(Daemon, sets_up_permission_inheritance) { - EXPECT_CALL(mock_platform, setup_permission_inheritance(true)); + EXPECT_CALL(mock_platform, setup_permission_inheritance(false)); config_builder.build(); } From 61f731e8a12676cccab47adc84e43b23d8a18b8f Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Mar 2025 11:19:05 +0100 Subject: [PATCH 082/114] [cert store] enforce ower_all permission to authenticated-certs sub-folder. --- src/cert/client_cert_store.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cert/client_cert_store.cpp b/src/cert/client_cert_store.cpp index 1b9591f32d..1753712d26 100644 --- a/src/cert/client_cert_store.cpp +++ b/src/cert/client_cert_store.cpp @@ -54,7 +54,7 @@ auto load_certs_from_file(const QDir& cert_dir) } // namespace mp::ClientCertStore::ClientCertStore(const multipass::Path& data_dir) - : cert_dir(MP_UTILS.make_dir(data_dir, mp::authenticated_certs_dir)), + : cert_dir(MP_UTILS.make_dir(data_dir, mp::authenticated_certs_dir, fs::perms::owner_all)), authenticated_client_certs{load_certs_from_file(cert_dir)} { mpl::log(mpl::Level::trace, category, fmt::format("Loading client certs from {}", cert_dir.absolutePath())); From d08a6bb4df6ed0354e7005fbb7b3f09b13fd9a61 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Mar 2025 11:19:38 +0100 Subject: [PATCH 083/114] [qemu platform] enforce ower_all permission to network sub-folder. --- src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp b/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp index ee76cf6150..b8e2a907a5 100644 --- a/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp +++ b/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp @@ -119,7 +119,7 @@ void delete_virtual_switch(const QString& bridge_name) mp::QemuPlatformDetail::QemuPlatformDetail(const mp::Path& data_dir) : bridge_name{multipass_bridge_name}, - network_dir{MP_UTILS.make_dir(QDir(data_dir), "network")}, + network_dir{MP_UTILS.make_dir(QDir(data_dir), "network", fs::perms::owner_all)}, subnet{MP_BACKEND.get_subnet(network_dir, bridge_name)}, dnsmasq_server{init_nat_network(network_dir, bridge_name, subnet)}, firewall_config{MP_FIREWALL_CONFIG_FACTORY.make_firewall_config(bridge_name, subnet)} From 90ef6205346c8245dd9b7f031b502d8ff4fc5071 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Mar 2025 11:20:00 +0100 Subject: [PATCH 084/114] [open ssh] enforce permission to ssh-keys sub-folder. --- src/ssh/openssh_key_provider.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/ssh/openssh_key_provider.cpp b/src/ssh/openssh_key_provider.cpp index 332416645a..04e2b5cc62 100644 --- a/src/ssh/openssh_key_provider.cpp +++ b/src/ssh/openssh_key_provider.cpp @@ -67,7 +67,8 @@ void mp::OpenSSHKeyProvider::KeyDeleter::operator()(ssh_key key) } mp::OpenSSHKeyProvider::OpenSSHKeyProvider(const mp::Path& cache_dir) - : ssh_key_dir{MP_UTILS.make_dir(cache_dir, "ssh-keys")}, priv_key{get_priv_key(ssh_key_dir)} + : ssh_key_dir{MP_UTILS.make_dir(cache_dir, "ssh-keys", std::filesystem::perms::owner_all)}, + priv_key{get_priv_key(ssh_key_dir)} { } From 3c4d0f4dd20a669266fd610cdc07c9205f4f1b17 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Mar 2025 11:37:19 +0100 Subject: [PATCH 085/114] [vault] enforce ower_all permission to vault sub-folder. --- src/daemon/default_vm_image_vault.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index fbec7a9e8f..aad63abf68 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -249,6 +249,7 @@ mp::DefaultVMImageVault::DefaultVMImageVault(std::vector image_hos prepared_image_records{load_db(cache_dir.filePath(image_db_name))}, instance_image_records{load_db(data_dir.filePath(instance_db_name))} { + MP_UTILS.make_dir(data_dir, std::filesystem::perms::owner_all); } mp::DefaultVMImageVault::~DefaultVMImageVault() From 3db5b2e7606a66f2804f7afaaa107054b7973f15 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Mar 2025 11:38:20 +0100 Subject: [PATCH 086/114] [daemon] enforce ower_all permission to multipassd-vm-instances.json file. --- src/daemon/daemon.cpp | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 9675858258..8495913a4b 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -2867,7 +2867,10 @@ void mp::Daemon::persist_instances() } QDir data_dir{ mp::utils::backend_directory_path(config->data_directory, config->factory->get_backend_directory_name())}; - MP_JSONUTILS.write_json(instance_records_json, data_dir.filePath(instance_db_name)); + + const QString instance_db_path = data_dir.filePath(instance_db_name); + MP_JSONUTILS.write_json(instance_records_json, instance_db_path); + MP_PLATFORM.set_permissions(fs::path{instance_db_path.toStdString()}, fs::perms::owner_all); } void mp::Daemon::release_resources(const std::string& instance) From eb39c87335ed7d928166bec563092bf27a7dc87c Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Mon, 3 Mar 2025 13:12:18 +0100 Subject: [PATCH 087/114] [daemon config] added explicit permission setting for overwriting purpose. This is needed for users on windows whose multipass folder was restricted previously. --- src/daemon/daemon_config.cpp | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index 2df73fc32b..a355e2116a 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -192,9 +192,17 @@ std::unique_ptr mp::DaemonConfigBuilder::build() std::make_unique(url_downloader.get(), cache_directory, manifest_ttl); } - // restrict permissions for all existing files and folders in cache directory, the data directory will have a more - // granular permissions setting - MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String()); + // restrict permissions for all existing files and folders in cache directory, the data directory opens execute + // permission for other users, the sub-directories of it will have a granular permissions setting + if (!storage_path.isEmpty()) + { + MP_PLATFORM.set_permissions(storage_path.toStdU16String(), fs::perms::owner_all | fs::perms::others_exec); + } + else + { + MP_PLATFORM.set_permissions(data_directory.toStdU16String(), fs::perms::owner_all | fs::perms::others_exec); + MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String()); + } return std::unique_ptr(new DaemonConfig{ std::move(url_downloader), std::move(factory), std::move(image_hosts), std::move(vault), From 37858e8b6a525aaf0890d30b064f6e27ca63d03b Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 11:40:32 +0100 Subject: [PATCH 088/114] [daemon] use toStdU16String for windows possible non-latin characters. --- src/daemon/daemon.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 8495913a4b..6e3fd69adf 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -2870,7 +2870,7 @@ void mp::Daemon::persist_instances() const QString instance_db_path = data_dir.filePath(instance_db_name); MP_JSONUTILS.write_json(instance_records_json, instance_db_path); - MP_PLATFORM.set_permissions(fs::path{instance_db_path.toStdString()}, fs::perms::owner_all); + MP_PLATFORM.set_permissions(fs::path{instance_db_path.toStdU16String()}, fs::perms::owner_all); } void mp::Daemon::release_resources(const std::string& instance) From c848554682aa41ce624da9a87a973fd90c456171 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 12:56:10 +0100 Subject: [PATCH 089/114] [ssl cert] enforced zero initialization for some objects creation. --- src/cert/ssl_cert_provider.cpp | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index c1a6cb217c..a98a90e196 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -229,7 +229,7 @@ class X509Cert const auto& issuer_cert = cert_type == CertType::Server ? root_certificate.value().cert : cert; // Add X509v3 extensions - X509V3_CTX ctx; + X509V3_CTX ctx{}; X509V3_set_ctx(&ctx, issuer_cert.get(), cert.get(), nullptr, nullptr, 0); // Subject Alternative Name @@ -310,12 +310,12 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const auto priv_root_key_path = cert_dir.filePath(prefix + "_root_key.pem"); - EVPKey root_cert_key; + EVPKey root_cert_key{}; X509Cert root_cert{root_cert_key, X509Cert::CertType::Root}; root_cert_key.write(priv_root_key_path); root_cert.write(root_cert_path.u8string().c_str()); - const EVPKey server_cert_key; + const EVPKey server_cert_key{}; const X509Cert signed_server_cert{server_cert_key, X509Cert::CertType::Server, server_name, @@ -332,7 +332,7 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, return {mp::utils::contents_of(cert_path), mp::utils::contents_of(priv_key_path)}; } - const EVPKey client_cert_key; + const EVPKey client_cert_key{}; const X509Cert client_cert{client_cert_key, X509Cert::CertType::Client}; client_cert_key.write(priv_key_path); client_cert.write(cert_path); From 3d22fb5dc45242f915d616fd7ab78fa913a5ac5f Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 14:50:58 +0100 Subject: [PATCH 090/114] [ssl cert] added some return code check for some open ssl apis. --- src/cert/ssl_cert_provider.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index a98a90e196..f760aee4ff 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -65,7 +65,10 @@ EVP_PKEY* create_raw_key_pointer() // the 3rd argument is length of the buffer, which is 0 in the case of static buffer like "P-256" OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), OSSL_PARAM_construct_end()}; - EVP_PKEY_CTX_set_params(ctx.get(), params.data()); + if (EVP_PKEY_CTX_set_params(ctx.get(), params.data()) != 1) + { + throw std::runtime_error("EVP_PKEY_CTX_set_params() failed"); + } // Generate the key EVP_PKEY* raw_key = nullptr; @@ -146,7 +149,10 @@ void set_random_serial_number(X509* cert) { // OpenSSL recommends a 20-byte (160-bit) serial number std::array serial_bytes{}; - RAND_bytes(serial_bytes.data(), serial_bytes.size()); + if (RAND_bytes(serial_bytes.data(), serial_bytes.size()) != 1) + { + throw std::runtime_error("Failed to set random bytes\n"); + } // Set the highest bit to 0 (unsigned) to ensure it's positive serial_bytes[0] &= 0x7F; @@ -168,7 +174,10 @@ void set_random_serial_number(X509* cert) } // Set the serial number in the certificate - X509_set_serialNumber(cert, serial); + if (X509_set_serialNumber(cert, serial) != 1) + { + throw std::runtime_error("Failed to set serial number!\n"); + } } class X509Cert From 2291748f6bd56cb11dcc306a9960049d30cea518 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 15:09:30 +0100 Subject: [PATCH 091/114] [ssl cert] improve the key creation function by returning unique pointer instead of raw pointer --- src/cert/ssl_cert_provider.cpp | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index f760aee4ff..4b38b6b91f 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -50,7 +50,7 @@ FILE* open_file(const QString& file_path) return raw_fp; } -EVP_PKEY* create_raw_key_pointer() +std::unique_ptr create_key() { std::unique_ptr ctx(EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), EVP_PKEY_CTX_free); @@ -77,7 +77,8 @@ EVP_PKEY* create_raw_key_pointer() throw std::runtime_error("Failed to generate EC key"); } - return raw_key; + return std::unique_ptr(raw_key, EVP_PKEY_free); + } class WritableFile @@ -101,7 +102,7 @@ class WritableFile class EVPKey { public: - EVPKey() : key{create_raw_key_pointer(), EVP_PKEY_free} + EVPKey() : key{create_key()} { } From b34d78ea7f1c06afb9fe79cbd10cd4fcf97187db Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 15:13:42 +0100 Subject: [PATCH 092/114] [ssl cert] changed create_key to be static private function and made an alias on unique pointer type. --- src/cert/ssl_cert_provider.cpp | 64 ++++++++++++++++++---------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 4b38b6b91f..03900d9691 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -50,36 +50,7 @@ FILE* open_file(const QString& file_path) return raw_fp; } -std::unique_ptr create_key() -{ - std::unique_ptr ctx(EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), - EVP_PKEY_CTX_free); - if (!ctx || EVP_PKEY_keygen_init(ctx.get()) <= 0) - { - throw std::runtime_error("Failed to initialize key generation"); - } - - // Set EC curve (P-256) - const std::array params = { - // the 3rd argument is length of the buffer, which is 0 in the case of static buffer like "P-256" - OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), - OSSL_PARAM_construct_end()}; - if (EVP_PKEY_CTX_set_params(ctx.get(), params.data()) != 1) - { - throw std::runtime_error("EVP_PKEY_CTX_set_params() failed"); - } - - // Generate the key - EVP_PKEY* raw_key = nullptr; - if (EVP_PKEY_generate(ctx.get(), &raw_key) <= 0) - { - throw std::runtime_error("Failed to generate EC key"); - } - - return std::unique_ptr(raw_key, EVP_PKEY_free); - -} class WritableFile { @@ -138,7 +109,40 @@ class EVPKey } private: - std::unique_ptr key; + using EVPKeyPtr = std::unique_ptr; + + static EVPKeyPtr create_key() + { + std::unique_ptr ctx( + EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), + EVP_PKEY_CTX_free); + + if (!ctx || EVP_PKEY_keygen_init(ctx.get()) <= 0) + { + throw std::runtime_error("Failed to initialize key generation"); + } + + // Set EC curve (P-256) + const std::array params = { + // the 3rd argument is length of the buffer, which is 0 in the case of static buffer like "P-256" + OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), + OSSL_PARAM_construct_end()}; + if (EVP_PKEY_CTX_set_params(ctx.get(), params.data()) != 1) + { + throw std::runtime_error("EVP_PKEY_CTX_set_params() failed"); + } + + // Generate the key + EVP_PKEY* raw_key = nullptr; + if (EVP_PKEY_generate(ctx.get(), &raw_key) <= 0) + { + throw std::runtime_error("Failed to generate EC key"); + } + + return EVPKeyPtr(raw_key, EVP_PKEY_free); + } + + EVPKeyPtr key; }; std::vector as_vector(const std::string& v) From 51c302542877d738fb5520dc00235b4798f43b75 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 15:20:22 +0100 Subject: [PATCH 093/114] [ssl cert] changing open_file to return unique pointer. --- src/cert/ssl_cert_provider.cpp | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 03900d9691..23fc2ce8e4 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -36,7 +36,7 @@ namespace mp = multipass; namespace { -FILE* open_file(const QString& file_path) +std::unique_ptr open_file(const QString& file_path) { const std::filesystem::path file_path_std{file_path.toStdString()}; std::filesystem::create_directories(file_path_std.parent_path()); @@ -47,15 +47,14 @@ FILE* open_file(const QString& file_path) if (raw_fp == nullptr) throw std::runtime_error(fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); - return raw_fp; + // return raw_fp; + return std::unique_ptr{raw_fp, fclose}; } - - class WritableFile { public: - explicit WritableFile(const QString& file_path) : fp{open_file(file_path), fclose} + explicit WritableFile(const QString& file_path) : fp{open_file(file_path)} { } From a5fbc974a283e2c31860d718c2d430665e786210 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 15:24:23 +0100 Subject: [PATCH 094/114] [ssl cert] move the open_file into the WritableFile class. --- src/cert/ssl_cert_provider.cpp | 33 +++++++++++++++++---------------- 1 file changed, 17 insertions(+), 16 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 23fc2ce8e4..4bf3443f83 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -36,21 +36,6 @@ namespace mp = multipass; namespace { -std::unique_ptr open_file(const QString& file_path) -{ - const std::filesystem::path file_path_std{file_path.toStdString()}; - std::filesystem::create_directories(file_path_std.parent_path()); - // make sure the parent directory exist - - const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); - - if (raw_fp == nullptr) - throw std::runtime_error(fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); - - // return raw_fp; - return std::unique_ptr{raw_fp, fclose}; -} - class WritableFile { public: @@ -64,9 +49,25 @@ class WritableFile } private: + using FilePtr = std::unique_ptr; + static FilePtr open_file(const QString& file_path) + { + const std::filesystem::path file_path_std{file_path.toStdString()}; + std::filesystem::create_directories(file_path_std.parent_path()); + // make sure the parent directory exist + + const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); + + if (raw_fp == nullptr) + throw std::runtime_error( + fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); + + return FilePtr{raw_fp, fclose}; + } + // decltype(&fclose) does not preserve these some extra function attributes of fclose, leads to warning and // compilation error - std::unique_ptr fp; + FilePtr fp; }; class EVPKey From 4b8b386f9eb99b3fce7ed4f0fe99ea32c8612c02 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Tue, 4 Mar 2025 16:15:52 +0100 Subject: [PATCH 095/114] [ssl cert] added nodiscard attribute to some functions. --- src/cert/ssl_cert_provider.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 4bf3443f83..973772656c 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -50,7 +50,7 @@ class WritableFile private: using FilePtr = std::unique_ptr; - static FilePtr open_file(const QString& file_path) + [[nodiscard]] static FilePtr open_file(const QString& file_path) { const std::filesystem::path file_path_std{file_path.toStdString()}; std::filesystem::create_directories(file_path_std.parent_path()); @@ -111,7 +111,7 @@ class EVPKey private: using EVPKeyPtr = std::unique_ptr; - static EVPKeyPtr create_key() + [[nodiscard]] static EVPKeyPtr create_key() { std::unique_ptr ctx( EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), From 19e52577da61a9aa8a9bda9794755229d0aef619 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 11:52:24 +0100 Subject: [PATCH 096/114] [utils] added check utility function for checking c-api return. --- include/multipass/utils.h | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/include/multipass/utils.h b/include/multipass/utils.h index 73f9936a8a..56c8a56359 100644 --- a/include/multipass/utils.h +++ b/include/multipass/utils.h @@ -195,6 +195,27 @@ void parallel_for_each(Container& input_container, UnaryOperation&& unary_op) empty_future.get(); } } + +// utility function for checking return code or pointer from C-apis +// TODO: constrain T to int or raw pointer once C++20 concepts is available +template +void check(T result, const std::string& errorMessage) +{ + if constexpr (std::is_pointer_v) + { + if (result == nullptr) + { + throw std::runtime_error(errorMessage); + } + } + else + { + if (result != 1) + { + throw std::runtime_error(errorMessage); + } + } +} } // namespace utils class Utils : public Singleton From 10f165034d295033e93e436c4b13ef7a7acd17b7 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 11:57:27 +0100 Subject: [PATCH 097/114] [ssl cert] used the check function checking raw file pointer. --- src/cert/ssl_cert_provider.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 973772656c..1245b08a64 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -49,6 +49,8 @@ class WritableFile } private: + // decltype(&fclose) does not preserve these some extra function attributes of fclose, leads to warning and + // compilation error using FilePtr = std::unique_ptr; [[nodiscard]] static FilePtr open_file(const QString& file_path) { @@ -57,16 +59,11 @@ class WritableFile // make sure the parent directory exist const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); - - if (raw_fp == nullptr) - throw std::runtime_error( - fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); + mp::utils::check(raw_fp, fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); return FilePtr{raw_fp, fclose}; } - // decltype(&fclose) does not preserve these some extra function attributes of fclose, leads to warning and - // compilation error FilePtr fp; }; From 9e6900363eecc7e6cfe3cc88ebc53cd0c461d514 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 12:08:10 +0100 Subject: [PATCH 098/114] [ssl cert] more invocations of the check utility function. --- src/cert/ssl_cert_provider.cpp | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 1245b08a64..f237d43cf1 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -124,17 +124,12 @@ class EVPKey // the 3rd argument is length of the buffer, which is 0 in the case of static buffer like "P-256" OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), OSSL_PARAM_construct_end()}; - if (EVP_PKEY_CTX_set_params(ctx.get(), params.data()) != 1) - { - throw std::runtime_error("EVP_PKEY_CTX_set_params() failed"); - } + + mp::utils::check(EVP_PKEY_CTX_set_params(ctx.get(), params.data()), "EVP_PKEY_CTX_set_params() failed"); // Generate the key EVP_PKEY* raw_key = nullptr; - if (EVP_PKEY_generate(ctx.get(), &raw_key) <= 0) - { - throw std::runtime_error("Failed to generate EC key"); - } + mp::utils::check(EVP_PKEY_generate(ctx.get(), &raw_key), "Failed to generate EC key"); return EVPKeyPtr(raw_key, EVP_PKEY_free); } From bb42414beeeb441f8e7d5c0750f5a8eaa9f7e35e Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 12:15:13 +0100 Subject: [PATCH 099/114] [ssl cert] more mp::utils::check based refactors --- src/cert/ssl_cert_provider.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index f237d43cf1..5c4bfc57e0 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -146,20 +146,14 @@ void set_random_serial_number(X509* cert) { // OpenSSL recommends a 20-byte (160-bit) serial number std::array serial_bytes{}; - if (RAND_bytes(serial_bytes.data(), serial_bytes.size()) != 1) - { - throw std::runtime_error("Failed to set random bytes\n"); - } + mp::utils::check(RAND_bytes(serial_bytes.data(), serial_bytes.size()), "Failed to set random bytes\n"); // Set the highest bit to 0 (unsigned) to ensure it's positive serial_bytes[0] &= 0x7F; // Convert bytes to an BIGNUM, an arbitrary-precision integer type std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), nullptr), BN_free); - if (!bn) - { - throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); - } + mp::utils::check(bn.get(), "Failed to convert serial bytes to BIGNUM\n"); assert(!BN_is_negative(bn.get())); // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number From 19466449585d47777b8092ed9878026c9ca61688 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 12:17:58 +0100 Subject: [PATCH 100/114] [ssl cert] more refactor based on mp::utils::check. --- src/cert/ssl_cert_provider.cpp | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 5c4bfc57e0..e401df3c2d 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -159,16 +159,10 @@ void set_random_serial_number(X509* cert) // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number // ASN1 is a standard binary format for encoding data like serial numbers in X.509 certificates ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn.get(), nullptr); - if (!serial) - { - throw std::runtime_error("Failed to convert serial bytes to BIGNUM\n"); - } + mp::utils::check(serial, "Failed to convert serial bytes to BIGNUM\n"); // Set the serial number in the certificate - if (X509_set_serialNumber(cert, serial) != 1) - { - throw std::runtime_error("Failed to set serial number!\n"); - } + mp::utils::check(X509_set_serialNumber(cert, serial), "Failed to set serial number!\n"); } class X509Cert From 9393794adb613b95c92e0574cc4d9150c02ded2a Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 12:22:14 +0100 Subject: [PATCH 101/114] [ssl cert] more refactor based on mp::utils::check. --- src/cert/ssl_cert_provider.cpp | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index e401df3c2d..4296ac57ff 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -182,8 +182,7 @@ class X509Cert const std::optional& root_certificate = std::nullopt) // generate root, client or signed server certificate, the third one requires the last three arguments populated { - if (cert == nullptr) - throw std::runtime_error("Failed to allocate x509 cert structure"); + mp::utils::check(cert.get(), "Failed to allocate x509 cert structure"); X509_set_version(cert.get(), 2); // 0 index based, 2 amounts to 3 @@ -218,8 +217,7 @@ class X509Cert cert_type == CertType::Server ? X509_get_subject_name(root_certificate.value().cert.get()) : subject_name; X509_set_issuer_name(cert.get(), issuer_name); - if (!X509_set_pubkey(cert.get(), key.get())) - throw std::runtime_error("Failed to set certificate public key"); + mp::utils::check(X509_set_pubkey(cert.get(), key.get()), "Failed to set certificate public key"); const auto& issuer_cert = cert_type == CertType::Server ? root_certificate.value().cert : cert; // Add X509v3 extensions @@ -246,8 +244,8 @@ class X509Cert (std::string("critical,CA:") + (cert_type == CertType::Root ? "TRUE" : "FALSE")).c_str()); const auto& signing_key = cert_type == CertType::Server ? *root_certificate_key : key; - if (!X509_sign(cert.get(), signing_key.get(), EVP_sha256())) - throw std::runtime_error("Failed to sign certificate"); + + mp::utils::check(X509_sign(cert.get(), signing_key.get(), EVP_sha256()), "Failed to sign certificate"); } std::string as_pem() const From 56c478f3d282c30e383409b0f41f0c64e56f83b2 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 12:43:03 +0100 Subject: [PATCH 102/114] [ssl cert] refined the check utility function. --- include/multipass/utils.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/include/multipass/utils.h b/include/multipass/utils.h index 56c8a56359..ba3f12fbf0 100644 --- a/include/multipass/utils.h +++ b/include/multipass/utils.h @@ -196,7 +196,7 @@ void parallel_for_each(Container& input_container, UnaryOperation&& unary_op) } } -// utility function for checking return code or pointer from C-apis +// utility function for checking return code or raw pointer from C-apis // TODO: constrain T to int or raw pointer once C++20 concepts is available template void check(T result, const std::string& errorMessage) @@ -210,7 +210,7 @@ void check(T result, const std::string& errorMessage) } else { - if (result != 1) + if (result <= 0) { throw std::runtime_error(errorMessage); } From c3f3bc9facf4356d45611a3e45b6eefc0a4d5356 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 12:58:40 +0100 Subject: [PATCH 103/114] [ssl cert] more refactor based mp::utils::check function. --- src/cert/ssl_cert_provider.cpp | 17 ++++------------- 1 file changed, 4 insertions(+), 13 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 4296ac57ff..c51c2c431f 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -114,10 +114,8 @@ class EVPKey EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), EVP_PKEY_CTX_free); - if (!ctx || EVP_PKEY_keygen_init(ctx.get()) <= 0) - { - throw std::runtime_error("Failed to initialize key generation"); - } + mp::utils::check(ctx.get(), "Failed to create EVP_PKEY_CTX"); + mp::utils::check(EVP_PKEY_keygen_init(ctx.get()), "Failed to initialize key generation"); // Set EC curve (P-256) const std::array params = { @@ -271,15 +269,8 @@ class X509Cert X509V3_EXT_conf_nid(nullptr, &ctx, nid, value), X509_EXTENSION_free); - if (!ext) - { - throw std::runtime_error("Failed to create X509 extension"); - } - - if (!X509_add_ext(cert.get(), ext.get(), -1)) - { - throw std::runtime_error("Failed to add X509 extension"); - } + mp::utils::check(ext.get(), "Failed to create X509 extension"); + mp::utils::check(X509_add_ext(cert.get(), ext.get(), -1), "Failed to add X509 extension"); } std::unique_ptr cert{X509_new(), X509_free}; From 7a5118a10c66d48140a1169a4a78a2070f02943c Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 14:28:14 +0100 Subject: [PATCH 104/114] [ssl cert] more refactor based on the mp::utils::check function. --- src/cert/ssl_cert_provider.cpp | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index c51c2c431f..a776137921 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -77,9 +77,8 @@ class EVPKey std::string as_pem() const { mp::BIOMem mem; - auto bytes = PEM_write_bio_PrivateKey(mem.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr); - if (bytes == 0) - throw std::runtime_error("Failed to export certificate in PEM format"); + mp::utils::check(PEM_write_bio_PrivateKey(mem.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr), + "Failed to export certificate in PEM format"); return mem.as_string(); } @@ -94,8 +93,8 @@ class EVPKey } WritableFile file{key_path}; - if (!PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr)) - throw std::runtime_error(fmt::format("Failed writing certificate private key to file '{}'", key_path)); + mp::utils::check(PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr), + fmt::format("Failed writing certificate private key to file '{}'", key_path)); QFile::setPermissions(key_path, QFile::ReadOwner); } @@ -249,17 +248,16 @@ class X509Cert std::string as_pem() const { mp::BIOMem mem; - auto bytes = PEM_write_bio_X509(mem.get(), cert.get()); - if (bytes == 0) - throw std::runtime_error("Failed to write certificate in PEM format"); + mp::utils::check(PEM_write_bio_X509(mem.get(), cert.get()), "Failed to write certificate in PEM format"); return mem.as_string(); } void write(const QString& cert_path) const { WritableFile file{cert_path}; - if (!PEM_write_X509(file.get(), cert.get())) - throw std::runtime_error(fmt::format("Failed writing certificate to file '{}'", cert_path)); + + mp::utils::check(PEM_write_X509(file.get(), cert.get()), + fmt::format("Failed writing certificate to file '{}'", cert_path)); } private: From af786897031459903989994d8f73d6411b849633 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Wed, 5 Mar 2025 15:22:45 +0100 Subject: [PATCH 105/114] [ssl cert] confined the check utility function into ssl_cert_provider.cpp --- include/multipass/utils.h | 21 ----------- src/cert/ssl_cert_provider.cpp | 64 +++++++++++++++++++++++----------- 2 files changed, 43 insertions(+), 42 deletions(-) diff --git a/include/multipass/utils.h b/include/multipass/utils.h index ba3f12fbf0..73f9936a8a 100644 --- a/include/multipass/utils.h +++ b/include/multipass/utils.h @@ -195,27 +195,6 @@ void parallel_for_each(Container& input_container, UnaryOperation&& unary_op) empty_future.get(); } } - -// utility function for checking return code or raw pointer from C-apis -// TODO: constrain T to int or raw pointer once C++20 concepts is available -template -void check(T result, const std::string& errorMessage) -{ - if constexpr (std::is_pointer_v) - { - if (result == nullptr) - { - throw std::runtime_error(errorMessage); - } - } - else - { - if (result <= 0) - { - throw std::runtime_error(errorMessage); - } - } -} } // namespace utils class Utils : public Singleton diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index a776137921..b8555a6255 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -36,6 +36,28 @@ namespace mp = multipass; namespace { +// utility function for checking return code or raw pointer from openssl C-apis +// TODO: constrain T to int or raw pointer once C++20 concepts is available +template +void openssl_check(T result, const std::string& errorMessage) +{ + // TODO: expand std::is_pointer_v check to cover all smart pointers as well + if constexpr (std::is_pointer_v) + { + if (result == nullptr) + { + throw std::runtime_error(errorMessage); + } + } + else + { + if (result <= 0) + { + throw std::runtime_error(fmt::format("{}, with the error code {}", errorMessage, result)); + } + } +} + class WritableFile { public: @@ -59,7 +81,7 @@ class WritableFile // make sure the parent directory exist const auto raw_fp = fopen(file_path_std.u8string().c_str(), "wb"); - mp::utils::check(raw_fp, fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); + openssl_check(raw_fp, fmt::format("failed to open file '{}': {}({})", file_path, strerror(errno), errno)); return FilePtr{raw_fp, fclose}; } @@ -77,8 +99,8 @@ class EVPKey std::string as_pem() const { mp::BIOMem mem; - mp::utils::check(PEM_write_bio_PrivateKey(mem.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr), - "Failed to export certificate in PEM format"); + openssl_check(PEM_write_bio_PrivateKey(mem.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr), + "Failed to export certificate in PEM format"); return mem.as_string(); } @@ -93,8 +115,8 @@ class EVPKey } WritableFile file{key_path}; - mp::utils::check(PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr), - fmt::format("Failed writing certificate private key to file '{}'", key_path)); + openssl_check(PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr), + fmt::format("Failed writing certificate private key to file '{}'", key_path)); QFile::setPermissions(key_path, QFile::ReadOwner); } @@ -113,8 +135,8 @@ class EVPKey EVP_PKEY_CTX_new_from_name(nullptr, "EC", nullptr), EVP_PKEY_CTX_free); - mp::utils::check(ctx.get(), "Failed to create EVP_PKEY_CTX"); - mp::utils::check(EVP_PKEY_keygen_init(ctx.get()), "Failed to initialize key generation"); + openssl_check(ctx.get(), "Failed to create EVP_PKEY_CTX"); + openssl_check(EVP_PKEY_keygen_init(ctx.get()), "Failed to initialize key generation"); // Set EC curve (P-256) const std::array params = { @@ -122,11 +144,11 @@ class EVPKey OSSL_PARAM_construct_utf8_string(OSSL_PKEY_PARAM_GROUP_NAME, const_cast("P-256"), 0), OSSL_PARAM_construct_end()}; - mp::utils::check(EVP_PKEY_CTX_set_params(ctx.get(), params.data()), "EVP_PKEY_CTX_set_params() failed"); + openssl_check(EVP_PKEY_CTX_set_params(ctx.get(), params.data()), "EVP_PKEY_CTX_set_params() failed"); // Generate the key EVP_PKEY* raw_key = nullptr; - mp::utils::check(EVP_PKEY_generate(ctx.get(), &raw_key), "Failed to generate EC key"); + openssl_check(EVP_PKEY_generate(ctx.get(), &raw_key), "Failed to generate EC key"); return EVPKeyPtr(raw_key, EVP_PKEY_free); } @@ -143,23 +165,23 @@ void set_random_serial_number(X509* cert) { // OpenSSL recommends a 20-byte (160-bit) serial number std::array serial_bytes{}; - mp::utils::check(RAND_bytes(serial_bytes.data(), serial_bytes.size()), "Failed to set random bytes\n"); + openssl_check(RAND_bytes(serial_bytes.data(), serial_bytes.size()), "Failed to set random bytes\n"); // Set the highest bit to 0 (unsigned) to ensure it's positive serial_bytes[0] &= 0x7F; // Convert bytes to an BIGNUM, an arbitrary-precision integer type std::unique_ptr bn(BN_bin2bn(serial_bytes.data(), serial_bytes.size(), nullptr), BN_free); - mp::utils::check(bn.get(), "Failed to convert serial bytes to BIGNUM\n"); + openssl_check(bn.get(), "Failed to convert serial bytes to BIGNUM\n"); assert(!BN_is_negative(bn.get())); // Convert BIGNUM to ASN1_INTEGER and set it as the certificate serial number // ASN1 is a standard binary format for encoding data like serial numbers in X.509 certificates ASN1_INTEGER* serial = BN_to_ASN1_INTEGER(bn.get(), nullptr); - mp::utils::check(serial, "Failed to convert serial bytes to BIGNUM\n"); + openssl_check(serial, "Failed to convert serial bytes to BIGNUM\n"); // Set the serial number in the certificate - mp::utils::check(X509_set_serialNumber(cert, serial), "Failed to set serial number!\n"); + openssl_check(X509_set_serialNumber(cert, serial), "Failed to set serial number!\n"); } class X509Cert @@ -179,7 +201,7 @@ class X509Cert const std::optional& root_certificate = std::nullopt) // generate root, client or signed server certificate, the third one requires the last three arguments populated { - mp::utils::check(cert.get(), "Failed to allocate x509 cert structure"); + openssl_check(cert.get(), "Failed to allocate x509 cert structure"); X509_set_version(cert.get(), 2); // 0 index based, 2 amounts to 3 @@ -214,7 +236,7 @@ class X509Cert cert_type == CertType::Server ? X509_get_subject_name(root_certificate.value().cert.get()) : subject_name; X509_set_issuer_name(cert.get(), issuer_name); - mp::utils::check(X509_set_pubkey(cert.get(), key.get()), "Failed to set certificate public key"); + openssl_check(X509_set_pubkey(cert.get(), key.get()), "Failed to set certificate public key"); const auto& issuer_cert = cert_type == CertType::Server ? root_certificate.value().cert : cert; // Add X509v3 extensions @@ -242,13 +264,13 @@ class X509Cert const auto& signing_key = cert_type == CertType::Server ? *root_certificate_key : key; - mp::utils::check(X509_sign(cert.get(), signing_key.get(), EVP_sha256()), "Failed to sign certificate"); + openssl_check(X509_sign(cert.get(), signing_key.get(), EVP_sha256()), "Failed to sign certificate"); } std::string as_pem() const { mp::BIOMem mem; - mp::utils::check(PEM_write_bio_X509(mem.get(), cert.get()), "Failed to write certificate in PEM format"); + openssl_check(PEM_write_bio_X509(mem.get(), cert.get()), "Failed to write certificate in PEM format"); return mem.as_string(); } @@ -256,8 +278,8 @@ class X509Cert { WritableFile file{cert_path}; - mp::utils::check(PEM_write_X509(file.get(), cert.get()), - fmt::format("Failed writing certificate to file '{}'", cert_path)); + openssl_check(PEM_write_X509(file.get(), cert.get()), + fmt::format("Failed writing certificate to file '{}'", cert_path)); } private: @@ -267,8 +289,8 @@ class X509Cert X509V3_EXT_conf_nid(nullptr, &ctx, nid, value), X509_EXTENSION_free); - mp::utils::check(ext.get(), "Failed to create X509 extension"); - mp::utils::check(X509_add_ext(cert.get(), ext.get(), -1), "Failed to add X509 extension"); + openssl_check(ext.get(), "Failed to create X509 extension"); + openssl_check(X509_add_ext(cert.get(), ext.get(), -1), "Failed to add X509 extension"); } std::unique_ptr cert{X509_new(), X509_free}; From 897c92428cbd3e7f31598585a3eade62a056db4f Mon Sep 17 00:00:00 2001 From: George Liao Date: Thu, 6 Mar 2025 10:30:14 +0100 Subject: [PATCH 106/114] Update src/daemon/daemon_config.cpp Co-authored-by: Ricardo Abreu <6698114+ricab@users.noreply.github.com> Signed-off-by: George Liao --- src/daemon/daemon_config.cpp | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index a355e2116a..170c998cf9 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -192,8 +192,7 @@ std::unique_ptr mp::DaemonConfigBuilder::build() std::make_unique(url_downloader.get(), cache_directory, manifest_ttl); } - // restrict permissions for all existing files and folders in cache directory, the data directory opens execute - // permission for other users, the sub-directories of it will have a granular permissions setting + // tighten permissions for cache and data if (!storage_path.isEmpty()) { MP_PLATFORM.set_permissions(storage_path.toStdU16String(), fs::perms::owner_all | fs::perms::others_exec); From ccac46aeac774159e7bf5c1538fe7df4c5d1f89d Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Mar 2025 14:51:43 +0100 Subject: [PATCH 107/114] [file permission] use restrictive default permission, and only open up the certificates folder and cert files. --- src/cert/ssl_cert_provider.cpp | 5 ++++- src/daemon/daemon_config.cpp | 7 ++++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index b8555a6255..9a673a62d6 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -118,7 +118,7 @@ class EVPKey openssl_check(PEM_write_PrivateKey(file.get(), key.get(), nullptr, nullptr, 0, nullptr, nullptr), fmt::format("Failed writing certificate private key to file '{}'", key_path)); - QFile::setPermissions(key_path, QFile::ReadOwner); + MP_PLATFORM.set_permissions(key_path_std, std::filesystem::perms::owner_read); } EVP_PKEY* get() const @@ -280,6 +280,9 @@ class X509Cert openssl_check(PEM_write_X509(file.get(), cert.get()), fmt::format("Failed writing certificate to file '{}'", cert_path)); + const std::filesystem::path cert_path_std = cert_path.toStdU16String(); + MP_PLATFORM.set_permissions(cert_path_std, + std::filesystem::perms::owner_all | std::filesystem::perms::others_read); } private: diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index 170c998cf9..cb34e9ec73 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -109,7 +109,7 @@ std::unique_ptr mp::DaemonConfigBuilder::build() auto multiplexing_logger = std::make_shared(std::move(logger)); mpl::set_logger(multiplexing_logger); - MP_PLATFORM.setup_permission_inheritance(false); + MP_PLATFORM.setup_permission_inheritance(true); auto storage_path = MP_PLATFORM.multipass_storage_location(); if (!storage_path.isEmpty()) @@ -170,8 +170,9 @@ std::unique_ptr mp::DaemonConfigBuilder::build() if (ssh_key_provider == nullptr) ssh_key_provider = std::make_unique(data_directory); if (cert_provider == nullptr) - cert_provider = - std::make_unique(data_directory + "/certificates", server_name_from(server_address)); + cert_provider = std::make_unique( + MP_UTILS.make_dir(data_directory, "certificates", fs::perms::owner_all | fs::perms::others_exec), + server_name_from(server_address)); if (client_cert_store == nullptr) client_cert_store = std::make_unique(data_directory); if (ssh_username.empty()) From d5d8ac6c71e4b54980550997c26f92018f374e48 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Mar 2025 15:04:01 +0100 Subject: [PATCH 108/114] Revert "[cert store] enforce ower_all permission to authenticated-certs sub-folder." This reverts commit 26415bc660812535986e64e84c4609e43226f2ea. Revert "[qemu platform] enforce ower_all permission to network sub-folder." This reverts commit 6a1879f538db07fbf25364c8b9452358b66752ea. Revert "[open ssh] enforce permission to ssh-keys sub-folder." This reverts commit b2afed461e40a9834fe5223456667b2c305ec2a3. Revert "[vault] enforce ower_all permission to vault sub-folder." This reverts commit 1ad954dc4c98e2f944298bde07c3763838cae586. Revert "[daemon] enforce ower_all permission to multipassd-vm-instances.json file." This reverts commit 72f55a977b8c4d9b2d8d7aa7f54446b089334fd8. --- src/cert/client_cert_store.cpp | 2 +- src/daemon/daemon.cpp | 5 +---- src/daemon/default_vm_image_vault.cpp | 1 - .../backends/qemu/linux/qemu_platform_detail_linux.cpp | 2 +- src/ssh/openssh_key_provider.cpp | 3 +-- 5 files changed, 4 insertions(+), 9 deletions(-) diff --git a/src/cert/client_cert_store.cpp b/src/cert/client_cert_store.cpp index 1753712d26..1b9591f32d 100644 --- a/src/cert/client_cert_store.cpp +++ b/src/cert/client_cert_store.cpp @@ -54,7 +54,7 @@ auto load_certs_from_file(const QDir& cert_dir) } // namespace mp::ClientCertStore::ClientCertStore(const multipass::Path& data_dir) - : cert_dir(MP_UTILS.make_dir(data_dir, mp::authenticated_certs_dir, fs::perms::owner_all)), + : cert_dir(MP_UTILS.make_dir(data_dir, mp::authenticated_certs_dir)), authenticated_client_certs{load_certs_from_file(cert_dir)} { mpl::log(mpl::Level::trace, category, fmt::format("Loading client certs from {}", cert_dir.absolutePath())); diff --git a/src/daemon/daemon.cpp b/src/daemon/daemon.cpp index 6e3fd69adf..9675858258 100644 --- a/src/daemon/daemon.cpp +++ b/src/daemon/daemon.cpp @@ -2867,10 +2867,7 @@ void mp::Daemon::persist_instances() } QDir data_dir{ mp::utils::backend_directory_path(config->data_directory, config->factory->get_backend_directory_name())}; - - const QString instance_db_path = data_dir.filePath(instance_db_name); - MP_JSONUTILS.write_json(instance_records_json, instance_db_path); - MP_PLATFORM.set_permissions(fs::path{instance_db_path.toStdU16String()}, fs::perms::owner_all); + MP_JSONUTILS.write_json(instance_records_json, data_dir.filePath(instance_db_name)); } void mp::Daemon::release_resources(const std::string& instance) diff --git a/src/daemon/default_vm_image_vault.cpp b/src/daemon/default_vm_image_vault.cpp index aad63abf68..fbec7a9e8f 100644 --- a/src/daemon/default_vm_image_vault.cpp +++ b/src/daemon/default_vm_image_vault.cpp @@ -249,7 +249,6 @@ mp::DefaultVMImageVault::DefaultVMImageVault(std::vector image_hos prepared_image_records{load_db(cache_dir.filePath(image_db_name))}, instance_image_records{load_db(data_dir.filePath(instance_db_name))} { - MP_UTILS.make_dir(data_dir, std::filesystem::perms::owner_all); } mp::DefaultVMImageVault::~DefaultVMImageVault() diff --git a/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp b/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp index b8e2a907a5..ee76cf6150 100644 --- a/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp +++ b/src/platform/backends/qemu/linux/qemu_platform_detail_linux.cpp @@ -119,7 +119,7 @@ void delete_virtual_switch(const QString& bridge_name) mp::QemuPlatformDetail::QemuPlatformDetail(const mp::Path& data_dir) : bridge_name{multipass_bridge_name}, - network_dir{MP_UTILS.make_dir(QDir(data_dir), "network", fs::perms::owner_all)}, + network_dir{MP_UTILS.make_dir(QDir(data_dir), "network")}, subnet{MP_BACKEND.get_subnet(network_dir, bridge_name)}, dnsmasq_server{init_nat_network(network_dir, bridge_name, subnet)}, firewall_config{MP_FIREWALL_CONFIG_FACTORY.make_firewall_config(bridge_name, subnet)} diff --git a/src/ssh/openssh_key_provider.cpp b/src/ssh/openssh_key_provider.cpp index 04e2b5cc62..332416645a 100644 --- a/src/ssh/openssh_key_provider.cpp +++ b/src/ssh/openssh_key_provider.cpp @@ -67,8 +67,7 @@ void mp::OpenSSHKeyProvider::KeyDeleter::operator()(ssh_key key) } mp::OpenSSHKeyProvider::OpenSSHKeyProvider(const mp::Path& cache_dir) - : ssh_key_dir{MP_UTILS.make_dir(cache_dir, "ssh-keys", std::filesystem::perms::owner_all)}, - priv_key{get_priv_key(ssh_key_dir)} + : ssh_key_dir{MP_UTILS.make_dir(cache_dir, "ssh-keys")}, priv_key{get_priv_key(ssh_key_dir)} { } From a8366def8769eadc9f52a17b096a282423e89a15 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 6 Mar 2025 17:47:03 +0100 Subject: [PATCH 109/114] [daemon config] used two steps approach for permission setting. Step1: recursively restrict everything before the creation of certificates directory, opens up the data directory only for other user read. Step 2: create certificate directory and certificates with the right permission. Or overwrite the permissions in the case of existing certificates directory and certificate files. --- src/daemon/daemon_config.cpp | 11 +++++++---- tests/test_daemon.cpp | 4 +++- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index cb34e9ec73..cccc53fa40 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -169,10 +169,6 @@ std::unique_ptr mp::DaemonConfigBuilder::build() server_address = platform::default_server_address(); if (ssh_key_provider == nullptr) ssh_key_provider = std::make_unique(data_directory); - if (cert_provider == nullptr) - cert_provider = std::make_unique( - MP_UTILS.make_dir(data_directory, "certificates", fs::perms::owner_all | fs::perms::others_exec), - server_name_from(server_address)); if (client_cert_store == nullptr) client_cert_store = std::make_unique(data_directory); if (ssh_username.empty()) @@ -196,14 +192,21 @@ std::unique_ptr mp::DaemonConfigBuilder::build() // tighten permissions for cache and data if (!storage_path.isEmpty()) { + MP_PERMISSIONS.restrict_permissions(storage_path.toStdU16String()); MP_PLATFORM.set_permissions(storage_path.toStdU16String(), fs::perms::owner_all | fs::perms::others_exec); } else { + MP_PERMISSIONS.restrict_permissions(data_directory.toStdU16String()); MP_PLATFORM.set_permissions(data_directory.toStdU16String(), fs::perms::owner_all | fs::perms::others_exec); MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String()); } + if (cert_provider == nullptr) + cert_provider = std::make_unique( + MP_UTILS.make_dir(data_directory, "certificates", fs::perms::owner_all | fs::perms::others_exec), + server_name_from(server_address)); + return std::unique_ptr(new DaemonConfig{ std::move(url_downloader), std::move(factory), std::move(image_hosts), std::move(vault), std::move(name_generator), std::move(ssh_key_provider), std::move(cert_provider), std::move(client_cert_store), diff --git a/tests/test_daemon.cpp b/tests/test_daemon.cpp index 92d5d6626a..ff5ee59f63 100644 --- a/tests/test_daemon.cpp +++ b/tests/test_daemon.cpp @@ -2519,6 +2519,7 @@ TEST_F(Daemon, sets_permissions_on_provided_storage_path) const std::filesystem::path std_path{path.toStdU16String()}; EXPECT_CALL(mock_platform, multipass_storage_location()).WillOnce(Return(path)); + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_path)); config_builder.build(); } @@ -2532,6 +2533,7 @@ TEST_F(Daemon, sets_permissions_on_storage_dirs) config_builder.cache_directory = "Pirate's secret cache"; const std::filesystem::path std_cache_path{config_builder.cache_directory.toStdU16String()}; + EXPECT_CALL(mock_permission_utils, restrict_permissions(std_data_path)); EXPECT_CALL(mock_permission_utils, restrict_permissions(std_cache_path)); config_builder.build(); @@ -2539,7 +2541,7 @@ TEST_F(Daemon, sets_permissions_on_storage_dirs) TEST_F(Daemon, sets_up_permission_inheritance) { - EXPECT_CALL(mock_platform, setup_permission_inheritance(false)); + EXPECT_CALL(mock_platform, setup_permission_inheritance(true)); config_builder.build(); } From aed842580dbb22fccff3ae3c007afa8e822dba3e Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Mar 2025 22:19:20 +0100 Subject: [PATCH 110/114] [snap] make snap to setup necessary directory for multipass --- snap/hooks/install | 2 ++ 1 file changed, 2 insertions(+) diff --git a/snap/hooks/install b/snap/hooks/install index 3e88f83067..86dd849c46 100755 --- a/snap/hooks/install +++ b/snap/hooks/install @@ -2,6 +2,8 @@ rm -f $SNAP_COMMON/snap_refresh +mkdir -p "$SNAP_COMMON/data" + # GDK pixbuf setup export LD_LIBRARY_PATH=/var/lib/snapd/lib/gl:/var/lib/snapd/lib/gl32:/var/lib/snapd/void:${SNAP}/graphics/lib:${SNAP}/lib:${SNAP}/usr/lib:${SNAP}/lib/${ARCH_TRIPLET}:${SNAP}/usr/lib/${ARCH_TRIPLET} $SNAP/usr/lib/${ARCH_TRIPLET}/gdk-pixbuf-2.0/gdk-pixbuf-query-loaders > "$GDK_PIXBUF_MODULE_FILE" From 7254f69db0c67e9672f63d47ca4b015558f61d7a Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Fri, 7 Mar 2025 22:35:01 +0100 Subject: [PATCH 111/114] [daemon config][ssl cert] added group user needed permission for accessing root certificate. --- src/cert/ssl_cert_provider.cpp | 3 ++- src/daemon/daemon_config.cpp | 10 +++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 9a673a62d6..e09821f2bb 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -282,7 +282,8 @@ class X509Cert fmt::format("Failed writing certificate to file '{}'", cert_path)); const std::filesystem::path cert_path_std = cert_path.toStdU16String(); MP_PLATFORM.set_permissions(cert_path_std, - std::filesystem::perms::owner_all | std::filesystem::perms::others_read); + std::filesystem::perms::owner_all | std::filesystem::perms::group_read | + std::filesystem::perms::others_read); } private: diff --git a/src/daemon/daemon_config.cpp b/src/daemon/daemon_config.cpp index cccc53fa40..79e06f0b7d 100644 --- a/src/daemon/daemon_config.cpp +++ b/src/daemon/daemon_config.cpp @@ -193,18 +193,22 @@ std::unique_ptr mp::DaemonConfigBuilder::build() if (!storage_path.isEmpty()) { MP_PERMISSIONS.restrict_permissions(storage_path.toStdU16String()); - MP_PLATFORM.set_permissions(storage_path.toStdU16String(), fs::perms::owner_all | fs::perms::others_exec); + MP_PLATFORM.set_permissions(storage_path.toStdU16String(), + fs::perms::owner_all | fs::perms::group_exec | fs::perms::others_exec); } else { MP_PERMISSIONS.restrict_permissions(data_directory.toStdU16String()); - MP_PLATFORM.set_permissions(data_directory.toStdU16String(), fs::perms::owner_all | fs::perms::others_exec); + MP_PLATFORM.set_permissions(data_directory.toStdU16String(), + fs::perms::owner_all | fs::perms::group_exec | fs::perms::others_exec); MP_PERMISSIONS.restrict_permissions(cache_directory.toStdU16String()); } if (cert_provider == nullptr) cert_provider = std::make_unique( - MP_UTILS.make_dir(data_directory, "certificates", fs::perms::owner_all | fs::perms::others_exec), + MP_UTILS.make_dir(data_directory, + "certificates", + fs::perms::owner_all | fs::perms::group_exec | fs::perms::others_exec), server_name_from(server_address)); return std::unique_ptr(new DaemonConfig{ From 268ac0df065c0124c039394b1b788ebc5d2099d0 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 27 Mar 2025 17:49:26 +0100 Subject: [PATCH 112/114] [ssl cert] fixed the server restart causes the root certificate lose the other users access --- src/cert/ssl_cert_provider.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index e09821f2bb..dfa1278dd8 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -312,6 +312,9 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const std::filesystem::path root_cert_path = MP_PLATFORM.get_root_cert_path(); if (std::filesystem::exists(root_cert_path) && QFile::exists(priv_key_path) && QFile::exists(cert_path)) { + MP_PLATFORM.set_permissions(root_cert_path, + std::filesystem::perms::owner_all | std::filesystem::perms::group_read | + std::filesystem::perms::others_read); return {mp::utils::contents_of(cert_path), mp::utils::contents_of(priv_key_path)}; } From fb4914879f82a435dad84b36c2194222e19c9813 Mon Sep 17 00:00:00 2001 From: Gerorge Liao Date: Thu, 27 Mar 2025 17:56:46 +0100 Subject: [PATCH 113/114] [ssl cert] added comment for the existing root cert permission overwrite. --- src/cert/ssl_cert_provider.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index dfa1278dd8..7cf4790929 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -312,6 +312,10 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const std::filesystem::path root_cert_path = MP_PLATFORM.get_root_cert_path(); if (std::filesystem::exists(root_cert_path) && QFile::exists(priv_key_path) && QFile::exists(cert_path)) { + // In cases where the root certificate exists within the data folder (such as in Snap or Windows + // environments), its file permissions is unintentionally overwritten due to recursive permission + // settings applied to the entire folder. To ensure the client process has access, we need to explicitly + // reset the file permissions on the root certificate. MP_PLATFORM.set_permissions(root_cert_path, std::filesystem::perms::owner_all | std::filesystem::perms::group_read | std::filesystem::perms::others_read); From 6c97765c86e0a09340b16ae28943c9b9104c4c3e Mon Sep 17 00:00:00 2001 From: George Liao Date: Thu, 27 Mar 2025 22:39:57 +0100 Subject: [PATCH 114/114] Update src/cert/ssl_cert_provider.cpp Co-authored-by: Ricardo Abreu <6698114+ricab@users.noreply.github.com> Signed-off-by: George Liao --- src/cert/ssl_cert_provider.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/cert/ssl_cert_provider.cpp b/src/cert/ssl_cert_provider.cpp index 7cf4790929..210da72b0b 100644 --- a/src/cert/ssl_cert_provider.cpp +++ b/src/cert/ssl_cert_provider.cpp @@ -312,10 +312,7 @@ mp::SSLCertProvider::KeyCertificatePair make_cert_key_pair(const QDir& cert_dir, const std::filesystem::path root_cert_path = MP_PLATFORM.get_root_cert_path(); if (std::filesystem::exists(root_cert_path) && QFile::exists(priv_key_path) && QFile::exists(cert_path)) { - // In cases where the root certificate exists within the data folder (such as in Snap or Windows - // environments), its file permissions is unintentionally overwritten due to recursive permission - // settings applied to the entire folder. To ensure the client process has access, we need to explicitly - // reset the file permissions on the root certificate. + // Unlike other daemon files, the root certificate needs to be accessible by everyone MP_PLATFORM.set_permissions(root_cert_path, std::filesystem::perms::owner_all | std::filesystem::perms::group_read | std::filesystem::perms::others_read);