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: Set{Read,Write}Deadline implementation is surprising/ineffective #13828

Open
rburchell opened this issue Jan 5, 2016 · 4 comments
Milestone

Comments

@rburchell
Copy link

Bear with me, this is a little long-winded, but I'm not sure how I can explain this more succinctly..

The current implementation of these functions is to pass them directly through to c.conn. Judging by my understanding of how TLS works, I think this is wrong. Picture a TLS listener socket which has just accepted a client (which won't ever complete a handshake, say, telnet or something).

Given:

type aClient struct {
    conn net.Conn
}

On accept of an incoming connection, an aClient instance is created (with conn populated from the TLS listener) & the server is instructed to time out the client after X seconds of not sending anything (through a goroutine, a Timer, and a channel to cancel the goroutine), something like this:

func authTimeout(client *aClient, controlChan chan int) {
    for {
        timer := time.NewTimer(time.Second * 5)
        select {
        case <-timer.C:
            client.Write("You didn't authenticate in time.")
            client.conn.Close()
            return
        case <-controlChan:
            return
        }
    }
}

client.Write's implementation looks something like this:

func (this *aClient) Write(format string, args ...interface{}) {
    // one would expect this to time out...
    this.conn.SetWriteDeadline(time.Now().Add(10 * time.Second))
    this.conn.Write(fmsg)
    // but if the client doesn't handshake, it'll hang!
}

authTimeout's timer will successfully fire, Write will be called, and set the Write deadline, and attempt to write. However, because the connection hasn't been read/written previously, it needs a handshake. And as handshaking requires reading from the underlying socket, this write deadline won't ever actually trigger.

This, to me, is incorrect/surprising. I'm not entirely sure what the behaviour of deadlines should be before handshake (maybe take whichever is the lowest? Maybe offer an explicit handshake deadline?) but I'm pretty sure there needs to be something done here even if that something is just a warning in the documentation. This bit me when blindly porting some code over from net/tcp to TLS.

It's worth noting, also, that net/http/transport.go tries to work around this very issue, but at least my own attempts to implement a similar pattern (handshake right after accepting, with a goroutine to time out the handshake and Close() on fail) seemed to suffer similar problems with Close() never returning unless I set both a ReadDeadline and WriteDeadline. I didn't (yet) analyze this problem, though.

@ianlancetaylor ianlancetaylor added this to the Unplanned milestone Jan 5, 2016
@rburchell
Copy link
Author

It looks like Close() hangs in my case because it also locks the handshake mutex. So I had one goroutine stuck in a handshake that would never progress (with the handshake mutex locked), and another tried to Close() - and blocked on the same mutex.

@bradfitz
Copy link
Contributor

Git rev 4ffba76 might help ("crypto/tls: don't block in Conn.Close if Writes are in-flight")

@rburchell
Copy link
Author

Thanks, yes, that looks like that would fix that particular problem. Then we're just left with the original report :)

@KenjiTakahashi
Copy link

I think that I've been hit by this (or related?) while implementing TLS in our Kafka cluster: IBM/sarama#1722

In a nutshell: When SetWriteDeadline is set on TLS connection, calling Close on it always results in write tcp [::1]:37390->[::1]:9092: i/o timeout. Without TLS, the same path works fine.

@rsc rsc unassigned agl Jun 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants