Allow applications to easily override extensions in cert requests

Refactor the code so it's possible to create a cert request without
going through x509self.h (PKCS10_Request::create).

Add Extensions::add_new, so we can add an extension to a PKCS10
request without stomping on one already included by the application.

Refactor the X509 unit tests to avoid (some) duplicated key creations.
Just create a key once at the start and use it for all of the tests.

GH #1428

9 files changed, 360 insertions, 281 deletions

diff --git a/src/lib/x509/pkcs10.cpp b/src/lib/x509/pkcs10.cpp
index 78fea8dc6..2da002cd1 100644
--- a/src/lib/x509/pkcs10.cpp
+++ b/src/lib/x509/pkcs10.cpp
@@ -9,6 +9,8 @@
 #include <botan/x509_ext.h>
 #include <botan/x509cert.h>
 #include <botan/ber_dec.h>
+#include <botan/der_enc.h>
+#include <botan/pubkey.h>
 #include <botan/oids.h>
 #include <botan/pem.h>
 
@@ -52,6 +54,60 @@ PKCS10_Request::PKCS10_Request(const std::string& fsname)
    }
 #endif
 
+//static
+PKCS10_Request PKCS10_Request::create(const Private_Key& key,
+                                      const X509_DN& subject_dn,
+                                      const Extensions& extensions,
+                                      const std::string& hash_fn,
+                                      RandomNumberGenerator& rng,
+                                      const std::string& padding_scheme,
+                                      const std::string& challenge)
+   {
+   const std::map<std::string,std::string> sig_opts = { {"padding", padding_scheme} };
+
+   AlgorithmIdentifier sig_algo;
+   std::unique_ptr<PK_Signer> signer = choose_sig_format(sig_algo, key, rng, hash_fn, padding_scheme);
+
+   const size_t PKCS10_VERSION = 0;
+
+   DER_Encoder tbs_req;
+
+   tbs_req.start_cons(SEQUENCE)
+      .encode(PKCS10_VERSION)
+      .encode(subject_dn)
+      .raw_bytes(key.subject_public_key())
+      .start_explicit(0);
+
+   if(challenge.empty() == false)
+      {
+      ASN1_String challenge_str(challenge, DIRECTORY_STRING);
+
+      tbs_req.encode(
+         Attribute("PKCS9.ChallengePassword",
+                   DER_Encoder().encode(challenge_str).get_contents_unlocked()
+            )
+         );
+      }
+
+   tbs_req.encode(
+      Attribute("PKCS9.ExtensionRequest",
+                DER_Encoder()
+                   .start_cons(SEQUENCE)
+                      .encode(extensions)
+                   .end_cons()
+               .get_contents_unlocked()
+         )
+      )
+      .end_explicit()
+      .end_cons();
+
+   const std::vector<uint8_t> req =
+      X509_Object::make_signed(signer.get(), rng, sig_algo,
+                               tbs_req.get_contents());
+
+   return PKCS10_Request(req);
+   }
+
 /*
 * Decode the CertificateRequestInfo
 */
