Fix ML-DSA private key encoding

[falko-strenzke]

This updates the ML-DSA private key format to RFC 9881.

The implementation now by default writes the "seed-only" format. If the generative seed is not contained in the private key, then it encodes in the "expanded-only" format. It can read all three formats. In case of the "both" format, the key expanded from the seed and the encoded expanded key are compared. If they differ, a Decoding_Error is thrown.

Some points to discuss:

One real problem of this fix is that it destroys backwards compatibility for those users who relied on the private key being the raw seed. Even in the "seed-only" format, now the encoded private key is wrapped into an ASN.1/DER TLV structure. Unfortunately, Botan provides no possibility to distinguish the context in which a key is encoded. It knows only a single format, which happended to be the tangible raw seed format previously and thus was tempting for downstream users to rely on it to provide the seed. The clean solution would be to have the encoding layer independent from the cryptographic keys and always force the user to explicitly choose an encoding context. In any case, a fundamental solution for the problem is out of reach of this PR. I see no other way than breaking backwards compatibility now.

For compatibility with the DilithiumRound3 and previous ML-DSA draft, the private key decoding in this implementation also still accepts a raw seed, which is not specified by RFC 9881. We could remove this for the decoding of ML-DSA keys but leave it for the DilithiumRound3 keys. Note that OpenSSL also still supports the raw seed for ML-DSA.

A minor point with the change of the format, the existing ML-DSA KAT tests show errors because the hash of the private key is not as expected. I circumvented this by checking the private key hash only for the Dilithium KAT tests and skipping it for the ML-DSA tests. This should be fine since the key expansion is effectively checked via the Dilithium KAT tests.

Fixes #5002.

[falko-strenzke]

@randombit @reneme
The crucial point to decide is whether this PR brings breaking change in the ML-DSA private key encoding. Downstream users who relied on public_key_bits() to return the raw seed will face errors. However, in my view, it is not a breaking change, since the function documentation says "Returns: BER encoded private key bits". Thus downstream users were already required to query the seed.

[coveralls]

coverage: 90.19% (-1.5%) from 91.672%
when pulling 62224a1 on fix-mldsa-priv-key-encoding
into 90b10f4 on master.

[reneme]

Thanks for taking that on. Apart from some initial comments regarding your implementation, below are some ideas regarding your discussion items.

Also note that CI is currently unhappy with relevant issues.

---

One real problem of this fix is that it destroys backwards compatibility for those users who relied on the private key being the raw seed. Even in the "seed-only" format, now the encoded private key is wrapped into an ASN.1/DER TLV structure. Unfortunately, Botan provides no possibility to distinguish the context in which a key is encoded. [...] I see no other way than breaking backwards compatibility now.

Yeah, we have a similar API-ergonomics issue for encoding ECC public keys that may be stored 'compressed' or 'uncompressed'. The solution there essentially boils down to a state setter in the key object (see below). This isn't optimal but is probably the best bet for the time being, instead of breaking backward compatibility

botan/src/lib/pubkey/ecc_key/ecc_key.h

Lines 80 to 84 in 963f575

	/**
	* Set the point encoding method to be used when encoding this key.
	* @param enc the encoding to use
	*/
	void set_point_encoding(EC_Point_Format enc);

The clean solution would be to have the encoding layer independent from the cryptographic keys and always force the user to explicitly choose an encoding context. In any case, a fundamental solution for the problem is out of reach of this PR.

I agree. In the past @randombit and I had sketched out a fairly generic builder-style API for such "ad-hoc configuration needs" (see #5021, #4996, #4593, #4694). Something like that would likely be helpful here as well. Probably it is time to resurrect this effort after the release of 3.11.0

For compatibility with the DilithiumRound3 and previous ML-DSA draft, the private key decoding in this implementation also still accepts a raw seed, which is not specified by RFC 9881.

We should definitely keep the possibility for importing raw keys. Not all applications rely on ASN.1 containers for storing their keys, and we want to be compatible with those. For instance, one can also import ECC private keys just via their private scalar (and their group parameters).

A minor point with the change of the format, the existing ML-DSA KAT tests show errors because the hash of the private key is not as expected.

This should be addressed by allowing to export the old "raw" format.

[reneme]

Suggested change

	#include "botan/pk_keys.h"
	#include "test_rng.h"
	#include "tests.h"
	#include <memory>
	#include <vector>
	#include "test_rng.h"
	#include "tests.h"
	#include <botan/pk_keys.h>
	#include <memory>
	#include <vector>

[falko-strenzke]

Adopted.

[falko-strenzke]

I agree. In the past @randombit and I had sketched out a fairly generic builder-style API for such "ad-hoc configuration needs" (see #5021, #4996, #4593, #4694). Something like that would likely be helpful here as well. Probably it is time to resurrect this effort after the release of 3.11.0

I don't think that the builder pattern is really addressing the problem. Encoding always happens in a context like X.509/PKCS#8 etc. or OpenPGP. Without a specific context, encoding of keys doesn't really make much sense. Agreeable, that has changed a bit since NIST defined explicit encodings for public and private keys for the PQC schemes. In my view that would mean that Botan should support the NIST and PKCS#8 encoding contexts.

The problem with the current implementation in Botan is that private keys offer private_key_bits() as a general encoding function that is agnostic to the context. So in one way or another, each invocation of an encoding operation should specifiy the encoding context. That would solve our problem.

[falko-strenzke]

This should be addressed by allowing to export the old "raw" format.

In my above comment I explained the problem with that. Allowing to export the raw format means allowing to create invalid PKCS#8 containers. This is a no-go for a cryptographic library. Invalid artifacts that are not interoperable and will cause problems for users that unintentionally create them.

[reneme]

The problem with the current implementation in Botan is that private keys offer private_key_bits() as a general encoding function that is agnostic to the context. So in one way or another, each invocation of an encoding operation should specifiy the encoding context. That would solve our problem.

One way of adding such an API would be the builder pattern in my opinion, though. Along the lines outlined here:

auto nist_encoding = private_key.export()     // -> returns some generic builder API
                                .as_raw()     // -> raw (NIST specified)
                                .as_seed();   // -> use the seed

auto pkcs8 = private_key.export()   // -> same generic builder API
                        .as_pkcs8()
                        .with_seed()           // use seed only
                        .without_encryption()  // no container encryption
                        .as_der();  // no ASCII armor

... for sure the devil is in the details here. E.g. most keys won't understand what with_seed() means, but the API has to be generic because it must be defined on the polymorphic Private_Key base class.

Or am I missing your point somehow?

[falko-strenzke]

One way of adding such an API would be the builder pattern in my opinion, though. Along the lines outlined here:

auto nist_encoding = private_key.export()     // -> returns some generic builder API
                                .as_raw()     // -> raw (NIST specified)
                                .as_seed();   // -> use the seed

auto pkcs8 = private_key.export()   // -> same generic builder API
                        .as_pkcs8()
                        .with_seed()           // use seed only
                        .without_encryption()  // no container encryption
                        .as_der();  // no ASCII armor

... for sure the devil is in the details here. E.g. most keys won't understand what with_seed() means, but the API has to be generic because it must be defined on the polymorphic Private_Key base class.

Or am I missing your point somehow?

I would prefer a solution where the key doesn't have a public function to encode itself, but there is an API

secure_vector<byte> PKCS8::Encode_Private_Key(Private_Key const& key);
bool PKCS8::Private_Key_supports_PKCS8_encoding(Private_Key const& key);

That keeps the private key interface clean from the encoding layer. Possibly it could also be enhanced to control the encoding format variant where such exists, but I don't think it is really necessary. Not many users will need that.

[reneme]

I'd be fine with freestanding encoding functions as well. Though, such an interface still needs to be configurable, though. I.e. to allow the user to choose the inclusion of the seed and/or the expanded key.

[falko-strenzke]

I'd be fine with freestanding encoding functions as well. Though, such an interface still needs to be configurable, though. I.e. to allow the user to choose the inclusion of the seed and/or the expanded key.

OK, sure, we can also offer the option of the concrete encoding variant in the new API. But I think it might take us some time before we have worked out and agreed on the details of a new API. For now I suggest that we get this bugfix upstream. Currently Botan produces invalid PKCS#8 for ML-DSA. This PR fixes the bug and uses fixed seed only encoding for ML-DSA and can still decode the previous "raw seed" format. DilithiumRound3 is left with raw seed decoding.

[falko-strenzke]

I now implemented raw_private_key_bits() to return the raw seed if available or otherwise throw. Accessing the raw seed is necessary for the MLDSA-composite certificates.

[falko-strenzke]

For the record: I can't figure out the problem with the remaining CI failure. Do I possibly have to exclude the test "mldsa_private_key" from the script call

src/scripts/run_tests_under_valgrind.py --test-binary=/home/runner/work/botan/botan/build/botan-test --verbose --bunch --track-origins --with-leak-check --skip-tests=argon2,bcrypt,bcrypt_pbkdf,compression_tests,cryptobox,dh_invalid,dh_kat,dh_keygen,dl_group_gen,dlies,dsa_kat_verify,dsa_param,ecc_basemul,ecdsa_verify_wycheproof,ed25519_sign,elgamal_decrypt,elgamal_encrypt,elgamal_keygen,ffi_dh,ffi_dsa,ffi_elgamal,frodo_kat_tests,hash_nist_mc,hss_lms_keygen,hss_lms_sign,mce_keygen,passhash9,pbkdf,pcurves_arith,pwdhash,rsa_encrypt,rsa_pss,rsa_pss_raw,scrypt,sphincsplus,sphincsplus_fors,slh_dsa_keygen,slh_dsa,srp6_kat,srp6_rt,unit_tls,x509_path_bsi,x509_path_rsa_pss,xmss_keygen,xmss_keygen_reference,xmss_sign,xmss_unit_tests,xmss_verify,xmss_verify_invalid,dilithium_kat_6x5_Deterministic,dilithium_kat_6x5_Randomized,dilithium_kat_8x7_Deterministic,dilithium_kat_8x7_Randomized,dilithium_kat_6x5_AES_Deterministic,dilithium_kat_6x5_AES_Randomized,dilithium_kat_8x7_AES_Deterministic,dilithium_kat_8x7_AES_Randomized,ml_dsa_kat_6x5_Deterministic,ml_dsa_kat_6x5_Randomized,ml_dsa_kat_8x7_Deterministic,ml_dsa_kat_8x7_Randomized

which supposedly failed? Is there possibly any performance problem associated with this test that breaks the execution on the CI machine?

Since the above call runs fine locally on my system (including the new test "mldsa_private_key") and the CI output doesn't indicated the error, I am now re-running the job with debug logging enabled.

[randombit]

@falko-strenzke Can you rebase and see if the ACVP and/or Wycheproof tests can be extended here - IIRC both have various tests relating to seed vs expanded-key encodings which are currently being skipped.