Add CertificateParametersBuilder as a replacement for X509_Cert_Options

[randombit]

No description provided.

[reneme]

I really like the idea!

Perhaps an obvious question: did you consider modeling this after the builder abstractions I had cooked up in the drafts #4694 and #4318?

[randombit]

did you consider modeling this after the builder abstractions

TBH I don't think the extra complexity of it makes sense here, where there is a single consumer of the builder output. I'm still not convinced it's worth the extra complexity even with many different consumers in the form of many different signature schemes (which is why #4318 is still stalled). But here, where there is one consumer, will only ever be one consumer, and that consumer's implementation sits right next to the option implementation? It seems to me very hard to justify.

[coveralls]

coverage: 90.649% (-0.02%) from 90.671%
when pulling 7665398 on jack/new-cert-builder
into a72cc07 on master.

[reneme]

I don't think the extra complexity of it makes sense here, where there is a single consumer of the builder output.

Frankly, that's understandable. I'm not super happy about the complexity in there either, for sure. A lot of it comes from the "consumption error handling" which indeed might be overkill here?

I do have hope that the compile-time reflection in C++26 will eventually make this easier to handle, when the compiler can take care of the boilerplate. I'd hope that this would eventually collapse into something more elegant (if one considers reflection elegant).

Independently, C++23's "deducing-this" would also be helpful for the rvalue-vs-lvalue builder handling -- which is a problem here too. You have to have an lvalue of the builder. The following doesn't work:

auto root_cert_params = Botan::CertificateParametersBuilder()
                                    .add_common_name("Benchmark Root")
                                    .add_country("DE")
                                    .add_organization("RS")
                                    .add_organizational_unit("CS")
                                    .add_dns("unobtainium.example.com")
                                    .add_email("[email protected]")
                                    .set_as_ca_certificate();

... admittedly, its not a big issue here, but it becomes annoying when trying to return a builder from a function (e.g. privkey.signer().with_emsa(...).sign()).

[reneme]

That's just my general two cents. If we start establishing builders throughout the API (which really makes a lot of sense in many places, IMHO), I would love to converge on an agreement as to how flexible they should be. I'm fully aware that my previous drafts might have taken it way too far. 😏

[randombit]

You have to have an lvalue of the builder. The following doesn't work:

Yeah what you have is how I initially wrote that code and of course was disappointed. I guess the type could implement a copy constructor, but it'll be relatively expensive and a potential performance issue if for example someone accidentally writes code that copies the params struct for each name they are processing.

[reneme]

Yeah what you have is how I initially wrote that code and of course was disappointed.

Time for C++23 then? 😜

I guess the type could implement a copy constructor, ...

It doesn't have to be expensive, if you use a std::shared_ptr<State>. But it might come with other gotchas because the shared state is highly non-obvious.

[reneme]

What actually bugs me quite a bit is the final instantiation that takes a number of positional arguments in into_self_signed_cert():

         Botan::CertificateParametersBuilder root_cert_params;
         auto cert = root_cert_params
            .add_common_name("Benchmark Root")
            .add_country("DE")
            .add_organization("RS")
            .add_organizational_unit("CS")
            .add_dns("unobtainium.example.com")
            .add_email("[email protected]")
            .set_as_ca_certificate()
            .into_self_signed_cert(not_before, not_after, *root_key, rng, get_hash_function());

... to my mind that somewhat spoils the advantage of the builder in that it isn't as extensible as it could be if all params were passed builder-style.

[randombit]

It doesn't have to be expensive, if you use a std::shared_ptr. But it might come with other gotchas because the shared state is highly non-obvious.

Something nice to write fails to compile and you have to write it in a different way > potential for silent performance issue >>>>>>>>>>>>>>>>> hidden shared state with an API that's entirely based around mutating that state. Pimpl with shared_ptr works great for immutable objects (eg X509_Certificate) but I would not consider it for anything that can be modified.

spoils the advantage of the builder in that it isn't as extensible as it could be if all params were passed builder-style.

Disagree I guess. There are common elements (eg the DN entries) but the two types (cert vs CSR) effectively have divergent inputs, and the final function call captures those. Also unlike the builder arguments, the final inputs are mostly mandatory; you can create a cert without a commonName, but you cannot create one without a key.

[arckoor]