diff --git a/src/lib/x509/pkcs10.h b/src/lib/x509/pkcs10.h
index b3a75fd5e..bd8cdb2e1 100644
--- a/src/lib/x509/pkcs10.h
+++ b/src/lib/x509/pkcs10.h
@@ -18,6 +18,7 @@
 
 namespace Botan {
 
+class Private_Key;
 class Extensions;
 struct PKCS10_Data;
 
@@ -109,6 +110,27 @@ class BOTAN_PUBLIC_API(2,0) PKCS10_Request final : public X509_Object
       * @param vec a std::vector containing the DER value
       */
       explicit PKCS10_Request(const std::vector<uint8_t>& vec);
+
+      /**
+      * Create a new PKCS10 certificate request
+      * @param key the key that will be included in the certificate request
+      * @param subject_dn the DN to be placed in the request
+      * @param extensions extensions to include in the request
+      * @param hash_fn the hash function to use to create the signature
+      * @param rng a random number generator
+      * @param padding_scheme if set specifies the padding scheme, otherwise an
+      *        algorithm-specific default is used.
+      * @param challenge a challenge string to be included in the PKCS10 request,
+      *        sometimes used for revocation purposes.
+      */
+      static PKCS10_Request create(const Private_Key& key,
+                                   const X509_DN& subject_dn,
+                                   const Extensions& extensions,
+                                   const std::string& hash_fn,
+                                   RandomNumberGenerator& rng,
+                                   const std::string& padding_scheme = "",
+                                   const std::string& challenge = "");
+
    private:
       std::string PEM_label() const override;
 
diff --git a/src/lib/x509/x509_ca.cpp b/src/lib/x509/x509_ca.cpp
index 1560140c7..294a564d5 100644
--- a/src/lib/x509/x509_ca.cpp
+++ b/src/lib/x509/x509_ca.cpp
@@ -277,76 +277,19 @@ PK_Signer* choose_sig_format(const Private_Key& key,
                              const std::string& hash_fn,
                              AlgorithmIdentifier& sig_algo)
    {
-   return choose_sig_format(key, std::map<std::string,std::string>(),
-                            rng, hash_fn, sig_algo);
+   return X509_Object::choose_sig_format(sig_algo, key, rng, hash_fn, "").release();
    }
 
-/*
-* Choose a signing format for the key
-*/
 PK_Signer* choose_sig_format(const Private_Key& key,
                              const std::map<std::string,std::string>& opts,
                              RandomNumberGenerator& rng,
                              const std::string& hash_fn,
                              AlgorithmIdentifier& sig_algo)
    {
-   const std::string algo_name = key.algo_name();
-
-   std::unique_ptr<HashFunction> hash(HashFunction::create_or_throw(hash_fn));
-   std::string hash_name = hash->name();
-
-   // check algo_name and set default
    std::string padding;
-   if(algo_name == "RSA")
-      {
-      // set to EMSA3 for compatibility reasons, originally it was the only option
-      padding = "EMSA3(" + hash_name + ")";
-      }
-   else if(algo_name == "DSA" ||
-           algo_name == "ECDSA" ||
-           algo_name == "ECGDSA" ||
-           algo_name == "ECKCDSA" ||
-           algo_name == "GOST-34.10")
-      {
-      padding = "EMSA1(" + hash_name + ")";
-      }
-   else
-      {
-      throw Invalid_Argument("Unknown X.509 signing key type: " + algo_name);
-      }
-
-   if(opts.count("padding") > 0 && !opts.at("padding").empty())
-      {
+   if(opts.count("padding"))
       padding = opts.at("padding");
-      }
-
-   // try to construct an EMSA object from the padding options or default
-   std::unique_ptr<EMSA> emsa = nullptr;
-   try
-      {
-      emsa.reset(get_emsa(padding));
-      }
-   /*
-    * get_emsa will throw if opts contains {"padding",<valid_padding>} but
-    * <valid_padding> does not specify a hash function.
-    * Omitting it is valid since it needs to be identical to hash_fn.
-    * If it still throws, something happened that we cannot repair here,
-    * e.g. the algorithm/padding combination is not supported.
-    */
-   catch(...)
-      {
-      emsa.reset(get_emsa(padding + "(" + hash_fn + ")"));
-      }
-   if(emsa == nullptr)
-      {
-      throw Invalid_Argument("Could not parse padding scheme " + padding);
-      }
-
-   const Signature_Format format = (key.message_parts() > 1) ? DER_SEQUENCE : IEEE_1363;
-
-   sig_algo = emsa->config_for_x509(key, hash_name);
-
-   return new PK_Signer(key, rng, emsa->name(), format);
+   return X509_Object::choose_sig_format(sig_algo, key, rng, hash_fn, padding).release();
    }
 
 }
diff --git a/src/lib/x509/x509_ext.cpp b/src/lib/x509/x509_ext.cpp
index 44c469c48..b969ad7cf 100644
--- a/src/lib/x509/x509_ext.cpp
+++ b/src/lib/x509/x509_ext.cpp
@@ -131,6 +131,21 @@ void Extensions::add(Certificate_Extension* extn, bool critical)
    m_extension_info.emplace(oid, info);
    }
 
