Skip to content

Add PK_Signature_Options#5489

Open
randombit wants to merge 1 commit into
masterfrom
jack/pk-signature-options
Open

Add PK_Signature_Options#5489
randombit wants to merge 1 commit into
masterfrom
jack/pk-signature-options

Conversation

@randombit

Copy link
Copy Markdown
Owner

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

The PR previously known as #5021 #4318

@randombit
randombit force-pushed the jack/pk-signature-options branch 2 times, most recently from 4276d00 to 31f25d1 Compare March 26, 2026 22:02
This allows controlling all details of how signatures are created,
without having to stuff values into the single parameters string
which was previously available.
@randombit
randombit force-pushed the jack/pk-signature-options branch from 31f25d1 to 384e804 Compare March 26, 2026 22:04
Comment thread src/lib/pubkey/pk_keys.h
* currently supported. In a future release, if Ed25519ctx was supported,
* then this function would start returning true for Ed25519 keys.
*/
virtual bool supports_context_data() const;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it - from a principle point of view - really the key that supports the context parameter or is it rather the signature scheme? In the case of the currently affected signature schemes (EdDSA, ML-DSA, SLH-DSA, and ML-DSA-composite), that doesn't seem to make a difference, since they tie the key to specific signature scheme. However, you never know, if it might make a difference in the future...

Comment thread src/cli/pubkey.cpp
oss << padding << "(" << hash << ")";
return oss.str();
Botan::PK_Signature_Options sig_options(
std::string_view key, std::string_view padding, std::string_view hash, bool use_der, std::string_view provider) {

@falko-strenzke falko-strenzke Mar 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using string arguments is not a good approach in my view. But maybe for now it is the simplest option. However, in the future, we can add a type framework for the parameters and start to define further constructors for the PK_Signature_Options.

@falko-strenzke

Copy link
Copy Markdown
Collaborator

I did not perform a full review yet. But regarding the general approach, I think it is a great improvement. The new signature options type is a good thing in my view because it facilitates signature options to some degree already and lays a foundation for future improvemets. Further improvements could be typed signature parameters and possibly even compile-time validation of the parameter specification in the future.

@falko-strenzke falko-strenzke mentioned this pull request Mar 27, 2026
5 tasks
* .with_der_encoded_signature()
* .with_context("Foo")
*/
class BOTAN_PUBLIC_API(3, 6) PK_Signature_Options {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably 3.12 and maybe final.

Comment on lines +36 to +41
PK_Signature_Options(PK_Signature_Options&& other) = default;
PK_Signature_Options& operator=(PK_Signature_Options&& other) = default;

PK_Signature_Options(const PK_Signature_Options&) = default;
PK_Signature_Options& operator=(const PK_Signature_Options& other) = default;
~PK_Signature_Options();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rule-of-zero? The d'tor is currently defaulted in the cpp file, but there's no real reason for that, as none of the members would actually require that AFAICS.

@randombit randombit added this to the Botan 3.13 milestone Apr 27, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants