Repository navigation
ssh: add opt-in pure ML-KEM key exchanges - #11750
alexandrejbr wants to merge 2 commits into
Conversation
CT Test Results 2 files 27 suites 11m 45s ⏱️ For more details on these failures, see this check. Results for commit 3df8a19. ♻️ This comment has been updated with latest results. To speed up review, make sure that you have read Contributing to Erlang/OTP and that all checks pass. See the TESTING and DEVELOPMENT HowTo guides for details about how to run test locally. Artifacts
// Erlang/OTP Github Action Bot |
Implement mlkem512-sha256, mlkem768-sha256, and mlkem1024-sha384 from draft-harrison-sshm-mlkem using crypto's existing ML-KEM APIs. Add the KEX init and reply messages, exact size validation, exchange hash and key derivation, negotiation, and rekey support. Advertise only methods supported by crypto, and keep these draft methods out of the default KEX list. Document configuration and add wire-format and connection/rekey tests.
01ba6f4 to
c69e46c
Compare
| %% Handle reason returned from handle_packet_part, or {start_packet_discard, ...} | ||
| %% A wrong first_kex_packet_follows guess is discarded before decoding: its | ||
| %% format can differ from the negotiated key exchange method. | ||
| handle_packet_part_result({packet_decrypted, <<Op, _/binary>>, EncryptedDataRest, Ssh}, |
There was a problem hiding this comment.
I'm looking into alternative implementations of this and worst case scenario cleaning up the dead code now in ssh_fsm_kexinit.erl as the clause to handle ignore_initial_kex_message will never be reached.
So expect some changes in this area. But I welcome feedback if you have.
There was a problem hiding this comment.
I think this is ok to do it like this, but if you have other ideas please share them.
Mikaka27
left a comment
Mikaka27
left a comment
There was a problem hiding this comment.
Besides the discussion about guess functionality looks ok to me.
Also this comment from another PR applies to this as well: #11752 (comment)
Mikaka27
left a comment
Mikaka27
left a comment
There was a problem hiding this comment.
Besides the discussion about guess functionality looks ok to me.
Also this comment from another PR applies to this as well: #11752 (comment)
| opts = Opts}) -> | ||
| Kem = kem(Kex), | ||
| try | ||
| true = byte_size(C_init) =:= kem_public_key_size(Kem), |
There was a problem hiding this comment.
This was already checked in decode_kem_init, I'm not sure if it's better to leave the check here or there (or both are fine?)
There was a problem hiding this comment.
I like that most of the things that the RFC specifies can be seen here in this implementation, but redundant checks are not so good and failing fast is better so the check when decoding seems better.
I will remove from here but add a comment just to try to help a future reader that looks at the RFC and the code to check if all is good.
| algorithms = #alg{kex = Kex}}) -> | ||
| try | ||
| true = Kem =:= kem(Kex), | ||
| true = byte_size(S_reply) =:= kem_ciphertext_size(Kem), |
There was a problem hiding this comment.
This was checked in decode_kem_reply, same as comment above.
There was a problem hiding this comment.
same reply as above
Mikaka27
added
testing
Implement mlkem512-sha256, mlkem768-sha256, and mlkem1024-sha384 from draft-harrison-sshm-mlkem using crypto's existing ML-KEM APIs.
Add the KEX init and reply messages, exact size validation, exchange hash and key derivation, negotiation, and rekey support. Advertise only methods supported by crypto, and keep these draft methods out of the default KEX list.
Document configuration and add wire-format and connection/rekey tests.
This was tested with CycloneSSH and a https://github.com/open-quantum-safe/openssh/blob/OQS-v10