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: server should send empty server_name extension when using SNI #16072

Closed
magiconair opened this issue Jun 15, 2016 · 4 comments
Closed

crypto/tls: server should send empty server_name extension when using SNI #16072

magiconair opened this issue Jun 15, 2016 · 4 comments
Labels
FrozenDueToAge
Milestone
Go1.8

Comments

@magiconair
Copy link
Contributor

magiconair commented Jun 15, 2016 •
edited

Please answer these questions before submitting your issue. Thanks!

  1. What version of Go are you using (go version)?
go version go1.6.2 darwin/amd64
  1. What operating system and processor architecture are you using (go env)?
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/frschroeder/gopath"
GORACE=""
GOROOT="/Users/frschroeder/go1.6.2"
GOTOOLDIR="/Users/frschroeder/go1.6.2/pkg/tool/darwin_amd64"
GO15VENDOREXPERIMENT="1"
CC="clang"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fno-common"
CXX="clang++"
CGO_ENABLED="1"
  1. What did you do?

Sample app: https://play.golang.org/p/Tk9CR4BUyU

a) Save certPEM from the sample app into a file, e.g. cert.pem
b) Start WireShark and capture traffic on lo or lo0
c) Set a filter tcp.port == 4433
d) Goto Preferences -> Protocols -> SSL and add the cert.pem to the RSA key list for 127.0.0.1:4433 protocol tcp

Run the sample program from as follows:

1st terminal: go run main.go -server
2nd terminal: go run main.go

This will start a server in the first terminal and a client in the second terminal which executes a SNI handshake.

WireShark will capture and decrypt the traffic

  1. What did you expect to see?

A Server Hello with an empty server_name extension, e.g.

Secure Sockets Layer
    TLSv1.2 Record Layer: Handshake Protocol: Server Hello
        Content Type: Handshake (22)
        Version: TLS 1.2 (0x0303)
        Length: 53
        Handshake Protocol: Server Hello
            Handshake Type: Server Hello (2)
            Length: 49
            Version: TLS 1.2 (0x0303)
            Random
            Session ID Length: 0
            Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
            Compression Method: null (0)
            Extensions Length: 9
            Extension: server_name
                Type: server_name (0x0000)
                Length: 0
            Extension: renegotiation_info
                Type: renegotiation_info (0xff01)
                Length: 1
                Renegotiation Info extension
                    Renegotiation info extension length: 0
  1. What did you see instead?
Secure Sockets Layer
    TLSv1.2 Record Layer: Handshake Protocol: Server Hello
        Content Type: Handshake (22)
        Version: TLS 1.2 (0x0303)
        Length: 49
        Handshake Protocol: Server Hello
            Handshake Type: Server Hello (2)
            Length: 45
            Version: TLS 1.2 (0x0303)
            Random
            Session ID Length: 0
            Cipher Suite: TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (0xc02f)
            Compression Method: null (0)
            Extensions Length: 5
            Extension: renegotiation_info
                Type: renegotiation_info (0xff01)
                Length: 1
                Renegotiation Info extension
                    Renegotiation info extension length: 0

The extension section of the ServerHello does not contain a server_name extension.

When you make a request with an SNI header the client sends a client hello with a server name extension. RFC 6606 Section 3 states:

   A server that receives a client hello containing the "server_name"
   extension MAY use the information contained in the extension to guide
   its selection of an appropriate certificate to return to the client,
   and/or other aspects of security policy.  In this event, the server
   SHALL include an extension of type "server_name" in the (extended)
   server hello.  The "extension_data" field of this extension SHALL be
   empty.

We found this when debugging a TLS connection from the AWS API GW to a go server instance.

I am not sure whether this is a real issue or something that can be ignored. In any case it seems to be mentioned in the RFC and I couldn't find any mention in the issues. So even if it doesn't get addressed people can still find out why.

I realize that you prefer discussion first over code but the following patch added the server_name extension:

diff --git a/src/crypto/tls/handshake_messages.go b/src/crypto/tls/handshake_messages.go
index 111ce53..34f8eaa 100644
--- a/src/crypto/tls/handshake_messages.go
+++ b/src/crypto/tls/handshake_messages.go
@@ -490,6 +490,7 @@ type serverHelloMsg struct {
    cipherSuite         uint16
    compressionMethod   uint8
    nextProtoNeg        bool
+   serverName          bool
    nextProtos          []string
    ocspStapling        bool
    scts                [][]byte
@@ -545,6 +546,9 @@ func (m *serverHelloMsg) marshal() []byte {
        nextProtoLen += len(m.nextProtos)
        extensionsLength += nextProtoLen
    }
+   if m.serverName {
+       numExtensions++
+   }
    if m.ocspStapling {
        numExtensions++
    }
@@ -614,6 +618,11 @@ func (m *serverHelloMsg) marshal() []byte {
            z = z[1+l:]
        }
    }
+   if m.serverName {
+       z[0] = byte(extensionServerName >> 8)
+       z[1] = byte(extensionServerName & 0xff)
+       z = z[4:]
+   }
    if m.ocspStapling {
        z[0] = byte(extensionStatusRequest >> 8)
        z[1] = byte(extensionStatusRequest)
diff --git a/src/crypto/tls/handshake_server.go b/src/crypto/tls/handshake_server.go
index e16cddc..66a9c02 100644
--- a/src/crypto/tls/handshake_server.go
+++ b/src/crypto/tls/handshake_server.go
@@ -122,6 +122,7 @@ func (hs *serverHandshakeState) readClientHello() (isResume bool, err error) {
    c.haveVers = true

    hs.hello = new(serverHelloMsg)
+   hs.hello.serverName = len(hs.clientHello.serverName) > 0

    supportedCurve := false
    preferredCurves := config.curvePreferences()
@ianlancetaylor ianlancetaylor added this to the Go1.8 milestone Jun 15, 2016
@ianlancetaylor
Copy link
Contributor

CC @agl

@agl agl self-assigned this Jun 15, 2016
@agl
Copy link
Contributor

agl commented Jun 15, 2016

I've never understood why the RFC says that and crypto/tls may not always know whether a callback used the SNI name or not. So the only reasonable answers are to never echo the extension or to always echo it. We currently do the former. Your change would lead to the latter. I welcome any suggestions about advantages!

@magiconair
Copy link
Contributor Author

Do other implementations depend on that behavior because it is in the RFC? I realize that the RFC says SHALL and not MUST.

@agl
Copy link
Contributor

agl commented Aug 17, 2016

There's no reported problems arising from this I believe. OpenSSL, for what it's worth, allows a callback to decide whether to echo this. Since we don't have that and there's no concrete motivation, I don't see impetus to change our behaviour here.

@agl agl closed this as completed Aug 17, 2016
@golang golang locked and limited conversation to collaborators Aug 17, 2017
@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.8
Development

No branches or pull requests
4 participants
@agl @magiconair @ianlancetaylor @gopherbot