restructure a bit to avoid code duplications and make find_cert more efficient, fix CI

4 files changed, 111 insertions, 93 deletions

diff --git a/src/build-data/os/windows.txt b/src/build-data/os/windows.txt
index 8eab164df..d11bacc37 100644
--- a/src/build-data/os/windows.txt
+++ b/src/build-data/os/windows.txt
@@ -32,6 +32,8 @@ virtual_lock
 
 threads
 filesystem
+
+certificate_store
 </target_features>
 
 <aliases>
diff --git a/src/lib/x509/certstor_system_windows/certstor_windows.cpp b/src/lib/x509/certstor_system_windows/certstor_windows.cpp
index 2605b10b4..8f3b66dd3 100644
--- a/src/lib/x509/certstor_system_windows/certstor_windows.cpp
+++ b/src/lib/x509/certstor_system_windows/certstor_windows.cpp
@@ -14,37 +14,16 @@
 
 #define NOMINMAX 1
 #define _WINSOCKAPI_ // stop windows.h including winsock.h
-#include <Windows.h>
-#include <Wincrypt.h>
+#include <windows.h>
+#include <wincrypt.h>
 
 namespace Botan {
 namespace {
 
+using Cert_Pointer = std::shared_ptr<const Botan::X509_Certificate>;
+using Cert_Vector = std::vector<Cert_Pointer>;
 const std::array<const char*, 2> cert_store_names{"Root", "CA"};
 
-HCERTSTORE openCertStore(const char* cert_store_name)
-   {
-   auto store = CertOpenSystemStore(NULL, cert_store_name);
-   if(!store)
-      {
-      throw Botan::Internal_Error(
-         "failed to open windows certificate store '" + std::string(cert_store_name) +
-         "' (Error Code: " +
-         std::to_string(::GetLastError()) + ")");
-      }
-   return store;
-   }
-
-bool already_contains_certificate(
-   const std::vector<std::shared_ptr<const Botan::X509_Certificate>>& certs, std::shared_ptr<Botan::X509_Certificate> cert)
-   {
-   return std::any_of(certs.begin(), certs.end(),
-                      [&](std::shared_ptr<const Botan::X509_Certificate> c)
-      {
-      return *c == *cert;
-      });
-   }
-
 /**
  * Abstract RAII wrapper for PCCERT_CONTEXT and HCERTSTORE
  * The Windows API partly takes care of those pointers destructions itself.
@@ -100,14 +79,8 @@ class Handle_Guard
          }
 
    private:
-      template<class T>
-      void close()
-         {
-         static_assert(false, "Handle_Guard is not available for this type");
-         }
-
-      template<>
-      void close<PCCERT_CONTEXT> ()
+      template<class T2 = T>
+      typename std::enable_if<std::is_same<T2, PCCERT_CONTEXT>::value>::type close()
          {
          if(m_context)
             {
@@ -115,8 +88,8 @@ class Handle_Guard
             }
          }
 
-      template<>
-      void close<HCERTSTORE> ()
+      template<class T2 = T>
+      typename std::enable_if<std::is_same<T2, HCERTSTORE>::value>::type close()
          {
          if(m_context)
             {
@@ -126,47 +99,66 @@ class Handle_Guard
 
       T m_context;
    };
-}
 
-Certificate_Store_Windows::Certificate_Store_Windows() {}
+HCERTSTORE open_cert_store(const char* cert_store_name)
+   {
+   auto store = CertOpenSystemStore(NULL, cert_store_name);
+   if(!store)
+      {
+      throw Botan::Internal_Error(
+         "failed to open windows certificate store '" + std::string(cert_store_name) +
+         "' (Error Code: " +
+         std::to_string(::GetLastError()) + ")");
+      }
+   return store;
+   }
 
-std::vector<X509_DN> Certificate_Store_Windows::all_subjects() const
+Cert_Vector search_cert_stores(const _CRYPTOAPI_BLOB& blob, const DWORD& find_type,
+                               std::function<bool(const Cert_Vector& certs, Cert_Pointer cert)> filter,
+                               bool return_on_first_found)
    {
-   std::vector<X509_DN> subject_dns;
+   Cert_Vector certs;
    for(auto& store_name : cert_store_names)
       {
-      Handle_Guard<HCERTSTORE> windows_cert_store = openCertStore(store_name);
+      Handle_Guard<HCERTSTORE> windows_cert_store = open_cert_store(store_name);
       Handle_Guard<PCCERT_CONTEXT> cert_context = nullptr;
-
-      // Handle_Guard::assign exchanges the underlying pointer. No RAII is needed here, because the Windows API takes care of
-      // freeing the previous context.
-      while(cert_context.assign(CertEnumCertificatesInStore(windows_cert_store.get(), cert_context.get())))
+      while(cert_context.assign(CertFindCertificateInStore(
+                                   windows_cert_store.get(), PKCS_7_ASN_ENCODING | X509_ASN_ENCODING,
+                                   NULL, find_type,
+                                   &blob, cert_context.get())))
          {
-         X509_Certificate cert(cert_context->pbCertEncoded, cert_context->cbCertEncoded);
-         subject_dns.push_back(cert.subject_dn());
+         auto cert = std::make_shared<X509_Certificate>(cert_context->pbCertEncoded, cert_context->cbCertEncoded);
+         if(filter(certs, cert))
+            {
+            if(return_on_first_found)
+               {
+               return {cert};
+               }
+            certs.push_back(cert);
+            }
          }
       }
 
-   return subject_dns;
+   return certs;
    }
 
-std::shared_ptr<const X509_Certificate>
-Certificate_Store_Windows::find_cert(const Botan::X509_DN&           subject_dn,
-                                     const std::vector<uint8_t>& key_id) const
+bool already_contains_certificate(const Cert_Vector& certs, Cert_Pointer cert)
    {
-   const auto certs = find_all_certs(subject_dn, key_id);
-   return certs.empty() ? nullptr : certs.front();
+   return std::any_of(certs.begin(), certs.end(), [&](std::shared_ptr<const Botan::X509_Certificate> c)
+      {
+      return *c == *cert;
+      });
    }
 
-std::vector<std::shared_ptr<const X509_Certificate>> Certificate_Store_Windows::find_all_certs(
-         const X509_DN& subject_dn,
-         const std::vector<uint8_t>& key_id) const
+Cert_Vector find_cert_by_dn_and_key_id(const Botan::X509_DN& subject_dn,
+                                       const std::vector<uint8_t>& key_id,
+                                       bool return_on_first_found)
    {
    _CRYPTOAPI_BLOB blob;
    DWORD find_type;
-   std::vector<std::shared_ptr<const X509_Certificate>> certs;
    std::vector<uint8_t> dn_data;
 
+   // if key_id is available, prefer searching that, as it should be "more unique" than the subject DN
    if(key_id.empty())
       {
       find_type = CERT_FIND_SUBJECT_NAME;
@@ -182,28 +174,52 @@ std::vector<std::shared_ptr<const X509_Certificate>> Certificate_Store_Windows::
       blob.pbData = const_cast<BYTE*>(key_id.data());
       }
 
+   auto filter = [&](const Cert_Vector& certs, Cert_Pointer cert)
+      {
+      return !already_contains_certificate(certs, cert) && (key_id.empty() || cert->subject_dn() == subject_dn);
+      };
+
+   return search_cert_stores(blob, find_type, filter, return_on_first_found);
+   }
+} // namespace
+
+Certificate_Store_Windows::Certificate_Store_Windows() {}
+
+std::vector<X509_DN> Certificate_Store_Windows::all_subjects() const
+   {
+   std::vector<X509_DN> subject_dns;
    for(auto& store_name : cert_store_names)
       {
-      Handle_Guard<HCERTSTORE> windows_cert_store = openCertStore(store_name);
+      Handle_Guard<HCERTSTORE> windows_cert_store = open_cert_store(store_name);
       Handle_Guard<PCCERT_CONTEXT> cert_context = nullptr;
-      while(cert_context.assign(CertFindCertificateInStore(
-                                   windows_cert_store.get(), PKCS_7_ASN_ENCODING | X509_ASN_ENCODING,
-                                   NULL, find_type,
-                                   &blob, cert_context.get())))
+
+      // Handle_Guard::assign exchanges the underlying pointer. No RAII is needed here, because the Windows API takes care of
+      // freeing the previous context.
+      while(cert_context.assign(CertEnumCertificatesInStore(windows_cert_store.get(), cert_context.get())))
          {
-         auto cert = std::make_shared<X509_Certificate>(cert_context->pbCertEncoded, cert_context->cbCertEncoded);
-         if(!already_contains_certificate(certs, cert) && (key_id.empty() || cert->subject_dn() == subject_dn))
-            {
-            certs.push_back(cert);
-            }
+         X509_Certificate cert(cert_context->pbCertEncoded, cert_context->cbCertEncoded);
+         subject_dns.push_back(cert.subject_dn());
          }
       }
-   return certs;
+
+   return subject_dns;
+   }
+
+Cert_Pointer Certificate_Store_Windows::find_cert(const Botan::X509_DN& subject_dn,
+      const std::vector<uint8_t>& key_id) const
+   {
+   const auto certs = find_cert_by_dn_and_key_id(subject_dn, key_id, true);
+   return certs.empty() ? nullptr : certs.front();
+   }
+
+Cert_Vector Certificate_Store_Windows::find_all_certs(
+   const X509_DN& subject_dn,
+   const std::vector<uint8_t>& key_id) const
+   {
+   return find_cert_by_dn_and_key_id(subject_dn, key_id, false);
    }
 
-std::shared_ptr<const Botan::X509_Certificate>
-Certificate_Store_Windows::find_cert_by_pubkey_sha1(
-   const std::vector<uint8_t>& key_hash) const
+Cert_Pointer Certificate_Store_Windows::find_cert_by_pubkey_sha1(const std::vector<uint8_t>& key_hash) const
    {
    if(key_hash.size() != 20)
       {
@@ -214,25 +230,14 @@ Certificate_Store_Windows::find_cert_by_pubkey_sha1(
    blob.cbData = static_cast<DWORD>(key_hash.size());
    blob.pbData = const_cast<BYTE*>(key_hash.data());
 
-   for(auto& store_name : cert_store_names)
-      {
-      Handle_Guard<HCERTSTORE> windows_cert_store = openCertStore(store_name);
-      Handle_Guard<PCCERT_CONTEXT> cert_context = CertFindCertificateInStore(
-               windows_cert_store.get(), PKCS_7_ASN_ENCODING | X509_ASN_ENCODING,
-               NULL, CERT_FIND_KEY_IDENTIFIER,
-               &blob, nullptr);
-
-      if(cert_context)
-         {
-         return std::make_shared<X509_Certificate>(cert_context->pbCertEncoded, cert_context->cbCertEncoded);
-         }
-      }
+   auto filter = [&](const Cert_Vector&, Cert_Pointer) { return true; };
 
-   return nullptr;
+   const auto certs = search_cert_stores(blob, CERT_FIND_KEY_IDENTIFIER, filter, true);
+   return certs.empty() ? nullptr : certs.front();
    }
 
-std::shared_ptr<const X509_Certificate>
-Certificate_Store_Windows::find_cert_by_raw_subject_dn_sha256(const std::vector<uint8_t>& subject_hash) const
+Cert_Pointer Certificate_Store_Windows::find_cert_by_raw_subject_dn_sha256(
+   const std::vector<uint8_t>& subject_hash) const
    {
    BOTAN_UNUSED(subject_hash);
    throw Not_Implemented("Certificate_Store_Windows::find_cert_by_raw_subject_dn_sha256");
diff --git a/src/lib/x509/certstor_system_windows/info.txt b/src/lib/x509/certstor_system_windows/info.txt
index 66e9fa9a9..8d6dce6c6 100644
--- a/src/lib/x509/certstor_system_windows/info.txt
+++ b/src/lib/x509/certstor_system_windows/info.txt
@@ -3,7 +3,7 @@ CERTSTOR_WINDOWS -> 20190430
 </defines>
 
 <os_features>
-win32
+win32,certificate_store
 </os_features>
 
 <header:public>
@@ -11,5 +11,5 @@ certstor_windows.h
 </header:public>
 
 <libs>
-windows -> crypto
+windows -> crypt32.lib
 </libs>
\ No newline at end of file
diff --git a/src/tests/test_certstor_system.cpp b/src/tests/test_certstor_system.cpp
index 6075acaeb..48bcc7246 100644
--- a/src/tests/test_certstor_system.cpp
+++ b/src/tests/test_certstor_system.cpp
@@ -92,7 +92,7 @@ Test::Result find_cert_by_utf8_subject_dn(Botan::Certificate_Store& certstore)
          {
          auto cns = cert->subject_dn().get_attribute("CN");
          result.test_is_eq("exactly one CN", cns.size(), size_t(1));
-         result.test_eq("CN", cns.front(), "D-TRUST Root CA 3 2013");
+         result.test_eq("CN", cns.front(), "D-TRUST Root Class 3 CA 2 EV 2009");
          }
       }
    catch(std::exception& e)
@@ -170,11 +170,20 @@ Test::Result find_all_certs_by_subject_dn(Botan::Certificate_Store& certstore)
       auto certs = certstore.find_all_certs(dn, std::vector<uint8_t>());
       result.end_timer();
 
-      if(result.confirm("result not empty", !certs.empty()) &&
-            result.test_eq("exactly one certificate", certs.size(), 1))
+      // check for duplications
+      sort(certs.begin(), certs.end());
+      for(int i = 1; i < certs.size(); ++i)
+         {
+         if(certs[i=1] == certs[i])
+            {
+            result.test_failure("find_all_certs produced duplicated result");
+            }
+         }
+
+      if(result.confirm("result not empty", !certs.empty()))
          {
          auto cns = certs.front()->subject_dn().get_attribute("CN");
-         result.test_is_eq("exactly one CN", cns.size(), size_t(1));
+         result.test_gte("at least one CN", cns.size(), size_t(1));
          result.test_eq("CN", cns.front(), "DST Root CA X3");
          }
       }
@@ -313,12 +322,14 @@ class Certstor_System_Tests final : public Test
 
          results.push_back(find_certificate_by_pubkey_sha1(*system));
          results.push_back(find_cert_by_subject_dn(*system));
-         results.push_back(find_cert_by_utf8_subject_dn(*system));
          results.push_back(find_cert_by_subject_dn_and_key_id(*system));
          results.push_back(find_all_certs_by_subject_dn(*system));
          results.push_back(find_certs_by_subject_dn_and_key_id(*system));
          results.push_back(find_all_subjects(*system));
          results.push_back(no_certificate_matches(*system));
+#if !defined(BOTAN_HAS_CERTSTOR_WINDOWS)
+         results.push_back(find_cert_by_utf8_subject_dn(*system));
+#endif
 #if defined(BOTAN_HAS_CERTSTOR_MACOS)
          results.push_back(certificate_matching_with_dn_normalization(*system));
 #endif