tor: add a new response reader for tor controller

This commit adds a new response reader which replaces the old
textproto.Reader.ReadResponse. The older reader cannot handle the case
when the reply from Tor server contains a data reply line, which uses
the symbol "+" to signal such a case.

tor/controller.go

@@ -65,6 +65,10 @@ var (
 	// message from the controller.
 	controllerKey = []byte("Tor safe cookie authentication " +
 		"controller-to-server hash")
+
+	// errCodeNotMatch is used when an expected response code is not
+	// returned.
+	errCodeNotMatch = errors.New("unexpected code")
 )
 
 // Controller is an implementation of the Tor Control protocol. This is used in
@@ -168,16 +172,17 @@ func (c *Controller) Stop() error {
 // sendCommand sends a command to the Tor server and returns its response, as a
 // single space-delimited string, and code.
 func (c *Controller) sendCommand(command string) (int, string, error) {
-	if err := c.conn.Writer.PrintfLine(command); err != nil {
+	id, err := c.conn.Cmd(command)
+	if err != nil {
 		return 0, "", err
 	}
 
-	// We'll use ReadResponse as it has built-in support for multi-line
-	// text protocol responses.
-	code, reply, err := c.conn.Reader.ReadResponse(success)
-	log.Tracef("sendCommand: %v got <code:%v>, <reply:%v>",
-		command, code, reply)
+	// Make sure our reader only process the response returned from the
+	// above command.
+	c.conn.StartResponse(id)
+	defer c.conn.EndResponse(id)
 
+	code, reply, err := c.readResponse(success)
 	if err != nil {
 		log.Debugf("sendCommand:%s got err:%v, reply:%v",
 			command, err, reply)
@@ -187,6 +192,119 @@ func (c *Controller) sendCommand(command string) (int, string, error) {
 	return code, reply, nil
 }
 
+// readResponse reads the replies from Tor to the controller. The reply has the
+// following format,
+//
+//   Reply = SyncReply / AsyncReply
+//   SyncReply = *(MidReplyLine / DataReplyLine) EndReplyLine
+//   AsyncReply = *(MidReplyLine / DataReplyLine) EndReplyLine
+//
+//   MidReplyLine = StatusCode "-" ReplyLine
+//   DataReplyLine = StatusCode "+" ReplyLine CmdData
+//   EndReplyLine = StatusCode SP ReplyLine
+//   ReplyLine = [ReplyText] CRLF
+//   ReplyText = XXXX
+//   StatusCode = 3DIGIT
+//
+// Unless specified otherwise, multiple lines in a single reply from Tor daemon
+// to the controller are guaranteed to share the same status code. Read more on
+// this topic:
+//   https://gitweb.torproject.org/torspec.git/tree/control-spec.txt#n158
+//
+// NOTE: this code is influenced by https://github.com/Yawning/bulb.
+func (c *Controller) readResponse(expected int) (int, string, error) {
+	// Clean the buffer inside the conn. This is needed when we encountered
+	// an error while reading the response, the remaining lines need to be
+	// cleaned before next read.
+	defer func() {
+		if _, err := c.conn.R.Discard(c.conn.R.Buffered()); err != nil {
+			log.Errorf("clean read buffer failed: %v", err)
+		}
+	}()
+
+	reply, code := "", 0
+	hasMoreLines := true
+
+	for hasMoreLines {
+		line, err := c.conn.Reader.ReadLine()
+		if err != nil {
+			return 0, reply, err
+		}
+		log.Tracef("Reading line: %v", line)
+
+		// Line being shortter than 4 is not allowed.
+		if len(line) < 4 {
+			err = textproto.ProtocolError("short line: " + line)
+			return 0, reply, err
+		}
+
+		// Parse the status code.
+		code, err = strconv.Atoi(line[0:3])
+		if err != nil {
+			return code, reply, err
+		}
+
+		switch line[3] {
+		// EndReplyLine = StatusCode SP ReplyLine.
+		// Example: 250 OK
+		// This is the end of the response, so we mark hasMoreLines to
+		// be false to exit the loop.
+		case ' ':
+			reply += line[4:]
+			hasMoreLines = false
+
+		// MidReplyLine = StatusCode "-" ReplyLine.
+		// Example: 250-version=...
+		// This is a continued response, so we keep reading the next
+		// line.
+		case '-':
+			reply += line[4:]
+
+		// DataReplyLine = StatusCode "+" ReplyLine CmdData.
+		// Example: 250+config-text=
+		//	    line1
+		//	    line2
+		//          more lines...
+		//          .
+		// This is a data response, meaning the following multiple
+		// lines are the actual data, and a dot(.) in the end means the
+		// end of the data response. The response will be formatted as,
+		// 	key=line1,line2,...
+		// The above example will then be,
+		// 	config-text=line1,line2,...
+		case '+':
+			// Add the key(config-text=)
+			reply += line[4:]
+
+			// Add the values.
+			resp, err := c.conn.Reader.ReadDotLines()
+			if err != nil {
+				return code, reply, err
+			}
+			reply += strings.Join(resp, ",")
+
+		// Invalid line separator found.
+		default:
+			err = textproto.ProtocolError("invalid line: " + line)
+			return code, reply, err
+		}
+
+		// We check the code here so that the error message is parsed
+		// from the line.
+		if code != expected {
+			return code, reply, errCodeNotMatch
+		}
+
+		// Separate each line using "\n".
+		if hasMoreLines {
+			reply += "\n"
+		}
+	}
+
+	log.Tracef("Parsed reply: %v", reply)
+	return code, reply, nil
+}
+
 // parseTorReply parses the reply from the Tor server after receiving a command
 // from a controller. This will parse the relevant reply parameters into a map
 // of keys and values.
@@ -225,6 +343,8 @@ func (c *Controller) authenticate() error {
 		return err
 	}
 
+	log.Debugf("received protocol info: %v", protocolInfo)
+
 	// With the version retrieved, we'll cache it now in case it needs to be
 	// used later on.
 	c.version = protocolInfo.version()

tor/controller_test.go

@@ -1,6 +1,12 @@
 package tor
 
-import "testing"
+import (
+	"net"
+	"net/textproto"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
 
 // TestParseTorVersion is a series of tests for different version strings that
 // check the correctness of determining whether they support creating v3 onion
@@ -74,3 +80,199 @@ func TestParseTorVersion(t *testing.T) {
 		}
 	}
 }
+
+// testProxy emulates a Tor daemon and contains the info used for the tor
+// controller to make connections.
+type testProxy struct {
+	// server is the proxy listener.
+	server net.Listener
+
+	// serverConn is the established connection from the server side.
+	serverConn net.Conn
+
+	// serverAddr is the tcp address the proxy is listening on.
+	serverAddr string
+
+	// clientConn is the established connection from the client side.
+	clientConn *textproto.Conn
+}
+
+// cleanUp is used after each test to properly close the ports/connections.
+func (tp *testProxy) cleanUp() {
+	// Don't bother cleanning if there's no a server created.
+	if tp.server == nil {
+		return
+	}
+
+	if err := tp.clientConn.Close(); err != nil {
+		log.Errorf("closing client conn got err: %v", err)
+	}
+	if err := tp.server.Close(); err != nil {
+		log.Errorf("closing proxy server got err: %v", err)
+	}
+}
+
+// createTestProxy creates a proxy server to listen on a random address,
+// creates a server and a client connection, and initializes a testProxy using
+// these params.
+func createTestProxy(t *testing.T) *testProxy {
+	// Set up the proxy to listen on given port.
+	//
+	// NOTE: we use a port 0 here to indicate we want a free port selected
+	// by the system.
+	proxy, err := net.Listen("tcp", ":0")
+	require.NoError(t, err, "failed to create proxy")
+
+	t.Logf("created proxy server to listen on address: %v", proxy.Addr())
+
+	// Accept the connection inside a goroutine.
+	serverChan := make(chan net.Conn, 1)
+	go func(result chan net.Conn) {
+		conn, err := proxy.Accept()
+		require.NoError(t, err, "failed to accept")
+
+		result <- conn
+	}(serverChan)
+
+	// Create the connection using tor controller.
+	client, err := textproto.Dial("tcp", proxy.Addr().String())
+	require.NoError(t, err, "failed to create connection")
+
+	tc := &testProxy{
+		server:     proxy,
+		serverConn: <-serverChan,
+		serverAddr: proxy.Addr().String(),
+		clientConn: client,
+	}
+
+	return tc
+}
+
+// TestReadResponse contructs a series of possible responses returned by Tor
+// and asserts the readResponse can handle them correctly.
+func TestReadResponse(t *testing.T) {
+	// Create mock server and client connection.
+	proxy := createTestProxy(t)
+	defer proxy.cleanUp()
+	server := proxy.serverConn
+
+	// Create a dummy tor controller.
+	c := &Controller{conn: proxy.clientConn}
+
+	testCase := []struct {
+		name       string
+		serverResp string
+
+		// expectedReply is the reply we expect the readResponse to
+		// return.
+		expectedReply string
+
+		// expectedCode is the code we expect the server to return.
+		expectedCode int
+
+		// returnedCode is the code we expect the readResponse to
+		// return.
+		returnedCode int
+
+		// expectedErr is the error we expect the readResponse to
+		// return.
+		expectedErr error
+	}{
+		{
+			// Test a simple response.
+			name:          "succeed on 250",
+			serverResp:    "250 OK\n",
+			expectedReply: "OK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test a mid reply(-) response.
+			name: "succeed on mid reply line",
+			serverResp: "250-field=value\n" +
+				"250 OK\n",
+			expectedReply: "field=value\nOK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test a data reply(+) response.
+			name: "succeed on data reply line",
+			serverResp: "250+field=\n" +
+				"line1\n" +
+				"line2\n" +
+				".\n" +
+				"250 OK\n",
+			expectedReply: "field=line1,line2\nOK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test a mixed reply response.
+			name: "succeed on mixed reply line",
+			serverResp: "250-field=value\n" +
+				"250+field=\n" +
+				"line1\n" +
+				"line2\n" +
+				".\n" +
+				"250 OK\n",
+			expectedReply: "field=value\nfield=line1,line2\nOK",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr:   nil,
+		},
+		{
+			// Test unexpected code.
+			name:          "fail on codes not matched",
+			serverResp:    "250 ERR\n",
+			expectedReply: "ERR",
+			expectedCode:  500,
+			returnedCode:  250,
+			expectedErr:   errCodeNotMatch,
+		},
+		{
+			// Test short response error.
+			name:          "fail on short response",
+			serverResp:    "123\n250 OK\n",
+			expectedReply: "",
+			expectedCode:  250,
+			returnedCode:  0,
+			expectedErr: textproto.ProtocolError(
+				"short line: 123"),
+		},
+		{
+			// Test short response error.
+			name:          "fail on invalid response",
+			serverResp:    "250?OK\n",
+			expectedReply: "",
+			expectedCode:  250,
+			returnedCode:  250,
+			expectedErr: textproto.ProtocolError(
+				"invalid line: 250?OK"),
+		},
+	}
+
+	for _, tc := range testCase {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+
+			// Let the server mocks a given response.
+			_, err := server.Write([]byte(tc.serverResp))
+			require.NoError(t, err, "server failed to write")
+
+			// Read the response and checks all expectations
+			// satisfied.
+			code, reply, err := c.readResponse(tc.expectedCode)
+			require.Equal(t, tc.expectedErr, err)
+			require.Equal(t, tc.returnedCode, code)
+			require.Equal(t, tc.expectedReply, reply)
+
+			// Check that the read buffer is cleaned.
+			require.Zero(t, c.conn.R.Buffered(),
+				"read buffer not empty")
+		})
+	}
+}