Opened 9 months ago

Last modified 13 days ago

#29206 needs_review 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 (27)

comment:1 Changed 9 months ago by cohosh

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

comment:2 Changed 9 months ago by cohosh

Component: Obfuscation/ObfsproxyObfuscation/Snowflake

comment:3 Changed 9 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 9 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 8 months ago by gaba

Points: 6

comment:6 Changed 8 months ago by gaba

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

comment:7 Changed 7 months ago by gaba

cohosh, are you working on this ticket?

comment:8 Changed 7 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 6 months ago by gaba

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

comment:10 Changed 5 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 4 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 4 months ago by phw

Sponsor: Sponsor19Sponsor28-must

Moving from Sponsor 19 to Sponsor 28.

comment:13 Changed 4 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 4 months ago by gaba

Reviewer: dcf

comment:15 Changed 4 months ago by gaba

Keywords: anti-censorship-roadmap added

comment:16 in reply to:  13 Changed 4 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 4 months ago by cohosh (next)

comment:17 in reply to:  13 ; Changed 4 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 4 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 4 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 3 months 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 3 months ago by gaba

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

comment:22 Changed 2 months 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

comment:23 Changed 7 weeks ago by cohosh

Status: needs_revisionneeds_review

This is finally ready for another review. Here's a rebased branch: https://github.com/cohosh/snowflake/compare/sequencing The main improvements from the last version are:

  • Each end of a SnowflakeConn will send empty acknowledgement packets (consisting of just the Snowflake header) upon the receipt of new data
  • Each write method spins up a goroutine that will wait for some specified time until checking to see if the acknowledgement packet for that data has been received. If not, it closes the underlying connection causing future reads and writes to fail.
  • On each call to SnowflakeConn's write method, data is saved in an internal buffer until it has been acknowledged
  • SnowflakeConn now has a new method to set and reset the underlying connection. If there is buffered data, SnowflakeConn will resend that data under the same session ID whenever a new underlying connection has been specified

My reasoning for implementing it this way is that the client and server shouldn't have to worry about buffering data after a call to Write(). This makes the buffer in `client/webrtc.go` redundant, but I'll remove it later when finishing up tying together the client and server interactions.

The next step of course is to allow for resetting the underlying connection in SnowflakeConn and using the sessionID to correlate new connections with old ones. There's going to have to be some tricky refactoring here. Right now when the webrtc connection times out (due to staleness), both the webrtc connection and the socks connection are closed and the client waits for a new socks connection to open. The SnowflakeConn should be persistent across snowflakes (and the way it is currently set up perhaps also across SOCKS connections (??)), so the question is where SnowflakeConn should "live".

I'm thinking of adding a new method to SnowflakeCollector that will set (and reset) the underlying connection, and then modifying the Handler function to redefine the snowflake rather than closing the SOCKS connection and waiting for a new one. This doesn't fit perfectly with what I'd assume a SnowflakeCollector does by name, but then again maybe it does. This would make the SnowflakeCollector responsible for both getting new snowflakes and also deciding which snowflake(s) to send traffic through.

Any feedback on decisions up to this point and on future plans is welcome :)

comment:24 Changed 5 weeks ago by dcf

I would like there to be a paragraph-long or so comment at the top of snowflake-proto/proto.go that briefly explains how things work.

windowSize = 1500 seems small; it would only allow for ≈1 full-sized packet in flight at a time. (I realize windowSize is not used yet.)

Does SnowflakeConn.Conn need to be exposed publicly? Maybe NewSnowflakeConn and SnowflakeConn.NewSnowflake should be the only ways to modify it externally.

LocalAddr, RemoteAddr should return non-nil. They could pass through to s.Conn.LocalAddr and s.Conn.RemoteAddr, but maybe better is to have them return an Addr type that reflects the sessionID, because that is our persistent addressing abstraction over ephemeral network connections. Like:

type sessionAddr []byte;
func (addr sessionAddr) Network() string {
	return "session"
}
func (addr sessionAddr) String() string {
	return string(addr)
}

The header.ack > s.acked comparison looks like it will have problems with overflow. It probably ought to be something like int32(header.ack - s.acked) > 0 instead. Same on the inside, it is probably important to use int32 rather than int. There should be a test for it, too.

		if header.ack > s.acked {
			// remove newly acknowledged bytes from buffer
			s.buf.Next(int(header.ack - s.acked))
			s.acked = header.ack
		}

