This is somewhat a duplicate of the implementations of EMSA::config_for_x509() scattered around the different EMSAs. I'm not quite happy with that, to be honest. Simply instantiating the respective EMSA and then actually using ::config_for_x509() doesn't work out-of-the-box, as it would currently require to pass a respective private key. I think it would be helpful to abstract both use cases and centralize this somehow.

Anyway: As is, this is fairly inefficient, as the AlgorithmIdentifiers are re-generated for each invocation. It would be an interesting exercise to constexpr the heck out of this. 🤡

I'm open for better ideas. :)

Yeah pretty ugly. Also we recompute the PSS params each time when in practice it will always be SHA-{256,512} 😭

Looking at config_for_x509 ... why the heck does this interface take a Private_Key? It could easily get away with a Public_Key? (Not that this change would address the issue here, but still kind of odd).

Anyway this falls in the category of definitely not great and pointing to a place where some refactoring time is needed but doesn't IMO need to block the PR merging.

After merging #3142, this now became: #3125 (comment)
Thanks for the help in adapting config_for_x509() to make it suitable for this use case!

This is now using EMSA::config_for_x509() instead of copy-pasting most of its scattered implementations.

The config_for_x509() implementations check that the passed hash function name is equal to the hash function used in the EMSA. This is done by string comparison (e.g. from emsa1.cpp):

if(cert_hash_name != m_hash->name())
   throw Invalid_Argument(...);

The library can deal with different string-identifiers for hash functions (e.g "SHA-1", "SHA1" and "SHA-160"). But m_hash->name() usually returns a hard-coded identifier. I tried dealing with that using the "SHA-1" to "SHA-160" mapping above. However, this PR's macOS builds fail, because the software-implementation (sha1.cpp) returns "SHA-160" when invoking Hash_Function::name() and the CommonCrypto provider (CommonCrypto_HashFunction) that might or might not be enabled on macOS returns "SHA-1". 😨

In my opinion, these string comparisons are a generic footgun that will cause recurring issues. 😞 Especially when they force us to scatter such mappings around the code base. Lets try to centralize the string-identifier mappings in the algorithm implementations? @randombit

This could be done by requiring a static method ::implements(std::string_view algo_name) in each concrete implementation. Now, that implementations "know" all their string-identifiers, a virtual bool Hash_Function::is(std::string_view algo_name) could allow convenient polymorphic checks as in config_for_x509(). Even the Hash_Function::create() factory could benefit from ::implements() and stop duplicating the string-identifiers. See this godbolt for a sketch.

One might even consider to auto-generate the Hash_Function::create() code based on self-registering sub-classes. But that might take it too far.

Obviously, getting rid of the ambiguous string-identifiers would be even better, but I doubt the massive public API change would be worth it.

However, this PR's macOS builds fail, because the software-implementation (sha1.cpp) returns "SHA-160" when invoking Hash_Function::name() and the CommonCrypto provider (CommonCrypto_HashFunction) that might or might not be enabled on macOS returns "SHA-1"

Can you take a look at #3184?

In my opinion, these string comparisons are a generic footgun that will cause recurring issues. disappointed Especially when they force us to scatter such mappings around the code base. Lets try to centralize the string-identifier mappings in the algorithm implementations?

They are. It was one of the big design mistakes.

One might even consider to auto-generate the Hash_Function::create() code based on self-registering sub-classes.

This used to be how it worked, actually. But that approach ran into weird problems with static libraries, and ended up having to be removed (in #279)

This could be done by requiring a static method ::implements(std::string_view algo_name) in each concrete implementation. Now, that implementations "know" all their string-identifiers, a virtual bool Hash_Function::is(std::string_view algo_name) could allow convenient polymorphic checks as in config_for_x509().

This might work but I wonder how this handles parameterizations such as output length or personalization flags. Quick example

auto hash = HashFunction::create("BLAKE2b(384)");
assert!(hash->is("BLAKE2b")); // true?
assert!(hash->is("BLAKE2b(384)")); // true?

As far as I can recall the main problematic case for this is SHA-1, largely because we use the idiosyncratic/non-standard "SHA-160" name. Maybe the real fix here is to just remove that entirely as a known name, and relnote it? It's not even a big compatibility issue since "SHA-1" always worked.

Obviously, getting rid of the ambiguous string-identifiers would be even better, but I doubt the massive public API change would be worth it.

We could consider having an enum approach and then implementing the string method on top of that, but I agree that we cannot really remove the string-based interface at this point since it's very widely baked into how the CLI, the tests, the FFI layer, etc all work.

This might work but I wonder how this handles parameterizations such as output length or personalization flags.

Fair point. A static BLAKE2b::implements("BLAKE2b(384)") should return true. But a specific instance of it probably shouldn't.

Maybe the real fix here is to just remove that entirely as a known name, and relnote it?

I tend to agree. Given that the string-identifiers are such a footgun its probably a good idea to make them as dumb as possible. As in: Don't allow for ambiguities during construction.

We could consider having an enum approach [...]

Though, I don't see how parameterization would work well with enums either. Hash_Types::BLAKE2b_384? That feels rigid to me.

Would it make sense to use class AlgorithmIdentifier for those use cases -- as its name suggests? De facto this is already implemented in the specific algorithm implementations. As a library user I always found it inconvenient/confusing to construct AlgorithmIdentifier out of thin air, but that might be fixable by some user-friendly API.