3 ms·

Interesting, though AFAIK a secure tunnel is only useful for a net.Conn, not an io.ReadWriter. What's the usecase for a "secure tunnel" over a bytes.Buffer? bt

Interesting, though AFAIK a secure tunnel is only useful for a net.Conn, not an io.ReadWriter. What's the usecase for a "secure tunnel" over a bytes.Buffer? btw, I noticed that the decrypt function reads a 32-bit message length and immediately allocates a slice of that size. That means an attacker can send 0xFFFFFFFF and cause you to allocate 4GiB.

ikiris 4y ago
Yeah this code review isn’t gonna go great.
jerf 4y ago
You're kinda looking at the interface issue backwards. Being an io.ReadWriter is a promise that the tunnel code won't do anything other than read or write. The tunnel code shouldn't be getting local and remote addresses or setting deadlines of its own, or most of the rest of what net.Conn can do. https://pkg.go.dev/net#Conn https://pkg.go.dev/net#Conn It is good that it doesn't take more than it needs. That bytes.Buffer happens to implement io.ReadWriter is not of consequence; for any given task you want a io.Reader or io.Writer for there may be any number of specific implementations that don't make sense to use with it, but that doesn't mean you should require more methods of the interface. I'm speaking solely about the interface here; the many other concerns are valid.
- nemo1618 4y ago
  Sure, in general you want to use the narrowest possible interface. But in this particular case, I think `net.Conn` communicates intent better than `io.ReadWriter` -- the former implies that two distinct parties are involved, whereas `io.ReadWriter` is usually for (single-user) buffers or files. Sometimes it's sensible to use a larger interface than necessary; e.g. if you have a helper function that only calls `SetDeadline` and `Read`, the argument should be a `net.Conn`, not a custom interface with just those methods. tbh though, it's a very minor quibble and not really worth worrying about ‾\_(ツ)_/‾
  - jrockway 4y ago
    I disagree with this. If the only surface area of the underlying transport that the implementation uses is Read() and Write(), then it should be an io.Reader and an io.Writer. If it wants to mess around with read/write deadlines, then you'd have to bring in net.Conn, but it doesn't, so don't. If you're worried about someone using bytes.Buffer as their transport layer, net.Conn doesn't fix that; there is net.Pipe. (It's not buffered, though.)

ikiris 4y ago

Yeah this code review isn’t gonna go great.

jerf 4y ago

You're kinda looking at the interface issue backwards. Being an io.ReadWriter is a promise that the tunnel code won't do anything other than read or write. The tunnel code shouldn't be getting local and remote addresses or setting deadlines of its own, or most of the rest of what net.Conn can do. https://pkg.go.dev/net#Conn https://pkg.go.dev/net#Conn It is good that it doesn't take more than it needs. That bytes.Buffer happens to implement io.ReadWriter is not of consequence; for any given task you want a io.Reader or io.Writer for there may be any number of specific implementations that don't make sense to use with it, but that doesn't mean you should require more methods of the interface. I'm speaking solely about the interface here; the many other concerns are valid.

nemo1618 4y ago
Sure, in general you want to use the narrowest possible interface. But in this particular case, I think `net.Conn` communicates intent better than `io.ReadWriter` -- the former implies that two distinct parties are involved, whereas `io.ReadWriter` is usually for (single-user) buffers or files. Sometimes it's sensible to use a larger interface than necessary; e.g. if you have a helper function that only calls `SetDeadline` and `Read`, the argument should be a `net.Conn`, not a custom interface with just those methods. tbh though, it's a very minor quibble and not really worth worrying about ‾\_(ツ)_/‾
- jrockway 4y ago
  I disagree with this. If the only surface area of the underlying transport that the implementation uses is Read() and Write(), then it should be an io.Reader and an io.Writer. If it wants to mess around with read/write deadlines, then you'd have to bring in net.Conn, but it doesn't, so don't. If you're worried about someone using bytes.Buffer as their transport layer, net.Conn doesn't fix that; there is net.Pipe. (It's not buffered, though.)