Opened 7 months ago

Last modified 6 days ago

#29206 needs_revision task

New design for client -- server protocol for Snowflake

Reported by: cohosh Owned by: cohosh
Priority: Medium Milestone:
Component: Circumvention/Snowflake Version:
Severity: Normal Keywords: anti-censorship-roadmap-september
Cc: cohosh, phw, dcf, arlolra Actual Points:
Parent ID: Points: 6
Reviewer: dcf Sponsor: Sponsor28-must

Description (last modified by cohosh)

Right now we just send traffic. We could add some control messages or another layer for reliability/sequencing.

Child Tickets

Change History (22)

comment:1 Changed 7 months ago by cohosh

Cc: cohosh added
Component: - Select a componentObfuscation/Obfsproxy
Owner: set to asn

comment:2 Changed 7 months ago by cohosh

Component: Obfuscation/ObfsproxyObfuscation/Snowflake

comment:3 Changed 7 months ago by cohosh

Priority: Very HighMedium
Sponsor: Sponsor19
Summary: New design for client -- proxy protocolNew design for client -- proxy protocol for Snowflake

comment:4 Changed 7 months ago by teor

Owner: asn deleted
Status: newassigned

asn does not need to own any obfuscation tickets any more. Default owners are trouble.

comment:5 Changed 7 months ago by gaba

Points: 6

comment:6 Changed 6 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 added

comment:7 Changed 6 months ago by gaba

cohosh, are you working on this ticket?

comment:8 Changed 6 months ago by cohosh

Owner: set to cohosh

At moment, we're focusing on the proxy--broker protocol, but it is somewhat related. I'll assign it to myself since we'll cover it together with #29207

comment:9 Changed 4 months ago by gaba

Keywords: network-team-roadmap-2019-Q1Q2 removed

comment:10 Changed 3 months ago by cohosh

Cc: phw dcf arlolra added

Here's a first stab at a very simple sequencing layer for snowflake: https://github.com/cohosh/snowflake/compare/sequencing

There is still a lot to do such as:

  • send acknowledement packets when data has been received
  • implement a timeout feature (which I will probably do by making SnowflakeReadWriter an actual net.Conn.
  • resend data chunks that have not been acknowledged
  • implement a fixed sized window to avoid sending too much data

But I thought I'd put this out there sooner to get feedback since this is a big change

comment:11 Changed 3 months ago by gaba

Keywords: ex-sponsor-19 added

Adding the keyword to mark everything that didn't fit into the time for sponsor 19.

comment:12 Changed 3 months ago by phw

Sponsor: Sponsor19Sponsor28-must

Moving from Sponsor 19 to Sponsor 28.

comment:13 Changed 3 months ago by cohosh

Status: assignedneeds_review

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

What I've done:

  • Added a ReadWriteCloser that takes data to be sent and adds a header before passing it on to the Write function of the webrtc connection and removes the header in a Read on the other side. This is implemented as a common package because it's used by both the client and server
  • Wrote some tests for the package
  • Send and acknowledgement packet at every read: https://github.com/cohosh/snowflake/commit/a7191b6b5ea4c9e58709c03fefc2dcd07571dc0f

Next steps:

  • 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)
  • Make it backwards compatable. I'm thinking of doing this by having an extra field in the header that should be set to all zeros. If it's not zeros, we just forward the data as is. If it is zeros, we extract the header.
  • Make the tests better. Right now I'm just reading and writing from a buffer, we should test with network connections

comment:14 Changed 2 months ago by gaba

Reviewer: dcf

comment:15 Changed 2 months ago by gaba

Keywords: anti-censorship-roadmap added

comment:16 in reply to:  13 Changed 2 months ago by cohosh

Replying to cohosh:

  • Make it backwards compatable. I'm thinking of doing this by having an extra field in the header that should be set to all zeros. If it's not zeros, we just forward the data as is. If it is zeros, we extract the header.

Actually another way of doing backwards compatability is to make a new handler for the new version.

Version 0, edited 2 months ago by cohosh (next)

comment:17 in reply to:  13 ; Changed 2 months ago by dcf

Replying to 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 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

net.Pipe can be good for this.

comment:18 Changed 2 months ago by cohosh

Description: modified (diff)
Status: needs_reviewneeds_revision
Summary: New design for client -- proxy protocol for SnowflakeNew design for client -- server protocol for Snowflake

comment:19 in reply to:  17 Changed 2 months ago by cohosh

Replying to dcf:

Replying to 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

Thanks for the feedback! I've addressed most of the feedback so far here: https://github.com/cohosh/snowflake/tree/sequencing

The fields would be better as uint32, because the size of int 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.

Fixed in c102894

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.

Fixed in 02bfa1b

Big-endian is more usual for network protocols.

Fixed in 7566cb

I'm not sure about the implementation of Read:
I would prefer to start with an implementation of Read that is simpler, if less efficient. I am picturing something like this [...]

I like this implementation a lot more. Fixed in b6fb987 and 9c1b12f

I'm going to start on implementing fixed size send windows and cleaning up the unit tests a bit.

comment:20 Changed 7 weeks ago by dcf

I'm thinking one other field that will be required is some kind of session ID, to enable the server to associate connections that come in via different proxies. meek has something like this, to link up separate HTTP requests:

QUIC uses a Connection ID that is just a random string generated by the initiator.

comment:21 Changed 5 weeks ago by gaba

Keywords: anti-censorship-roadmap-september added; ex-sponsor-19 anti-censorship-roadmap removed

comment:22 Changed 6 days ago by cohosh

Updating this for those following along at home. branch

Recent changes:

  • added a sessionID
  • rewrote the snowflake tests to use net.Pipe
  • added a timeout feature to close the underlying connection if sent data was not acknowledged

Next steps:

  • send acknoweldgements
  • correlate the sessionID on both ends
Note: See TracTickets for help on using tickets.