[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #29206 [Circumvention/Snowflake]: New design for client -- proxy protocol for Snowflake
#29206: New design for client -- proxy protocol for Snowflake
-------------------------------------------------+-------------------------
Reporter: cohosh | Owner: cohosh
Type: task | Status:
| needs_review
Priority: Medium | Milestone:
Component: Circumvention/Snowflake | Version:
Severity: Normal | Resolution:
Keywords: ex-sponsor-19, anti-censorship- | Actual Points:
roadmap |
Parent ID: | Points: 6
Reviewer: dcf | Sponsor:
| Sponsor28-must
-------------------------------------------------+-------------------------
Comment (by dcf):
Replying to [comment:13 cohosh]:
> I'm going to ask for an incremental review on this one, mostly just to
get another pair of eyes on what I've done and my next steps before I sink
more time into going possibly the wrong direction:
https://github.com/cohosh/snowflake/compare/sequencing
High-level summary: splits the data stream into frames. Each frame is
preceded by a 12-byte header consisting of seq, ack, and length fields.
The receiver buffers an entire frame in memory, then accepts it if its
sequence number exactly matches the expected sequence number, or drops it
otherwise.
Overall the design looks good.
{{{
type snowflakeHeader struct {
seq int
ack int
length int //length of the accompanying data (including header
length)
}
}}}
The fields would be better as `uint32`, because the size of `int`
[https://golang.org/ref/spec#Numeric_types may vary] (though I think you
handled it correctly by always specifying 32 bits in serialization
functions). `length` may be better as `uint16`; that's plenty for one call
to `Write` and it avoids the risk that an implementation will try to
buffer 4 GB in memory.
IMO `length` should be ''exclusive'' of the header size. Otherwise you
have to define what it means if `length` < 12. (I believe the current code
doesn't work in this case because `header.length - snowflakeHeaderLen`
underflows.) The packets sent by `sendAck` should have `length` = 0.
{{{
h.seq = int(binary.LittleEndian.Uint32(b[0:4]))
h.ack = int(binary.LittleEndian.Uint32(b[4:8]))
h.length = int(binary.LittleEndian.Uint32(b[8:12]))
}}}
Big-endian is more usual for network protocols.
I'm not sure about the implementation of `Read`:
* Should return `0` here, not `length`. `b` hasn't really been filled in
with any de-serialized data yet.
{{{
length, err := s.Conn.Read(b)
if err != nil {
return length, err
}
}}}
* This should have `b[:length]` in place of `b`:
{{{
s.buffer = append(s.buffer, b...)
}}}
* This order of operations (`Read` then `copy`) will lose any data
remaining in `s.out` at EOF. It would be better to do the `copy` first,
and only when `s.out` is empty do a `Read` and possibly return an error.
{{{
length, err := s.Conn.Read(b)
if err != nil {
return length, err
}
s.buffer = append(s.buffer, b...)
n := copy(b, s.out)
s.out = s.out[n:]
if n == len(b) {
return n, err
}
}}}
I would prefer to start with an implementation of `Read` that is simpler,
if less efficient. I am picturing something like this (untested):
{{{
func NewSnowflakeReadWriter(sConn io.ReadWriteCloser) *SnowflakeReadWriter
{
pr, pw := io.Pipe()
s := &SnowflakeReadWriter{
Conn: sConn,
pr: pr,
}
go s.readLoop(pw)
return s
}
func (s *SnowflakeReadWriter) readLoop(pw io.PipeWriter) {
var err error
for err == nil {
// strip headers and write data into the pipe
var header snowflakeHeader
err = readHeader(s.Conn, &header)
if err != nil {
break
}
if header.seq == s.ack {
_, err = io.CopyN(pw, s.Conn,
int64(header.length))
} else {
_, err = io.CopyN(ioutil.Discard, s.Conn,
int64(header.length))
}
}
pw.CloseWithError(err)
}
func (s *SnowflakeReadWriter) Read(b []byte) (int, error) {
// read de-headered data from the pipe
return s.pr.Read(b)
}
}}}
> * Implement a timeout (maybe making the ReadWriteCloser a net.Conn and
implementing SetReadDeadline and a fixed size window to stop sending
packets before others have been acknowledged)
I agree that ultimately `SnowflakeReadWriter` should be a `net.Conn`,
though I don't see why the read deadline should affect the sending of
packets.
> * Make the tests better. Right now I'm just reading and writing from a
buffer, we should test with network connections
[https://golang.org/pkg/net/#Pipe net.Pipe] can be good for this.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/29206#comment:17>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs