Refactor BindToDevice and NetworkID logging

- Move logging into core psiphon package, so
  all clients and tests use the same logic.

- Use repetitive notices.

- Actually enable NetworkID logging.

ConsoleClient/main.go

@@ -272,16 +272,6 @@ func main() {
 		defer tunDeviceFile.Close()
 	}
 
-	// Enable tactics when a NetworkID is specified
-	//
-	// Limitation: unlike psi.NetworkIDGetter, which calls back to platform
-	// APIs, this method of network identification is not dynamic and will not
-	// reflect network changes that occur while running.
-
-	if config.NetworkID != "" {
-		config.NetworkIDGetter = &networkGetter{networkID: config.NetworkID}
-	}
-
 	// Run Psiphon
 
 	controller, err := psiphon.NewController(config)
@@ -348,8 +338,8 @@ type tunProvider struct {
 }
 
 // BindToDevice implements the psiphon.DeviceBinder interface.
-func (p *tunProvider) BindToDevice(fileDescriptor int) error {
-	return tun.BindToDevice(fileDescriptor, p.bindInterface)
+func (p *tunProvider) BindToDevice(fileDescriptor int) (string, error) {
+	return p.bindInterface, tun.BindToDevice(fileDescriptor, p.bindInterface)
 }
 
 // GetPrimaryDnsServer implements the psiphon.DnsServerGetter interface.
@@ -361,11 +351,3 @@ func (p *tunProvider) GetPrimaryDnsServer() string {
 func (p *tunProvider) GetSecondaryDnsServer() string {
 	return p.secondaryDNS
 }
-
-type networkGetter struct {
-	networkID string
-}
-
-func (n *networkGetter) GetNetworkID() string {
-	return n.networkID
-}

MobileLibrary/psi/psi.go

@@ -111,7 +111,7 @@ func Start(
 	config.NetworkIDGetter = provider
 
 	if useDeviceBinder {
-		config.DeviceBinder = newLoggingDeviceBinder(provider)
+		config.DeviceBinder = provider
 		config.DnsServerGetter = provider
 	}
 
@@ -316,46 +316,3 @@ func (p *mutexPsiphonProvider) GetNetworkID() string {
 	defer p.Unlock()
 	return p.p.GetNetworkID()
 }
-
-type loggingDeviceBinder struct {
-	p PsiphonProvider
-}
-
-func newLoggingDeviceBinder(p PsiphonProvider) *loggingDeviceBinder {
-	return &loggingDeviceBinder{p: p}
-}
-
-func (d *loggingDeviceBinder) BindToDevice(fileDescriptor int) error {
-	deviceInfo, err := d.p.BindToDevice(fileDescriptor)
-	if err == nil && deviceInfo != "" {
-		psiphon.NoticeInfo("BindToDevice: %s", deviceInfo)
-	}
-	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/LookupIP.go

@@ -44,7 +44,7 @@ func LookupIP(ctx context.Context, host string, config *DialConfig) ([]net.IP, e
 		return []net.IP{ip}, nil
 	}
 
-	if config.DeviceBinder != nil {
+	if config.deviceBinder != nil {
 
 		dnsServer := config.DnsServerGetter.GetPrimaryDnsServer()
 
@@ -124,7 +124,7 @@ func bindLookupIP(
 		return nil, common.ContextError(err)
 	}
 
-	err = config.DeviceBinder.BindToDevice(socketFd)
+	_, err = config.deviceBinder.BindToDevice(socketFd)
 	if err != nil {
 		syscall.Close(socketFd)
 		return nil, common.ContextError(fmt.Errorf("BindToDevice failed: %s", err))

psiphon/LookupIP_nobind.go

@@ -33,7 +33,7 @@ import (
 // simply uses net.LookupIP.
 func LookupIP(ctx context.Context, host string, config *DialConfig) ([]net.IP, error) {
 
-	if config.DeviceBinder != nil {
+	if config.deviceBinder != nil {
 		return nil, common.ContextError(errors.New("LookupIP with DeviceBinder not supported on this platform"))
 	}
 

psiphon/TCPConn_bind.go

@@ -136,8 +136,8 @@ func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, er
 
 		tcpDialSetAdditionalSocketOptions(socketFD)
 
-		if config.DeviceBinder != nil {
-			err = config.DeviceBinder.BindToDevice(socketFD)
+		if config.deviceBinder != nil {
+			_, err = config.deviceBinder.BindToDevice(socketFD)
 			if err != nil {
 				syscall.Close(socketFD)
 				lastErr = common.ContextError(fmt.Errorf("BindToDevice failed: %s", err))

psiphon/TCPConn_nobind.go

@@ -32,7 +32,7 @@ import (
 // tcpDial is the platform-specific part of DialTCP
 func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, error) {
 
-	if config.DeviceBinder != nil {
+	if config.deviceBinder != nil {
 		return nil, common.ContextError(errors.New("psiphon.interruptibleTCPDial with DeviceBinder not supported"))
 	}
 

psiphon/config.go

@@ -226,11 +226,9 @@ type Config struct {
 	// This parameter is only applicable to library deployments.
 	NetworkIDGetter NetworkIDGetter
 
-	// NetworkID, when specified, is used as the identifier for the host's
+	// NetworkID, when not blank, is used as the identifier for the host's
 	// current active network.
-	//
-	// This parameter is only applicable to non-library deployments which
-	// do not use NetworkIDGetter.
+	// NetworkID is ignored when NetworkIDGetter is set.
 	NetworkID string
 
 	// DisableTactics disables tactics operations including requests, payload
@@ -460,6 +458,9 @@ type Config struct {
 	// New tactics must be applied by calling Config.SetClientParameters;
 	// calling clientParameters.Set directly will fail to add config values.
 	clientParameters *parameters.ClientParameters
+
+	deviceBinder    DeviceBinder
+	networkIDGetter NetworkIDGetter
 }
 
 // LoadConfig parses and validates a JSON format Psiphon config JSON
@@ -632,6 +633,35 @@ func LoadConfig(configJson []byte) (*Config, error) {
 		return nil, common.ContextError(err)
 	}
 
+	// Initialize config.deviceBinder and config.config.networkIDGetter. These
+	// wrap config.DeviceBinder and config.NetworkIDGetter/NetworkID with
+	// loggers.
+	//
+	// New variables are set to avoid mutating input config fields.
+	// Internally, code must use config.deviceBinder and
+	// config.networkIDGetter and not the input/exported fields.
+
+	if config.DeviceBinder != nil {
+		config.deviceBinder = &loggingDeviceBinder{config.DeviceBinder}
+	}
+
+	networkIDGetter := config.NetworkIDGetter
+
+	if networkIDGetter == nil && config.NetworkID != "" {
+
+		// Enable tactics when a NetworkID is specified
+		//
+		// Limitation: unlike NetworkIDGetter, which calls back to platform APIs
+		// this method of network identification is not dynamic and will not reflect
+		// network changes that occur while running.
+
+		networkIDGetter = newStaticNetworkGetter(config.NetworkID)
+	}
+
+	if networkIDGetter != nil {
+		config.networkIDGetter = &loggingNetworkIDGetter{networkIDGetter}
+	}
+
 	return &config, nil
 }
 
@@ -779,3 +809,58 @@ func promoteLegacyDownloadURL(URL string) parameters.DownloadURLs {
 	}
 	return downloadURLs
 }
+
+type loggingDeviceBinder struct {
+	d DeviceBinder
+}
+
+func newLoggingDeviceBinder(d DeviceBinder) *loggingDeviceBinder {
+	return &loggingDeviceBinder{d: d}
+}
+
+func (d *loggingDeviceBinder) BindToDevice(fileDescriptor int) (string, error) {
+	deviceInfo, err := d.d.BindToDevice(fileDescriptor)
+	if err == nil && deviceInfo != "" {
+		NoticeBindToDevice(deviceInfo)
+	}
+	return deviceInfo, err
+}
+
+type staticNetworkGetter struct {
+	networkID string
+}
+
+func newStaticNetworkGetter(networkID string) *staticNetworkGetter {
+	return &staticNetworkGetter{networkID: networkID}
+}
+
+func (n *staticNetworkGetter) GetNetworkID() string {
+	return n.networkID
+}
+
+type loggingNetworkIDGetter struct {
+	n NetworkIDGetter
+}
+
+func newLoggingNetworkIDGetter(n NetworkIDGetter) *loggingNetworkIDGetter {
+	return &loggingNetworkIDGetter{n: n}
+}
+
+func (n *loggingNetworkIDGetter) GetNetworkID() string {
+	networkID := n.n.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>"
+	}
+	NoticeNetworkID(logNetworkID)
+
+	return networkID
+}

psiphon/controller.go

@@ -100,7 +100,7 @@ func NewController(config *Config) (controller *Controller, err error) {
 	untunneledDialConfig := &DialConfig{
 		UpstreamProxyURL:              config.UpstreamProxyURL,
 		CustomHeaders:                 config.CustomHeaders,
-		DeviceBinder:                  config.DeviceBinder,
+		DeviceBinder:                  config.deviceBinder,
 		DnsServerGetter:               config.DnsServerGetter,
 		IPv6Synthesizer:               config.IPv6Synthesizer,
 		UseIndistinguishableTLS:       config.UseIndistinguishableTLS,
@@ -1181,7 +1181,7 @@ func (controller *Controller) launchEstablishing() {
 	// canceled when establishment is stopped.
 
 	doTactics := !controller.config.DisableTactics &&
-		controller.config.NetworkIDGetter != nil
+		controller.config.networkIDGetter != nil
 
 	if doTactics {
 
@@ -1280,7 +1280,7 @@ func (controller *Controller) getTactics(done chan struct{}) {
 
 	tacticsRecord, err := tactics.UseStoredTactics(
 		GetTacticsStorer(),
-		controller.config.NetworkIDGetter.GetNetworkID())
+		controller.config.networkIDGetter.GetNetworkID())
 	if err != nil {
 		NoticeAlert("get stored tactics failed: %s", err)
 
@@ -1453,7 +1453,7 @@ func (controller *Controller) doFetchTactics(
 		ctx,
 		controller.config.clientParameters,
 		GetTacticsStorer(),
-		controller.config.NetworkIDGetter.GetNetworkID,
+		controller.config.networkIDGetter.GetNetworkID,
 		apiParams,
 		serverEntry.Region,
 		tacticsProtocol,

psiphon/controller_test.go

@@ -514,7 +514,7 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 	// Enable tactics requests. This will passively exercise the code
 	// paths. server_test runs a more comprehensive test that checks
 	// that the tactics request succeeds.
-	config.NetworkIDGetter = &testNetworkGetter{}
+	config.NetworkID = "NETWORK1"
 
 	os.Remove(config.UpgradeDownloadFilename)
 	os.Remove(config.RemoteServerListDownloadFilename)
@@ -1141,9 +1141,3 @@ func initUpstreamProxy() {
 	// TODO: wait until listener is active?
 }
 
-type testNetworkGetter struct {
-}
-
-func (testNetworkGetter) GetNetworkID() string {
-	return "NETWORK1"
-}

psiphon/net.go

@@ -107,8 +107,9 @@ type NetworkConnectivityChecker interface {
 // 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.
+// The string return value should report device information for diagnostics.
 type DeviceBinder interface {
-	BindToDevice(fileDescriptor int) error
+	BindToDevice(fileDescriptor int) (string, error)
 }
 
 // DnsServerGetter defines the interface to the external GetDnsServer provider

psiphon/notice.go

@@ -738,6 +738,18 @@ func NoticeActiveAuthorizationIDs(activeAuthorizationIDs []string) {
 		"IDs", activeAuthorizationIDs)
 }
 
+func NoticeBindToDevice(deviceInfo string) {
+	outputRepetitiveNotice(
+		"BindToDevice", deviceInfo, 0,
+		"BindToDevice", 0, "regions", deviceInfo)
+}
+
+func NoticeNetworkID(networkID string) {
+	outputRepetitiveNotice(
+		"NetworkID", networkID, 0,
+		"NetworkID", 0, "regions", networkID)
+}
+
 type repetitiveNoticeState struct {
 	message string
 	repeats int

psiphon/server/server_test.go

@@ -589,7 +589,7 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 		// ensure tactics from different runs don't apply; this is
 		// a workaround for the singleton datastore.
 		prefix := time.Now().String()
-		clientConfig.NetworkIDGetter = &testNetworkGetter{prefix: prefix}
+		clientConfig.NetworkID = prefix+"NETWORK1"
 	}
 
 	if doTactics {
@@ -1259,11 +1259,3 @@ const dummyClientVerificationPayload = `
 	"status": 0,
 	"payload": ""
 }`
-
-type testNetworkGetter struct {
-	prefix string
-}
-
-func (t *testNetworkGetter) GetNetworkID() string {
-	return t.prefix + "NETWORK1"
-}

psiphon/serverApi.go

@@ -126,7 +126,7 @@ func (serverContext *ServerContext) doHandshakeRequest(
 	params := serverContext.getBaseAPIParameters()
 
 	doTactics := !serverContext.tunnel.config.DisableTactics &&
-		serverContext.tunnel.config.NetworkIDGetter != nil
+		serverContext.tunnel.config.networkIDGetter != nil
 
 	networkID := ""
 	if doTactics {
@@ -142,7 +142,7 @@ func (serverContext *ServerContext) doHandshakeRequest(
 		// doesn't detect all cases of changing networks, it reduces the already
 		// narrow window.
 
-		networkID = serverContext.tunnel.config.NetworkIDGetter.GetNetworkID()
+		networkID = serverContext.tunnel.config.networkIDGetter.GetNetworkID()
 
 		err := tactics.SetTacticsAPIParameters(
 			serverContext.tunnel.config.clientParameters, GetTacticsStorer(), networkID, params)
@@ -265,7 +265,7 @@ func (serverContext *ServerContext) doHandshakeRequest(
 	NoticeActiveAuthorizationIDs(handshakeResponse.ActiveAuthorizationIDs)
 
 	if doTactics && handshakeResponse.TacticsPayload != nil &&
-		networkID == serverContext.tunnel.config.NetworkIDGetter.GetNetworkID() {
+		networkID == serverContext.tunnel.config.networkIDGetter.GetNetworkID() {
 
 		var payload *tactics.Payload
 		err := json.Unmarshal(handshakeResponse.TacticsPayload, &payload)

psiphon/tunnel.go

@@ -796,7 +796,7 @@ func initDialConfig(
 	dialConfig := &DialConfig{
 		UpstreamProxyURL:              config.UpstreamProxyURL,
 		CustomHeaders:                 dialCustomHeaders,
-		DeviceBinder:                  config.DeviceBinder,
+		DeviceBinder:                  config.deviceBinder,
 		DnsServerGetter:               config.DnsServerGetter,
 		IPv6Synthesizer:               config.IPv6Synthesizer,
 		UseIndistinguishableTLS:       config.UseIndistinguishableTLS,
@@ -1457,7 +1457,7 @@ func (tunnel *Tunnel) sendSshKeepAlive(isFirstKeepAlive bool, timeout time.Durat
 		// considering that only the last SpeedTestMaxSampleCount samples are
 		// retained, enables tuning the sampling frequency.
 
-		if err == nil && requestOk && tunnel.config.NetworkIDGetter != nil &&
+		if err == nil && requestOk && tunnel.config.networkIDGetter != nil &&
 			(isFirstKeepAlive ||
 				tunnel.config.clientParameters.Get().WeightedCoinFlip(
 					parameters.SSHKeepAliveSpeedTestSampleProbability)) {
@@ -1465,7 +1465,7 @@ func (tunnel *Tunnel) sendSshKeepAlive(isFirstKeepAlive bool, timeout time.Durat
 			err = tactics.AddSpeedTestSample(
 				tunnel.config.clientParameters,
 				GetTacticsStorer(),
-				tunnel.config.NetworkIDGetter.GetNetworkID(),
+				tunnel.config.networkIDGetter.GetNetworkID(),
 				tunnel.serverEntry.Region,
 				tunnel.protocol,
 				elapsedTime,