Split psiphon.Config initialization into two phases

- LoadConfig parses JSON and populates Config.

- Commit finalizes initialization after Config
  fields are further populated programatically.

- Ensures that fields are properly used when
  set after LoadConfig.

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,22 +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()
-	}
-
 	// Run Psiphon
 
 	controller, err := psiphon.NewController(config)

MobileLibrary/psi/psi.go

@@ -118,11 +118,19 @@ func Start(
 		config.IPv6Synthesizer = provider
 	}
 
+	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)

psiphon/config.go

@@ -461,10 +461,18 @@ type Config struct {
 
 	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
@@ -473,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 {
@@ -501,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 == "" {
@@ -518,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"))
 	}
 
@@ -551,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"))
 	}
 
@@ -559,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"))
 			}
 		}
 
@@ -579,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.
@@ -606,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
 	}
@@ -615,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(
@@ -623,14 +646,14 @@ 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)
 	}
 
 	// Initialize config.deviceBinder and config.config.networkIDGetter. These
@@ -662,7 +685,9 @@ func LoadConfig(configJson []byte) (*Config, error) {
 		config.networkIDGetter = &loggingNetworkIDGetter{networkIDGetter}
 	}
 
-	return &config, nil
+	config.committed = true
+
+	return nil
 }
 
 // GetClientParameters returns a snapshot of the current client parameters.

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()))
 

psiphon/controller_test.go

@@ -511,6 +511,12 @@ 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.
@@ -1140,4 +1146,3 @@ func initUpstreamProxy() {
 
 	// TODO: wait until listener is active?
 }
-

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

@@ -553,7 +553,6 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 		jsonNetworkID = fmt.Sprintf(`,"NetworkID" : "%s-%s"`, prefix, "NETWORK1")
 	}
 
-	// Note: calling LoadConfig ensures the Config is fully initialized
 	clientConfigJSON := fmt.Sprintf(`
     {
         "ClientPlatform" : "Windows",
@@ -567,14 +566,13 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
         "TunnelProtocols" : ["%s"]
         %s
     }`, numTunnels, runConfig.tunnelProtocol, jsonNetworkID)
-	clientConfig, _ := psiphon.LoadConfig([]byte(clientConfigJSON))
 
-	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
@@ -594,6 +592,11 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 		clientConfig.Authorizations = []string{clientAuthorization}
 	}
 
+	err = clientConfig.Commit()
+	if err != nil {
+		t.Fatalf("error committing configuration file: %s", err)
+	}
+
 	if doTactics {
 		// Configure nonfunctional values that must be overridden by tactics.
 
@@ -608,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)

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)