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

tls: add eddilithium2 support and fix eddilithium3 #176

Merged
merged 1 commit into from
Jul 3, 2024
Merged

Conversation

bwesterb
Copy link
Member

@bwesterb bwesterb commented Jul 3, 2024

Closes #175

We didn't move from eddilithium3 to eddilithium2 when dilithium3 was renamed to dilithium3.

@bwesterb bwesterb requested review from Lekensteyn and cjpatton July 3, 2024 11:46
Copy link

@thibmeu thibmeu left a comment

Choose a reason for hiding this comment

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

looks good. minor comments

src/crypto/tls/tls_cf.go Show resolved Hide resolved
Comment on lines +283 to +284
EdDilithium2: "Ed25519-Dilithium2",
EdDilithium3: "Ed448-Dilithium3",
Copy link

Choose a reason for hiding this comment

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

nits: should these be named differently, like Ed25519Dilithium2 and Ed448Dilithium3?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to break current users. Let's reconsider naming when we add Ed25519-ML-DSA-44.

Copy link

Choose a reason for hiding this comment

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

is it worth creating aliases which are more precise?

Closes #175

We didn't move from eddilithium3 to eddilithium2 when dilithium3
was renamed to dilithium3.
@bwesterb bwesterb merged commit a920985 into cf Jul 3, 2024
2 checks passed
Lekensteyn added a commit that referenced this pull request Jul 3, 2024
To avoid having to regenerate all testdata files, add an option to
control whether PQ signature algorithms are advertised. Tests were added
for the client side.

Since Go 1.19, FIPS-only mode must remain disabled to enable PQ sigalgs.

 [pwu: Go 1.17: moved parsePublicKey changes from x509/x509.go to x509/parser.go]
 [pwu: Go 1.22.5: add eddilithium2 support, fix eddilithium3, by Bas in #176]

Co-authored-by: Christopher Patton <[email protected]>
Co-authored-by: Peter Wu <[email protected]>
dfunkt pushed a commit to dfunkt/go that referenced this pull request Aug 9, 2024
To avoid having to regenerate all testdata files, add an option to
control whether PQ signature algorithms are advertised. Tests were added
for the client side.

Since Go 1.19, FIPS-only mode must remain disabled to enable PQ sigalgs.

 [pwu: Go 1.17: moved parsePublicKey changes from x509/x509.go to x509/parser.go]
 [pwu: Go 1.22.5: add eddilithium2 support, fix eddilithium3, by Bas in cloudflare#176]

Co-authored-by: Christopher Patton <[email protected]>
Co-authored-by: Peter Wu <[email protected]>
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.

Incorrect PublicKeyAlgorithm.String() of x509.EdDilithium3 in parsed certificate
3 participants