Skip to content

Conversation

meisterluk
Copy link

Content:

  • add dependency to pkcs8 and const-oid crate and define features pkcs8 & alloc
  • implement AssociatedOid and AssociatedAlgorithmIdentifier for ML-KEM parameter sets {MlKem512Params,MlKem768Params,MlKem1024Params}
  • implement AssociatedAlgorithmIdentifier for EncapsulationKey & DecapsulationKey
  • serialization:
    implement EncodePublicKey for EncapsulationKey
    implement EncodePrivateKey for DecapsulationKey
  • deserialization:
    implement TryFrom<pkcs8::SubjectPublicKeyInfoRef> for EncapsulationKey
    implement TryFrom<pkcs8::PrivateKeyInfo> for DecapsulationKey
  • add testcase with DER serialization roundtrip

Remark:
The encapsulation key is serialized into a Subject Public Key Info as defined in RFC 5280 § 4.1.2.7 or more specifically for ML-KEM, draft-ietf-lamps-kyber-certificates-latest from 2025-05-27 § 4: https://lamps-wg.github.io/kyber-certificates/draft-ietf-lamps-kyber-certificates.html#name-subject-public-key-fields

The decapsulation key is serialized into a Private Key Info as defined in RFC 5958 § 2 or more specifically for ML-KEM, draft-ietf-lamps-kyber-certificates-latest from 2025-05-27 § 6: https://lamps-wg.github.io/kyber-certificates/draft-ietf-lamps-kyber-certificates.html#name-private-key-format

Implementation was paid for by our company fragmentiX, because we have the usecase internally as well. Many thanks! And also to my colleague for the helpful remarks.

Solves issue #54

@tarcieri
Copy link
Member

tarcieri commented Jul 3, 2025

@tarcieri
Copy link
Member

tarcieri commented Jul 3, 2025

Please add test vectors from: https://github.com/lamps-wg/kyber-certificates/tree/main/example

@meisterluk
Copy link
Author

Please add test vectors from: https://github.com/lamps-wg/kyber-certificates/tree/main/example

What should the test structure look like? Shall I copy the test vectors as const values into the codebase?

@tarcieri
Copy link
Member

tarcieri commented Jul 4, 2025

@meisterluk we usually add a tests/examples/ directory containing the files and use e.g. include_str! to load the file

@meisterluk
Copy link
Author

Just a heads-up about the current state: I do have a difference in the structure between the Go implementation (which generated the test vectors) and my implementation. The Go implementation turns the byte slice []byte wrapping the byte array [PrivateKeySize]byte into an OCTET_STRING inside an OCTET_STRING. I don't understand why this would be appropriate, but I need to study the ASN1 specification which I do not understand yet.

@tarcieri
Copy link
Member

tarcieri commented Jul 9, 2025

@meisterluk I believe it’s because the key is a CHOICE which supports various combinations of expanded key and seed.

Ed25519 works similarly, FWIW

@meisterluk
Copy link
Author

Yes, thanks. But I do not see how this is specified in any way. I'll continue next Monday.

@tarcieri
Copy link
Member

tarcieri commented Jul 10, 2025

I think that's what's being described here, albeit in somewhat arcane language:

For the keys defined in this document, the private key is always an opaque byte sequence. The ASN.1 type PqckemPrivateKey is defined in this document to hold the byte sequence. Thus, when encoding a OneAsymmetricKey object, the private key is wrapped in a PqckemPrivateKey object and wrapped by the OCTET STRING of the "privateKey" field.

PqckemPrivateKey ::= OCTET STRING

i.e. PqckemPrivateKey is the inner OCTET STRING, and privateKey is the outer OCTET STRING

@meisterluk meisterluk force-pushed the meisterluk/mlkem-pkcs8-rc branch from b9bbaf7 to 06a992c Compare August 22, 2025 17:50
@meisterluk
Copy link
Author

meisterluk commented Aug 22, 2025

I managed to implement the CHOICE distinction (took some time; I was not familiar with the der crate). The changes can be found in the commit “implement DER document structure suggested in IETF draft spec”. I rebased the branch onto the latest master branch state.

