Merge pull request #201 from rod-hynes/master

Change MeekStats to TunnelDialStats

psiphon/notice.go

@@ -318,15 +318,18 @@ func NoticeLocalProxyError(proxyType string, err error) {
 		"LocalProxyError", noticeIsDiagnostic, "message", err.Error())
 }
 
-// NoticeConnectedMeekStats reports extra network details for a meek tunnel connection.
-func NoticeConnectedMeekStats(ipAddress string, meekStats *MeekStats) {
-	outputNotice("ConnectedMeekStats", noticeIsDiagnostic,
+// NoticeConnectedTunnelDialStats reports extra network details for tunnel connections that required extra configuration.
+func NoticeConnectedTunnelDialStats(ipAddress string, tunnelDialStats *TunnelDialStats) {
+	outputNotice("ConnectedTunnelDialStats", noticeIsDiagnostic,
 		"ipAddress", ipAddress,
-		"dialAddress", meekStats.DialAddress,
-		"resolvedIPAddress", meekStats.ResolvedIPAddress,
-		"sniServerName", meekStats.SNIServerName,
-		"hostHeader", meekStats.HostHeader,
-		"transformedHostName", meekStats.TransformedHostName)
+		"upstreamProxyType", tunnelDialStats.UpstreamProxyType,
+		"upstreamProxyCustomHeaderNames", strings.Join(tunnelDialStats.UpstreamProxyCustomHeaderNames, ","),
+		"meekDialAddress", tunnelDialStats.MeekDialAddress,
+		"meekDialAddress", tunnelDialStats.MeekDialAddress,
+		"meekResolvedIPAddress", tunnelDialStats.MeekResolvedIPAddress,
+		"meekSNIServerName", tunnelDialStats.MeekSNIServerName,
+		"meekHostHeader", tunnelDialStats.MeekHostHeader,
+		"meekTransformedHostName", tunnelDialStats.MeekTransformedHostName)
 }
 
 // NoticeBuildInfo reports build version info.

psiphon/server/api.go

@@ -22,7 +22,6 @@ package server
 import (
 	"crypto/subtle"
 	"encoding/json"
-	"errors"
 	"fmt"
 	"net"
 	"regexp"
@@ -97,7 +96,7 @@ func handshakeAPIRequestHandler(
 	err := validateRequestParams(support, params, baseRequestParams)
 	if err != nil {
 		// TODO: fail2ban?
-		return nil, psiphon.ContextError(errors.New("invalid params"))
+		return nil, psiphon.ContextError(err)
 	}
 
 	log.WithContextFields(
@@ -170,7 +169,7 @@ func connectedAPIRequestHandler(
 	err := validateRequestParams(support, params, connectedRequestParams)
 	if err != nil {
 		// TODO: fail2ban?
-		return nil, psiphon.ContextError(errors.New("invalid params"))
+		return nil, psiphon.ContextError(err)
 	}
 
 	log.WithContextFields(
@@ -216,7 +215,7 @@ func statusAPIRequestHandler(
 	err := validateRequestParams(support, params, statusRequestParams)
 	if err != nil {
 		// TODO: fail2ban?
-		return nil, psiphon.ContextError(errors.New("invalid params"))
+		return nil, psiphon.ContextError(err)
 	}
 
 	statusData, err := getJSONObjectRequestParam(params, "statusData")
@@ -332,7 +331,7 @@ func clientVerificationAPIRequestHandler(
 	err := validateRequestParams(support, params, baseRequestParams)
 	if err != nil {
 		// TODO: fail2ban?
-		return nil, psiphon.ContextError(errors.New("invalid params"))
+		return nil, psiphon.ContextError(err)
 	}
 
 	// Ignoring error as params are validated
@@ -365,11 +364,14 @@ type requestParamSpec struct {
 const (
 	requestParamOptional  = 1
 	requestParamNotLogged = 2
+	requestParamArray     = 4
 )
 
 // baseRequestParams is the list of required and optional
 // request parameters; derived from COMMON_INPUTS and
 // OPTIONAL_COMMON_INPUTS in psi_web.
+// Each param is expected to be a string, unless requestParamArray
+// is specified, in which case an array of string is expected.
 var baseRequestParams = []requestParamSpec{
 	requestParamSpec{"server_secret", isServerSecret, requestParamNotLogged},
 	requestParamSpec{"client_session_id", isHexDigits, requestParamOptional},
@@ -380,6 +382,8 @@ var baseRequestParams = []requestParamSpec{
 	requestParamSpec{"relay_protocol", isRelayProtocol, 0},
 	requestParamSpec{"tunnel_whole_device", isBooleanFlag, requestParamOptional},
 	requestParamSpec{"device_region", isRegionCode, requestParamOptional},
+	requestParamSpec{"upstream_proxy_type", isAnyString, requestParamOptional},
+	requestParamSpec{"upstream_proxy_custom_header_names", isAnyString, requestParamOptional | requestParamArray},
 	requestParamSpec{"meek_dial_address", isDialAddress, requestParamOptional},
 	requestParamSpec{"meek_resolved_ip_address", isIPAddress, requestParamOptional},
 	requestParamSpec{"meek_sni_server_name", isDomain, requestParamOptional},
@@ -404,20 +408,56 @@ func validateRequestParams(
 			return psiphon.ContextError(
 				fmt.Errorf("missing param: %s", expectedParam.name))
 		}
-		strValue, ok := value.(string)
-		if !ok {
-			return psiphon.ContextError(
-				fmt.Errorf("unexpected param type: %s", expectedParam.name))
+		var err error
+		if expectedParam.flags&requestParamArray != 0 {
+			err = validateStringArrayRequestParam(support, expectedParam, value)
+		} else {
+			err = validateStringRequestParam(support, expectedParam, value)
 		}
-		if !expectedParam.validator(support, strValue) {
-			return psiphon.ContextError(
-				fmt.Errorf("invalid param: %s", expectedParam.name))
+		if err != nil {
+			return psiphon.ContextError(err)
 		}
 	}
 
 	return nil
 }
 
+func validateStringRequestParam(
+	support *SupportServices,
+	expectedParam requestParamSpec,
+	value interface{}) error {
+
+	strValue, ok := value.(string)
+	if !ok {
+		return psiphon.ContextError(
+			fmt.Errorf("unexpected string param type: %s", expectedParam.name))
+	}
+	if !expectedParam.validator(support, strValue) {
+		return psiphon.ContextError(
+			fmt.Errorf("invalid param: %s", expectedParam.name))
+	}
+	return nil
+}
+
+func validateStringArrayRequestParam(
+	support *SupportServices,
+	expectedParam requestParamSpec,
+	value interface{}) error {
+
+	arrayValue, ok := value.([]interface{})
+	if !ok {
+		return psiphon.ContextError(
+			fmt.Errorf("unexpected string param type: %s", expectedParam.name))
+	}
+	for _, value := range arrayValue {
+		err := validateStringRequestParam(support, expectedParam, value)
+		if err != nil {
+			return psiphon.ContextError(err)
+		}
+	}
+	return nil
+}
+
 // getRequestLogFields makes LogFields to log the API event following
 // the legacy psi_web and current ELK naming conventions.
 func getRequestLogFields(
@@ -457,34 +497,42 @@ func getRequestLogFields(
 				continue
 			}
 		}
-		strValue, ok := value.(string)
-		if !ok {
-			// This type assertion should be checked already in
-			// validateRequestParams, so failure is unexpected.
-			continue
-		}
 
-		// Special cases:
-		// - Number fields are encoded as integer types.
-		// - For ELK performance we record these domain-or-IP
-		//   fields as one of two different values based on type;
-		//   we also omit port from host:port fields for now.
-		switch expectedParam.name {
-		case "client_version":
-			intValue, _ := strconv.Atoi(strValue)
-			logFields[expectedParam.name] = intValue
-		case "meek_dial_address":
-			host, _, _ := net.SplitHostPort(strValue)
-			if isIPAddress(support, host) {
-				logFields["meek_dial_ip_address"] = host
-			} else {
-				logFields["meek_dial_domain"] = host
+		switch v := value.(type) {
+		case string:
+			strValue := v
+
+			// Special cases:
+			// - Number fields are encoded as integer types.
+			// - For ELK performance we record these domain-or-IP
+			//   fields as one of two different values based on type;
+			//   we also omit port from host:port fields for now.
+			switch expectedParam.name {
+			case "client_version":
+				intValue, _ := strconv.Atoi(strValue)
+				logFields[expectedParam.name] = intValue
+			case "meek_dial_address":
+				host, _, _ := net.SplitHostPort(strValue)
+				if isIPAddress(support, host) {
+					logFields["meek_dial_ip_address"] = host
+				} else {
+					logFields["meek_dial_domain"] = host
+				}
+			case "meek_host_header":
+				host, _, _ := net.SplitHostPort(strValue)
+				logFields[expectedParam.name] = host
+			default:
+				logFields[expectedParam.name] = strValue
 			}
-		case "meek_host_header":
-			host, _, _ := net.SplitHostPort(strValue)
-			logFields[expectedParam.name] = host
+
+		case []interface{}:
+			// Note: actually validated as an array of strings
+			logFields[expectedParam.name] = v
+
 		default:
-			logFields[expectedParam.name] = strValue
+			// This type assertion should be checked already in
+			// validateRequestParams, so failure is unexpected.
+			continue
 		}
 	}
 
@@ -587,6 +635,10 @@ func isMobileClientPlatform(clientPlatform string) bool {
 
 // Input validators follow the legacy validations rules in psi_web.
 
+func isAnyString(support *SupportServices, value string) bool {
+	return true
+}
+
 func isServerSecret(support *SupportServices, value string) bool {
 	return subtle.ConstantTimeCompare(
 		[]byte(value),

psiphon/server/webServer.go

@@ -155,8 +155,29 @@ func convertHTTPRequestToAPIRequest(
 
 	for name, values := range r.URL.Query() {
 		for _, value := range values {
-			params[name] = value
 			// Note: multiple values per name are ignored
+
+			// TODO: faster lookup?
+			isArray := false
+			for _, paramSpec := range baseRequestParams {
+				if paramSpec.name == name {
+					isArray = (paramSpec.flags&requestParamArray != 0)
+					break
+				}
+			}
+
+			if isArray {
+				// Special case: a JSON encoded array
+				var arrayValue []interface{}
+				err := json.Unmarshal([]byte(value), &arrayValue)
+				if err != nil {
+					return nil, psiphon.ContextError(err)
+				}
+				params[name] = arrayValue
+			} else {
+				// All other query parameters are simple strings
+				params[name] = value
+			}
 			break
 		}
 	}

psiphon/serverApi.go

@@ -60,15 +60,6 @@ type ServerContext struct {
 	serverHandshakeTimestamp string
 }
 
-// MeekStats holds extra stats that are only gathered for meek tunnels.
-type MeekStats struct {
-	DialAddress         string
-	ResolvedIPAddress   string
-	SNIServerName       string
-	HostHeader          string
-	TransformedHostName bool
-}
-
 // nextTunnelNumber is a monotonically increasing number assigned to each
 // successive tunnel connection. The sessionId and tunnelNumber together
 // form a globally unique identifier for tunnels, which is used for
@@ -751,21 +742,27 @@ func (serverContext *ServerContext) getBaseParams() requestJSONObject {
 	if tunnel.config.DeviceRegion != "" {
 		params["device_region"] = tunnel.config.DeviceRegion
 	}
-	if tunnel.meekStats != nil {
-		if tunnel.meekStats.DialAddress != "" {
-			params["meek_dial_address"] = tunnel.meekStats.DialAddress
+	if tunnel.dialStats != nil {
+		if tunnel.dialStats.UpstreamProxyType != "" {
+			params["upstream_proxy_type"] = tunnel.dialStats.UpstreamProxyType
+		}
+		if tunnel.dialStats.UpstreamProxyCustomHeaderNames != nil {
+			params["upstream_proxy_custom_header_names"] = tunnel.dialStats.UpstreamProxyCustomHeaderNames
 		}
-		if tunnel.meekStats.ResolvedIPAddress != "" {
-			params["meek_resolved_ip_address"] = tunnel.meekStats.ResolvedIPAddress
+		if tunnel.dialStats.MeekDialAddress != "" {
+			params["meek_dial_address"] = tunnel.dialStats.MeekDialAddress
 		}
-		if tunnel.meekStats.SNIServerName != "" {
-			params["meek_sni_server_name"] = tunnel.meekStats.SNIServerName
+		if tunnel.dialStats.MeekResolvedIPAddress != "" {
+			params["meek_resolved_ip_address"] = tunnel.dialStats.MeekResolvedIPAddress
 		}
-		if tunnel.meekStats.HostHeader != "" {
-			params["meek_host_header"] = tunnel.meekStats.HostHeader
+		if tunnel.dialStats.MeekSNIServerName != "" {
+			params["meek_sni_server_name"] = tunnel.dialStats.MeekSNIServerName
+		}
+		if tunnel.dialStats.MeekHostHeader != "" {
+			params["meek_host_header"] = tunnel.dialStats.MeekHostHeader
 		}
 		transformedHostName := "0"
-		if tunnel.meekStats.TransformedHostName {
+		if tunnel.dialStats.MeekTransformedHostName {
 			transformedHostName = "1"
 		}
 		params["meek_transformed_host_name"] = transformedHostName
@@ -816,20 +813,37 @@ func makeRequestUrl(tunnel *Tunnel, port, path string, params requestJSONObject)
 	requestUrl.WriteString(port)
 	requestUrl.WriteString("/")
 	requestUrl.WriteString(path)
+
 	firstParam := true
 	for name, value := range params {
-		if strValue, ok := value.(string); ok {
-			if firstParam {
-				requestUrl.WriteString("?")
-				firstParam = false
-			} else {
-				requestUrl.WriteString("&")
+
+		if firstParam {
+			requestUrl.WriteString("?")
+			firstParam = false
+		} else {
+			requestUrl.WriteString("&")
+		}
+
+		requestUrl.WriteString(name)
+		requestUrl.WriteString("=")
+
+		strValue := ""
+		switch v := value.(type) {
+		case string:
+			strValue = v
+		case []string:
+			// String array param encoded as JSON
+			// (URL encoding will be done by http.Client)
+			jsonValue, err := json.Marshal(v)
+			if err != nil {
+				break
 			}
-			requestUrl.WriteString(name)
-			requestUrl.WriteString("=")
-			requestUrl.WriteString(strValue)
+			strValue = string(jsonValue)
 		}
+
+		requestUrl.WriteString(strValue)
 	}
+
 	return requestUrl.String()
 }
 

psiphon/tunnel.go

@@ -27,6 +27,7 @@ import (
 	"fmt"
 	"io"
 	"net"
+	"net/url"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -78,10 +79,26 @@ type Tunnel struct {
 	signalPortForwardFailure     chan struct{}
 	totalPortForwardFailures     int
 	startTime                    time.Time
-	meekStats                    *MeekStats
+	dialStats                    *TunnelDialStats
 	newClientVerificationPayload chan string
 }
 
+// TunnelDialStats records additional dial config that is sent to the server for stats
+// recording. This data is used to analyze which configuration settings are successful
+// in various circumvention contexts, and includes meek dial params and upstream proxy
+// params.
+// For upstream proxy, only proxy type and custom header names are recorded; proxy
+// address and custom header values are considered PII.
+type TunnelDialStats struct {
+	UpstreamProxyType              string
+	UpstreamProxyCustomHeaderNames []string
+	MeekDialAddress                string
+	MeekResolvedIPAddress          string
+	MeekSNIServerName              string
+	MeekHostHeader                 string
+	MeekTransformedHostName        bool
+}
+
 // EstablishTunnel first makes a network transport connection to the
 // Psiphon server and then establishes an SSH client session on top of
 // that transport. The SSH server is authenticated using the public
@@ -106,7 +123,7 @@ func EstablishTunnel(
 	}
 
 	// Build transport layers and establish SSH connection
-	conn, sshClient, meekStats, err := dialSsh(
+	conn, sshClient, dialStats, err := dialSsh(
 		config, pendingConns, serverEntry, selectedProtocol, sessionId)
 	if err != nil {
 		return nil, ContextError(err)
@@ -135,7 +152,7 @@ func EstablishTunnel(
 		// A buffer allows at least one signal to be sent even when the receiver is
 		// not listening. Senders should not block.
 		signalPortForwardFailure: make(chan struct{}, 1),
-		meekStats:                meekStats,
+		dialStats:                dialStats,
 		// Buffer allows SetClientVerificationPayload to submit one new payload
 		// without blocking or dropping it.
 		newClientVerificationPayload: make(chan string, 1),
@@ -504,13 +521,13 @@ func initMeekConfig(
 }
 
 // dialSsh is a helper that builds the transport layers and establishes the SSH connection.
-// When a meek protocols is selected, additional MeekStats are recorded and returned.
+// When additional dial configuration is used, DialStats are recorded and returned.
 func dialSsh(
 	config *Config,
 	pendingConns *Conns,
 	serverEntry *ServerEntry,
 	selectedProtocol,
-	sessionId string) (net.Conn, *ssh.Client, *MeekStats, error) {
+	sessionId string) (net.Conn, *ssh.Client, *TunnelDialStats, error) {
 
 	// The meek protocols tunnel obfuscated SSH. Obfuscated SSH is layered on top of SSH.
 	// So depending on which protocol is used, multiple layers are initialized.
@@ -667,22 +684,39 @@ func dialSsh(
 		return nil, nil, nil, ContextError(result.err)
 	}
 
-	var meekStats *MeekStats
-	if meekConfig != nil {
-		meekStats = &MeekStats{
-			DialAddress:         meekConfig.DialAddress,
-			ResolvedIPAddress:   resolvedIPAddress.Load().(string),
-			SNIServerName:       meekConfig.SNIServerName,
-			HostHeader:          meekConfig.HostHeader,
-			TransformedHostName: meekConfig.TransformedHostName,
+	var dialStats *TunnelDialStats
+
+	if dialConfig.UpstreamProxyUrl != "" || meekConfig != nil {
+		dialStats = &TunnelDialStats{}
+
+		if dialConfig.UpstreamProxyUrl != "" {
+
+			// Note: UpstreamProxyUrl should have parsed correctly in the dial
+			proxyURL, err := url.Parse(dialConfig.UpstreamProxyUrl)
+			if err == nil {
+				dialStats.UpstreamProxyType = proxyURL.Scheme
+			}
+
+			dialStats.UpstreamProxyCustomHeaderNames = make([]string, len(dialConfig.UpstreamProxyCustomHeaders))
+			for name, _ := range dialConfig.UpstreamProxyCustomHeaders {
+				dialStats.UpstreamProxyCustomHeaderNames = append(dialStats.UpstreamProxyCustomHeaderNames, name)
+			}
+		}
+
+		if meekConfig != nil {
+			dialStats.MeekDialAddress = meekConfig.DialAddress
+			dialStats.MeekResolvedIPAddress = resolvedIPAddress.Load().(string)
+			dialStats.MeekSNIServerName = meekConfig.SNIServerName
+			dialStats.MeekHostHeader = meekConfig.HostHeader
+			dialStats.MeekTransformedHostName = meekConfig.TransformedHostName
 		}
 
-		NoticeConnectedMeekStats(serverEntry.IpAddress, meekStats)
+		NoticeConnectedTunnelDialStats(serverEntry.IpAddress, dialStats)
 	}
 
 	cleanupConn = nil
 
-	return conn, result.sshClient, meekStats, nil
+	return conn, result.sshClient, dialStats, nil
 }
 
 // operateTunnel monitors the health of the tunnel and performs