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: Expose "http: server gave HTTP response to HTTPS client" error #44855

Open
AkihiroSuda opened this issue Mar 8, 2021 · 5 comments
Open
Milestone

Comments

@AkihiroSuda
Copy link

@AkihiroSuda AkihiroSuda commented Mar 8, 2021

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

$ go version
go version go1.16 linux/amd64

Does this issue reproduce with the latest release?

Yes

What did you expect to see?

I'd like to see the http: server gave HTTP response to HTTPS client error in the net/http package to be exposed as a constant that can be compared with errors.Is

err = errors.New("http: server gave HTTP response to HTTPS client")

Context

I'm working on a clone of Docker called nerdctl (contaiNERD ctl).
I want to compare http: server gave HTTP response to HTTPS client with errors.Is, for ease of reimplementing dockerd --insecure-registries, which attempts HTTPS first and then falls back to HTTP.

What did you see instead?

The http: server gave HTTP response to HTTPS client error is not exposed.

@toothrot toothrot added this to the Backlog milestone Mar 8, 2021
@toothrot
Copy link
Contributor

@toothrot toothrot commented Mar 8, 2021

@neild
Copy link
Contributor

@neild neild commented Mar 8, 2021

This seems reasonable to me, especially given that this error is replacing a more-informative (to programs) tls.RecordHeaderError.

Perhaps the error returned here should wrap the tls.RecordHeaderError rather than replacing it.

@neild
Copy link
Contributor

@neild neild commented Mar 9, 2021

Three concrete possibilities that I can think of:

  1. Expose an ErrHTTPResponse var:

    ErrHTTPResponse = errors.New("http: server gave HTTP response to HTTPS client")

    [+] Simple.
    [-] Inconsistently returns ErrHTTPResponse or tls.RecordHeaderError depending on the response.

  2. Don't expose a var, but make the error wrap the underlying tls.RecordHeaderError. In the case where someone wants to detect this condition, they can use errors.As to recover the underlying error and examine it:

    var tlsErr tls.RecordHeaderError
    if errors.As(err, &tlsErr) && string(tlsErr.RecordHeader[:]) == "HTTP/" {
        // ...
    }

    [+] Consistently return the underlying tls.RecordHeaderError.
    [-] Detecting the error condition is more work for the caller.
    [+] Contract with the caller is clear: We provide the tls.RecordHeaderError and they can decide how to interpret it.

  3. Both: Return an error which errors.Is equivalent to an ErrHTTPResponse sentinel and which also wraps tls.RecordHeaderError.
    [-] Lot of mechanism for a fairly obscure case.

Option (2) requires the least change to the http API surface, but (1) is the simplest. Either of those seems reasonable to me.

@AkihiroSuda
Copy link
Author

@AkihiroSuda AkihiroSuda commented Mar 11, 2021

Can we wrap (2) in a function like func IsNotHTTPS(err error) bool?

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 12, 2021

I'm fine with (2). Or (1) if we name it, say, ErrSchemeMismatch ("HTTP" in "ErrHTTPResponse" sounds too generic, as "HTTP" often also means "HTTPS").

I wouldn't want to see a new function. That's too heavy in godoc for how fringe this is.

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.

None yet
4 participants