Fix a bug in SAN handling when creating certs and PKCS 10 requests

If the Subject Alternative Name extension was set in opts.extensions,
then we would ignore any values set in other fields (eg opts.dns)

src/lib/x509/x509self.cpp

@@ -21,25 +21,48 @@ namespace {
 /*
 * Load information from the X509_Cert_Options
 */
-void load_info(const X509_Cert_Options& opts, X509_DN& subject_dn, AlternativeName& subject_alt) {
+X509_DN load_dn_info(const X509_Cert_Options& opts) {
+   X509_DN subject_dn;
+
    subject_dn.add_attribute("X520.CommonName", opts.common_name);
    subject_dn.add_attribute("X520.Country", opts.country);
    subject_dn.add_attribute("X520.State", opts.state);
    subject_dn.add_attribute("X520.Locality", opts.locality);
    subject_dn.add_attribute("X520.Organization", opts.organization);
    subject_dn.add_attribute("X520.OrganizationalUnit", opts.org_unit);
+   subject_dn.add_attribute("X520.SerialNumber", opts.serial_number);
+
    for(const auto& extra_ou : opts.more_org_units) {
       subject_dn.add_attribute("X520.OrganizationalUnit", extra_ou);
    }
 
-   subject_dn.add_attribute("X520.SerialNumber", opts.serial_number);
-   subject_alt = AlternativeName(opts.email, opts.uri, opts.dns, opts.ip);
+   return subject_dn;
+}
+
+auto create_alt_name_ext(const X509_Cert_Options& opts, Extensions& extensions) {
+   AlternativeName subject_alt;
+
+   /*
+   If the extension was already created in opts.extension we need to
+   merge the values provied in opts with the values set in the extension.
+   */
+   if(auto ext = extensions.get_extension_object_as<Cert_Extension::Subject_Alternative_Name>()) {
+      subject_alt = ext->get_alt_name();
+   }
+
+   subject_alt.add_attribute("DNS", opts.dns);
+   subject_alt.add_attribute("URI", opts.uri);
+   subject_alt.add_attribute("RFC822", opts.email);
+   subject_alt.add_attribute("IP", opts.ip);
    subject_alt.add_othername(OID::from_string("PKIX.XMPPAddr"), opts.xmpp, ASN1_Type::Utf8String);
 
    for(const auto& dns : opts.more_dns) {
       subject_alt.add_attribute("DNS", dns);
    }
+
+   return std::make_unique<Cert_Extension::Subject_Alternative_Name>(subject_alt);
 }
+
 }  // namespace
 
 namespace X509 {
@@ -56,9 +79,7 @@ X509_Certificate create_self_signed_cert(const X509_Cert_Options& opts,
    const AlgorithmIdentifier sig_algo = signer->algorithm_identifier();
    BOTAN_ASSERT_NOMSG(sig_algo.oid().has_value());
 
-   X509_DN subject_dn;
-   AlternativeName subject_alt;
-   load_info(opts, subject_dn, subject_alt);
+   const auto subject_dn = load_dn_info(opts);
 
    Extensions extensions = opts.extensions;
 
@@ -79,7 +100,7 @@ X509_Certificate create_self_signed_cert(const X509_Cert_Options& opts,
    extensions.add_new(std::make_unique<Cert_Extension::Authority_Key_ID>(skid->get_key_id()));
    extensions.add_new(std::move(skid));
 
-   extensions.add_new(std::make_unique<Cert_Extension::Subject_Alternative_Name>(subject_alt));
+   extensions.replace(create_alt_name_ext(opts, extensions));
 
    extensions.add_new(std::make_unique<Cert_Extension::Extended_Key_Usage>(opts.ex_constraints));
 
@@ -93,9 +114,7 @@ PKCS10_Request create_cert_req(const X509_Cert_Options& opts,
                                const Private_Key& key,
                                std::string_view hash_fn,
                                RandomNumberGenerator& rng) {
-   X509_DN subject_dn;
-   AlternativeName subject_alt;
-   load_info(opts, subject_dn, subject_alt);
+   const auto subject_dn = load_dn_info(opts);
 
    const auto constraints = opts.is_CA ? Key_Constraints::ca_constraints() : opts.constraints;
 
@@ -111,8 +130,9 @@ PKCS10_Request create_cert_req(const X509_Cert_Options& opts,
       extensions.add_new(std::make_unique<Cert_Extension::Key_Usage>(constraints));
    }
 
-   extensions.add_new(std::make_unique<Cert_Extension::Extended_Key_Usage>(opts.ex_constraints));
-   extensions.add_new(std::make_unique<Cert_Extension::Subject_Alternative_Name>(subject_alt));
+   extensions.replace(create_alt_name_ext(opts, extensions));
+
+   create_alt_name_ext(opts, extensions);
 
    return PKCS10_Request::create(key, subject_dn, extensions, hash_fn, rng, opts.padding_scheme, opts.challenge);
 }

src/tests/unit_x509.cpp

@@ -818,27 +818,32 @@ Test::Result test_pkcs10_ext(const Botan::Private_Key& key,
 
    Botan::X509_Cert_Options opts;
 
+   opts.dns = "main.example.org";
+   opts.more_dns.push_back("more1.example.org");
+   opts.more_dns.push_back("more2.example.org");
+
    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");
+   alt_name.add_attribute("DNS", "bonus.example.org");
 
    opts.extensions.add(std::make_unique<Botan::Cert_Extension::Subject_Alternative_Name>(alt_name));
 
    Botan::PKCS10_Request req = Botan::X509::create_cert_req(opts, key, hash_fn, 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);
+   result.test_eq("Expected number of DNS names", alt_dns_names.size(), 4);
 
    // 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");
+   if(alt_dns_names.size() == 4) {
+      result.test_eq("Expected DNS name 1", alt_dns_names.at(0), "bonus.example.org");
+      result.test_eq("Expected DNS name 2", alt_dns_names.at(1), "main.example.org");
+      result.test_eq("Expected DNS name 3", alt_dns_names.at(2), "more1.example.org");
+      result.test_eq("Expected DNS name 3", alt_dns_names.at(3), "more2.example.org");
+   }
 
    return result;
 }