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

crypto/tls: cleanup handshake state #39406

Open
FiloSottile opened this issue Jun 4, 2020 · 8 comments · May be fixed by MabinGo/go#3
Open

crypto/tls: cleanup handshake state #39406

FiloSottile opened this issue Jun 4, 2020 · 8 comments · May be fixed by MabinGo/go#3
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@FiloSottile
Copy link
Contributor

We should refactor where and when the hs and Conn state is accessed and modified during the handshake. For example checkForResumption should probably be side-effect free.

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Jun 4, 2020
@FiloSottile FiloSottile added this to the Backlog milestone Jun 4, 2020
@FiloSottile FiloSottile self-assigned this Jun 4, 2020
@SparrowLii
Copy link
Contributor

SparrowLii commented Jul 25, 2020

I checked the checkForResumption() function and found that SessinState and hs.suite will be rewritten even if the function return false.

SparrowLii added a commit to SparrowLii/go that referenced this issue Jul 28, 2020
Fixes golang#39406
When use checkForResumption() function it could be side-effect sometime.
1. hs.sessionState will be changed and retained although the function return false.
2. hs.suite will be changed and retained when the statements below return false.
So  we should use a local variable, cilentSessionState, to replace hs.sessionState in the function. And move the set-suite statements down to avoid being changed too early.
@gopherbot
Copy link

Change https://golang.org/cl/245160 mentions this issue: crypto/tls: make checkForResumption side-effect free

SparrowLii added a commit to SparrowLii/go that referenced this issue Jul 31, 2020
Updates golang#39406

The process of checking server's cipher suites now is based on the logical relationship following: If the cipherSuite supports ECDHE,judge hs.ecdheOK ,then judge ecSignOK and rsaSignOk to check signer; if not supports, judge rsaOK. Then check if the suite needs tls1.2 version. This relationship is complicated, Fragile and hard to modify.Besides, it need 4 bool parameters in hs and a lot of "if...else" statements in cipherSuiteOk() function.

So we can use one parameter, cipherSuite, to replace those 4 in hs.Then we just need two bit operations in cipherSuiteOK() instead of many "if...else"s.What we need to do is completing the const and cipherSuites list in cipher_suites.go(They are partially omitted based on logical relationship, which is not plain at all.)
@gopherbot
Copy link

Change https://golang.org/cl/246038 mentions this issue: crypto/tls: simplify the process of cipher suites checking.

@gopherbot
Copy link

Change https://golang.org/cl/245837 mentions this issue: crypto/tls: supprtedVersions:return rightly for nil pointer parameter and plainer

@gopherbot
Copy link

Change https://golang.org/cl/246263 mentions this issue: crypto/tls: delete one useless judge statement.

@odeke-em
Copy link
Member

odeke-em commented Feb 5, 2021

@FiloSottile, a couple of CLs were mailed during Go1.16 -- thanks @SparrowLii. However, we didn't land any of them during Go1.16, and thus I shall punt this issue to Go1.17. Please feel free to change priorities though as you please. Also kindly cc-ing @katiehockman @rolandshoemaker.

@odeke-em odeke-em modified the milestones: Go1.16, Go1.17 Feb 5, 2021
@ianlancetaylor
Copy link
Contributor

Is this still going to be done during 1.17? Thanks.

@mknyszek
Copy link
Contributor

Since there was no response, I'm kicking this to the backlog to clean up the milestone. Please let me know if that's wrong. Thanks!

@mknyszek mknyszek modified the milestones: Go1.17, Backlog May 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
6 participants