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

Jump to bottom

Fix encodeCipherSuites ordering #200

Merged
merged 2 commits into from Feb 22, 2020
Merged

Fix encodeCipherSuites ordering #200

merged 2 commits into from Feb 22, 2020

Conversation

Reviewers

@at-wat at-wat

@cohosh cohosh

@daenney daenney

@igolaizola igolaizola

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
@Sean-Der @at-wat
@Sean-Der
Copy link
Member

@Sean-Der Sean-Der commented Feb 21, 2020

Before encodeCipherSuites would improperly encode backwards, putting
our least preferred suite first. Fix this function so it properly encodes
and update TestHandshakeMessageClientHello so it has multiple CipherSuites
to make sure this doesn't regress.

Relates to #199
@Sean-Der
Fix encodeCipherSuites ordering
77adfe1 
Before encodeCipherSuites would improperly encode
backwards, putting our least preferred suite first. Fix
this function so it properly encodes and update
TestHandshakeMessageClientHello so it has multiple CipherSuites
to make sure this doesn't regress.

Relates to #199
at-wat
at-wat approved these changes Feb 22, 2020
View changes
Copy link
Member

@at-wat at-wat left a comment

LGTM

cipher_suite.go
@@ -115,10 +115,10 @@ func cipherSuiteForID(id CipherSuiteID) cipherSuite {
// CipherSuites we support in order of preference
func defaultCipherSuites() []cipherSuite {
return []cipherSuite{
&cipherSuiteTLSEcdheRsaWithAes256CbcSha{},
&cipherSuiteTLSEcdheEcdsaWithAes256CbcSha{},
&cipherSuiteTLSEcdheRsaWithAes128GcmSha256{},
&cipherSuiteTLSEcdheEcdsaWithAes128GcmSha256{},
Copy link
Member

@at-wat at-wat Feb 22, 2020

#199 (comment)
In Chromium, ECDSA is prior to RSA?

Copy link
Member Author

@Sean-Der Sean-Der Feb 22, 2020

👍 will put ECDSA first.

@Sean-Der
Prefer GCM over CBC suites
1b73271 
Match OpenSSL (and Chromium's) ordering of CipherSuites.
Go also has a hardware accelerated implementation, so this should
be a better experience in almost all cases.

Relates to #199
@Sean-Der Sean-Der merged commit 147774b into master Feb 22, 2020
1 check passed
@daenney daenney deleted the issue-199 branch Feb 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment