Navigation Menu

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

crypto/tls: Conn.ConnectionState should set state.ServerName from c.serverName unconditionally - handshakeComplete is not necessary for SNI #15571

Closed
amalaviy opened this issue May 6, 2016 · 2 comments
Closed

crypto/tls: Conn.ConnectionState should set state.ServerName from c.serverName unconditionally - handshakeComplete is not necessary for SNI #15571

amalaviy opened this issue May 6, 2016 · 2 comments
Labels
FrozenDueToAge
Milestone
Go1.8Maybe

Comments

@amalaviy
Copy link
Contributor

amalaviy commented May 6, 2016

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?

go1.6.1

  1. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOBIN=""
    GOEXE=""
    GOHOSTARCH="amd64"
    GOHOSTOS="linux"
    GOOS="linux"
  2. What did you do?

Called c.ConnectionState() on a connection that failed Handshake() intentionally. c.serverName is set correctly, but ConnectionState() doesn't copy it over to state.ServerName due to unnecessary check for handshakeComplete. SNI is plaintext, it does not need to check for handshakeComplete - it is available even on failure and is purely a TLS clientHello field and is useful to have available for logging.

If possible, provide a recipe for reproducing the error.
A complete runnable program is good.
A link on play.golang.org is best.

  1. What did you expect to see?
    c.ConnectionState().ServerName to be filled with the SNI set by the tls client.
  2. What did you see instead?
    c.Connectionstate().ServerName == ""
@amalaviy amalaviy changed the title tls.ConnectionState() should set state.ServerName from c.serverName unconditionally - handshakeComplete is not necessary for SNI tls.Conn.ConnectionState() should set state.ServerName from c.serverName unconditionally - handshakeComplete is not necessary for SNI May 6, 2016
@bradfitz bradfitz changed the title tls.Conn.ConnectionState() should set state.ServerName from c.serverName unconditionally - handshakeComplete is not necessary for SNI crypto/tls: Conn.ConnectionState should set state.ServerName from c.serverName unconditionally - handshakeComplete is not necessary for SNI May 6, 2016
@bradfitz bradfitz added this to the Go1.8Maybe milestone May 6, 2016
@bradfitz
Copy link
Contributor

bradfitz commented May 6, 2016

For @agl to decide.

amalaviy added a commit to amalaviy/go that referenced this issue May 6, 2016
@amalaviy
crypto/tls: Conn.ConnectionState should set state.ServerName from c.s…
508797e 
…erverName unconditionally - handshakeComplete is not necessary for SNI (golang#15571)

Moves the state.ServerName assignment to outside the if statement that checks for handshakeComplete.
@gopherbot
Copy link

CL https://golang.org/cl/22862 mentions this issue.

@golang golang locked and limited conversation to collaborators Aug 17, 2017
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@amalaviy @agl
crypto/tls: set Conn.ConnectionState.ServerName unconditionally
4b4493f 
Moves the state.ServerName assignment to outside the if
statement that checks for handshakeComplete.

Fixes golang#15571

Change-Id: I6c4131ddb16389aed1c410a975f9aa3b52816965
Reviewed-on: https://go-review.googlesource.com/22862
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
FiloSottile pushed a commit to FiloSottile/go that referenced this issue Oct 12, 2018
@amalaviy @agl
crypto/tls: set Conn.ConnectionState.ServerName unconditionally
4b78482 
Moves the state.ServerName assignment to outside the if
statement that checks for handshakeComplete.

Fixes golang#15571

Change-Id: I6c4131ddb16389aed1c410a975f9aa3b52816965
Reviewed-on: https://go-review.googlesource.com/22862
Run-TryBot: Adam Langley <agl@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Adam Langley <agl@golang.org>
@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8Maybe
Development

No branches or pull requests
4 participants
@bradfitz @agl @gopherbot @amalaviy