This is slightly annoying, X509_CA::sign_request takes an X509_Time, this takes a time point. Would be nice to use only one

[randombit]

I'll add a second version of sign_request taking time points. It's X509_Time there mostly because that API predates adoption of C++11 and std::chrono.... but it's generally more useful for an application to specify time as a chrono type vs our ASN.1 type

[arckoor]

It's fine to leave it as is, it just means one 3 line conversion function for the ffi.
I already have one anyway for uint64_t -> X509_Time, so adding one for uint64_t -> time_point -> X509_Time isn't a big deal

[reneme]

Pimpl with shared_ptr works great for immutable objects (eg X509_Certificate) but I would not consider it for anything that can be modified.

Fully agree. That's the point I was trying to make.

Unlike the builder arguments, the final inputs are mostly mandatory

I would totally agree if the final inputs actually were mandatory. Like in #4694, something like: privkey->serialize().as_encrypted_pem(rng, "password").

But here you have to pass optional signing-related arguments (hash_fn and/or padding) and it depends on the specific key type which of them are needed:

      PKCS10_Request into_pkcs10_request(const Private_Key& key,
                                         RandomNumberGenerator& rng,
                                         std::optional<std::string_view> challenge_password = {},
                                         std::optional<std::string_view> hash_fn = {},
                                         std::optional<std::string_view> padding = {});

... don't get me wrong, compromises have to be made and it is what it is sometimes. Though, I think we could really do better here.

---

Throwing in a wild idea: In a perfect world these builders would be composable, I think. Assuming the API from #4318 would already be in master:

auto csr = CertificateParametersBuilder()
             .add_common_name("lol")
             // ...
             .with_signer(key.signer().with_padding(...))
             .into_pkcs10("challenge pw");

[randombit]

Though, I think we could really do better here.

I think possibly we just have different models of the inputs to the certificate/CSR. This class is for building up the metadata which is eventually cryptographically bound to the key. The builder is for the metadata, the final arguments are for the binding. I wonder if some different name for this type would be helpful.

Throwing in a wild idea: In a perfect world these builders would be composable, I think.

That would be pretty neat. But I think there (clearly) has not been consensus reached on what that should look like, which is why it's not on master

[Copilot]

Pull Request Overview

This pull request adds CertificateParametersBuilder as a modern replacement for the legacy X509_Cert_Options class. The new builder provides a more convenient fluent interface for creating X.509 certificates and PKCS10 requests.

Key changes:

• Introduces new CertificateParametersBuilder class with fluent API design
• Refactors X509_Cert_Options to use the new builder internally for compatibility
• Updates all test code to use the new builder API instead of the legacy options

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
src/lib/x509/x509_builder.h	Header defining the new CertificateParametersBuilder class
src/lib/x509/x509_builder.cpp	Implementation of the CertificateParametersBuilder
src/lib/x509/x509self.h	Added conversion method to new builder
src/lib/x509/x509self.cpp	Refactored to delegate to new builder
src/lib/x509/x509opt.cpp	Added conversion logic from options to builder
src/lib/x509/pkix_enums.h	Added operator for Key_Constraints
src/tests/test_x509_rpki.cpp	Updated test code to use new builder API
src/cli/perf_x509.cpp	Updated CLI performance test to use new builder

[arckoor]

Even if I manually construct Cert_Extension::Subject_Alternative_Name and add it as critical, doesn't this always make it non-critical?

[arckoor]

The same should be true for Extended_Key_Usage below, though I don't know if that is ever "critical" in real-world use

[arckoor]

Maybe as a bit of general feedback for the entire cert creation infrastructure, I'm repeatedly running into the issue of trying to figure out what extensions are already taken care of by the builder and just named differently, and what extensions I need to add myself - some documentation would be very helpful for the future

[arckoor]

Not entirely sure if a big-endian encoded value is a good choice here. subject_info / get_user_cert_info explicitly return a decimal-dotted string when asked for the IP field, so I think accepting one would be the way to go here too.

[arckoor]

So we tested a bit, and unfortunately the PKCS#10 flow won't work for our usecase.
Instead I would like to somehow expose X509_CA::make_cert to the FFI, but in a way that also uses the builder and does things similar to CertificateParametersBuilder::into_pkcs10_request (so that I don't have to expose extensions like SKID, Basic_Constraints, Key_Usage, ...)
Would that be possible?