Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Drop DER encoding for composite ML-KEM functions #94

Merged
merged 13 commits into from
Nov 28, 2024

Conversation

chris-wood
Copy link
Contributor

Closes #88

I also moved content around in this PR to make it flow a bit better, e.g., by moving security-relevant stuff to the security considerations and keeping the main implementation details in the primary document.

@ounsworth
Copy link
Contributor

ounsworth commented Nov 19, 2024

Thanks @chris-wood lots of good changes. @johngray-dev we'll need to sync up the section re-arranging in composite-sigs to match.

Chris, ironically, I think the one thing you have not done in your PR is drop the DER encoding. The ASN.1 SEQUENCE wrapper is still there, so in fact this does not close #88

 CompositeKEMPublicKey ::= SEQUENCE SIZE (2) OF BIT STRING

What was your intention there?

@ounsworth ounsworth self-requested a review November 19, 2024 18:56
@chris-wood
Copy link
Contributor Author

The DER encoding is still present for those that want to use it. However, the outputs of the composite ML-KEM key generation function are distinctly not these CompositeKEMPublicKey values. They're internal, implementation-specific values. Callers can then invoke SerializePublicKey, which is the model for HPKE, to produce a simple, wire-format-encoded public key, or they can encode it using CompositeKEMPublicKey if they need to.

The point of this PR is to allow both encodings. Does that clarify?

@ounsworth
Copy link
Contributor

ounsworth commented Nov 19, 2024

The point of this PR is to allow both encodings. Does that clarify?

That sounds super confusing for implementers. Let's just do one encoding. Let's drop the DER entirely.

But that does mean that we're going to have to list out explicit byte cutoffs where the first ends and the next begins.

@chris-wood
Copy link
Contributor Author

chris-wood commented Nov 19, 2024

That sounds super confusing for implementers. Let's just do one encoding. Let's drop the DER entirely.

I would be pleased with this outcome. I'll update the PR to match.

But that does mean that we're going to have to list out explicit byte cutoffs where the first ends and the next begins.

No? These outputs are fixed-length. There's no need to encode the lengths.

@johngray-dev
Copy link
Collaborator

We are trying to keep the Composite Signatures and Composite KEM documents in similar alignment structurally. I have opened a similar issue for composite signatures (lamps-wg/draft-composite-sigs#85). We had planned to completely remove the ASN.1 wrapping and keep the similar document structure (where possible) and not have multiple encodings. It looks like you are okay with that Chris which is great. So Chris I'll use what you did in Composite KEM as a kind of template to update composite signatures, and then maybe add some tweaks into your branch (fully remove ASN.1) before final merging. I hope you are okay with that. I was thinking of adding a simple ML-KEM / ML-DSA size table for key reconstitution.

@chris-wood
Copy link
Contributor Author

@johngray-dev that works for me -- do you want to take over this PR?

@johngray-dev
Copy link
Collaborator

Sure, I can take over this PR since I was just starting the same thing on composite sigs, and I like the way you specified the serialization process, so I'll replicate that in Signatures as well (since the composite key serialization procedures are basically the same thing)... :)

Add a couple tweaks to serialization and deSerialization method to be more clear
Add back the Composite KEM structures for uses in X509, but with the SEQUENCE wrappers removed.  Make sure this aligns with the composite signatures document
Remove tabs that appeared in the document
Add back 5280 and 5652 references
Update the ASN.1 module to remove the SEQUENCE wrappings so only the concatenated values are embedded in the BIT STRING or OCTET STRING depending on whether it is a Composite KEM Public Key, Private Key, or CompositeKEMCiphertextValue
@johngray-dev johngray-dev merged commit 0c6ecfa into lamps-wg:main Nov 28, 2024
1 check passed
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.

Drop DER encoding
3 participants