The good story:

  • All three CHOICE variants, namely formats seed, expanded, and both can be read for all ML-KEM variants.
  • The expanded format is written.
  • I introduced the suggested testcases for test vectors in directory tests/examples/ with include_str!

The bad story:

  • The expanded format is written and therefore not the suggested seed format. This is directly represented by issue ml-kem: seed support for DecapsulationKey #53.
  • I had to include an auxiliary RNG implementation (SeedBasedRng implements rand_core::RngCore & CryptoRng) for a static seed in testcases.
  • It is undocumented which part of bad-* test files is bad. So I did not implement them.

@meisterluk meisterluk force-pushed the meisterluk/mlkem-pkcs8-rc branch 5 times, most recently from 70b584c to 4b3f66b Compare August 22, 2025 18:36
@meisterluk
Copy link
Author

Okay, the CI is stricter here than my tools. Finally, I made it work.

Comment on lines 269 to 295
/// The serialization of the private key is a choice between three different formats
/// [according to PKCS#8](https://lamps-wg.github.io/kyber-certificates/draft-ietf-lamps-kyber-certificates.html#name-private-key-format).
///
/// “For ML-KEM private keys, the privateKey field in `OneAsymmetricKey`
/// contains one of the following DER-encoded `CHOICE` structures.
/// The seed format is a fixed 64-byte `OCTET STRING` (66 bytes total
/// with the 0x8040 tag and length) for all security levels,
/// while the expandedKey and both formats vary in size by security level”
#[cfg(feature = "pkcs8")]
#[derive(Clone, Debug, pkcs8::der::Choice)]
pub enum PrivateKeyChoice<'o> {
/// FIPS 203 format for an ML-KEM private key: a 64-octet seed
#[asn1(tag_mode = "IMPLICIT", context_specific = "0")]
Seed(OctetStringRef<'o>),
/// FIPS 203 format for an ML-KEM private key: the decapsulation key resulting from PKE's `KeyGen` operation
Expanded(OctetStringRef<'o>),
/// In this setting both key formats are provided in a `PrivateKeyBothChoice` `struct`
Both(PrivateKeyBothChoice<'o>),
}

/// The private key's `Both` variant contains the seed as well as the expanded key.
#[cfg(feature = "pkcs8")]
#[derive(Clone, Debug, pkcs8::der::Sequence)]
pub struct PrivateKeyBothChoice<'o> {
/// FIPS 203 format for an ML-KEM private key: a 64-octet seed
pub seed: OctetStringRef<'o>,
/// FIPS 203 format for an ML-KEM private key: the decapsulation key resulting from PKE's `KeyGen` operation
pub expanded: OctetStringRef<'o>,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how I feel about supporting all of these. IMO this approach is something of a mistake which has been widely criticized and exists only because a handful of HSM vendors shipped draft standards too soon and refuse to change.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, but the question is what options we have here…

because the consensus seems to be to have the distinction with CHOICE and simultaneously seed-only, which this library cannot support… I could only change this implementation in such a way that “CHOICE is supported” and “only PrivateKeyChoice::Expanded is allowed”.

Copy link
Member

@tarcieri tarcieri Sep 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't exactly say there's a "consensus" around the draft. The consensus is definitely around seed-only APIs, but some hardware vendors shipped the draft NIST standard, refused to change, and now this PKCS#8 draft is trying to accommodate them for doing that.

So the question is how much do we want to go out of our way to support those hardware vendors and what effect will that have on interoperability in the overall ecosystem.

I think for an initial implementation we should just support seeds. Supporting seeds is uncontroversial and it will unblock this PR from debate about supporting the other formats.

After that you can do a follow-up to add in the more complex functionality, and we can discuss its relative merits.

Lukas PROKOP added 8 commits September 15, 2025 11:52
• add dependency to pkcs8 and const-oid crate and define features pkcs8 & alloc
• implement AssociatedOid and AssociatedAlgorithmIdentifier
  for ML-KEM parameter sets {MlKem512Params,MlKem768Params,MlKem1024Params}
• implement AssociatedAlgorithmIdentifier
  for EncapsulationKey & DecapsulationKey
• serialization:
  implement EncodePublicKey for EncapsulationKey
  implement EncodePrivateKey for DecapsulationKey
• deserialization:
  implement TryFrom<pkcs8::SubjectPublicKeyInfoRef> for EncapsulationKey
  implement TryFrom<pkcs8::PrivateKeyInfoRef> for DecapsulationKey
• add testcase with DER serialization roundtrip

REMARK:
The encapsulation key is serialized into a Subject Public Key Info
as defined in RFC 5280 § 4.1.2.7 or more specifically for ML-KEM,
draft-ietf-lamps-kyber-certificates-latest from 2025-05-27 § 4:
https://lamps-wg.github.io/kyber-certificates/draft-ietf-lamps-kyber-certificates.html#name-subject-public-key-fields

The decapsulation key is serialized into a Private Key Info
as defined in RFC 5958 § 2 or more specifically for ML-KEM,
draft-ietf-lamps-kyber-certificates-latest from 2025-05-27 § 6:
https://lamps-wg.github.io/kyber-certificates/draft-ietf-lamps-kyber-certificates.html#name-private-key-format
• previously, the associated type Error was defined to be
  pkcs8::spki::Error instead of pkcs8::Error
• As a result, the blanket implementation
  """
  impl<T> DecodePrivateKey for T
  where
    T: for<'a> TryFrom<PrivateKeyInfoRef<'a>, Error = Error>,
  """
  https://docs.rs/pkcs8/0.11.0-rc.4/pkcs8/trait.DecodePrivateKey.html#impl-DecodePrivateKey-for-T
  was not effective and DecodePrivateKey was not implemented for
  DecapsulationKey
…PublicKey

• these traits are supported through blanket implementations
  and shall be tested as well
Let us consider the test vectors from this repository:

  https://github.com/lamps-wg/kyber-certificates/tree/624bcaa4bd9ea9e72de5b51d81ce2d90cbd7e54a

This implementation uses a seed of values [0, 1, …, 63].
Now let us generate the public and private keys.

My previous implementation generated an OCTET STRING of length 1632
for ML-KEM 512:

  $ openssl asn1parse -in ml-kem-512-private-key-generated.pem -dump | head -n6 | tail -n2
   20:d=1  hl=4 l=1632 prim: OCTET STRING
      0000 - 70 55 4f d4 36 34 4f 27-85 b1 b3 b1 ba c1 84 b6   pUO.64O'........

The IETF draft specification suggests a structure involving ASN.1
CHOICE to distinguish between seed, expanded, and both formats:

  https://lamps-wg.github.io/kyber-certificates/draft-ietf-lamps-kyber-certificates.html#name-private-key-format

This leads to four additional bytes:

  $ openssl asn1parse -in ML-KEM-512-expanded.priv -dump | head -n6 | tail -n2
   20:d=1  hl=4 l=1636 prim: OCTET STRING
      0000 - 04 82 06 60 70 55 4f d4-36 34 4f 27 85 b1 b3 b1   ...`pUO.64O'....

This implementation now introduces the CHOICE discriminator.

Recognize that the draft suggests serialization to the seed format per
default. Sadly we cannot implement this. The data stored in
EncapsulationKey does not store the seed which lead to this
encapsulation key. As a result, the expanded format is always used.

The previous text talked about changes in the serialization.
In terms of deserialization all three formats must be supported which
also I implemented.
@meisterluk meisterluk force-pushed the meisterluk/mlkem-pkcs8-rc branch from 4b3f66b to c105ec9 Compare September 15, 2025 10:00
@meisterluk
Copy link
Author

I may have misunderstood your last review thread. It might be about the public API and not about seed/expanded-only. Thus I …

  • unified use-statements for cfg conditions
  • I declared KeyChoice* structs as pub(crate) and not pub
  • I rebased to master
  • I updated Cargo.lock due to crate updates for pkcs8-specific crates
  • I reworded previous commits on this branch to use the ml-kem: prefix in git commit messages

@meisterluk
Copy link
Author

(My work colleague gave me some implementation feedback. I added it in another commit)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants