Reduced combinations

[feventura]

This PR was created to facilitate the identification of the changes made to implement composite signatures and certificates.

This branch only contains:

• MLDSA65-RSA3072-PKCS15-SHA512
• MLDSA65-ECDSA-P256-SHA512

[ounsworth]

That is actually "pkixAlgs", not "sigAlgs". For example, key exchanges and KEMs are also in the same arc.

https://www.iana.org/assignments/smi-numbers#smi-numbers-1.3.6.1.5.5.7.6

I suggest renaming this to "pkixAlgs". Also, if you're trying to be really efficient this arc is probably already defined somewhere in the openssl includes (but maybe it's just easier to define it fresh here).

[feventura]

Make sense, I will change its name.

[ounsworth]

This function seems unintuitive to me and is probably worth a header comment.

(Part of my problem is that I don't know what openssl ossl_ml_dsa_key_has is, but a quick Google AI search tells me that this is checking for the ML-DSA private key in Full, Seed, or Both mode).

Will a composite key ever have the ML-DSA component in Full or Both mode? Or only ever Seed?

[feventura]

This function verify if the composite contains both a mldsa and a classic components, returning 1 if so or 0 if either is missing.

The ossl_ml_dsa_key_has will return 1 if the key->ml_dsa_key contains the data requested by selection, e.g. public key, private key, or both.

I will add a comment explaining this.

[ounsworth]

I know from the reference implementation that the RSA and EC public keys can change their length by one or two bytes (I think, depending on leading zeros, which get truncated in the DER encoding). You also need to account for the DER length tags in the classic component.

Is this code actually counting the encoded length, or is it assuming a maximum length? It looks like EVP_PKEY_get_bits is the maximum size that you could ever need for a buffer for this.

If that's the case, that the size_t returned from ossl_composite_key_get_pub_len might actually be longer than the key it contains, then you should say that in the header comment.

[feventura]

This does not return the maximum length, instead this function approximate combined public key length in bytes.
This is not supposed to be used for buffer sizing, only for reporting to OSSL_PKEY_PARAM_BITS.

I rewrite the comment making this more explict.

[ounsworth]

That's a bit of a weird way to measure the security strength of a composite. In the draft, we tried very hard to not do this because it's rather difficult to compare the security of PQ and classical algs.

Is this as function that you're required to implement in order for the EVP APIs to work correctly? If you deleted this function, does anything break?

[feventura]

Starting from the second question, having a security bits related to the algorithm is a requirement for the TLS/X.509 verification.
If this function does not exist the x509 verify code will receive a 0 as the security bits, which will fail the verification.

For the first part, I agree that its difficult to compare the security of PQ and classical, and that's why I chose to use the min (by thinking of what is the weakest link), instead of trying to do some calculation to get the "actual" security of the combination.

[ounsworth]

This might be my unfamiliarity with openssl, but what is this validating? That the public keys and private keys match? It's not validating a signature.
It's probably worth adding a header comment.
(also, maybe there is a header file somewhere that declares these functions, and there's a comment on that?)

[feventura]

Yes, this does not validate a signature. This validates the key material itself for consistency.

I will add a comment to this.

[ounsworth]

This is excellent and shows that you're ready for the RSA-PSS stuff, even though it's not in this PR.

[feventura]

Actually this is something I forgot to remove when I created this branch by removing the other combinations from the main branch. (:

[ounsworth]

I'm not super convinced that a single-line helper function is all that helpful, but that's really a nit-pick.

[feventura]

Valid comment. In my head was simpler to read composite_compute_hash than EVP_Q_digest, but also having the pre-hash isolated felt the right decision.
I do not have strong feelings about this and don't mind going either way.
I will keep as it is for now, but if more people complain about this its not a hard change.

[ounsworth]

I think this needs to be a bit careful here because, as I understand this, this will return a max length, not necessarily the exact length.
That should be noted in the comment here for future maintainers, and check that this is only used for sizing buffers, which are ok to be oversized and contain some trailing un-used bytes, and nothing is expecting this to be an exact length.

[feventura]

You are right on your assessment.
I will add more information in the comment above this.

[ounsworth]

Would it be better to pre-compute the DER strings and hard-code them as hex, rather than having this code?

"Better" in the sense that it's easier to code review?

[feventura]

It might be better for code review, but because the OIDs already exist in the code, having the DER pre-computed would also add another place where we have OIDs, having two source of truth in the code, which might not be an actual problem because I don't think these information will change any time soon.
But, while investigating this I realized that other alg in openssl do that using WPACKET, so I changed this code to follow the patter other alg use.

[ounsworth]

These could be implemented, right?
If I am guessing correctly at what these are supposed to do, they could be supported.
_init() would need to initialize the composite PH, each chunk of message handed into _update() would be fed into that hash, and _final() would finish the PH, construct the M' and finish the operation.

You might find it easier to refactor so that you have a composite_sign_ph() and composite_verify_ph() as a shared building block between the streaming and one-shot functions.

Moreover, it would be worth exposing the composite_sign_ph() externally to openssl because calling applications may want to pre-compute the PH themselves (for example on another machine so that you only need to send the hash value, and not the whole message, over the network).

I think these are definitely worth implementing.