+bool Extensions::add_new(Certificate_Extension* extn, bool critical)
+   {
+   if(m_extension_info.count(extn->oid_of()) > 0)
+      {
+      delete extn;
+      return false; // already exists
+      }
+
+   const OID oid = extn->oid_of();
+   Extensions_Info info(critical, extn);
+   m_extension_oids.push_back(oid);
+   m_extension_info.emplace(oid, info);
+   return true;
+   }
+
 void Extensions::replace(Certificate_Extension* extn, bool critical)
    {
    // Remove it if it existed
diff --git a/src/lib/x509/x509_ext.h b/src/lib/x509/x509_ext.h
index 8e702daf1..7c8a8569c 100644
--- a/src/lib/x509/x509_ext.h
+++ b/src/lib/x509/x509_ext.h
@@ -155,6 +155,16 @@ class BOTAN_PUBLIC_API(2,0) Extensions final : public ASN1_Object
       void add(Certificate_Extension* extn, bool critical = false);
 
       /**
+      * Adds a new extension to the list unless it already exists. If the extension
+      * already exists within the Extensions object, the extn pointer will be deleted.
+      *
+      * @param extn pointer to the certificate extension (Extensions takes ownership)
+      * @param critical whether this extension should be marked as critical
+      * @return true if the object was added false if the extension was already used
+      */
+      bool add_new(Certificate_Extension* extn, bool critical = false);
+
+      /**
       * Adds an extension to the list or replaces it.
       * @param extn the certificate extension
       * @param critical whether this extension should be marked as critical
diff --git a/src/lib/x509/x509_obj.cpp b/src/lib/x509/x509_obj.cpp
index 01a92a001..78413c121 100644
--- a/src/lib/x509/x509_obj.cpp
+++ b/src/lib/x509/x509_obj.cpp
@@ -12,6 +12,7 @@
 #include <botan/ber_dec.h>
 #include <botan/parsing.h>
 #include <botan/pem.h>
+#include <botan/emsa.h>
 #include <algorithm>
 
 namespace Botan {
@@ -284,4 +285,70 @@ std::vector<uint8_t> X509_Object::make_signed(PK_Signer* signer,
    .get_contents_unlocked();
    }
 
+/*
+* Choose a signing format for the key
+*/
+std::unique_ptr<PK_Signer> X509_Object::choose_sig_format(AlgorithmIdentifier& sig_algo,
+                                                          const Private_Key& key,
+                                                          RandomNumberGenerator& rng,
+                                                          const std::string& hash_fn,
+                                                          const std::string& padding_algo)
+   {
+   const std::string algo_name = key.algo_name();
+
+   std::string padding;
+
+   // check algo_name and set default
+   if(algo_name == "RSA")
+      {
+      // set to EMSA3 for compatibility reasons, originally it was the only option
+      padding = "EMSA3(" + hash_fn + ")";
+      }
+   else if(algo_name == "DSA" ||
+           algo_name == "ECDSA" ||
+           algo_name == "ECGDSA" ||
+           algo_name == "ECKCDSA" ||
+           algo_name == "GOST-34.10")
+      {
+      padding = "EMSA1(" + hash_fn + ")";
+      }
+   else
+      {
+      throw Invalid_Argument("Unknown X.509 signing key type: " + algo_name);
+      }
+
+   if(padding_algo.empty() == false)
+      {
+      padding = padding_algo;
+      }
+
+   // try to construct an EMSA object from the padding options or default
+   std::unique_ptr<EMSA> emsa = nullptr;
+   try
+      {
+      emsa.reset(get_emsa(padding));
+      }
+   /*
+    * get_emsa will throw if opts contains {"padding",<valid_padding>} but
+    * <valid_padding> does not specify a hash function.
+    * Omitting it is valid since it needs to be identical to hash_fn.
+    * If it still throws, something happened that we cannot repair here,
+    * e.g. the algorithm/padding combination is not supported.
+    */
+   catch(...)
+      {
+      emsa.reset(get_emsa(padding + "(" + hash_fn + ")"));
+      }
+   if(emsa == nullptr)
+      {
+      throw Invalid_Argument("Could not parse padding scheme " + padding);
+      }
+
+   const Signature_Format format = (key.message_parts() > 1) ? DER_SEQUENCE : IEEE_1363;
+
+   sig_algo = emsa->config_for_x509(key, hash_fn);
+
+   return std::unique_ptr<PK_Signer>(new PK_Signer(key, rng, emsa->name(), format));
+   }
+
 }
diff --git a/src/lib/x509/x509_obj.h b/src/lib/x509/x509_obj.h
index 72ba0f534..1e4abe00b 100644
--- a/src/lib/x509/x509_obj.h
+++ b/src/lib/x509/x509_obj.h
@@ -16,6 +16,7 @@
 namespace Botan {
 
 class Public_Key;
+class Private_Key;
 class RandomNumberGenerator;
 
 /**
@@ -119,6 +120,14 @@ class BOTAN_PUBLIC_API(2,0) X509_Object : public ASN1_Object
          { return std::vector<std::string>(); }
 
       virtual ~X509_Object() = default;
+
+      static std::unique_ptr<PK_Signer>
+         choose_sig_format(AlgorithmIdentifier& sig_algo,
+                           const Private_Key& key,
+                           RandomNumberGenerator& rng,
+                           const std::string& hash_fn,
+                           const std::string& padding_algo);
+
    protected:
 
       X509_Object() = default;
diff --git a/src/lib/x509/x509self.cpp b/src/lib/x509/x509self.cpp
index 108e0496b..78cdfe741 100644
--- a/src/lib/x509/x509self.cpp
+++ b/src/lib/x509/x509self.cpp
@@ -56,6 +56,8 @@ X509_Certificate create_self_signed_cert(const X509_Cert_Options& opts,
    std::unique_ptr<PK_Signer> signer(choose_sig_format(key, sig_opts, rng, hash_fn, sig_algo));
    load_info(opts, subject_dn, subject_alt);
 
+   Extensions extensions = opts.extensions;
+
    Key_Constraints constraints;
    if(opts.is_CA)
       {
@@ -67,23 +69,21 @@ X509_Certificate create_self_signed_cert(const X509_Cert_Options& opts,
       constraints = opts.constraints;
       }
 
-   Extensions extensions = opts.extensions;
-
-   extensions.add(
+   extensions.add_new(
       new Cert_Extension::Basic_Constraints(opts.is_CA, opts.path_limit),
       true);
 
    if(constraints != NO_CONSTRAINTS)
       {
-      extensions.add(new Cert_Extension::Key_Usage(constraints), true);
+      extensions.add_new(new Cert_Extension::Key_Usage(constraints), true);
       }
 
-   extensions.add(new Cert_Extension::Subject_Key_ID(pub_key, hash_fn));
+   extensions.add_new(new Cert_Extension::Subject_Key_ID(pub_key, hash_fn));
 
-   extensions.add(
+   extensions.add_new(
       new Cert_Extension::Subject_Alternative_Name(subject_alt));
 
-   extensions.add(
+   extensions.add_new(
       new Cert_Extension::Extended_Key_Usage(opts.ex_constraints));
 
    return X509_CA::make_cert(signer.get(), rng, sig_algo, pub_key,
@@ -100,19 +100,10 @@ PKCS10_Request create_cert_req(const X509_Cert_Options& opts,
                                const std::string& hash_fn,
                                RandomNumberGenerator& rng)
    {
-   AlgorithmIdentifier sig_algo;
    X509_DN subject_dn;
    AlternativeName subject_alt;
-
-   // for now, only the padding option is used
-   std::map<std::string,std::string> sig_opts = { {"padding",opts.padding_scheme} };
-
-   std::vector<uint8_t> pub_key = X509::BER_encode(key);
-   std::unique_ptr<PK_Signer> signer(choose_sig_format(key, sig_opts, rng, hash_fn, sig_algo));
    load_info(opts, subject_dn, subject_alt);
 
-   const size_t PKCS10_VERSION = 0;
-
    Key_Constraints constraints;
    if(opts.is_CA)
       {
@@ -126,55 +117,22 @@ PKCS10_Request create_cert_req(const X509_Cert_Options& opts,
 
    Extensions extensions = opts.extensions;
 
-   extensions.add(
-      new Cert_Extension::Basic_Constraints(opts.is_CA, opts.path_limit));
+   extensions.add_new(new Cert_Extension::Basic_Constraints(opts.is_CA, opts.path_limit));
 
    if(constraints != NO_CONSTRAINTS)
       {
-      extensions.add(
-         new Cert_Extension::Key_Usage(constraints));
-      }
-   extensions.add(
-      new Cert_Extension::Extended_Key_Usage(opts.ex_constraints));
-   extensions.add(
-      new Cert_Extension::Subject_Alternative_Name(subject_alt));
-
-   DER_Encoder tbs_req;
-
-   tbs_req.start_cons(SEQUENCE)
-      .encode(PKCS10_VERSION)
-      .encode(subject_dn)
-      .raw_bytes(pub_key)
-      .start_explicit(0);
-
-   if(!opts.challenge.empty())
-      {
-      ASN1_String challenge(opts.challenge, DIRECTORY_STRING);
-
-      tbs_req.encode(
-         Attribute("PKCS9.ChallengePassword",
-                   DER_Encoder().encode(challenge).get_contents_unlocked()
-            )
-         );
+      extensions.add_new(new Cert_Extension::Key_Usage(constraints));
       }
-
-   tbs_req.encode(
-      Attribute("PKCS9.ExtensionRequest",
-                DER_Encoder()
-                   .start_cons(SEQUENCE)
-                      .encode(extensions)
-                   .end_cons()
-               .get_contents_unlocked()
-         )
-      )
-      .end_explicit()
-      .end_cons();
-
-   const std::vector<uint8_t> req =
-      X509_Object::make_signed(signer.get(), rng, sig_algo,
-                               tbs_req.get_contents());
-
-   return PKCS10_Request(req);
+   extensions.add_new(new Cert_Extension::Extended_Key_Usage(opts.ex_constraints));
+   extensions.add_new(new Cert_Extension::Subject_Alternative_Name(subject_alt));
+
+   return PKCS10_Request::create(key,
+                                 subject_dn,
+                                 extensions,
+                                 hash_fn,
+                                 rng,
+                                 opts.padding_scheme,
+                                 opts.challenge);
    }
 
 }
diff --git a/src/tests/unit_x509.cpp b/src/tests/unit_x509.cpp
index 5e787d72d..d0f284bf3 100644
--- a/src/tests/unit_x509.cpp
+++ b/src/tests/unit_x509.cpp
@@ -5,6 +5,7 @@
 * Botan is released under the Simplified BSD License (see license.txt)
 */
 
+#include <iostream>
 #include "tests.h"
 
 #if defined(BOTAN_HAS_X509_CERTIFICATES)
@@ -617,22 +618,48 @@ Test::Result test_padding_config() {
 
 #endif
 
-Test::Result test_x509_cert(const std::string& sig_algo, const std::string& sig_padding = "", const std::string& hash_fn = "SHA-256")
+Test::Result test_pkcs10_ext(const Botan::Private_Key& key,
+                             const std::string& sig_padding,
+                             const std::string& hash_fn = "SHA-256")
    {
-   Test::Result result("X509 Unit");
+   Test::Result result("PKCS10 extensions");
 
-   /* Create the CA's key and self-signed cert */
-   std::unique_ptr<Botan::Private_Key> ca_key(make_a_private_key(sig_algo));
+   Botan::X509_Cert_Options opts;
 
-   if(!ca_key)
-      {
-      // Failure because X.509 enabled but requested signature algorithm is not present
-      result.test_note("Skipping due to missing signature algorithm: " + sig_algo);
-      return result;
-      }
+   opts.padding_scheme = sig_padding;
+
+   Botan::AlternativeName alt_name;
+   alt_name.add_attribute("DNS", "example.org");
+   alt_name.add_attribute("DNS", "example.com");
+   alt_name.add_attribute("DNS", "example.net");
+
+   opts.extensions.add(new Botan::Cert_Extension::Subject_Alternative_Name(alt_name));
+
+   Botan::PKCS10_Request req = Botan::X509::create_cert_req(opts, key, hash_fn, Test::rng());
+
+   std::vector<std::string> alt_dns_names = req.subject_alt_name().get_attribute("DNS");
+
+   result.test_eq("Expected number of DNS names", alt_dns_names.size(), 3);
+
+   // The order is not guaranteed so sort before comparing
+   std::sort(alt_dns_names.begin(), alt_dns_names.end());
+
+   result.test_eq("Expected DNS name 1", alt_dns_names.at(0), "example.com");
+   result.test_eq("Expected DNS name 2", alt_dns_names.at(1), "example.net");
+   result.test_eq("Expected DNS name 3", alt_dns_names.at(2), "example.org");
+
+   return result;
+   }
+
+Test::Result test_x509_cert(const Botan::Private_Key& ca_key,
+                            const std::string& sig_algo,
+                            const std::string& sig_padding = "",
+                            const std::string& hash_fn = "SHA-256")
+   {
+   Test::Result result("X509 Unit");
 
    /* Create the self-signed cert */
-   const auto ca_cert = Botan::X509::create_self_signed_cert(ca_opts(sig_padding), *ca_key, hash_fn, Test::rng());
+   const auto ca_cert = Botan::X509::create_self_signed_cert(ca_opts(sig_padding), ca_key, hash_fn, Test::rng());
 
       {
       const auto constraints = Botan::Key_Constraints(Botan::KEY_CERT_SIGN | Botan::CRL_SIGN);
@@ -661,7 +688,7 @@ Test::Result test_x509_cert(const std::string& sig_algo, const std::string& sig_
                                    Test::rng());
 
    /* Create the CA object */
-   Botan::X509_CA ca(ca_cert, *ca_key, {{"padding",sig_padding}}, hash_fn, Test::rng());
+   Botan::X509_CA ca(ca_cert, ca_key, {{"padding",sig_padding}}, hash_fn, Test::rng());
 
    /* Sign the requests to create the certs */
    Botan::X509_Certificate user1_cert =
@@ -787,31 +814,23 @@ Test::Result test_x509_cert(const std::string& sig_algo, const std::string& sig_
    return result;
    }
 
-Test::Result test_usage(const std::string& sig_algo, const std::string& hash_fn = "SHA-256")
+Test::Result test_usage(const Botan::Private_Key& ca_key,
+                        const std::string& sig_algo,
+                        const std::string& hash_fn = "SHA-256")
    {
    using Botan::Key_Constraints;
 
    Test::Result result("X509 Usage");
 
-   /* Create the CA's key and self-signed cert */
-   std::unique_ptr<Botan::Private_Key> ca_key(make_a_private_key(sig_algo));
-
-   if(!ca_key)
-      {
-      // Failure because X.509 enabled but requested signature algorithm is not present
-      result.test_note("Skipping due to missing signature algorithm: " + sig_algo);
-      return result;
-      }
-
    /* Create the self-signed cert */
    const Botan::X509_Certificate ca_cert = Botan::X509::create_self_signed_cert(
          ca_opts(),
-         *ca_key,
+         ca_key,
          hash_fn,
          Test::rng());
 
    /* Create the CA object */
-   const Botan::X509_CA ca(ca_cert, *ca_key, hash_fn, Test::rng());
+   const Botan::X509_CA ca(ca_cert, ca_key, hash_fn, Test::rng());
 
    std::unique_ptr<Botan::Private_Key> user1_key(make_a_private_key(sig_algo));
 
@@ -875,28 +894,21 @@ Test::Result test_usage(const std::string& sig_algo, const std::string& hash_fn
    return result;
    }
 
-Test::Result test_self_issued(const std::string& sig_algo, const std::string& sig_padding = "", const std::string& hash_fn = "SHA-256")
+Test::Result test_self_issued(const Botan::Private_Key& ca_key,
+                              const std::string& sig_algo,
+                              const std::string& sig_padding = "",
+                              const std::string& hash_fn = "SHA-256")
    {
    using Botan::Key_Constraints;
 
    Test::Result result("X509 Self Issued");
 
-   // create the CA's key and self-signed cert
-   std::unique_ptr<Botan::Private_Key> ca_key(make_a_private_key(sig_algo));
-
-   if(!ca_key)
-      {
-      // Failure because X.509 enabled but requested signature algorithm is not present
-      result.test_note("Skipping due to missing signature algorithm: " + sig_algo);
-      return result;
-      }
-
    // create the self-signed cert
    const Botan::X509_Certificate ca_cert = Botan::X509::create_self_signed_cert(
-         ca_opts(sig_padding), *ca_key, hash_fn, Test::rng());
+         ca_opts(sig_padding), ca_key, hash_fn, Test::rng());
 
    /* Create the CA object */
-   const Botan::X509_CA ca(ca_cert, *ca_key, {{"padding",sig_padding}}, hash_fn, Test::rng());
+   const Botan::X509_CA ca(ca_cert, ca_key, {{"padding",sig_padding}}, hash_fn, Test::rng());
 
    std::unique_ptr<Botan::Private_Key> user_key(make_a_private_key(sig_algo));
 
@@ -983,21 +995,13 @@ struct typical_usage_constraints
    };
 
 
-Test::Result test_valid_constraints(const std::string& pk_algo)
+Test::Result test_valid_constraints(const Botan::Private_Key& key,
+                                    const std::string& pk_algo)
    {
    Test::Result result("X509 Valid Constraints");
 
-   std::unique_ptr<Botan::Private_Key> key(make_a_private_key(pk_algo));
-
-   if(!key)
-      {
-      // Failure because X.509 enabled but requested algorithm is not present
-      result.test_note("Skipping due to missing signature algorithm: " + pk_algo);
-      return result;
-      }
-
    // should not throw on empty constraints
-   verify_cert_constraints_valid_for_key_type(*key, Key_Constraints(Key_Constraints::NO_CONSTRAINTS));
+   verify_cert_constraints_valid_for_key_type(key, Key_Constraints(Key_Constraints::NO_CONSTRAINTS));
 
    // now check some typical usage scenarios for the given key type
    typical_usage_constraints typical_usage;
@@ -1007,40 +1011,40 @@ Test::Result test_valid_constraints(const std::string& pk_algo)
       // DH and ECDH only for key agreement
       result.test_throws("all constraints not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.all);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.all);
          });
       result.test_throws("cert sign not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.ca);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.ca);
          });
       result.test_throws("signature not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.sign_data);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.sign_data);
          });
       result.test_throws("non repudiation not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.non_repudiation);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.non_repudiation);
          });
       result.test_throws("key encipherment not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_encipherment);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_encipherment);
          });
       result.test_throws("data encipherment not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.data_encipherment);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.data_encipherment);
          });
 
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_encipher_only);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_decipher_only);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_encipher_only);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_decipher_only);
 
       result.test_throws("crl sign not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.crl_sign);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.crl_sign);
          });
       result.test_throws("sign, cert sign, crl sign not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.sign_everything);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.sign_everything);
          });
       }
    else if(pk_algo == "RSA")
