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

Always select matching CipherSuite #308

Closed

Conversation

CarsonHoffman
Copy link
Contributor

@CarsonHoffman CarsonHoffman commented Nov 8, 2020

Description

Currently, the server checks to see if any of the client's cipher suites match any of its own, but always selects the first that the client offered. This can lead to a situation where the server selects a cipher suite it does not support; for example, if the client offers suites [A, B] and the server supports only [B], the server will see the match of B, but will then select A, which it does not support.

This PR makes the server always select a matching cipher suite, and adds a test to confirm this functionality; HEAD currently fails the test.

Make sure that the server selects a cipher suite that it
supports, rather than the first that the client presents.
Add Carson Hoffman to the contributors list
@codecov
Copy link

codecov bot commented Nov 8, 2020

Codecov Report

Merging #308 (3d758aa) into master (d55b64a) will increase coverage by 2.53%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #308      +/-   ##
==========================================
+ Coverage   71.08%   73.62%   +2.53%     
==========================================
  Files          77       77              
  Lines        3534     3533       -1     
==========================================
+ Hits         2512     2601      +89     
+ Misses        756      654     -102     
- Partials      266      278      +12     
Flag Coverage Δ
go 73.64% <100.00%> (+2.53%) ⬆️
wasm 72.76% <100.00%> (+2.65%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
flight0handler.go 74.46% <100.00%> (-0.54%) ⬇️
internal/net/dpipe/dpipe.go 91.89% <0.00%> (-2.71%) ⬇️
prf.go 57.28% <0.00%> (+8.73%) ⬆️
util.go 89.13% <0.00%> (+39.13%) ⬆️
crypto_cbc.go 68.96% <0.00%> (+68.96%) ⬆️
...pher_suite_tls_ecdhe_ecdsa_with_aes_256_cbc_sha.go 80.00% <0.00%> (+71.42%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d55b64a...3d758aa. Read the comment docs.

Copy link
Member

@daenney daenney left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this 🤦

@Sean-Der
Copy link
Member

Sean-Der commented Nov 8, 2020

Yea amazing first PR :D

@Sean-Der
Copy link
Member

Sean-Der commented Nov 8, 2020

Merged with ca3d982 just squashed into one commit.

Amazing work @CarsonHoffman and thanks for the quick review @daenney :)

@Sean-Der Sean-Der closed this Nov 8, 2020
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.

3 participants