TCP+SNI support arbitrary large Client Hello

[DanSipola]

Use the TLS data to calculate the length of the Client Hello
message so that arbitrary large messages are supported. Previoulsy
messages larger than 1024 bytes failed due to a fixed size buffer.
Note that a Client Hello message must still fit in one TLS record.

The code for parsing the TLS data has been moved around
a bit to ease unit testing.

This is my first attempt using golang so please be aware of that! I don't think
that the performance impact should be significant. I may even improve the
memory footprint per connection since you often don't need as much as 1024 bytes for the
client hello.

[magiconair]

Yeah, I've punted on extracting the buffer size since why would the ClientHello ever be bigger than the 350 bytes of my test case? ;)

In general, this looks solid and I've added some comments on what to refactor. Thanks for taking this on.

[magiconair]

I think I'd prefer if this function would only determine the size of the buffer and not allocate it. Also, keep it in the tls_clienthello.go since this is where the parsing logic for the TLS ClientHello header is. Also, I'd keep data instead of tlsData since there is no difference.

[magiconair]

Please add a comment where the 5: comes from (magic number).

[magiconair]

I really like these test cases. Can you move them to the tls_clienthello_test.go since they test the parsing.

When you refactor the method to just return the size of the buffer you can test for that as well. My suggestion is:

tests := []struct {
    name string
    data []byte
    fail bool
    len int
}{
    {
        name: "bad handshake length",
        data: ...,
        fail: true,
    },
    ...
}

for _, tt := range tests {
    t.Run(tt.name, func(t *testing.T) {
        n, err := clientHelloBufferSize(tt.data)
        if tt.fail && err == nil {
            t.Fatal("should fail")
        }
        if got, want := n, tt.len; !tt.fail && got != want {
            t.Fatalf("got buffer size %d want %d", got, want)
        }
    }
}

Also I

[magiconair]

You can then probably roll that test case into the previous table driven test.

[magiconair]

Can you check whether you can also write a table driven test like the one I've outlined above?

[DanSipola]

Thanks for the review! I will have a look at it and get it fixed.

[CLAassistant]

All committers have signed the CLA.

[DanSipola]

All valid comments. Thanks for that! I hope that I got it right. I'm not used to the github flow so let me know if I should change something.

[magiconair]

Thanks!