Add PK_Signature_Options

[randombit]

This allows controlling all details of how signatures are created, without having to stuff values into the single parameters string which was previously available.

This is still missing some bits, mostly around each scheme has to validate that the requests are sensible. For example right now for most algorithm if you set a context, we just ignore it instead of rejecting it. Should be covered now.

There are also some future extension points to consider; for example for ECDSA/DSA we currently gate deterministic signatures just on the compile time existence of RFC6979. In the future this could be changed so that RFC6979 is only used if both available at compile time and requested.

Future work will add analogous options for encryption/decryption, and for KEM.

XMSS previously completely ignored it's parameters string. Eg a XMSS-SHA-512 key you could set params to "SHA-256" or "WHATEVER" or anything and it just worked. Now the parameters string, if set, must be exactly the hash function used by the key.

Todos

• Also update verification
• Fully check arguments (reject context if not supported, etc)
• Move validation logic to an internal header to avoid exposing more than we need to (even if nominally _ prefixed and "private")
• RSA logic works for compat mode but is confusing using the new API. We'd want to support eg PK_Signature_Options("SHA-256").with_padding("PSSR") and right now I think that doesn't work.
• Maybe add PK_Signature_Options::with_hash so the user can be extra explicit??
• Finish gutting EMSA::create
• Figure out what to do for DS2/DS3 implicit vs explicit
• Update cli and examples
• Compat shims for someone calling create_signature_op
• Support providing a hash function object (more details in this collapsed comment)
• Make the RNG optional (currently it is required, even when creating a deterministic signature)
• Update documentation
• Modify tests (?) [maybe better to leave them as is to ensure coverage on _parse, IDK]
• Comprehensive tests for all algorithms that we reject parameters that are unexpected
• Figure out a story for FFI. In the short term (until Botan4) the old constructors do everything, but we might need to expose something like PK_Signature_Options to FFI in the long run.

[reneme]

Just a few minor comments from reading over the patch. I love the direction this is taking!

[reneme]

One could say that people have been warned in the doxygen comment, but its still a public member nevertheless. For good measure: perhaps leave a deprecated create_verification_op that sets up the PK_Signature_Options and calls into this new method?

Same suggestion for the signature op, obviously.

[randombit]

I always considered these functions internal and - had the convention existed earlier - they would have been _ prefixed. That said, they were not in fact _ prefixed, and I guess a compatability shim here isn’t that onerous.

[reneme]

Not 100% sure, but another alternative perhaps: couldn't we move those into a protected: region and declare the generic PK_Signer, ...classes as friends? That might need a sort-of "landing pad" non-virtual method in the base class and protected pure virtual methods as customization points to delegate to (think update vs. add_data in Buffered_Computation)

Certainly a lot of boilerplate just to omit underscored public methods. Just thinking out loud...

I'm in team "friends before private-ish public methods", if you couldn't tell. 😅

[reneme]

❤️

[reneme]

Probably its worth to also adapt the padding documentation in this regard: https://botan.randombit.net/handbook/api_ref/pubkey.html#available-encryption-padding-schemes. It lists those "alternative names" without giving guidance on what is the "preferred" name.

[reneme]

Oh, just noticed one more thing: When instantiating PSS, and asking the instance for its name(), it identifies as "EMSA4". This is somewhat inconsistent with this deprecation notice. Also note, that this assumption is hard-coded at places. At least I just stumbled over this in RSA_Signature_Operation::algorithm_identifier() in rsa.cpp.

[randombit]

Yeah currently it's a bit of a mess atm. Similarly the OID definitions use the (would be) deprecated names. Fixing this should happen but it's a significant bit of work all on its own.

[reneme]

Some more observations.

[reneme]

I'm a bit surprised by the constraints on the builder methods. I would have expected:

PK_Signature_Options& with_hash(std::string_view hash) {
  set_or_throw(m_hash, hash);
  return *this;
}

Some users would probably want to express things like this:

auto key = get_private_key();

auto opts = PK_Signature_Options("SHA-256");
if(key->algo_name() == "RSA) {
   opts.with_padding("PSS");
}
if(key->algo_name() == "ML-DSA") {
  opts.with_context("some context");
}

PK_Signer signer(key, opts);

I think people will grumble and then go for std::move(opts).with_context(...) as a workaround. See pk_options_impl.cpp 😜.

[randombit]

I went this way to avoid the problem that if there is a unique_ptr<HashFunction> in there we have to clone it for each and every with_foo. There is maybe a better way to handle this.

[reneme]

Returning a PK_Options& (return *this) avoids the copy, as long as the user doesn't assign it to another variable. That allows chaining with_* just fine.

[reneme]

I have to correct myself to a certain extent: When there's a unique_ptr<HashFunction> you are forced to first create an lvalue of the PK_Options. E.g. this wouldn't work:

