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. -- Update Falko 2026-07-30: test works now as before.

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.

[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.

[falko-strenzke]

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

Done.

[randombit]

Some review comments. This also really needs a rebase to latest master to pick up for instance the new ACVP and Wycheproof test runs in CI.

We had to deal with the same issue for ML-KEM in #4817 and it probably makes sense to align those interfaces. [At a minimum adding an enum identifying the key serialization in use and allowing multiple different serializations]

[randombit]

This should verify the decoded seed is the correct length

[randombit]

And also that expanded is the correct size for the mode

[falko-strenzke]

Also here, both size checks are (now) done in the decoding functions.

[falko-strenzke]

Done.

[falko-strenzke]

@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.

Yes, done. expanded format is supported in both now.

[falko-strenzke]

We had to deal with the same issue for ML-KEM in #4817 and it probably makes sense to align those interfaces. [At a minimum adding an enum identifying the key serialization in use and allowing multiple different serializations]

I see, but the additions in #4817 are not complete. ML-KEM private key encoding is also still broken, i.e., is not compliant with RFC 9935. We now need to cover the distinction between raw, i.e. mere private key material as in FIPS 204, and the properly ASN.1/DER wrapped structure required for RFC-conforming PKCS#8 encoding. So the whole picture looks to me like this:

• We create a new module ml_common
  • which defines an interface class that both ML-DSA and ML-KEM inherit from. The interface defines:
    • enum class MlPrivateKeyFormat
      • which is extended by both format
    • MlPrivateKeyFormat private_key_format() const;
    • secure_vector<uint8_t> private_key_bits_with_format(MlPrivateKeyFormat format) const;
    • secure_vector<uint8_t> raw_private_key_bits_with_format(MlPrivateKeyFormat format) const;
  • General behaviour:
    • A key decoded from a specific format (seed, expanded, both) is encoded in the same format again unless the format is explicitly requested (as it is currently realized by ML-KEM). I think this point is clear.
    • A newly generated key encodes itself in a predefined format, which we have to decide on: either the seed format or both format. OpenSSL outputs in both by default, from my point of view that is the most reasonable and conservative choice.

The current implementation status is:

Feature	ML-KEM	ML-DSA
private_key_format()	✓	□
private_key_bits_with_format(MlPrivateKeyFormat format)	✓	□
raw_private_key_bits_with_format(MlPrivateKeyFormat format)	□	□

So the decisions that need to be taken before I can move on with this PR are in my view:

• Introduce the interface class in a new module ml_common or just mirror the functions and logic in both ML-... Pro (new module and interface): the common interface underlines the commitment to the same functions and logic. Con: If the key encoding mechanism gets reworked fundamentally, this module will become obsolete again.
• Encode newly generated keys as seed-only or both.

[reneme]

One side note regarding "a new common module": If we go that way, you could use the existing pqcrystals module which currently implements helpers and base classes for both ML-DSA and ML-KEM. This is currently an "Internal" module, we might have to change that to let it expose public headers if needed.

[reneme]

I agree with your suggestions regarding the API additions. Also, moving and extending the MlPrivateKeyFormat enum as you suggested makes sense to me. As mentioned before, the common pqcrystals module may be a good place for that.

Allow me to extend on your table to make sure we're all on the same page:

API	Behavior	ML-KEM	ML-DSA	Behavior Change
private_key_format()	Returns whatever MlPrivateKeyFormat is currently set	✓	□	none
raw_private_key_bits_with_format(MlPrivateKeyFormat format)	Encodes the private key according to format	□	□	none
private_key_bits_with_format(MlPrivateKeyFormat format)	Wraps the output of raw_private_key_bits_with_format() into ASN.1/DER	✓	□	none
raw_private_key_bits()	Encodes the private key according to private_key_format()	✓	✓	now depends on private_key_format()
private_key_bits()	Wraps the output of raw_private_key_bits() into ASN.1/DER	?	?	now produces "wrapped" keys and depends on private_key_format()

In my opinion the changes in the existing API are okay and/or outright bugfixes: Especially private_key_bits() just being an alias for raw_private_key_bits() was always incorrect. The documentation in the base class in pk_keys.h literally states: "Return the PKCS8 private key encoding". Hence, I think it is safe to assume that existing applications stayed away from this method and used raw_private_key_bits() anyway. 🤞

Additionally, I think we should offer set_private_key_format(MlPrivateKeyFormat format) for both ML-KEM/ML-DSA that sanity checks whether or not the desired encoding is possible (i.e. throw if the private seed would be needed but isn't available). That setter would make the API consistent with EC_Public_Key::set_point_encoding().

[reneme]

Encode newly generated keys as seed-only or both.

My vote is on "both" as well. Applications can always override that choice anyway.

[falko-strenzke]

@reneme Just to be clear: for private_key_bits_with_format(MlPrivateKeyFormat format) that would mean a breaking change: previously, it returned the raw key material, and now we plan to let it return the properly ASN.1-wrapped format as needed for the inner PKCS#8. The table thus should look like this in my understanding:

API	Behavior	ML-KEM	ML-DSA	Behavior Change
private_key_format()	Returns whatever MlPrivateKeyFormat is currently set	✓	□	none
raw_private_key_bits_with_format(MlPrivateKeyFormat format)	Encodes the private key according to format	□	□	N/A
private_key_bits_with_format(MlPrivateKeyFormat format)	Wraps the output of raw_private_key_bits_with_format() into ASN.1/DER	✓	□	none Previously returned the raw key material, now returns ASN.1/DER-wrapped key
raw_private_key_bits()	Encodes the private key according to private_key_format()	✓	✓	now depends on private_key_format(), no change: yields raw key material
private_key_bits()	Wraps the output of raw_private_key_bits() into ASN.1/DER	✓	✓	now produces "wrapped" keys and depends on private_key_format()

[reneme]

You are right. In #4817 we should have called the new ML-KEM-specific function raw_private_key_bits_with_format(). That's too much of a SemVer violation for my taste, to be honest. Because this is a very specific API that some application might actually depend on legitimately. 😒

1) Alternative suggestion: Let's deprecate that API and instead introduce new methods:

• BOTAN_DEPRECATED("Use formatted_raw_private_key_bits() instead") private_key_bits_with_format(MlPrivateKeyFormat)
• formatted_raw_private_key_bits(MlPrivateKeyFormat format) ... returning the raw key in the passed format
• formatted_private_key_bits(MlPrivateKeyFormat format) ... returning the ASN.1/DER wrapped key in the passed format

2) Another alternative: just deprecate private_key_bits_with_format(MlPrivateKeyFormat) and don't introduce any explicit format-aware API. Instead just introduce a set_private_key_format(MlPrivateKeyFormat) and use the existing polymorphic functions for serialization based on the object's internal state. That's what we've been doing for the ECC public point encoding as well after all.

Sucks a little, but I guess it's what it is...

@randombit what do you think about this?

[falko-strenzke]

1) Alternative suggestion: Let's deprecate that API and instead introduce new methods:

* `BOTAN_DEPRECATED("Use formatted_raw_private_key_bits() instead") private_key_bits_with_format(MlPrivateKeyFormat)`

* `formatted_raw_private_key_bits(MlPrivateKeyFormat format)` ... returning the raw key in the passed format

* `formatted_private_key_bits(MlPrivateKeyFormat format)` ... returning the ASN.1/DER wrapped key in the passed format

2) Another alternative: just deprecate private_key_bits_with_format(MlPrivateKeyFormat) and don't introduce any explicit format-aware API. Instead just introduce a set_private_key_format(MlPrivateKeyFormat) and use the existing polymorphic functions for serialization based on the object's internal state. That's what we've been doing for the ECC public point encoding as well after all.

Letting the key's state decide the encoding format is not the proper way in my view. This is for the same reason that we don't tell an integer that it is a "hex" or "decimal" integer. The functionality for choosing alternative encodings should live outside the object being encoded as much as possible. (The limitation here is the availability of the seed.)

Another suggestion would be the introduction of a framework for key encoding, for now maybe only applied to ML-... This framework could also address decoding. I don't think accepting just any 32-byte strings as secret keys, which allow for backwards compatibility, is what is suitable in every scenario. Where one expects a properly encoded key, one should be able to specify that in the API.

