Merge pull request #465 from rod-hynes/master

Bug fixes

ConsoleClient/main.go

@@ -186,7 +186,33 @@ func main() {
 		os.Exit(1)
 	}
 
-	// BuildInfo is a diagnostic notice, so emit only after LoadConfig
+	if interfaceName != "" {
+		config.ListenInterface = interfaceName
+	}
+
+	// Configure packet tunnel, including updating the config.
+
+	if tun.IsSupported() && tunDevice != "" {
+		tunDeviceFile, err := configurePacketTunnel(
+			config, tunDevice, tunBindInterface, tunPrimaryDNS, tunSecondaryDNS)
+		if err != nil {
+			psiphon.SetEmitDiagnosticNotices(true)
+			psiphon.NoticeError("error configuring packet tunnel: %s", err)
+			os.Exit(1)
+		}
+		defer tunDeviceFile.Close()
+	}
+
+	// All config fields should be set before calling Commit.
+
+	err = config.Commit()
+	if err != nil {
+		psiphon.SetEmitDiagnosticNotices(true)
+		psiphon.NoticeError("error loading configuration file: %s", err)
+		os.Exit(1)
+	}
+
+	// BuildInfo is a diagnostic notice, so emit only after config.Commit
 	// sets EmitDiagnosticNotices.
 
 	psiphon.NoticeBuildInfo()
@@ -256,32 +282,6 @@ func main() {
 		}
 	}
 