@@ -1048,65 +1052,65 @@ Test::Result test_valid_constraints(const std::string& pk_algo)
       // RSA can do everything except key agreement
       result.test_throws("all constraints not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.all);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.all);
          });
 
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.ca);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.sign_data);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.non_repudiation);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_encipherment);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.data_encipherment);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.ca);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.sign_data);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.non_repudiation);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.key_encipherment);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.data_encipherment);
 
       result.test_throws("key agreement not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement);
          });
       result.test_throws("key agreement, encipher only not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_encipher_only);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_encipher_only);
          });
       result.test_throws("key agreement, decipher only not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_decipher_only);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_decipher_only);
          });
 
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.crl_sign);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.sign_everything);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.crl_sign);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.sign_everything);
       }
    else if(pk_algo == "ElGamal")
       {
       // only ElGamal encryption is currently implemented
       result.test_throws("all constraints not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.all);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.all);
          });
       result.test_throws("cert sign not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.ca);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.ca);
          });
 
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.data_encipherment);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_encipherment);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.data_encipherment);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.key_encipherment);
 
       result.test_throws("key agreement not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement);
          });
       result.test_throws("key agreement, encipher only not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_encipher_only);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_encipher_only);
          });
       result.test_throws("key agreement, decipher only not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_decipher_only);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_decipher_only);
          });
       result.test_throws("crl sign not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.crl_sign);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.crl_sign);
          });
       result.test_throws("sign, cert sign, crl sign not permitted not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.sign_everything);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.sign_everything);
          });
       }
    else if(pk_algo == "DSA" || pk_algo == "ECDSA" || pk_algo == "ECGDSA" || pk_algo == "ECKCDSA" ||
@@ -1115,36 +1119,36 @@ Test::Result test_valid_constraints(const std::string& pk_algo)
       // these are signature algorithms only
       result.test_throws("all constraints not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.all);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.all);
          });
 
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.ca);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.sign_data);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.non_repudiation);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.ca);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.sign_data);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.non_repudiation);
 
       result.test_throws("key encipherment not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_encipherment);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_encipherment);
          });
       result.test_throws("data encipherment not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.data_encipherment);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.data_encipherment);
          });
       result.test_throws("key agreement not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement);
          });
       result.test_throws("key agreement, encipher only not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_encipher_only);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_encipher_only);
          });
       result.test_throws("key agreement, decipher only not permitted", [&key, &typical_usage]()
          {
-         verify_cert_constraints_valid_for_key_type(*key, typical_usage.key_agreement_decipher_only);
+         verify_cert_constraints_valid_for_key_type(key, typical_usage.key_agreement_decipher_only);
          });
 
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.crl_sign);
-      verify_cert_constraints_valid_for_key_type(*key, typical_usage.sign_everything);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.crl_sign);
+      verify_cert_constraints_valid_for_key_type(key, typical_usage.sign_everything);
       }
 
    return result;