auto opts = PK_Options().with_hash(Hash_Function::create_or_throw("SHA-256"));
// Above fails, because `with_hash()` returns a `PK_Options&` that cannot be copied into `auto opts`
// (because `PK_Options` contains a unique_ptr).

if (key->algo() == "ML-DSA") {
   opts.with_context("some context");
}

In C++20 we'd have to overload with_hash() for lvalue-ref and rvalue-ref. Providing both overloads, would compile the above example, as the rvalue-ref variant of with_hash() would be called on the temporary object and the temporary would just propagate through with_hash() and eventually end up initializing the auto opts lvalue.

class PK_Options {
   PK_Options& with_hash(std::unique_ptr<HashFunction> hash) & {
      m_hash = std::move(hash);
      return *this;
   }

   PK_Options with_hash(std::unique_ptr<HashFunction> hash) && {
      return std::move(with_hash(std::move(hash)));
   }
};

C++23 introduces "deducing this" which would come in handy here. Essentially, self is deduced to either an lvalue-ref or an rvalue-ref. And the decltype(auto) would just pass the lvalue/rvalue-ref along. The below snippet would also compile the above example in C++23 😢

class PK_Options {
   decltype(auto) with_hash(this auto&& self, std::unique_ptr<HashFunction> hash) {
      self.m_hash = std::move(hash);
      return self;
   }
};

[reneme]

Here's another iteration (godbolt) that splits "building" and "consuming" into two classes. We hope that this results in a clearer interface that's harder to misuse: i.e. users cannot reach into the built-up options but have to push it into a consumer. This forces users to "commit" on a configuration and prevents consumers from retroactively adding options.

The prototype in Godbolt might look quite complex, but the actual usage surface is super neat, in my opinion. Roughly like this:

void end_user_view() {
   auto opts = SignerBuilder()
                  .with_padding("OAEP")
                  .with_hash("SHA-256")
                  .with_context(as_byteview("API discussion"));
   PK_Signer signer(key, opts.commit() /* transforms SignerBuilder into a SignerOptions */);
}

PK_Signer::PK_Signer(Private_Key& key, SignerOptions options) 
   m_op(key._create_signing_op(options)
{
   options.validate_option_consumption(); // -> throws Invalid_Argument when some option was not consumed
}

PK_Signing_Op RSA_PrivateKey::_create_signing_op(SignerOptions& opts) {
   const auto padding = opts.padding().required(); // throws Invalid_Argument if not provided
   const auto hash = opts.hash().optional(); // std::nullopt if not provided (algorithm can decide what to do)
   opts.context().not_implemented("comes in 3.8.0"); // explicitly consumes an option but throws Not_Implemented when provided
}

The helpers validate_options_consumption() and .required(), .optional() and .not_implemented() greatly simplify the sanity checks of the options in the concrete algorithms. Internally, the base classes of SignerBuilder and SignerOptions take care of tracking the usage of each option and produce decent error messages (see program outputs in godbolt). They are also meant to be reusable for future options builder use cases.

[reneme]

Rebased once more. The fix for SLH-DSA in 3.6.1 created a conflict again.

[reneme]

Figure out a story for FFI.

Some initial idea (also regarding #4411). Inspired by: https://stackoverflow.com/questions/17604811/builder-pattern-in-c

botan_pk_sign_options_t options;
botan_pk_sign_config_create(&options, rsa_private_key);

botan_pk_sign_config_set_ptr(options, BOTAN_PK_SIGN_RNG, rng);
botan_pk_sign_config_set_str(options, BOTAN_PK_SIGN_PADDING, "PSS");
botan_pk_sign_config_set_str(options, BOTAN_PK_SIGN_HASH, "SHA-256");
// or (?):
botan_pk_sign_config_set_ptr(options, BOTAN_PK_SIGN_HASH, hash_instance);
botan_pk_sign_config_set_int(options, BOTAN_PK_SIGN_SALT_SIZE, 64);
botan_pk_sign_config_set_opt(options, BOTAN_PK_SIGN_DER_ENCODED);
botan_pk_sign_config_set_opt(options, -BOTAN_PK_SIGN_DETERMINISTIC); // negative to unset

botan_pk_op_sign_t signer;
botan_pk_op_sign_create_from_options(&signer, options);
botan_pk_sign_destroy(options); // this could also be implicit in the previous call, not sure (?)

// use 'signer' as usual...

[dirkz]

Re the FFI story, looks good to me. Preferably with auto-destruction of options.

[reneme]

@randombit I'd love to restart the discussion on this and hopefully converge on a design for the builder.

We'd like to develop the pre-hash feature for SLH-DSA and ML-DSA based on it. That is the final work item for the BSI project and it would be great to have it land in 3.7.0.

Let me know if you would like me to walk you through the idea of this rework.