[reneme]

I 100% agree with you, that the internal serialization state in the key is crappy. I really do. I also support the introduction of a versatile encoding/decoding framework to disentangle live key objects and their serialization concerns.

However, for the sake of progress on this particular PR, I would suggest to go with one of the suggested solutions. I'm happy to throw around ideas and prototype APIs independent of this. Perhaps to be introduced in Botan4.

[falko-strenzke]

However, for the sake of progress on this particular PR, I would suggest to go with one of the suggested solutions.

You are right, makes no sense to make this a blocker now. Then I vote for your propsal 1.:

1) Alternative suggestion: Let's deprecate that API and instead introduce new methods:

• BOTAN_DEPRECATED("Use formatted_raw_private_key_bits() instead") private_key_bits_with_format(MlPrivateKeyFormat)
• formatted_raw_private_key_bits(MlPrivateKeyFormat format) ... returning the raw key in the passed format
• formatted_private_key_bits(MlPrivateKeyFormat format) ... returning the ASN.1/DER wrapped key in the passed format

@randombit Would you agree? This is at least a mid-term interface decision it seems.

[falko-strenzke]

@reneme The new interface is in place. The "pqcrystals" module, since it was not a public one, was renamed to "module_lattice" (src/lib/pubkey/module_lattice/).

[reneme]

This is in good shape now. Thanks for the patience on this. Please address the few nits and adaptions mentioned below and clean up the history. Then, that's good to go from my side.

[reneme]

