randombit
Add CertificateParametersBuilder as a replacement for X509_Cert_Options
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?
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.
coverage: 90.649% (-0.02%) from 90.671% when pulling 76653987e46306eb93b8230ad2bf9ebe5ec98388 on jack/new-cert-builder into a72cc07c6ed021b8b35d803aceb69d5c721c2777 on master.
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()).
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. 😏
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.
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.
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.
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.
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.
Pimpl with
shared_ptrworks great for immutable objects (egX509_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");
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
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
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?