-
Notifications
You must be signed in to change notification settings - Fork 88
ML-DSA support #556
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
ML-DSA support #556
Conversation
1ec72f2
to
2811344
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, wouldn't it be also good to add tests for other MLDSA keys, not just MLDSA-65? Or perhaps at least add MLDSA-87?
@ep69 reviewed 6 of 9 files at r7, 1 of 13 files at r12, 3 of 4 files at r13, 1 of 1 files at r15, 8 of 8 files at r16, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @tomato42)
tlslite/tlsconnection.py
line 1583 at r16 (raw file):
client_certificate = self._create_cert_msg( "client", certificate_request,
How is this change related to MLDSA support?
tlslite/tlsconnection.py
line 2121 at r16 (raw file):
"TLS version"): yield result if cert_type not in settings.more_sig_schemes:
This looks weird to check in the mldsa* branch, wasn't it meant to be indented less? But then, the else branch for RSA/DSA does not make sense..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's next to no code that does anything different based on the specific type of ml-dsa keys so I'm not sure if it's that necessary to add test coverage for more key times... I will have better test coverage for that with tlsfuzzer (there we will have to have extensive tests for all three key sizes)
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ep69)
tlslite/tlsconnection.py
line 1583 at r16 (raw file):
Previously, ep69 (Stanislav Židek) wrote…
How is this change related to MLDSA support?
it's not, I fixed it to make it work with OpenSSL 😁 can move it to separate PR...
tlslite/tlsconnection.py
line 2121 at r16 (raw file):
Previously, ep69 (Stanislav Židek) wrote…
This looks weird to check in the mldsa* branch, wasn't it meant to be indented less? But then, the else branch for RSA/DSA does not make sense..
why is it? it's an error to use mldsa cert in TLS 1.2 or earlier, or when the peer did use it, but we didn't advertise support for it... and no, it needs to be on the same level as the EdDSA branch...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @tomato42)
Thanks for the review! |
Add support for ML-DSA certificates
This change is