Please rebase to latest master (to pull in #5946) and then:

Suggested change

	Botan::DilithiumInternalKeypair decode_seed_only(std::span<const uint8_t> key_bits, Botan::DilithiumConstants mode) {
	Botan::secure_vector<uint8_t> seed;
	Botan::BER_Decoder(key_bits)
	.decode(seed, Botan::ASN1_Type::OctetString, Botan::ASN1_Type(0), Botan::ASN1_Class::ContextSpecific)
	.verify_end();
	if(seed.size() != Botan::DilithiumConstants::SEED_RANDOMNESS_BYTES) {
	throw Botan::Decoding_Error("invalid length of ML-DSA private key seed");
	}
	return Botan::Dilithium_Algos::expand_keypair(Botan::DilithiumSeedRandomness(std::move(seed)), std::move(mode));
	}
	Botan::DilithiumInternalKeypair decode_seed_only(std::span<const uint8_t> key_bits, Botan::DilithiumConstants mode) {
	Botan::DilithiumSeedRandomness seed;
	Botan::BER_Decoder(key_bits)
	.decode(seed, Botan::ASN1_Type::OctetString, Botan::ASN1_Type(0), Botan::ASN1_Class::ContextSpecific)
	.verify_end();
	if(seed.size() != Botan::DilithiumConstants::SEED_RANDOMNESS_BYTES) {
	throw Botan::Decoding_Error("invalid length of ML-DSA private key seed");
	}
	return Botan::Dilithium_Algos::expand_keypair(std::move(seed), std::move(mode));
	}

.. technically, you should be able to drop a lot of Botan:: namspace qualifications in this file, but that's really just a nit.

[falko-strenzke]

Done. Also removed the Botan::.

[reneme]

Please rebase to latest master (to pull in #5946) and then:

Suggested change

	Botan::DilithiumInternalKeypair decode_expanded_only(std::span<const uint8_t> key_bits,
	Botan::DilithiumConstants mode) {
	Botan::secure_vector<uint8_t> expanded;
	Botan::BER_Decoder(key_bits).decode(expanded, Botan::ASN1_Type::OctetString).verify_end();
	if(expanded.size() != mode.private_key_bytes()) {
	throw Botan::Decoding_Error("invalid length of ML-DSA (or Dilithium) expanded private key byte string");
	}
	return Botan::Dilithium_Algos::decode_keypair(Botan::DilithiumSerializedPrivateKey(std::move(expanded)),
	std::move(mode));
	}
	Botan::DilithiumInternalKeypair decode_expanded_only(std::span<const uint8_t> key_bits,
	Botan::DilithiumConstants mode) {
	Botan::DilithiumSerializedPrivateKey expanded;
	Botan::BER_Decoder(key_bits).decode(expanded, Botan::ASN1_Type::OctetString).verify_end();
	if(expanded.size() != mode.private_key_bytes()) {
	throw Botan::Decoding_Error("invalid length of ML-DSA (or Dilithium) expanded private key byte string");
	}
	return Botan::Dilithium_Algos::decode_keypair(expanded, std::move(mode));
	}

... note that expanded isn't moved on purpose. decode_keypair takes this by strong-span.

[falko-strenzke]

Done.

[reneme]

Please rebase to latest master (to pull in #5946) and then:

Suggested change

	Botan::DilithiumInternalKeypair decode_seed_plus_expanded(std::span<const uint8_t> key_bits,
	Botan::DilithiumConstants mode) {
	Botan::secure_vector<uint8_t> expanded;
	Botan::secure_vector<uint8_t> seed;
	Botan::BER_Decoder(key_bits)
	.start_sequence()
	.decode(seed, Botan::ASN1_Type::OctetString)
	.decode(expanded, Botan::ASN1_Type::OctetString)
	.end_cons()
	.verify_end();
	// expanded is still needed for the consistency check below, hence no std::move
	const Botan::DilithiumInternalKeypair key_pair =
	Botan::Dilithium_Algos::decode_keypair(Botan::DilithiumSerializedPrivateKey(expanded), mode);
	const Botan::DilithiumInternalKeypair key_pair_from_seed =
	Botan::Dilithium_Algos::expand_keypair(Botan::DilithiumSeedRandomness(std::move(seed)), std::move(mode));
	DilithiumSerializedPrivateKey expanded_from_seed = Dilithium_Algos::encode_keypair(key_pair_from_seed);
	if(expanded_from_seed.get() != expanded) {
	throw Botan::Decoding_Error("seed and expanded key in ML-DSA serialized key do not match");
	}
	return key_pair_from_seed;
	}
	Botan::DilithiumInternalKeypair decode_seed_plus_expanded(std::span<const uint8_t> key_bits,
	Botan::DilithiumConstants mode) {
	Botan::DilithiumSerializedPrivateKey expanded;
	Botan::DilithiumSeedRandomness seed;
	Botan::BER_Decoder(key_bits)
	.start_sequence()
	.decode(seed, Botan::ASN1_Type::OctetString)
	.decode(expanded, Botan::ASN1_Type::OctetString)
	.end_cons()
	.verify_end();
	// expanded is still needed for the consistency check below, hence no std::move
	const Botan::DilithiumInternalKeypair key_pair =
	Botan::Dilithium_Algos::decode_keypair(expanded, mode);
	const Botan::DilithiumInternalKeypair key_pair_from_seed =
	Botan::Dilithium_Algos::expand_keypair(std::move(seed), std::move(mode));
	const auto expanded_from_seed = Dilithium_Algos::encode_keypair(key_pair_from_seed);
	if(expanded_from_seed != expanded) {
	throw Botan::Decoding_Error("seed and expanded key in ML-DSA serialized key do not match");
	}
	return key_pair_from_seed;
	}

[falko-strenzke]

Done.

[reneme]

Do we really need to parse the expanded key? If the expansion is faulty, the sanity-check below will not pass anyway. And we're returning the result of the seed-expansion in the end.

If we'd remove this, we would likely miss out on some more on-point error message that might (or might not) be produced by the parser. I'm quite indifferent, because this won't likely happen on any critical path.

I would say: either remove or add a comment why it should stay.

[falko-strenzke]

You are right, it's unneeded, removed the parsing of expanded, length check on the expanded part remains.

[reneme]

This comparison could be done using a constant-time comparison for good measure, but it would really be cosmetic, I guess. Attacker-controlled private key seems like trouble anyway.

[falko-strenzke]

Done.

[reneme]

Please rename to is_ml_dsa(). The public-facing Dilithium_Mode uses this naming scheme already, and I'd like to stay consistent. As discussed earlier in the year: this function is likely to disappear in Botan4.

[falko-strenzke]

OK, done, comes in next commit.