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

net/http: HTTPS requests when the server does not support it -> record overflow #11111

Closed
eidge opened this issue Jun 7, 2015 · 8 comments

Comments

Projects
None yet
5 participants
@eidge
Copy link

commented Jun 7, 2015

Making an HTTPS request against a server that has no TLS enabled returns an 'record overflow' error.

I wonder if the message couldn't be a bit more helpful, like: 'No valid TLS certificate found. Aborting request.'

The sample program below illustrates the problem.

package main

import "net"
import "net/http"
import "net/http/httptest"
import "fmt"

func main() {
    server := httptest.NewUnstartedServer(
        http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
            fmt.Fprintf(w, "it works")
        }))

    server.Listener, _ = net.Listen("tcp", ":8080")
    server.Start()
    defer server.Close()

    req, err := http.NewRequest("GET", "https://localhost:8080", nil)
    if err != nil {
        panic(err.Error())
    }
    _, err = http.DefaultClient.Do(req)
    if err != nil {
        panic(err.Error()) // Will throw 'record overflow' here, works fine if one requests http instead of https though.
    }
}

@ianlancetaylor ianlancetaylor added this to the Go1.6 milestone Jun 7, 2015

@minux

This comment has been minimized.

Copy link
Member

commented Jun 8, 2015

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 8, 2015

I mean, we could do something here and record the first few bytes before giving it to crypto/tls. And then when crypto/tls gives us an error, we could look at the bytes we copied and see if it looks like "HTTP/*" and return a better error.

Pretty low priority, but a fine feature request.

@minux

This comment has been minimized.

Copy link
Member

commented Jun 8, 2015

@eidge

This comment has been minimized.

Copy link
Author

commented Jun 8, 2015

Yes that would be a lot more helpful.

I lost a good half-an-hour trying to pin point the problem. @minux suggestion would send me immediately in the right direction.

Thanks

@bradfitz

This comment has been minimized.

Copy link
Member

commented Oct 17, 2015

Turns out this is pretty easy to propagate. crypto/tls returns an untyped fmt.Errorf error, so we can make it a real type with the data. The data is easily accessible.

diff --git a/src/crypto/tls/conn.go b/src/crypto/tls/conn.go 
index e3dcf15..2a41beb 100644
--- a/src/crypto/tls/conn.go
+++ b/src/crypto/tls/conn.go
@@ -568,7 +568,7 @@ Again:
        }
        if n > maxCiphertext { 
                c.sendAlert(alertRecordOverflow) 
-               return c.in.setErrorLocked(fmt.Errorf("tls: oversized record received with length %d", n)) 
+               return c.in.setErrorLocked(fmt.Errorf("tls: oversized record received with length %d ; data=%q", n, b.data)) 
        }
        if !c.haveVers {
                // First message, be extra suspicious: this might not be a TLS

Makes the demo program above error out with the "HTTP/1.1 400 Bad Request\r\nContent-Type: text/plain\r\nConnection: close\r\n\r\n400 Bad Request"

@gopherbot

This comment has been minimized.

Copy link

commented Oct 19, 2015

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

@gopherbot

This comment has been minimized.

Copy link

commented Oct 20, 2015

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

@gopherbot

This comment has been minimized.

Copy link

commented Oct 20, 2015

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

agl added a commit that referenced this issue Nov 16, 2015

crypto/tls: return a typed error on invalid record headers
The user can inspect the record data to detect that the other side is
not using the TLS protocol.

This will be used by the net/http client (in a follow-on CL) to detect
when an HTTPS client is speaking to an HTTP server.

Updates #11111.

Change-Id: I872f78717aa8e8e98cebd8075436209a52039a73
Reviewed-on: https://go-review.googlesource.com/16078
Reviewed-by: Adam Langley <agl@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>

@bradfitz bradfitz closed this in babdb38 Nov 17, 2015

@golang golang locked and limited conversation to collaborators Nov 16, 2016

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.