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 · 5 comments · May be fixed by MabinGo/go#3
Open

crypto/tls: cleanup handshake state #39406

FiloSottile opened this issue Jun 4, 2020 · 5 comments · May be fixed by MabinGo/go#3
Assignees
Labels
Milestone

Comments

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Jun 4, 2020

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 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 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

@gopherbot gopherbot commented Jul 28, 2020

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

@gopherbot gopherbot commented Jul 31, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Jul 31, 2020

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

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 3, 2020

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

@FiloSottile FiloSottile modified the milestones: Backlog, Go1.16 Oct 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.