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: 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
Assignees
Milestone

Comments

@amalaviy
Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented May 6, 2016

For @agl to decide.

amalaviy added a commit to amalaviy/go that referenced this issue May 6, 2016
…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

This comment has been minimized.

Copy link

@gopherbot gopherbot commented May 6, 2016

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

@gopherbot gopherbot closed this in ebcd179 Aug 17, 2016
@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
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
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>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.