You don't need an extra check here because io.Writer says "Write must return a non-nil error if it returns n < len(p)."

		//TODO: check to make sure we wrote out all of the bytes
  • SnowflakeConn now has a new method to set and reset the underlying connection. If there is buffered data, SnowflakeConn will resend that data under the same session ID whenever a new underlying connection has been specified

The NewSnowflake function writes the locally buffered bytes directly to the underlying Conn, without prepending new headers as SnowflakeConn.Write does. I expected it to chop up the buffer into new packets and send them with headers (and up-to-date ack numbers).

io.ReadFull(r, b) would be more clear than io.ReadAtLeast(r, b, len(b)).

	b := make([]byte, snowflakeHeaderLen, snowflakeHeaderLen)
	_, err := io.ReadAtLeast(r, b, snowflakeHeaderLen)

I don't think this is worth checking for in SnowflakeConn.Read. I would just let it crash. Or else, panic instead of returning an error. In any case, a check on s.Conn would be better in readLoop than Read, because Read does not even access s.Conn. A better overall design may be to have NewSnowflakeConn take a Conn as a parameter, so that it's impossible to create one that's uninitialized.

	if s.Conn == nil {
		return 0, fmt.Errorf("No network connection to read from ")
	}

Replying to cohosh:

The next step of course is to allow for resetting the underlying connection in SnowflakeConn and using the sessionID to correlate new connections with old ones. There's going to have to be some tricky refactoring here. Right now when the webrtc connection times out (due to staleness), both the webrtc connection and the socks connection are closed and the client waits for a new socks connection to open. The SnowflakeConn should be persistent across snowflakes (and the way it is currently set up perhaps also across SOCKS connections (??)), so the question is where SnowflakeConn should "live".

I'm thinking of adding a new method to SnowflakeCollector that will set (and reset) the underlying connection, and then modifying the Handler function to redefine the snowflake rather than closing the SOCKS connection and waiting for a new one. This doesn't fit perfectly with what I'd assume a SnowflakeCollector does by name, but then again maybe it does. This would make the SnowflakeCollector responsible for both getting new snowflakes and also deciding which snowflake(s) to send traffic through.

I think there should be a 1:1 correspondence between SnowflakeConns and SOCKS connections. SnowflakeConn is analogous to the long-lived TCP connection to a user's guard. If the SnowflakeConn somehow fails unrecoverably, we should close the SOCKS connection to signal to tor that it's dead.

comment:25 Changed 4 weeks ago by cohosh

Status: needs_reviewneeds_revision

comment:26 Changed 4 weeks ago by cohosh

Thanks for the feedback! I added a bunch of new commits onto the sequencing branch

comment:27 Changed 13 days ago by cohosh

Status: needs_revisionneeds_review

Okay I made a lot of changes, and it's done in the sense that the sequencing and reliability layer is fully integrated to the client and server and all the features I think we want are there. I've squashed these changes into two commits on a new branch: https://github.com/cohosh/snowflake/compare/sequencing_squashed

The first commit adds the new SnowflakeConn network connection and the second integrates it into both the client and the server.

Honestly I think it'll need another round of revisions for the following reasons:

  • The locks are a bit messy and I think we'll need more
  • We don't currently have unit tests that check the client and server integration and we should have them
  • In my integration tests I'm seeing the server occasionally fail, but I haven't figured out how yet

Right now there's a plain http test deployment of this server running on ws://68.183.200.16 if you want to try it out.

Some additional notes:

  • I see some motivation for another feature that allows us to set some kind of FIN or RST flag to notify the client that the OR port has been closed and the server that the client has closed the connection. Right now each endpoint can't tell the difference between a problem with the snowflake proxy and with an endpoint.
  • Even though we aren't closing the OR connection on the server side when a snowflake dies, from my tests it looks like the second snowflake connection isn't happening fast enough and I'm still getting a connection reset by peer error when I try to write to it again.

Perhaps 10s is too long a timeout?

Is there a client keep-alive functionality that should happen that we can simulate at the bridge?

Note: See TracTickets for help on using tickets.