Merge pull request #455 from rod-hynes/master

tactics and server_tunnel changes

MobileLibrary/Android/PsiphonTunnel/PsiphonTunnel.java

@@ -373,17 +373,23 @@ public class PsiphonTunnel extends Psi.PsiphonProvider.Stub {
 
         // The network ID contains potential PII. In tunnel-core, the network ID
         // is used only locally in the client and not sent to the server.
+        //
+        // See network ID requirements here:
+        // https://godoc.org/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon#NetworkIDGetter
 
-        String networkID = "";
+        String networkID = "UNKNOWN";
 
         Context context = mHostService.getContext();
-        ConnectivityManager connectivityManager = (ConnectivityManager)context.getSystemService(Context.CONNECTIVITY_SERVICE);;
+        ConnectivityManager connectivityManager = (ConnectivityManager)context.getSystemService(Context.CONNECTIVITY_SERVICE);
         NetworkInfo activeNetworkInfo = null;
         try {
             activeNetworkInfo = connectivityManager.getActiveNetworkInfo();
 
         } catch (java.lang.Exception e) {
             // May get exceptions due to missing permissions like android.permission.ACCESS_NETWORK_STATE.
+
+            // Apps using the Psiphon Library and lacking android.permission.ACCESS_NETWORK_STATE will
+            // proceed and use tactics, but with "UNKNOWN" as the sole network ID.
         }
 
         if (activeNetworkInfo != null && activeNetworkInfo.getType() == ConnectivityManager.TYPE_WIFI) {

MobileLibrary/iOS/PsiphonTunnel/PsiphonTunnel/PsiphonTunnel.m

@@ -1094,8 +1094,11 @@
 
     // The network ID contains potential PII. In tunnel-core, the network ID
     // is used only locally in the client and not sent to the server.
+    //
+    // See network ID requirements here:
+    // https://godoc.org/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon#NetworkIDGetter
 
-    NSMutableString *networkID = [NSMutableString stringWithString:@""];
+    NSMutableString *networkID = [NSMutableString stringWithString:@"UNKNOWN"];
     NetworkStatus status = [[Reachability reachabilityForInternetConnection] currentReachabilityStatus];
     if (status == ReachableViaWiFi) {
         [networkID setString:@"WIFI"];

MobileLibrary/psi/psi.go

@@ -28,6 +28,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"strings"
 	"sync"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon"
@@ -325,3 +326,30 @@ func (d *loggingDeviceBinder) BindToDevice(fileDescriptor int) error {
 	}
 	return err
 }
+
+type loggingNetworkIDGetter struct {
+	p PsiphonProvider
+}
+
+func newLoggingNetworkIDGetter(p PsiphonProvider) *loggingNetworkIDGetter {
+	return &loggingNetworkIDGetter{p: p}
+}
+
+func (d *loggingNetworkIDGetter) GetNetworkID() string {
+	networkID := d.p.GetNetworkID()
+
+	// All PII must appear after the initial "-"
+	// See: https://godoc.org/github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon#NetworkIDGetter
+	logNetworkID := networkID
+	index := strings.Index(logNetworkID, "-")
+	if index != -1 {
+		logNetworkID = logNetworkID[:index]
+	}
+	if len(logNetworkID)+1 < len(networkID) {
+		// Indicate when additional network info was present after the first "-".
+		logNetworkID += "+<network info>"
+	}
+	psiphon.NoticeInfo("GetNetworkID: %s", logNetworkID)
+
+	return networkID
+}

psiphon/common/tactics/tactics.go

@@ -466,10 +466,18 @@ func NewServer(
 // Validate checks for correct tactics configuration values.
 func (server *Server) Validate() error {
 
-	if len(server.RequestPublicKey) != 32 ||
-		len(server.RequestPrivateKey) != 32 ||
-		len(server.RequestObfuscatedKey) != common.OBFUSCATE_KEY_LENGTH {
-		return common.ContextError(errors.New("invalid request key"))
+	// Key material must either be entirely omitted, or fully populated.
+	if len(server.RequestPublicKey) == 0 {
+		if len(server.RequestPrivateKey) != 0 ||
+			len(server.RequestObfuscatedKey) != 0 {
+			return common.ContextError(errors.New("unexpected request key"))
+		}
+	} else {
+		if len(server.RequestPublicKey) != 32 ||
+			len(server.RequestPrivateKey) != 32 ||
+			len(server.RequestObfuscatedKey) != common.OBFUSCATE_KEY_LENGTH {
+			return common.ContextError(errors.New("invalid request key"))
+		}
 	}
 
 	validateTactics := func(tactics *Tactics, isDefault bool) error {
@@ -901,10 +909,12 @@ func (server *Server) HandleEndPoint(
 
 	server.ReloadableFile.RLock()
 	loaded := server.loaded
+	hasRequestKeys := len(server.RequestPublicKey) > 0
 	server.ReloadableFile.RUnlock()
 
-	if !loaded {
-		// No tactics configuration was loaded.
+	if !loaded || !hasRequestKeys {
+		// No tactics configuration was loaded, or the configuration contained
+		// no key material for tactics requests.
 		return false
 	}
 
@@ -1150,12 +1160,19 @@ func UseStoredTactics(
 // when there is an unexpired stored tactics record available. The
 // caller is expected to set any overall timeout in the context input.
 //
+// Limitation: it is assumed that the network ID obtained from getNetworkID
+// is the one that is active when the tactics request is received by the
+// server. However, it is remotely possible to switch networks
+// immediately after invoking the GetNetworkID callback and initiating
+// the request. This is partially mitigated by rechecking the network ID
+// after the request and failing if it differs from the initial network ID.
+//
 // FetchTactics modifies the apiParams input.
 func FetchTactics(
 	ctx context.Context,
 	clientParameters *parameters.ClientParameters,
 	storer Storer,
-	networkID string,
+	getNetworkID func() string,
 	apiParams common.APIParameters,
 	endPointRegion string,
 	endPointProtocol string,
@@ -1163,6 +1180,8 @@ func FetchTactics(
 	encodedRequestObfuscatedKey string,
 	roundTripper RoundTripper) (*Record, error) {
 
+	networkID := getNetworkID()
+
 	record, err := getStoredTacticsRecord(storer, networkID)
 	if err != nil {
 		return nil, common.ContextError(err)
@@ -1196,6 +1215,10 @@ func FetchTactics(
 			return nil, common.ContextError(err)
 		}
 
+		if networkID != getNetworkID() {
+			return nil, common.ContextError(errors.New("network ID changed"))
+		}
+
 		err = AddSpeedTestSample(
 			clientParameters,
 			storer,
@@ -1251,6 +1274,10 @@ func FetchTactics(
 		return nil, common.ContextError(err)
 	}
 
+	if networkID != getNetworkID() {
+		return nil, common.ContextError(errors.New("network ID changed"))
+	}
+
 	// Process and store the response payload.
 
 	var payload *Payload

psiphon/common/tactics/tactics_test.go

@@ -219,6 +219,8 @@ func TestTactics(t *testing.T) {
 
 	networkID := "NETWORK1"
 
+	getNetworkID := func() string { return networkID }
+
 	apiParams := common.APIParameters{
 		"client_platform": "P1",
 		"client_version":  "V1"}
@@ -316,7 +318,7 @@ func TestTactics(t *testing.T) {
 		ctx,
 		clientParams,
 		storer,
-		networkID,
+		getNetworkID,
 		apiParams,
 		endPointProtocol,
 		endPointRegion,
@@ -388,7 +390,7 @@ func TestTactics(t *testing.T) {
 		context.Background(),
 		clientParams,
 		storer,
-		networkID,
+		getNetworkID,
 		apiParams,
 		endPointProtocol,
 		endPointRegion,
@@ -453,7 +455,7 @@ func TestTactics(t *testing.T) {
 		context.Background(),
 		clientParams,
 		storer,
-		networkID,
+		getNetworkID,
 		apiParams,
 		endPointProtocol,
 		endPointRegion,
@@ -609,7 +611,7 @@ func TestTactics(t *testing.T) {
 		context.Background(),
 		clientParams,
 		storer,
-		networkID,
+		getNetworkID,
 		apiParams,
 		endPointProtocol,
 		endPointRegion,
@@ -624,7 +626,7 @@ func TestTactics(t *testing.T) {
 		context.Background(),
 		clientParams,
 		storer,
-		networkID,
+		getNetworkID,
 		apiParams,
 		endPointProtocol,
 		endPointRegion,
@@ -635,6 +637,49 @@ func TestTactics(t *testing.T) {
 		t.Fatalf("FetchTactics succeeded unexpectedly with incorrect obfuscated key")
 	}
 
+	// When no keys are supplied, untunneled tactics requests are not supported, but
+	// handshake tactics (GetTacticsPayload) should still work.
+
+	tacticsConfig = fmt.Sprintf(
+		tacticsConfigTemplate,
+		"",
+		"",
+		"",
+		tacticsProbability,
+		tacticsNetworkLatencyMultiplier,
+		tacticsConnectionWorkerPoolSize,
+		jsonTacticsLimitTunnelProtocols,
+		tacticsConnectionWorkerPoolSize+1)
+
+	err = ioutil.WriteFile(configFileName, []byte(tacticsConfig), 0600)
+	if err != nil {
+		t.Fatalf("WriteFile failed: %s", err)
+	}
+
+	reloaded, err = server.Reload()
+	if err != nil {
+		t.Fatalf("Reload failed: %s", err)
+	}
+
+	if !reloaded {
+		t.Fatalf("Server config failed to reload")
+	}
+
+	_, err = server.GetTacticsPayload(clientGeoIPData, handshakeParams)
+	if err != nil {
+		t.Fatalf("GetTacticsPayload failed: %s", err)
+	}
+
+	handled := server.HandleEndPoint(TACTICS_END_POINT, clientGeoIPData, nil, nil)
+	if handled {
+		t.Fatalf("HandleEndPoint unexpectedly handled request")
+	}
+
+	handled = server.HandleEndPoint(SPEED_TEST_END_POINT, clientGeoIPData, nil, nil)
+	if handled {
+		t.Fatalf("HandleEndPoint unexpectedly handled request")
+	}
+
 	// TODO: test replay attack defence
 
 	// TODO: test Server.Validate with invalid tactics configurations

psiphon/config.go

@@ -186,35 +186,38 @@ type Config struct {
 	UpstreamProxyCustomHeaders http.Header
 
 	// NetworkConnectivityChecker is an interface that enables tunnel-core to
-	// call into the host application to check for network connectivity. This
-	// parameter is only applicable to library deployments.
+	// call into the host application to check for network connectivity. See:
+	// NetworkConnectivityChecker doc.
+	//
+	// This parameter is only applicable to library deployments.
 	NetworkConnectivityChecker NetworkConnectivityChecker
 
 	// DeviceBinder is an interface that enables tunnel-core to call into the
-	// host application to bind sockets to specific devices. This is used for
-	// VPN routing exclusion. This parameter is only applicable to library
-	// deployments.
+	// host application to bind sockets to specific devices. See: DeviceBinder
+	// doc.
+	//
+	// This parameter is only applicable to library deployments.
 	DeviceBinder DeviceBinder
 
 	// IPv6Synthesizer is an interface that allows tunnel-core to call into
-	// the host application to synthesize IPv6 addresses from IPv4 ones. This
-	// is used to correctly lookup IPs on DNS64/NAT64 networks. This parameter
-	// is only applicable to library deployments.
+	// the host application to synthesize IPv6 addresses. See: IPv6Synthesizer
+	// doc.
+	//
+	// This parameter is only applicable to library deployments.
 	IPv6Synthesizer IPv6Synthesizer
 
 	// DnsServerGetter is an interface that enables tunnel-core to call into
 	// the host application to discover the native network DNS server
-	// settings. This parameter is only applicable to library deployments.
+	// settings. See: DnsServerGetter doc.
+	//
+	// This parameter is only applicable to library deployments.
 	DnsServerGetter DnsServerGetter
 
 	// NetworkIDGetter in an interface that enables tunnel-core to call into
 	// the host application to get an identifier for the host's current active
-	// network. The identifier is a free-form string that should indicate the
-	// network type and identify; for example "WIFI-<BSSID>" or
-	// "MOBILE-<MCC/MNC>". As this network ID is personally identifying, it is
-	// only used locally in the client to determine network context and is not
-	// sent to the Psiphon server. This parameter is only applicable to
-	// library deployments.
+	// network. See: NetworkIDGetter doc.
+	//
+	// This parameter is only applicable to library deployments.
 	NetworkIDGetter NetworkIDGetter
 
 	// TransformHostNames specifies whether to use hostname transformation

psiphon/controller.go

@@ -1284,6 +1284,12 @@ func (controller *Controller) getTactics(done chan struct{}) {
 
 		for iteration := 0; ; iteration++ {
 
+			if !WaitForNetworkConnectivity(
+				controller.runCtx,
+				controller.config.NetworkConnectivityChecker) {
+				return
+			}
+
 			serverEntry, err := iterator.Next()
 			if err != nil {
 				NoticeAlert("tactics iterator failed: %s", err)
@@ -1403,22 +1409,15 @@ func (controller *Controller) doFetchTactics(
 		timeout)
 	defer cancelFunc()
 
-	// Limitation: it is assumed that the network ID obtained here is the
-	// one that is active when the tactics request is received by the
-	// server. However, it is remotely possible to switch networks
-	// immediately after invoking the GetNetworkID callback and initiating
-	// the request.
-	//
-	// TODO: ensure that meek in round trip mode will fail the request when
-	// the pre-dial connection is broken.
-
-	networkID := controller.config.NetworkIDGetter.GetNetworkID()
-
 	// DialMeek completes the TCP/TLS handshakes for HTTPS
 	// meek protocols but _not_ for HTTP meek protocols.
 	//
 	// TODO: pre-dial HTTP protocols to conform with speed
 	// test RTT spec.
+	//
+	// TODO: ensure that meek in round trip mode will fail
+	// the request when the pre-dial connection is broken,
+	// to minimize the possibility of network ID mismatches.
 
 	meekConn, err := DialMeek(ctx, meekConfig, dialConfig)
 	if err != nil {
@@ -1437,7 +1436,7 @@ func (controller *Controller) doFetchTactics(
 		ctx,
 		controller.config.clientParameters,
 		GetTacticsStorer(),
-		networkID,
+		controller.config.NetworkIDGetter.GetNetworkID,
 		apiParams,
 		serverEntry.Region,
 		tacticsProtocol,

psiphon/net.go

@@ -97,29 +97,54 @@ type DialConfig struct {
 }
 
 // NetworkConnectivityChecker defines the interface to the external
-// HasNetworkConnectivity provider
+// HasNetworkConnectivity provider, which call into the host application to
+// check for network connectivity.
 type NetworkConnectivityChecker interface {
 	// TODO: change to bool return value once gobind supports that type
 	HasNetworkConnectivity() int
 }
 
 // DeviceBinder defines the interface to the external BindToDevice provider
+// which calls into the host application to bind sockets to specific devices.
+// This is used for VPN routing exclusion.
 type DeviceBinder interface {
 	BindToDevice(fileDescriptor int) error
 }
 
 // DnsServerGetter defines the interface to the external GetDnsServer provider
+// which calls into the host application to discover the native network DNS
+// server settings.
 type DnsServerGetter interface {
 	GetPrimaryDnsServer() string
 	GetSecondaryDnsServer() string
 }
 
-// IPv6Synthesizer defines the interface to the external IPv6Synthesize provider
+// IPv6Synthesizer defines the interface to the external IPv6Synthesize
+// provider which calls into the host application to synthesize IPv6 addresses
+// from IPv4 ones. This is used to correctly lookup IPs on DNS64/NAT64
+// networks.
 type IPv6Synthesizer interface {
 	IPv6Synthesize(IPv4Addr string) string
 }
 
-// NetworkIDGetter defines the interface to the external GetNetworkID provider
+// NetworkIDGetter defines the interface to the external GetNetworkID
+// provider, which returns an identifier for the host's current active
+// network.
+//
+// The identifier is a string that should indicate the network type and
+// identity; for example "WIFI-<BSSID>" or "MOBILE-<MCC/MNC>". As this network
+// ID is personally identifying, it is only used locally in the client to
+// determine network context and is not sent to the Psiphon server. The
+// identifer will be logged in diagnostics messages; in this case only the
+// substring before the first "-" is logged, so all PII must appear after the
+// first "-".
+//
+// NetworkIDGetter.GetNetworkID should always return an identifier value, as
+// logic that uses GetNetworkID, including tactics, is intended to proceed
+// regardless of whether an accurate network identifier can be obtained. By
+// convention, the provider should return "UNKNOWN" when an accurate network
+// identifier cannot be obtained. Best-effort is acceptable: e.g., return just
+// "WIFI" when only the type of the network but no details can be determined.
 type NetworkIDGetter interface {
 	GetNetworkID() string
 }

psiphon/server/psinet/psinet.go

@@ -57,6 +57,8 @@ type Host struct {
 	MeekCookieEncryptionPublicKey string `json:"meek_cookie_encryption_public_key"`
 	MeekServerObfuscatedKey       string `json:"meek_server_obfuscated_key"`
 	MeekServerPort                int    `json:"meek_server_port"`
+	TacticsRequestPublicKey       string `json:"tactics_request_public_key"`
+	TacticsRequestObfuscatedKey   string `json:"tactics_request_obfuscated_key"`
 	Region                        string `json:"region"`
 }
 
@@ -453,6 +455,8 @@ func (db *Database) getEncodedServerEntry(server Server) string {
 		MeekServerPort                int      `json:"meekServerPort"`
 		MeekCookieEncryptionPublicKey string   `json:"meekCookieEncryptionPublicKey"`
 		MeekObfuscatedKey             string   `json:"meekObfuscatedKey"`
+		TacticsRequestPublicKey       string   `json:"tacticsRequestPublicKey"`
+		TacticsRequestObfuscatedKey   string   `json:"tacticsRequestObfuscatedKey"`
 	}
 
 	// NOTE: also putting original values in extended config for easier parsing by new clients
@@ -493,6 +497,8 @@ func (db *Database) getEncodedServerEntry(server Server) string {
 	extendedConfig.MeekCookieEncryptionPublicKey = host.MeekCookieEncryptionPublicKey
 	extendedConfig.MeekServerPort = host.MeekServerPort
 	extendedConfig.MeekObfuscatedKey = host.MeekServerObfuscatedKey
+	extendedConfig.TacticsRequestPublicKey = host.TacticsRequestPublicKey
+	extendedConfig.TacticsRequestObfuscatedKey = host.TacticsRequestObfuscatedKey
 
 	serverCapabilities := make(map[string]bool, 0)
 	for capability, enabled := range server.Capabilities {

psiphon/server/tunnelServer.go

@@ -1693,6 +1693,13 @@ func (sshClient *sshClient) logTunnel(additionalMetrics LogFields) {
 	logFields["peak_concurrent_port_forward_count_udp"] = sshClient.udpTrafficState.peakConcurrentPortForwardCount
 	logFields["total_port_forward_count_udp"] = sshClient.udpTrafficState.totalPortForwardCount
 
+	// Pre-calculate a total-tunneled-bytes field. This total is used
+	// extensively in analytics and is more performant when pre-calculated.
+	logFields["bytes"] = sshClient.tcpTrafficState.bytesUp +
+		sshClient.tcpTrafficState.bytesDown +
+		sshClient.udpTrafficState.bytesUp +
+		sshClient.udpTrafficState.bytesDown
+
 	// Merge in additional metrics from the optional metrics source
 	if additionalMetrics != nil {
 		for name, value := range additionalMetrics {

psiphon/serverApi.go

@@ -130,10 +130,15 @@ func (serverContext *ServerContext) doHandshakeRequest(
 	if doTactics {
 
 		// Limitation: it is assumed that the network ID obtained here is the
-		// one that is active when the tactics request is received by the
+		// one that is active when the handshake request is received by the
 		// server. However, it is remotely possible to switch networks
 		// immediately after invoking the GetNetworkID callback and initiating
 		// the handshake, if the tunnel protocol is meek.
+		//
+		// The response handling code below calls GetNetworkID again and ignores
+		// any tactics payload if the network ID is not the same. While this
+		// doesn't detect all cases of changing networks, it reduces the already
+		// narrow window.
 
 		networkID = serverContext.tunnel.config.NetworkIDGetter.GetNetworkID()
 
@@ -254,7 +259,8 @@ func (serverContext *ServerContext) doHandshakeRequest(
 
 	NoticeActiveAuthorizationIDs(handshakeResponse.ActiveAuthorizationIDs)
 
-	if doTactics && handshakeResponse.TacticsPayload != nil {
+	if doTactics && handshakeResponse.TacticsPayload != nil &&
+		networkID == serverContext.tunnel.config.NetworkIDGetter.GetNetworkID() {
 
 		var payload *tactics.Payload
 		err = json.Unmarshal(handshakeResponse.TacticsPayload, &payload)