hash_algo for Ed25519 and Ed448 should not expose internals

[saper]

Unlike RSA which requires hashing prior to signing, pure
Ed25519 and Ed448 signing algorithm identifiers do not refer
to the "hashing algorithm" at all.

certvalidator uses this to detect weak hashing algorithms,
but passing hashing algorithm separately to EdDSA signing/verification
functions makes no sense.

pyca/cryptography prefers to say None here.

Let's call it "raw" then.

[saper]

Looks like two conflicting meanings of "hash_algo" here:

• do we say "Ed25519 uses sha256 internally so if sha256 becomes weak, it should be disallowed"?
• or do we say "When signing or doing verification, this value should be used in addition to the algorithm selection".

(the examples are from certvalidator, but I think it is asn1crypto which should define its meaning and values).

[saper]

There are pre-hashed variants called Ed25519ph and Ed448ph, but IETF decided not to assign OID values to the hashed variants yet and therefore cannot be used in the X.509 certificates.

[MatthiasValvekens]

Hi, author of #205 here. You're of course right that these hashes are for internal use, and I'm also aware that there are no OIDs for the pre-hashed variants of EdDSA.

When I drafted the PR, I decided to supply values for hash_algo for the following two reasons:

• Consistency with other SignedDigestAlgorithmId entries in asn1crypto.
• In CMS signatures, the data that is actually signed is (typically) a set of attributes, one of which is a digest of the message. In traditional signature schemes that literally sign digests (RSA/ECDSA/...), it is common practice to use the same digest algorithm to digest the message as to digest the signed attributes. RFC 8933 even makes that mandatory. For all the reasons you point out, it's debatable whether that applies to Ed25519 and Ed448 (especially the latter), but that's one case where assigning default digest functions would appear to make sense.