@@ -1203,28 +1207,21 @@ class String_Extension final : public Botan::Certificate_Extension
       std::string m_contents;
    };
 
-Test::Result test_x509_extensions(const std::string& sig_algo, const std::string& sig_padding = "", const std::string& hash_fn = "SHA-256")
+Test::Result test_x509_extensions(const Botan::Private_Key& ca_key,
+                                  const std::string& sig_algo,
+                                  const std::string& sig_padding = "",
+                                  const std::string& hash_fn = "SHA-256")
    {
    using Botan::Key_Constraints;
 
    Test::Result result("X509 Extensions");
 
-   /* Create the CA's key and self-signed cert */
-   std::unique_ptr<Botan::Private_Key> ca_key(make_a_private_key(sig_algo));
-
-   if(!ca_key)
-      {
-      // Failure because X.509 enabled but requested signature algorithm is not present
-      result.test_note("Skipping due to missing signature algorithm: " + sig_algo);
-      return result;
-      }
-
    /* Create the self-signed cert */
    Botan::X509_Certificate ca_cert =
-      Botan::X509::create_self_signed_cert(ca_opts(sig_padding), *ca_key, hash_fn, Test::rng());
+      Botan::X509::create_self_signed_cert(ca_opts(sig_padding), ca_key, hash_fn, Test::rng());
 
    /* Create the CA object */
-   Botan::X509_CA ca(ca_cert, *ca_key, {{"padding",sig_padding}}, hash_fn, Test::rng());
+   Botan::X509_CA ca(ca_cert, ca_key, {{"padding",sig_padding}}, hash_fn, Test::rng());
 
    std::unique_ptr<Botan::Private_Key> user_key(make_a_private_key(sig_algo));
 
@@ -1290,18 +1287,11 @@ Test::Result test_x509_extensions(const std::string& sig_algo, const std::string
    return result;
    }
 
-Test::Result test_hashes(const std::string& algo, const std::string& hash_fn = "SHA-256")
+Test::Result test_hashes(const Botan::Private_Key& key,
+                         const std::string& hash_fn = "SHA-256")
    {
    Test::Result result("X509 Hashes");
 
-   const std::unique_ptr<Botan::Private_Key> key(make_a_private_key(algo));
-
-   if(!key)
-      {
-      result.test_note("Skipping due to missing signature algorithm: " + algo);
-      return result;
-      }
-
    struct TestData
       {
       const std::string issuer, subject, issuer_hash, subject_hash;
@@ -1345,14 +1335,14 @@ Test::Result test_hashes(const std::string& algo, const std::string& hash_fn = "
       opts.CA_key();
 
       const Botan::X509_Certificate issuer_cert =
-         Botan::X509::create_self_signed_cert(opts, *key, hash_fn, Test::rng());
+         Botan::X509::create_self_signed_cert(opts, key, hash_fn, Test::rng());
 
       result.test_eq(a.issuer, Botan::hex_encode(issuer_cert.raw_issuer_dn_sha256()), a.issuer_hash);
       result.test_eq(a.issuer, Botan::hex_encode(issuer_cert.raw_subject_dn_sha256()), a.issuer_hash);
 
-      const Botan::X509_CA ca(issuer_cert, *key, hash_fn, Test::rng());
+      const Botan::X509_CA ca(issuer_cert, key, hash_fn, Test::rng());
       const Botan::PKCS10_Request req =
-         Botan::X509::create_cert_req(a.subject, *key, hash_fn, Test::rng());
+         Botan::X509::create_cert_req(a.subject, key, hash_fn, Test::rng());
       const Botan::X509_Certificate subject_cert =
          ca.sign_request(req, Test::rng(), from_date(2008, 01, 01), from_date(2033, 01, 01));
 
@@ -1370,10 +1360,6 @@ class X509_Cert_Unit_Tests final : public Test
          std::vector<Test::Result> results;
 
          const std::string sig_algos[] { "RSA", "DSA", "ECDSA", "ECGDSA", "ECKCDSA", "GOST-34.10" };
-         Test::Result cert_result("X509 Unit");
-         Test::Result usage_result("X509 Usage");
-         Test::Result self_issued_result("X509 Self Issued");
-         Test::Result extensions_result("X509 Extensions");
 
          for(const std::string& algo : sig_algos)
             {
@@ -1381,53 +1367,85 @@ class X509_Cert_Unit_Tests final : public Test
             if(algo == "RSA")
                continue;
 #endif
-            for(auto padding_scheme : Botan::get_sig_paddings(algo))
-               {
-               try
-                  {
-                  cert_result.merge(test_x509_cert(algo, padding_scheme));
-                  }
-               catch(std::exception& e)
-                  {
-                  cert_result.test_failure("test_x509_cert " + algo, e.what());
-                  }
-               }
+
+            std::unique_ptr<Botan::Private_Key> key = make_a_private_key(algo);
+
+            if(key == nullptr)
+               continue;
+
+            results.push_back(test_hashes(*key));
+            results.push_back(test_valid_constraints(*key, algo));
+
+            Test::Result usage_result("X509 Usage");
             try
                {
-               usage_result.merge(test_usage(algo));
+               usage_result.merge(test_usage(*key, algo));
                }
             catch(std::exception& e)
                {
                usage_result.test_failure("test_usage " + algo, e.what());
                }
+            results.push_back(usage_result);
+
             for(auto padding_scheme : Botan::get_sig_paddings(algo))
                {
+               Test::Result cert_result("X509 Unit");
                try
                   {
-                  self_issued_result.merge(test_self_issued(algo, padding_scheme));
+                  cert_result.merge(test_x509_cert(*key, algo, padding_scheme));
+                  }
+               catch(std::exception& e)
+                  {
+                  cert_result.test_failure("test_x509_cert " + algo, e.what());
+                  }
+               results.push_back(cert_result);
+
+               Test::Result pkcs10_result("PKCS10 extensions");
+               try
+                  {
+                  pkcs10_result.merge(test_pkcs10_ext(*key, padding_scheme));
+                  }
+               catch(std::exception& e)
+                  {
+                  pkcs10_result.test_failure("test_pkcs10_ext " + algo, e.what());
+                  }
+               results.push_back(pkcs10_result);
+
+               Test::Result self_issued_result("X509 Self Issued");
+               try
+                  {
+                  self_issued_result.merge(test_self_issued(*key, algo, padding_scheme));
                   }
                catch(std::exception& e)
                   {
                   self_issued_result.test_failure("test_self_issued " + algo, e.what());
                   }
-               }
-            for(auto padding_scheme : Botan::get_sig_paddings(algo))
-               {
+               results.push_back(self_issued_result);
+
+               Test::Result extensions_result("X509 Extensions");
                try
                   {
-                  extensions_result.merge(test_x509_extensions(algo, padding_scheme));
+                  extensions_result.merge(test_x509_extensions(*key, algo, padding_scheme));
                   }
                catch(std::exception& e)
                   {
                   extensions_result.test_failure("test_extensions " + algo, e.what());
                   }
+               results.push_back(extensions_result);
                }
+
             }
 
-         results.push_back(cert_result);
-         results.push_back(usage_result);
-         results.push_back(self_issued_result);
-         results.push_back(extensions_result);
+         /*
+         These are algos which cannot sign but can be included in certs
+         */
+         const std::vector<std::string> enc_algos = { "DH", "ECDH", "ElGamal" };
+
+         for(std::string algo : enc_algos)
+            {
+            std::unique_ptr<Botan::Private_Key> key = make_a_private_key(algo);
+            results.push_back(test_valid_constraints(*key, algo));
+            }
 
 #if defined(BOTAN_TARGET_OS_HAS_FILESYSTEM)
          Test::Result pad_config_result("X509 Padding Config");
@@ -1442,23 +1460,6 @@ class X509_Cert_Unit_Tests final : public Test
          results.push_back(pad_config_result);
 #endif
 
-         const std::vector<std::string> pk_algos
-            {
-            "DH", "ECDH", "RSA", "ElGamal", "GOST-34.10",
-            "DSA", "ECDSA", "ECGDSA", "ECKCDSA"
-            };
-
-         Test::Result valid_constraints_result("X509 Valid Constraints");
-
-         for(const std::string& algo : pk_algos)
-            {
-#if !defined(BOTAN_HAS_EMSA_PKCS1)
-            if(algo == "RSA")
-               continue;
-#endif
-            valid_constraints_result.merge(test_valid_constraints(algo));
-            }
-
 #if defined(BOTAN_TARGET_OS_HAS_FILESYSTEM)
          results.push_back(test_x509_utf8());
          results.push_back(test_x509_bmpstring());
@@ -1467,10 +1468,8 @@ class X509_Cert_Unit_Tests final : public Test
          results.push_back(test_x509_authority_info_access_extension());
 #endif
 
-         results.push_back(valid_constraints_result);
          results.push_back(test_x509_dates());
          results.push_back(test_cert_status_strings());
-         results.push_back(test_hashes("ECDSA"));
          results.push_back(test_x509_uninit());
 
          return results;