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

proposal: crypto/tls: add Conn.PeekHandshake #24401

Closed
bradfitz opened this issue Mar 15, 2018 · 2 comments
Closed

proposal: crypto/tls: add Conn.PeekHandshake #24401

bradfitz opened this issue Mar 15, 2018 · 2 comments

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Mar 15, 2018

I was looking at ways to fix #23689 (making net/http respond nicely to clear-text HTTP clients sending requests to HTTPS servers) and of the many possible ways to fix it, most are gross.

You would think that https://golang.org/pkg/crypto/tls/#RecordHeaderError as returned by https://golang.org/pkg/crypto/tls/#Conn.Handshake would be sufficient, but Handshake sends a record alert error to the client before returning, so the connection already has binary gibberish written on it (as far as the HTTP/1.x plaintext client is concerned).

The options I didn't like:

  • I don't really want to make crypto/tls aware of HTTP semantics.
  • I don't want to add a HandshakeOptions
  • I don't want to add a hook to Conn or Handshake to alter alert-sending behavior based on the content
  • Writing an alternate net.Listener or net.Conn wrapper type doesn't help, because net/http accepts any ol' net.Listener and then type-checks the responses for *tls.Conns. (please avoid talking about making that an interface in this bug)
  • exposing tls.Conn's underlying Conn, even with a bunch of documented warnings.

The only thing non-offensive way I can see to fix this is to add:

package tls

// PeekHandshake can be called before a handshake to return some number of bytes from the
// the underlying net.Conn, before the handshake is processed. The bytes should not be mutated
// or retained, and PeekHandshake should not be called concurrently with any other operations.
func (c *Conn) PeekHandshake() ([]byte, error) { ... }

Then net/http can call PeekHandshake before its Handshake and see if it starts with GET or HEAD or POST, etc, and return a nice 400 message.

Open to naming suggestions.

/cc @FiloSottile @agl

@bradfitz bradfitz added the Proposal label Mar 15, 2018
@gopherbot gopherbot added this to the Proposal milestone Mar 15, 2018
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Mar 15, 2018

exposing tls.Conn's underlying Conn, even with a bunch of documented warnings.

I'm not sure I understand, if you don't have the underlying Conn, how are you going to send the 400 response?

Writing an alternate net.Listener or net.Conn wrapper type doesn't help, because net/http accepts any ol' net.Listener and then type-checks the responses for *tls.Conns. (please avoid talking about making that an interface in this bug)

(Linking to #21753 as the place to have that discussion.)

@bradfitz

This comment has been minimized.

Copy link
Contributor Author

@bradfitz bradfitz commented Mar 19, 2018

I'm not sure I understand, if you don't have the underlying Conn, how are you going to send the 400 response?

Oh, duh, right.

Okay, closing this.

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
3 participants
You can’t perform that action at this time.