(Granted, I'm not 100% sure about the implications of using SHAKE256 as a message digest function in a CMS signature, but at least for Ed25519, SHA512 is a very reasonable default.)

I confess that I was thinking more of CMS than of X.509 when I made that PR, but I think a case can be made for both options. That said, regardless of the option we pick, a comment explaining the rationale wouldn't be out of place, I guess...

Thoughts?

[saper]

Thank you for your insightful answer @MatthiasValvekens ! RFC 8419 Use of Edwards-Curve Digital Signature Algorithm (EdDSA) Signatures in the Cryptographic Message Syntax (CMS) tells us exactly this:

2.3. Message Digest Algorithm Identifiers

When the signer includes signed attributes, a message digest
algorithm is used to compute the message digest on the eContent
value. When signing with Ed25519, the message digest algorithm MUST
be SHA-512 [FIPS180]. Additional information on SHA-512 is available
in [RFC6234]. When signing with Ed448, the message digest algorithm
MUST be SHAKE256 [FIPS202] with a 512-bit output value.

So we need this information somewhere to make CMS work.

In my patch to certvalidator I override this value explicity since to makes no sense to certificate validation.

Maybe this function should be named differently (or moved into CMS-specific code)? What should be the one-line description of that hash_algo is supposed to return?

[MatthiasValvekens]

Right, thanks for the reference! I should've known, I've cited that exact text in another draft standard some time last summer... Memory is a funny thing.

Maybe splitting this property into two separate ones would indeed make sense, but I hesitate to make that call without maintainer input...

Is there any reason why the current way of doing things breaks certvalidator? As it happens, I maintain my own fork of certvalidator; it includes support for ed25519 and ed448 signatures, in addition to a bunch of other, more fundamental changes (those aren't upstreamable because I dropped Python <3.7 support). FYI, this is how I addressed the problem over there: https://github.com/MatthiasValvekens/certvalidator/blob/8f723a123845c7783e1e10ca907a3d9568ae616b/pyhanko_certvalidator/util.py#L210-L275. As you can see, my validation routine simply ignores the hash parameter.

[saper]

It does not break it, it is my own lousy programming that did it:

Instead of a series of ifs like you did, it assigns verifyfunc so that all verification functions should have the same signature:

https://github.com/wbond/certvalidator/blob/e53c06ad7f450e85808962a20f14825a43eccbc9/certvalidator/validate.py#L337

For this, I have added a compatible eddsa_verify function https://github.com/wbond/oscrypto/pull/64/files#diff-0120f9134a175b63e48fa5439900ec6f2e7c06790624f0e8ced8b5eb5a4c6a6fR1231 that accepts a hash_algorithm parameter.

Anticipating pre-hashed EdDSA support (this is certainly https://www.martinfowler.com/bliki/Yagni.html) the only currently acceptable value is "raw" (maybe the value should be "pure" or None - good question).

And then https://github.com/wbond/certvalidator/blob/e53c06ad7f450e85808962a20f14825a43eccbc9/certvalidator/validate.py#L304 hash_algo is extracted from the certificate and then sha256 or shake512 are provided, but not expected by the new eddsa_verify.

So maybe SignedDigestAlgorithm.hash_algo should be left alone for the purpose of CMS, and the mapping should be changed in Certificate.hash_algo - by returning "raw" or None or whatever for the EdDSA.

[wbond]

From reading through the discussion, I think what makes sense to me is:

1. Change the hash_algo to None to adapt to the current usage of this attribute in certvalidator. This would also require updating the docstring.
2. Add a new attribute that would be almost identical, called .cms_hash_algo with the relevant info

Does that sound reasonable?

[MatthiasValvekens]

That sounds reasonable, yes! The current hash_algo values for EdDSA would make perfect sense in a cms_hash_algo property (and I would recommend keeping them there), but returning None might be a better choice from the perspective of certvalidator's usage.

So, functionally, the change is equivalent to this:

• Put in place a cms_hash_algo property that does exactly what hash_algo does today
• Make hash_algo return None in the case of Ed448/Ed25519.

[saper]

I like it, too. But we need to update cms code for this, right?

[MatthiasValvekens]

I suppose it's technically a breaking change, but it only affects EdDSA usage, and the number of people using Ed25519 in CMS is still relatively low, since other tech standards that rely on CMS (e.g. PDF) are only now starting to catch up.

The number of asn1crypto users in this category is probably even smaller---I'm one of them, though.

Ideally, this should come with a major version increase (does this project follow semver?), but even if not, the impact is comparatively minor, and the change on the caller end is quite trivial.

TL;DR: Regardless of how we make the change, there's no better time frame than now.

[wbond]

We could probably get away with not doing a 2.0 by throwing a ValueError if you try to get the hash_algo for Ed25519 or Ed448.

[saper]

We could probably get away with not doing a 2.0 by throwing a ValueError if you try to get the hash_algo for Ed25519 or Ed448.

Would we? Raising ValueError instead of returning sha512 or shake256 respectively might be considered a breaking change, too...

[saper]

Now I wonder if https://github.com/wbond/certvalidator/blob/e53c06ad7f450e85808962a20f14825a43eccbc9/certvalidator/validate.py#L304 should not really access what we just called .cms_hash_algo ... suppose SHA-256 is considered weak one day - therefore we might consider ed25519 to be weak, too. Maybe .internal_hash_algo or something and let it be used in https://github.com/wbond/certvalidator/blob/b69d3b745b5af9e5ccd4b9781407ab7e82076d6b/certvalidator/validate.py#L306 as instead of hash_algo. "New" hash_algo would have been called in https://github.com/wbond/certvalidator/blob/e53c06ad7f450e85808962a20f14825a43eccbc9/certvalidator/validate.py#L337 instead and we get rid of a temporary variable.

[wbond]

Would we? Raising ValueError instead of returning sha512 or shake256 respectively might be considered a breaking change, too...

The idea is that technically it is bug that .hash_algo returned the values of sha512 and shake256, based on (the modular crypto project) certvalidator's usage of asn1crypto. It really shouldn't. To make it easy to users to identify if they had been relying on the buggy behavior, we would raise a ValueError.

[MatthiasValvekens]

One more remark:

Now I wonder if https://github.com/wbond/certvalidator/blob/e53c06ad7f450e85808962a20f14825a43eccbc9/certvalidator/validate.py#L304 should not really access what we just called .cms_hash_algo ... suppose SHA-256 is considered weak one day - therefore we might consider ed25519 to be weak, too. Maybe .internal_hash_algo or something and let it be used in https://github.com/wbond/certvalidator/blob/b69d3b745b5af9e5ccd4b9781407ab7e82076d6b/certvalidator/validate.py#L306 as instead of hash_algo.

Not sure I agree with this, to be honest. I'd argue that that problem is better addressed by allowing signature algorithms to be marked as deprecated. After all, it could theoretically also happen that SHA-256 winds up broken in a way that doesn't really affect ed25519 security in any meaningful way. Supporting signature algorithm deprecation seems better suited for another PR, IMHO.

[saper]

I am fully with you @MatthiasValvekens on this one - I just wanted to look at the current status and didn't dare to think what the proper solution would be. I somehow like NSS' bureaucratic "policy" settings, where you can turn your ciphers on and off as you like.

Maybe we just fire up cert_hash_algo and cms_hash_algo and move on :) I'd like to have ed25519 cert_verify released on some form soon...

[MatthiasValvekens]

I am fully with you @MatthiasValvekens on this one - I just wanted to look at the current status and didn't dare to think what the proper solution would be. I somehow like NSS' bureaucratic "policy" settings, where you can turn your ciphers on and off as you like.

Yep, sounds about right.

Maybe we just fire up cert_hash_algo and cms_hash_algo and move on :) I'd like to have ed25519 cert_verify released on some form soon...

Just to make sure I understand what you meant: is cert_hash_algo a typo for hash_algo here?

[saper]

no, not a typo - just a tongue in cheek proposal to introduce yet another function for that :)

[wbond]

Since certvalidator is the main and largest consumer of the attribute, I'd like to drive the fix via that. Introducing a new attribute would mean that certvalidator would also need to be updated. It is also the reason I think I would consider the current behavior a bug, and I'd want an exception to help users identify if they were relying on the (previous) buggy behavior.

[wbond]

@MatthiasValvekens @saper I've posted #265 as the proposed fix. I'll probably cut it as 2.0 along with any other minor breaking changes that have been pending.

[wbond]

I have committed a fix for this in #265