Additional cleanup

- Fix test
- Rename protocol as SHADOWSOCKS-OSSH

psiphon/common/protocol/protocol.go

@@ -36,6 +36,7 @@ const (
 	TUNNEL_PROTOCOL_SSH                              = "SSH"
 	TUNNEL_PROTOCOL_OBFUSCATED_SSH                   = "OSSH"
 	TUNNEL_PROTOCOL_TLS_OBFUSCATED_SSH               = "TLS-OSSH"
+	TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH                 = "SHADOWSOCKS-OSSH"
 	TUNNEL_PROTOCOL_UNFRONTED_MEEK                   = "UNFRONTED-MEEK-OSSH"
 	TUNNEL_PROTOCOL_UNFRONTED_MEEK_HTTPS             = "UNFRONTED-MEEK-HTTPS-OSSH"
 	TUNNEL_PROTOCOL_UNFRONTED_MEEK_SESSION_TICKET    = "UNFRONTED-MEEK-SESSION-TICKET-OSSH"
@@ -45,7 +46,6 @@ const (
 	TUNNEL_PROTOCOL_FRONTED_MEEK_QUIC_OBFUSCATED_SSH = "FRONTED-MEEK-QUIC-OSSH"
 	TUNNEL_PROTOCOL_TAPDANCE_OBFUSCATED_SSH          = "TAPDANCE-OSSH"
 	TUNNEL_PROTOCOL_CONJURE_OBFUSCATED_SSH           = "CONJURE-OSSH"
-	TUNNEL_PROTOCOL_SHADOWSOCKS_SSH                  = "SHADOWSOCKS-SSH"
 
 	FRONTING_TRANSPORT_HTTPS = "FRONTED-HTTPS"
 	FRONTING_TRANSPORT_HTTP  = "FRONTED-HTTP"
@@ -199,7 +199,7 @@ var SupportedTunnelProtocols = TunnelProtocols{
 	TUNNEL_PROTOCOL_FRONTED_MEEK_QUIC_OBFUSCATED_SSH,
 	TUNNEL_PROTOCOL_TAPDANCE_OBFUSCATED_SSH,
 	TUNNEL_PROTOCOL_CONJURE_OBFUSCATED_SSH,
-	TUNNEL_PROTOCOL_SHADOWSOCKS_SSH,
+	TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH,
 }
 
 var DefaultDisabledTunnelProtocols = TunnelProtocols{
@@ -359,7 +359,7 @@ func TunnelProtocolUsesConjure(protocol string) bool {
 
 func TunnelProtocolUsesShadowsocks(protocol string) bool {
 	protocol = TunnelProtocolMinusInproxy(protocol)
-	return protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_SSH
+	return protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH
 }
 
 func TunnelProtocolIsResourceIntensive(protocol string) bool {
@@ -380,7 +380,7 @@ func TunnelProtocolIsCompatibleWithFragmentor(protocol string) bool {
 		protocol == TUNNEL_PROTOCOL_FRONTED_MEEK ||
 		protocol == TUNNEL_PROTOCOL_FRONTED_MEEK_HTTP ||
 		protocol == TUNNEL_PROTOCOL_CONJURE_OBFUSCATED_SSH ||
-		protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_SSH
+		protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH
 }
 
 func TunnelProtocolIsDirect(protocol string) bool {
@@ -391,7 +391,7 @@ func TunnelProtocolIsDirect(protocol string) bool {
 		protocol == TUNNEL_PROTOCOL_UNFRONTED_MEEK_HTTPS ||
 		protocol == TUNNEL_PROTOCOL_UNFRONTED_MEEK_SESSION_TICKET ||
 		protocol == TUNNEL_PROTOCOL_QUIC_OBFUSCATED_SSH ||
-		protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_SSH
+		protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH
 }
 
 func TunnelProtocolRequiresTLS12SessionTickets(protocol string) bool {
@@ -429,7 +429,7 @@ func TunnelProtocolMayUseServerPacketManipulation(protocol string) bool {
 		protocol == TUNNEL_PROTOCOL_UNFRONTED_MEEK ||
 		protocol == TUNNEL_PROTOCOL_UNFRONTED_MEEK_HTTPS ||
 		protocol == TUNNEL_PROTOCOL_UNFRONTED_MEEK_SESSION_TICKET ||
-		protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_SSH
+		protocol == TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH
 }
 
 func TunnelProtocolMayUseClientBPF(protocol string) bool {

psiphon/common/protocol/serverEntry.go

@@ -754,7 +754,7 @@ func (serverEntry *ServerEntry) GetDialPortNumber(tunnelProtocol string) (int, e
 			TUNNEL_PROTOCOL_UNFRONTED_MEEK:
 			return serverEntry.MeekServerPort, nil
 
-		case TUNNEL_PROTOCOL_SHADOWSOCKS_SSH:
+		case TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH:
 			return serverEntry.SshShadowsocksPort, nil
 		}
 
@@ -793,7 +793,7 @@ func (serverEntry *ServerEntry) GetDialPortNumber(tunnelProtocol string) (int, e
 			TUNNEL_PROTOCOL_UNFRONTED_MEEK:
 			return serverEntry.InproxyMeekPort, nil
 
-		case TUNNEL_PROTOCOL_SHADOWSOCKS_SSH:
+		case TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH:
 			return serverEntry.InproxyShadowsocksPort, nil
 		}
 

psiphon/config.go

@@ -183,7 +183,7 @@ type Config struct {
 	// "UNFRONTED-MEEK-HTTPS-OSSH", "UNFRONTED-MEEK-SESSION-TICKET-OSSH",
 	// "FRONTED-MEEK-OSSH", "FRONTED-MEEK-HTTP-OSSH", "QUIC-OSSH",
 	// "FRONTED-MEEK-QUIC-OSSH", "TAPDANCE-OSSH", "CONJURE-OSSH", and
-	// "SHADOWSOCKS-SSH".
+	// "SHADOWSOCKS-OSSH".
 	// For the default, an empty list, all protocols are used.
 	LimitTunnelProtocols []string
 

psiphon/controller_test.go

@@ -155,13 +155,13 @@ func TestTLSOSSH(t *testing.T) {
 		})
 }
 
-func TestShadowsocksSSH(t *testing.T) {
+func TestShadowsocks(t *testing.T) {
 
 	t.Skipf("temporarily disabled")
 
 	controllerRun(t,
 		&controllerRunConfig{
-			protocol:                 protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_SSH,
+			protocol:                 protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH,
 			disableUntunneledUpgrade: true,
 		})
 }

psiphon/dialParameters.go

@@ -31,7 +31,6 @@ import (
 	"sync/atomic"
 	"time"
 
-	"github.com/Jigsaw-Code/outline-sdk/transport/shadowsocks"
 	tls "github.com/Psiphon-Labs/psiphon-tls"
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common"
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/errors"
@@ -118,8 +117,6 @@ type DialParameters struct {
 	TLSOSSHSNIServerName            string
 	TLSOSSHObfuscatorPaddingSeed    *prng.Seed
 
-	ShadowsocksEncryptionKey *shadowsocks.EncryptionKey
-
 	SelectedUserAgent bool
 	UserAgent         string
 
@@ -629,14 +626,6 @@ func MakeDialParameters(
 		}
 	}
 
-	if !isReplay && protocol.TunnelProtocolUsesShadowsocks(dialParams.TunnelProtocol) {
-		// TODO: will ShadowsocksEncryptionKey work with replay?
-		dialParams.ShadowsocksEncryptionKey, err = shadowsocks.NewEncryptionKey(shadowsocks.CHACHA20IETFPOLY1305, dialParams.ServerEntry.SshShadowsocksKey)
-		if err != nil {
-			return nil, errors.Trace(err)
-		}
-	}
-
 	if !isReplay || !replayFragmentor {
 		dialParams.FragmentorSeed, err = prng.NewSeed()
 		if err != nil {
@@ -1326,7 +1315,7 @@ func MakeDialParameters(
 		protocol.TUNNEL_PROTOCOL_CONJURE_OBFUSCATED_SSH,
 		protocol.TUNNEL_PROTOCOL_QUIC_OBFUSCATED_SSH,
 		protocol.TUNNEL_PROTOCOL_TLS_OBFUSCATED_SSH,
-		protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_SSH:
+		protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH:
 
 		dialParams.DirectDialAddress = net.JoinHostPort(serverEntry.IpAddress, dialParams.DialPortNumber)
 
@@ -1722,7 +1711,7 @@ func (dialParams *DialParameters) GetTLSOSSHConfig(config *Config) *TLSTunnelCon
 func (dialParams *DialParameters) GetShadowsocksConfig() *ShadowsockConfig {
 	return &ShadowsockConfig{
 		dialAddr: dialParams.DirectDialAddress,
-		key:      dialParams.ShadowsocksEncryptionKey,
+		key:      dialParams.ServerEntry.SshShadowsocksKey,
 	}
 }
 

psiphon/server/config.go

@@ -60,6 +60,7 @@ const (
 	SSH_PASSWORD_BYTE_LENGTH                            = 32
 	SSH_RSA_HOST_KEY_BITS                               = 2048
 	SSH_OBFUSCATED_KEY_BYTE_LENGTH                      = 32
+	SHADOWSOCKS_KEY_BYTE_LENGTH                         = 32
 	PEAK_UPSTREAM_FAILURE_RATE_MINIMUM_SAMPLE_SIZE      = 10
 	PERIODIC_GARBAGE_COLLECTION                         = 120 * time.Second
 	STOP_ESTABLISH_TUNNELS_ESTABLISHED_CLIENT_THRESHOLD = 20
@@ -130,7 +131,7 @@ type Config struct {
 	// "SSH", "OSSH", "TLS-OSSH", "UNFRONTED-MEEK-OSSH", "UNFRONTED-MEEK-HTTPS-OSSH",
 	// "UNFRONTED-MEEK-SESSION-TICKET-OSSH", "FRONTED-MEEK-OSSH",
 	// "FRONTED-MEEK-QUIC-OSSH", "FRONTED-MEEK-HTTP-OSSH", "QUIC-OSSH",
-	// "TAPDANCE-OSSH", "CONJURE-OSSH", and "SHADOWSOCKS-SSH".
+	// "TAPDANCE-OSSH", "CONJURE-OSSH", and "SHADOWSOCKS-OSSH".
 	TunnelProtocolPorts map[string]int
 
 	// TunnelProtocolPassthroughAddresses specifies passthrough addresses to be
@@ -188,6 +189,8 @@ type Config struct {
 	// run by this server instance, which use Obfuscated SSH.
 	ObfuscatedSSHKey string
 
+	// ShadowsocksKey is the secret key for use in the Shadowsocks
+	// protocol.
 	ShadowsocksKey string
 
 	// MeekCookieEncryptionPrivateKey is the NaCl private key used
@@ -970,14 +973,12 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, []byt
 
 	// Shadowsocks config
 
-	// TODO: assuming shadowsocks.NewEncryptionKey is deterministic for now
-	shadowsocksKey := "test1234"
-
-	// TODO: use proper secret text
-	// shadowsocksKey, err := shadowsocks.NewEncryptionKey(shadowsocks.CHACHA20IETFPOLY1305, "test1234")
-	// if err != nil {
-	// 	return nil, nil, nil, nil, nil, errors.Trace(err)
-	// }
+	// TODO: double check there are enough bytes of entropy
+	shadowsocksKeyBytes, err := common.MakeSecureRandomBytes(SHADOWSOCKS_KEY_BYTE_LENGTH)
+	if err != nil {
+		return nil, nil, nil, nil, nil, errors.Trace(err)
+	}
+	shadowsocksKey := hex.EncodeToString(shadowsocksKeyBytes)
 
 	// Meek config
 
@@ -1221,7 +1222,7 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, []byt
 	// - Only one meek port may be specified per server entry.
 	// - Neither fronted meek nor Conjuure protocols are supported here.
 
-	var sshPort, obfuscatedSSHPort, meekPort, obfuscatedSSHQUICPort, tlsOSSHPort, shadowsocksSSHPort int
+	var sshPort, obfuscatedSSHPort, meekPort, obfuscatedSSHQUICPort, tlsOSSHPort, shadowsocksPort int
 	var inproxySSHPort, inproxyOSSHPort, inproxyQUICPort, inproxyMeekPort, inproxyTlsOSSHPort, inproxyShadowsocksPort int
 
 	for tunnelProtocol, port := range params.TunnelProtocolPorts {
@@ -1240,8 +1241,8 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, []byt
 				protocol.TUNNEL_PROTOCOL_UNFRONTED_MEEK_SESSION_TICKET,
 				protocol.TUNNEL_PROTOCOL_UNFRONTED_MEEK:
 				meekPort = port
-			case protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_SSH:
-				shadowsocksSSHPort = port
+			case protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH:
+				shadowsocksPort = port
 			}
 		} else {
 			switch protocol.TunnelProtocolMinusInproxy(tunnelProtocol) {
@@ -1257,7 +1258,7 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, []byt
 				protocol.TUNNEL_PROTOCOL_UNFRONTED_MEEK_SESSION_TICKET,
 				protocol.TUNNEL_PROTOCOL_UNFRONTED_MEEK:
 				inproxyMeekPort = port
-			case protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_SSH:
+			case protocol.TUNNEL_PROTOCOL_SHADOWSOCKS_OSSH:
 				inproxyShadowsocksPort = port
 			}
 		}
@@ -1279,7 +1280,7 @@ func GenerateConfig(params *GenerateConfigParams) ([]byte, []byte, []byte, []byt
 		SshObfuscatedPort:                   obfuscatedSSHPort,
 		SshObfuscatedQUICPort:               obfuscatedSSHQUICPort,
 		SshShadowsocksKey:                   shadowsocksKey,
-		SshShadowsocksPort:                  shadowsocksSSHPort,
+		SshShadowsocksPort:                  shadowsocksPort,
 		LimitQUICVersions:                   params.LimitQUICVersions,
 		SshObfuscatedKey:                    obfuscatedSSHKey,
 		Capabilities:                        capabilities,

psiphon/server/server_test.go

@@ -208,7 +208,7 @@ func TestTLSOSSH(t *testing.T) {
 func TestShadowsocks(t *testing.T) {
 	runServer(t,
 		&runServerConfig{
-			tunnelProtocol:       "SHADOWSOCKS-SSH",
+			tunnelProtocol:       "SHADOWSOCKS-OSSH",
 			requireAuthorization: true,
 			doTunneledWebRequest: true,
 			doTunneledNTPRequest: true,

psiphon/server/shadowsocks.go

@@ -28,12 +28,15 @@ import (
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/errors"
 )
 
+// ShadowsocksServer tunnels TCP traffic (in the case of Psiphon, SSH traffic)
+// over Shadowsocks.
 type ShadowsocksServer struct {
 	support  *SupportServices
 	listener net.Listener
 	key      *shadowsocks.EncryptionKey
 }
 
+// ListenShadowsocks returns the listener of a new ShadowsocksServer.
 func ListenShadowsocks(
 	support *SupportServices,
 	listener net.Listener,
@@ -54,6 +57,7 @@ func NewShadowsocksServer(
 	listener net.Listener,
 	ssEncryptionKey string) (*ShadowsocksServer, error) {
 
+	// TODO: consider using other AEAD ciphers; client cipher needs to match.
 	key, err := shadowsocks.NewEncryptionKey(shadowsocks.CHACHA20IETFPOLY1305, ssEncryptionKey)
 	if err != nil {
 		return nil, errors.TraceMsg(err, "shadowsocks.NewEncryptionKey failed")
@@ -68,6 +72,8 @@ func NewShadowsocksServer(
 	return shadowsocksServer, nil
 }
 
+// ShadowsocksListener implements the net.Listener interface. Accept returns a
+// net.Conn which implements the common.MetricsSource interface.
 type ShadowsocksListener struct {
 	net.Listener
 	server *ShadowsocksServer

psiphon/shadowsocksConn.go

@@ -28,22 +28,29 @@ import (
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon/common/errors"
 )
 
-// TODO: do fields need to be exported for replay?
+// ShadowsockConfig specifies the behavior of a shadowsocksConn.
 type ShadowsockConfig struct {
 	dialAddr string
-
-	key *shadowsocks.EncryptionKey
+	key      string
 }
 
+// shadowsocksConn is a network connection that tunnels net.Conn flows over Shadowsocks.
 type shadowsocksConn struct {
 	net.Conn
 }
 
+// DialShadowsocksTunnel returns an initialized Shadowsocks connection.
 func DialShadowsocksTunnel(
 	ctx context.Context,
 	shadowsocksConfig *ShadowsockConfig,
 	dialConfig *DialConfig) (*shadowsocksConn, error) {
 
+	// TODO: consider using other AEAD ciphers; server cipher needs to match.
+	key, err := shadowsocks.NewEncryptionKey(shadowsocks.CHACHA20IETFPOLY1305, shadowsocksConfig.key)
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+
 	conn, err := DialTCP(ctx, shadowsocksConfig.dialAddr, dialConfig)
 	if err != nil {
 		return nil, errors.Trace(err)
@@ -51,8 +58,9 @@ func DialShadowsocksTunnel(
 
 	// Based on shadowsocks.DialStream
 	// TODO: explicitly set SaltGenerator?
-	ssw := shadowsocks.NewWriter(conn, shadowsocksConfig.key)
-	ssr := shadowsocks.NewReader(conn, shadowsocksConfig.key)
+	ssw := shadowsocks.NewWriter(conn, key)
+	ssr := shadowsocks.NewReader(conn, key)
+	// TODO: is this cast correct/safe?
 	ssConn := transport.WrapConn(conn.(*TCPConn).Conn.(*net.TCPConn), ssr, ssw)
 
 	return &shadowsocksConn{