-	if interfaceName != "" {
-		config.ListenInterface = interfaceName
-	}
-
-	// Configure packet tunnel
-
-	if tun.IsSupported() && tunDevice != "" {
-		tunDeviceFile, err := configurePacketTunnel(
-			config, tunDevice, tunBindInterface, tunPrimaryDNS, tunSecondaryDNS)
-		if err != nil {
-			psiphon.NoticeError("error configuring packet tunnel: %s", err)
-			os.Exit(1)
-		}
-		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 +348,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 +361,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

@@ -28,7 +28,6 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
-	"strings"
 	"sync"
 
 	"github.com/Psiphon-Labs/psiphon-tunnel-core/psiphon"
@@ -111,7 +110,7 @@ func Start(
 	config.NetworkIDGetter = provider
 
 	if useDeviceBinder {
-		config.DeviceBinder = newLoggingDeviceBinder(provider)
+		config.DeviceBinder = provider
 		config.DnsServerGetter = provider
 	}
 
@@ -119,11 +118,21 @@ func Start(
 		config.IPv6Synthesizer = provider
 	}
 
+	// All config fields should be set before calling Commit.
+
+	err = config.Commit()
+	if err != nil {
+		return fmt.Errorf("error committing configuration file: %s", err)
+	}
+
 	psiphon.SetNoticeWriter(psiphon.NewNoticeReceiver(
 		func(notice []byte) {
 			provider.Notice(string(notice))
 		}))
 
+	// BuildInfo is a diagnostic notice, so emit only after config.Commit
+	// sets EmitDiagnosticNotices.
+
 	psiphon.NoticeBuildInfo()
 
 	err = psiphon.InitDataStore(config)
@@ -316,46 +325,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

@@ -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

@@ -137,7 +137,7 @@ func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, er
 		tcpDialSetAdditionalSocketOptions(socketFD)
 
 		if config.DeviceBinder != nil {
-			err = config.DeviceBinder.BindToDevice(socketFD)
+			_, 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/common/tactics/tactics.go

@@ -1582,6 +1582,7 @@ func applyTacticsPayload(
 
 	if payload.Tag != record.Tag {
 		record.Tag = payload.Tag
+		record.Tactics = Tactics{}
 		err := json.Unmarshal(payload.Tactics, &record.Tactics)
 		if err != nil {
 			return common.ContextError(err)

psiphon/common/tactics/tactics_test.go

@@ -22,7 +22,6 @@ package tactics
 import (
 	"bytes"
 	"context"
-	"encoding/json"
 	"fmt"
 	"io/ioutil"
 	"net"
@@ -85,7 +84,7 @@ func TestTactics(t *testing.T) {
           },
           "Tactics" : {
             "Parameters" : {
-              "LimitTunnelProtocols" : %s
+              %s
             }
           }
         },
@@ -125,7 +124,9 @@ func TestTactics(t *testing.T) {
 	tacticsNetworkLatencyMultiplier := 2.0
 	tacticsConnectionWorkerPoolSize := 5
 	tacticsLimitTunnelProtocols := protocol.TunnelProtocols{"OSSH", "SSH"}
-	jsonTacticsLimitTunnelProtocols, _ := json.Marshal(tacticsLimitTunnelProtocols)
+	jsonTacticsLimitTunnelProtocols := `"LimitTunnelProtocols" : ["OSSH", "SSH"]`
+
+	expectedApplyCount := 3
 
 	listenerProtocol := "OSSH"
 	listenerProhibitedGeoIP := func(string) common.GeoIPData { return common.GeoIPData{Country: "R7"} }
@@ -304,7 +305,7 @@ func TestTactics(t *testing.T) {
 			t.Fatalf("Apply failed: %s", err)
 		}
 
-		if counts[0] != 3 {
+		if counts[0] != expectedApplyCount {
 			t.Fatalf("Unexpected apply count: %d", counts[0])
 		}
 
@@ -439,6 +440,18 @@ func TestTactics(t *testing.T) {
 
 	tacticsConnectionWorkerPoolSize = 6
 
+	tacticsLimitTunnelProtocols = protocol.TunnelProtocols{}
+	jsonTacticsLimitTunnelProtocols = ``
+	expectedApplyCount = 2
+
+	// Omitting LimitTunnelProtocols entirely tests this bug fix: When a new
+	// tactics payload is obtained, all previous parameters should be cleared.
+	//
+	// In the bug, any previous parameters not in the new tactics were
+	// incorrectly retained. In this test case, LimitTunnelProtocols is
+	// omitted in the new tactics; if FetchTactics fails to clear the old
+	// LimitTunnelProtocols then the test will fail.
+
 	tacticsConfig = fmt.Sprintf(
 		tacticsConfigTemplate,
 		encodedRequestPublicKey,

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,10 +458,21 @@ 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
+
+	committed bool
 }
 
-// LoadConfig parses and validates a JSON format Psiphon config JSON
-// string and returns a Config struct populated with config values.
+// LoadConfig parses a JSON format Psiphon config JSON string and returns a
+// Config struct populated with config values.
+//
+// The Config struct may then be programmatically populated with additional
+// values, including callbacks such as DeviceBinder.
+//
+// Before using the Config, Commit must be called, which will  perform further
+// validation and initialize internal data structures.
 func LoadConfig(configJson []byte) (*Config, error) {
 
 	var config Config
@@ -472,6 +481,20 @@ func LoadConfig(configJson []byte) (*Config, error) {
 		return nil, common.ContextError(err)
 	}
 
+	return &config, nil
+}
+
+// IsCommitted checks if Commit was called.
+func (config *Config) IsCommitted() bool {
+	return config.committed
+}
+
+// Commit validates Config fields finalizes initialization.
+//
+// Config fields should not be set after calling Config, as any changes may
+// not be reflected in internal data structures.
+func (config *Config) Commit() error {
+
 	// Do SetEmitDiagnosticNotices first, to ensure config file errors are emitted.
 
 	if config.EmitDiagnosticNotices {
@@ -500,10 +523,11 @@ func LoadConfig(configJson []byte) (*Config, error) {
 	// Supply default values.
 
 	if config.DataStoreDirectory == "" {
-		config.DataStoreDirectory, err = os.Getwd()
+		wd, err := os.Getwd()
 		if err != nil {
-			return nil, common.ContextError(err)
+			return common.ContextError(err)
 		}
+		config.DataStoreDirectory = wd
 	}
 
 	if config.ClientVersion == "" {
@@ -517,32 +541,32 @@ func LoadConfig(configJson []byte) (*Config, error) {
 	// Validate config fields.
 
 	if config.PropagationChannelId == "" {
-		return nil, common.ContextError(
+		return common.ContextError(
 			errors.New("propagation channel ID is missing from the configuration file"))
 	}
 	if config.SponsorId == "" {
-		return nil, common.ContextError(
+		return common.ContextError(
 			errors.New("sponsor ID is missing from the configuration file"))
 	}
 
-	_, err = strconv.Atoi(config.ClientVersion)
+	_, err := strconv.Atoi(config.ClientVersion)
 	if err != nil {
-		return nil, common.ContextError(
+		return common.ContextError(
 			fmt.Errorf("invalid client version: %s", err))
 	}
 
 	if config.NetworkConnectivityChecker != nil {
-		return nil, common.ContextError(
+		return common.ContextError(
 			errors.New("NetworkConnectivityChecker interface must be set at runtime"))
 	}
 
 	if config.DeviceBinder != nil {
-		return nil, common.ContextError(
+		return common.ContextError(
 			errors.New("DeviceBinder interface must be set at runtime"))
 	}
 
 	if config.DnsServerGetter != nil {
-		return nil, common.ContextError(
+		return common.ContextError(
 			errors.New("DnsServerGetter interface must be set at runtime"))
 	}
 
@@ -550,7 +574,7 @@ func LoadConfig(configJson []byte) (*Config, error) {
 		[]string{"", protocol.PSIPHON_SSH_API_PROTOCOL, protocol.PSIPHON_WEB_API_PROTOCOL},
 		config.TargetApiProtocol) {
 
-		return nil, common.ContextError(
+		return common.ContextError(
 			errors.New("invalid TargetApiProtocol"))
 	}
 
@@ -558,19 +582,19 @@ func LoadConfig(configJson []byte) (*Config, error) {
 
 		if config.RemoteServerListURLs != nil {
 			if config.RemoteServerListSignaturePublicKey == "" {
-				return nil, common.ContextError(errors.New("missing RemoteServerListSignaturePublicKey"))
+				return common.ContextError(errors.New("missing RemoteServerListSignaturePublicKey"))
 			}
 			if config.RemoteServerListDownloadFilename == "" {
-				return nil, common.ContextError(errors.New("missing RemoteServerListDownloadFilename"))
+				return common.ContextError(errors.New("missing RemoteServerListDownloadFilename"))
 			}
 		}
 
 		if config.ObfuscatedServerListRootURLs != nil {
 			if config.RemoteServerListSignaturePublicKey == "" {
-				return nil, common.ContextError(errors.New("missing RemoteServerListSignaturePublicKey"))
+				return common.ContextError(errors.New("missing RemoteServerListSignaturePublicKey"))
 			}
 			if config.ObfuscatedServerListDownloadDirectory == "" {
-				return nil, common.ContextError(errors.New("missing ObfuscatedServerListDownloadDirectory"))
+				return common.ContextError(errors.New("missing ObfuscatedServerListDownloadDirectory"))
 			}
 		}
 
@@ -578,26 +602,26 @@ func LoadConfig(configJson []byte) (*Config, error) {
 
 	if config.SplitTunnelRoutesURLFormat != "" {
 		if config.SplitTunnelRoutesSignaturePublicKey == "" {
-			return nil, common.ContextError(errors.New("missing SplitTunnelRoutesSignaturePublicKey"))
+			return common.ContextError(errors.New("missing SplitTunnelRoutesSignaturePublicKey"))
 		}
 		if config.SplitTunnelDNSServer == "" {
-			return nil, common.ContextError(errors.New("missing SplitTunnelDNSServer"))
+			return common.ContextError(errors.New("missing SplitTunnelDNSServer"))
 		}
 	}
 
 	if config.UpgradeDownloadURLs != nil {
 		if config.UpgradeDownloadClientVersionHeader == "" {
-			return nil, common.ContextError(errors.New("missing UpgradeDownloadClientVersionHeader"))
+			return common.ContextError(errors.New("missing UpgradeDownloadClientVersionHeader"))
 		}
 		if config.UpgradeDownloadFilename == "" {
-			return nil, common.ContextError(errors.New("missing UpgradeDownloadFilename"))
+			return common.ContextError(errors.New("missing UpgradeDownloadFilename"))
 		}
 	}
 
 	// This constraint is expected by logic in Controller.runTunnels().
 
 	if config.PacketTunnelTunFileDescriptor > 0 && config.TunnelPoolSize != 1 {
-		return nil, common.ContextError(errors.New("packet tunnel mode requires TunnelPoolSize to be 1"))
+		return common.ContextError(errors.New("packet tunnel mode requires TunnelPoolSize to be 1"))
 	}
 
 	// SessionID must be PSIPHON_API_CLIENT_SESSION_ID_LENGTH lowercase hex-encoded bytes.
@@ -605,7 +629,7 @@ func LoadConfig(configJson []byte) (*Config, error) {
 	if config.SessionID == "" {
 		sessionID, err := MakeSessionId()
 		if err != nil {
-			return nil, common.ContextError(err)
+			return common.ContextError(err)
 		}
 		config.SessionID = sessionID
 	}
@@ -614,7 +638,7 @@ func LoadConfig(configJson []byte) (*Config, error) {
 		-1 != strings.IndexFunc(config.SessionID, func(c rune) bool {
 			return !unicode.Is(unicode.ASCII_Hex_Digit, c) || unicode.IsUpper(c)
 		}) {
-		return nil, common.ContextError(errors.New("invalid SessionID"))
+		return common.ContextError(errors.New("invalid SessionID"))
 	}
 
 	config.clientParameters, err = parameters.NewClientParameters(
@@ -622,17 +646,48 @@ func LoadConfig(configJson []byte) (*Config, error) {
 			NoticeAlert("ClientParameters getValue failed: %s", err)
 		})
 	if err != nil {
-		return nil, common.ContextError(err)
+		return common.ContextError(err)
 	}
 
 	// clientParameters.Set will validate the config fields applied to parameters.
 
 	err = config.SetClientParameters("", false, nil)
 	if err != nil {
-		return nil, common.ContextError(err)
+		return common.ContextError(err)
 	}
 
-	return &config, nil
+	// 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}
+	}
+
+	config.committed = true
+
+	return nil
 }
 
 // GetClientParameters returns a snapshot of the current client parameters.
@@ -668,9 +723,12 @@ func (config *Config) SetClientParameters(tag string, skipOnError bool, applyPar
 	NoticeInfo("applied %v parameters with tag '%s'", counts, tag)
 
 	// Emit certain individual parameter values for quick reference in diagnostics.
-	NoticeInfo(
-		"NetworkLatencyMultiplier: %f",
-		config.clientParameters.Get().Float(parameters.NetworkLatencyMultiplier))
+	networkLatencyMultiplier := config.clientParameters.Get().Float(parameters.NetworkLatencyMultiplier)
+	if networkLatencyMultiplier != 0.0 {
+		NoticeInfo(
+			"NetworkLatencyMultiplier: %f",
+			config.clientParameters.Get().Float(parameters.NetworkLatencyMultiplier))
+	}
 
 	return nil
 }
@@ -732,6 +790,13 @@ func (config *Config) makeConfigParameters() map[string]interface{} {
 		applyParameters[parameters.FetchUpgradeRetryPeriod] = fmt.Sprintf("%dms", *config.FetchUpgradeRetryPeriodMilliseconds)
 	}
 
+	switch config.TransformHostNames {
+	case "always":
+		applyParameters[parameters.TransformHostNameProbability] = 1.0
+	case "never":
+		applyParameters[parameters.TransformHostNameProbability] = 0.0
+	}
+
 	if !config.DisableRemoteServerListFetcher {
 
 		if config.RemoteServerListURLs != nil {
@@ -769,3 +834,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/config_test.go

@@ -67,7 +67,10 @@ func TestConfigTestSuite(t *testing.T) {
 
 // Tests good config
 func (suite *ConfigTestSuite) Test_LoadConfig_BasicGood() {
-	_, err := LoadConfig(suite.confStubBlob)
+	config, err := LoadConfig(suite.confStubBlob)
+	if err == nil {
+		err = config.Commit()
+	}
 	suite.Nil(err, "a basic config should succeed")
 }
 
@@ -83,7 +86,10 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 	var testObjJSON []byte
 
 	// JSON with none of our fields
-	_, err := LoadConfig([]byte(`{"f1": 11, "f2": "two"}`))
+	config, err := LoadConfig([]byte(`{"f1": 11, "f2": "two"}`))
+	if err == nil {
+		err = config.Commit()
+	}
 	suite.NotNil(err, "JSON with none of our fields should fail")
 
 	// Test all required fields
@@ -92,28 +98,40 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 		json.Unmarshal(suite.confStubBlob, &testObj)
 		delete(testObj, field)
 		testObjJSON, _ = json.Marshal(testObj)
-		_, err = LoadConfig(testObjJSON)
+		config, err = LoadConfig(testObjJSON)
+		if err == nil {
+			err = config.Commit()
+		}
 		suite.NotNil(err, "JSON with one of our required fields missing should fail: %s", field)
 
 		// Bad type for required field
 		json.Unmarshal(suite.confStubBlob, &testObj)
 		testObj[field] = false // basically guessing a wrong type
 		testObjJSON, _ = json.Marshal(testObj)
-		_, err = LoadConfig(testObjJSON)
+		config, err = LoadConfig(testObjJSON)
+		if err == nil {
+			err = config.Commit()
+		}
 		suite.NotNil(err, "JSON with one of our required fields with the wrong type should fail: %s", field)
 
 		// One of our required fields is null
 		json.Unmarshal(suite.confStubBlob, &testObj)
 		testObj[field] = nil
 		testObjJSON, _ = json.Marshal(testObj)
-		_, err = LoadConfig(testObjJSON)
+		config, err = LoadConfig(testObjJSON)
+		if err == nil {
+			err = config.Commit()
+		}
 		suite.NotNil(err, "JSON with one of our required fields set to null should fail: %s", field)
 
 		// One of our required fields is an empty string
 		json.Unmarshal(suite.confStubBlob, &testObj)
 		testObj[field] = ""
 		testObjJSON, _ = json.Marshal(testObj)
-		_, err = LoadConfig(testObjJSON)
+		config, err = LoadConfig(testObjJSON)
+		if err == nil {
+			err = config.Commit()
+		}
 		suite.NotNil(err, "JSON with one of our required fields set to an empty string should fail: %s", field)
 	}
 
@@ -123,7 +141,10 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 		json.Unmarshal(suite.confStubBlob, &testObj)
 		testObj[field] = false // basically guessing a wrong type
 		testObjJSON, _ = json.Marshal(testObj)
-		_, err = LoadConfig(testObjJSON)
+		config, err = LoadConfig(testObjJSON)
+		if err == nil {
+			err = config.Commit()
+		}
 		suite.NotNil(err, "JSON with one of our optional fields with the wrong type should fail: %s", field)
 	}
 }
@@ -141,11 +162,17 @@ func (suite *ConfigTestSuite) Test_LoadConfig_GoodJson() {
 		delete(testObj, suite.nonRequiredFields[i])
 	}
 	testObjJSON, _ = json.Marshal(testObj)
-	_, err := LoadConfig(testObjJSON)
+	config, err := LoadConfig(testObjJSON)
+	if err == nil {
+		err = config.Commit()
+	}
 	suite.Nil(err, "JSON with good values for our required fields but no optional fields should succeed")
 
 	// Has all of our required fields, and all optional fields
-	_, err = LoadConfig(suite.confStubBlob)
+	config, err = LoadConfig(suite.confStubBlob)
+	if err == nil {
+		err = config.Commit()
+	}
 	suite.Nil(err, "JSON with all good values for required and optional fields should succeed")
 
 	// Has null for optional fields
@@ -154,6 +181,9 @@ func (suite *ConfigTestSuite) Test_LoadConfig_GoodJson() {
 		testObj[suite.nonRequiredFields[i]] = nil
 	}
 	testObjJSON, _ = json.Marshal(testObj)
-	_, err = LoadConfig(testObjJSON)
+	config, err = LoadConfig(testObjJSON)
+	if err == nil {
+		err = config.Commit()
+	}
 	suite.Nil(err, "JSON with null for optional values should succeed")
 }

psiphon/controller.go

@@ -90,6 +90,10 @@ type candidateServerEntry struct {
 // NewController initializes a new controller.
 func NewController(config *Config) (controller *Controller, err error) {
 
+	if !config.IsCommitted() {
+		return nil, common.ContextError(errors.New("uncommitted config"))
+	}
+
 	// Needed by regen, at least
 	rand.Seed(int64(time.Now().Nanosecond()))
 
@@ -100,7 +104,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,
@@ -171,8 +175,6 @@ func (controller *Controller) Run(ctx context.Context) {
 	// an initial instance of any repetitive error notice, etc.
 	ResetRepetitiveNotices()
 
-	ReportAvailableRegions(controller.config)
-
 	runCtx, stopRunning := context.WithCancel(ctx)
 	defer stopRunning()
 
@@ -1183,7 +1185,7 @@ func (controller *Controller) launchEstablishing() {
 	// canceled when establishment is stopped.
 
 	doTactics := !controller.config.DisableTactics &&
-		controller.config.NetworkIDGetter != nil
+		controller.config.networkIDGetter != nil
 
 	if doTactics {
 
@@ -1212,6 +1214,20 @@ func (controller *Controller) launchEstablishing() {
 		}
 	}
 
+	// Unconditionally report available egress regions. After a fresh install,
+	// the outer client may not have a list of regions to display, so we
+	// always report here. Events that trigger ReportAvailableRegions,
+	// including storing new server entries and applying tactics, are not
+	// guaranteed to occur.
+	//
+	// This report is delayed until after tactics are likely to be applied, as
+	// tactics can impact the list of available regions; this avoids a
+	// ReportAvailableRegions reporting too many regions, followed shortly by
+	// a ReportAvailableRegions reporting fewer regions. That sequence could
+	// cause issues in the outer client UI.
+
+	ReportAvailableRegions(controller.config)
+
 	// The ConnectionWorkerPoolSize may be set by tactics.
 
 	size := controller.config.clientParameters.Get().Int(
@@ -1268,7 +1284,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)
 
@@ -1441,7 +1457,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

@@ -446,6 +446,8 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 		t.Skipf("error loading configuration file: %s", err)
 	}
 
+	// Note: a successful tactics request may modify config parameters.
+
 	// These fields must be filled in before calling LoadConfig
 	var modifyConfig map[string]interface{}
 	json.Unmarshal(configJSON, &modifyConfig)
@@ -468,6 +470,12 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 		modifyConfig["UpgradeDownloadClientVersionHeader"] = "invalid-value"
 	}
 
+	if runConfig.transformHostNames {
+		modifyConfig["TransformHostNames"] = "always"
+	} else {
+		modifyConfig["TransformHostNames"] = "never"
+	}
+
 	configJSON, _ = json.Marshal(modifyConfig)
 
 	config, err := LoadConfig(configJSON)
@@ -503,26 +511,16 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 		config.CustomHeaders = upstreamProxyCustomHeaders
 	}
 
+	// All config fields should be set before calling Commit.
+	err = config.Commit()
+	if err != nil {
+		t.Fatalf("error committing configuration file: %s", err)
+	}
+
 	// 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{}
-
-	// The following values can only be applied through client parameters.
-	// TODO: a successful tactics request can reset these parameters.
-
-	applyParameters := make(map[string]interface{})
-
-	if runConfig.transformHostNames {
-		applyParameters[parameters.TransformHostNameProbability] = 1.0
-	} else {
-		applyParameters[parameters.TransformHostNameProbability] = 0.0
-	}
-
-	err = config.SetClientParameters("", true, applyParameters)
-	if err != nil {
-		t.Fatalf("SetClientParameters failed: %s", err)
-	}
+	config.NetworkID = "NETWORK1"
 
 	os.Remove(config.UpgradeDownloadFilename)
 	os.Remove(config.RemoteServerListDownloadFilename)
@@ -1148,10 +1146,3 @@ func initUpstreamProxy() {
 
 	// TODO: wait until listener is active?
 }
-
-type testNetworkGetter struct {
-}
-
-func (testNetworkGetter) GetNetworkID() string {
-	return "NETWORK1"
-}

psiphon/feedback.go

@@ -108,6 +108,10 @@ func SendFeedback(configJson, diagnosticsJson, b64EncodedPublicKey, uploadServer
 	if err != nil {
 		return common.ContextError(err)
 	}
+	err = config.Commit()
+	if err != nil {
+		return common.ContextError(err)
+	}
 
 	untunneledDialConfig := &DialConfig{
 		UpstreamProxyURL:              config.UpstreamProxyURL,

psiphon/memory_test/memory_test.go

@@ -118,6 +118,10 @@ func runMemoryTest(t *testing.T, testMode int) {
 	if err != nil {
 		t.Fatalf("error processing configuration file: %s", err)
 	}
+	err = config.Commit()
+	if err != nil {
+		t.Fatalf("error committing configuration file: %s", err)
+	}
 
 	// Don't wait for a tactics request.
 	applyParameters := map[string]interface{}{

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/remoteServerList_test.go

@@ -391,7 +391,14 @@ func testObfuscatedRemoteServerLists(t *testing.T, omitMD5Sums bool) {
 		obfuscatedServerListDownloadDirectory,
 		disruptorProxyURL)
 
-	clientConfig, _ := LoadConfig([]byte(clientConfigJSON))
+	clientConfig, err := LoadConfig([]byte(clientConfigJSON))
+	if err != nil {
+		t.Fatalf("error processing configuration file: %s", err)
+	}
+	err = clientConfig.Commit()
+	if err != nil {
+		t.Fatalf("error committing configuration file: %s", err)
+	}
 
 	controller, err := NewController(clientConfig)
 	if err != nil {

psiphon/server/meek_test.go

@@ -356,10 +356,10 @@ func TestMeekResiliency(t *testing.T) {
 type fileDescriptorInterruptor struct {
 }
 
-func (interruptor *fileDescriptorInterruptor) BindToDevice(fileDescriptor int) error {
+func (interruptor *fileDescriptorInterruptor) BindToDevice(fileDescriptor int) (string, error) {
 	fdDup, err := syscall.Dup(fileDescriptor)
 	if err != nil {
-		return err
+		return "", err
 	}
 	minAfter := 500 * time.Millisecond
 	maxAfter := 1 * time.Second
@@ -369,5 +369,5 @@ func (interruptor *fileDescriptorInterruptor) BindToDevice(fileDescriptor int) e
 		syscall.Close(fdDup)
 		fmt.Printf("interrupted TCP connection\n")
 	})
-	return nil
+	return "", nil
 }

psiphon/server/server_test.go

@@ -544,7 +544,15 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 	localSOCKSProxyPort := 1081
 	localHTTPProxyPort := 8081
 
-	// Note: calling LoadConfig ensures the Config is fully initialized
+	jsonNetworkID := ""
+	if doTactics {
+		// Use a distinct prefix for network ID for each test run to
+		// ensure tactics from different runs don't apply; this is
+		// a workaround for the singleton datastore.
+		prefix := time.Now().String()
+		jsonNetworkID = fmt.Sprintf(`,"NetworkID" : "%s-%s"`, prefix, "NETWORK1")
+	}
+
 	clientConfigJSON := fmt.Sprintf(`
     {
         "ClientPlatform" : "Windows",
@@ -556,15 +564,15 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
         "EstablishTunnelPausePeriodSeconds" : 1,
         "ConnectionWorkerPoolSize" : %d,
         "TunnelProtocols" : ["%s"]
-    }`, numTunnels, runConfig.tunnelProtocol)
-	clientConfig, _ := psiphon.LoadConfig([]byte(clientConfigJSON))
+        %s
+    }`, numTunnels, runConfig.tunnelProtocol, jsonNetworkID)
 
-	clientConfig.DataStoreDirectory = testDataDirName
-	err = psiphon.InitDataStore(clientConfig)
+	clientConfig, err := psiphon.LoadConfig([]byte(clientConfigJSON))
 	if err != nil {
-		t.Fatalf("error initializing client datastore: %s", err)
+		t.Fatalf("error processing configuration file: %s", err)
 	}
-	psiphon.DeleteSLOKs()
+
+	clientConfig.DataStoreDirectory = testDataDirName
 
 	if !runConfig.doDefaultSponsorID {
 		clientConfig.SponsorId = sponsorID
@@ -584,12 +592,9 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 		clientConfig.Authorizations = []string{clientAuthorization}
 	}
 
-	if doTactics {
-		// Use a distinct prefix for network ID for each test run to
-		// 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}
+	err = clientConfig.Commit()
+	if err != nil {
+		t.Fatalf("error committing configuration file: %s", err)
 	}
 
 	if doTactics {
@@ -606,6 +611,12 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 		}
 	}
 
+	err = psiphon.InitDataStore(clientConfig)
+	if err != nil {
+		t.Fatalf("error initializing client datastore: %s", err)
+	}
+	psiphon.DeleteSLOKs()
+
 	controller, err := psiphon.NewController(clientConfig)
 	if err != nil {
 		t.Fatalf("error creating client controller: %s", err)
@@ -1259,11 +1270,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,

psiphon/userAgent_test.go

@@ -204,12 +204,20 @@ func attemptConnectionsWithUserAgent(
         "TransformHostNames" : "never",
         "UpstreamProxyUrl" : "http://127.0.0.1:2163"
     }`
-	clientConfig, _ := LoadConfig([]byte(clientConfigJSON))
+	clientConfig, err := LoadConfig([]byte(clientConfigJSON))
+	if err != nil {
+		t.Fatalf("error processing configuration file: %s", err)
+	}
 
 	clientConfig.TargetServerEntry = string(encodedServerEntry)
 	clientConfig.TunnelProtocol = tunnelProtocol
 	clientConfig.DataStoreDirectory = testDataDirName
 
+	err = clientConfig.Commit()
+	if err != nil {
+		t.Fatalf("error committing configuration file: %s", err)
+	}
+
 	err = InitDataStore(clientConfig)
 	if err != nil {
 		t.Fatalf("error initializing client datastore: %s", err)
@@ -246,7 +254,7 @@ func attemptConnectionsWithUserAgent(
 
 	// repeat attempts for long enough to select each user agent
 
-	time.Sleep(20 * time.Second)
+	time.Sleep(30 * time.Second)
 
 	cancelFunc()