Merge branch 'master' into staging-client

ClientLibrary/clientlib/clientlib.go

@@ -156,7 +156,7 @@ func StartTunnel(ctx context.Context,
 
 	// config.Commit must be called before calling config.SetClientParameters
 	// or attempting to connect.
-	err = config.Commit()
+	err = config.Commit(true)
 	if err != nil {
 		return nil, errors.TraceMsg(err, "config.Commit failed")
 	}

ConsoleClient/main.go

@@ -209,7 +209,7 @@ func main() {
 
 	// All config fields should be set before calling Commit.
 
-	err = config.Commit()
+	err = config.Commit(true)
 	if err != nil {
 		psiphon.SetEmitDiagnosticNotices(true, false)
 		psiphon.NoticeError("error loading configuration file: %s", err)

MobileLibrary/psi/psi.go

@@ -152,7 +152,7 @@ func Start(
 
 	// All config fields should be set before calling Commit.
 
-	err = config.Commit()
+	err = config.Commit(true)
 	if err != nil {
 		return fmt.Errorf("error committing configuration file: %s", err)
 	}

psiphon/common/logger.go

@@ -50,3 +50,9 @@ type MetricsSource interface {
 	// metrics from the MetricsSource
 	GetMetrics() LogFields
 }
+
+func SetIrregularTunnelErrorLogField(
+	logFields LogFields, tunnelError error) {
+
+	logFields["tunnel_error"] = tunnelError
+}

psiphon/common/net.go

@@ -39,18 +39,25 @@ type NetDialer interface {
 	DialContext(ctx context.Context, network, address string) (net.Conn, error)
 }
 
-// Closer defines the interface to a type, typically
-// a net.Conn, that can be closed.
+// Closer defines the interface to a type, typically a net.Conn, that can be
+// closed.
 type Closer interface {
 	IsClosed() bool
 }
 
-// CloseWriter defines the interface to a type, typically
-// a net.TCPConn, that implements CloseWrite.
+// CloseWriter defines the interface to a type, typically a net.TCPConn, that
+// implements CloseWrite.
 type CloseWriter interface {
 	CloseWrite() error
 }
 
+// IrregularIndicator defines the interface for a type, typically a net.Conn,
+// that detects and reports irregular conditions during initial network
+// connection establishment.
+type IrregularIndicator interface {
+	IrregularTunnelError() error
+}
+
 // TerminateHTTPConnection sends a 404 response to a client and also closes
 // the persistent connection.
 func TerminateHTTPConnection(

psiphon/common/obfuscator/obfuscator.go

@@ -304,7 +304,7 @@ func readSeedMessage(
 		errStr := "duplicate obfuscation seed"
 		if duplicateLogFields != nil {
 			if config.IrregularLogger != nil {
-				setIrregularTunnelErrorLogField(
+				common.SetIrregularTunnelErrorLogField(
 					*duplicateLogFields, errors.BackTraceNew(errBackTrace, errStr))
 				config.IrregularLogger(clientIP, *duplicateLogFields)
 			}
@@ -357,7 +357,7 @@ func readSeedMessage(
 	if errStr != "" {
 		if config.IrregularLogger != nil {
 			errLogFields := make(common.LogFields)
-			setIrregularTunnelErrorLogField(
+			common.SetIrregularTunnelErrorLogField(
 				errLogFields, errors.BackTraceNew(errBackTrace, errStr))
 			config.IrregularLogger(clientIP, errLogFields)
 		}
@@ -393,9 +393,3 @@ func readSeedMessage(
 
 	return clientToServerCipher, serverToClientCipher, paddingPRNGSeed, nil
 }
-
-func setIrregularTunnelErrorLogField(
-	logFields common.LogFields, tunnelError error) {
-
-	logFields["tunnel_error"] = tunnelError
-}

psiphon/common/tapdance/tapdance.go

@@ -117,8 +117,8 @@ func (l *stationListener) Accept() (net.Conn, error) {
 		return nil, errors.TraceNew("missing station address")
 	}
 	return &stationConn{
-		Conn:              conn,
-		stationRemoteAddr: stationRemoteAddr,
+		Conn:             conn,
+		stationIPAddress: common.IPAddressFromAddr(stationRemoteAddr),
 	}, nil
 }
 
@@ -132,13 +132,38 @@ func (l *stationListener) Addr() net.Addr {
 
 type stationConn struct {
 	net.Conn
-	stationRemoteAddr net.Addr
+	stationIPAddress string
+}
+
+// IrregularTunnelError implements the common.IrregularIndicator interface.
+func (c *stationConn) IrregularTunnelError() error {
+
+	// We expect a PROXY protocol header, but go-proxyproto does not produce an
+	// error if the "PROXY " prefix is absent; instead the connection will
+	// proceed. To detect this case, check if the go-proxyproto RemoteAddr IP
+	// address matches the underlying connection IP address. When these values
+	// match, there was no PROXY protocol header.
+	//
+	// Limitation: the values will match if there is a PROXY protocol header
+	// containing the same IP address as the underlying connection. This is not
+	// an expected case.
+
+	if common.IPAddressFromAddr(c.RemoteAddr()) == c.stationIPAddress {
+		return errors.TraceNew("unexpected station IP address")
+	}
+	return nil
 }
 
 // GetMetrics implements the common.MetricsSource interface.
 func (c *stationConn) GetMetrics() common.LogFields {
+
 	logFields := make(common.LogFields)
-	logFields["station_ip_address"] = common.IPAddressFromAddr(c.stationRemoteAddr)
+
+	// Ensure we don't log a potential non-station IP address.
+	if c.IrregularTunnelError() == nil {
+		logFields["station_ip_address"] = c.stationIPAddress
+	}
+
 	return logFields
 }
 

psiphon/config.go

@@ -765,7 +765,10 @@ func (config *Config) IsCommitted() bool {
 // Config fields should not be set after calling Config, as any changes may
 // not be reflected in internal data structures.
 //
-// File migrations:
+// If migrateFromLegacyFields is set to true, then an attempt to migrate from
+// legacy fields is made.
+//
+// Migration from legacy fields:
 // Config fields of the naming Migrate* (e.g. MigrateDataStoreDirectory) specify
 // a file migration operation which should be performed. These fields correspond
 // to deprecated fields, which previously could be used to specify where Psiphon
@@ -793,7 +796,7 @@ func (config *Config) IsCommitted() bool {
 // fails. It is better to not endlessly retry file migrations on each Commit()
 // because file system errors are expected to be rare and persistent files will
 // be re-populated over time.
-func (config *Config) Commit() error {
+func (config *Config) Commit(migrateFromLegacyFields bool) error {
 
 	// Do SetEmitDiagnosticNotices first, to ensure config file errors are
 	// emitted.
@@ -842,26 +845,28 @@ func (config *Config) Commit() error {
 	homepageFilePath := config.GetHomePageFilename()
 	noticesFilePath := config.GetNoticesFilename()
 
-	if needMigration {
+	if migrateFromLegacyFields {
+		if needMigration {
 
-		// Move notice files that exist at legacy file paths under the data root
-		// directory.
+			// Move notice files that exist at legacy file paths under the data root
+			// directory.
 
-		noticeMigrationInfoMsgs = append(noticeMigrationInfoMsgs, "Config migration: need migration")
-		noticeMigrations := migrationsFromLegacyNoticeFilePaths(config)
+			noticeMigrationInfoMsgs = append(noticeMigrationInfoMsgs, "Config migration: need migration")
+			noticeMigrations := migrationsFromLegacyNoticeFilePaths(config)
 
-		for _, migration := range noticeMigrations {
-			err := common.DoFileMigration(migration)
-			if err != nil {
-				alertMsg := fmt.Sprintf("Config migration: %s", errors.Trace(err))
-				noticeMigrationAlertMsgs = append(noticeMigrationAlertMsgs, alertMsg)
-			} else {
-				infoMsg := fmt.Sprintf("Config migration: moved %s to %s", migration.OldPath, migration.NewPath)
-				noticeMigrationInfoMsgs = append(noticeMigrationInfoMsgs, infoMsg)
+			for _, migration := range noticeMigrations {
+				err := common.DoFileMigration(migration)
+				if err != nil {
+					alertMsg := fmt.Sprintf("Config migration: %s", errors.Trace(err))
+					noticeMigrationAlertMsgs = append(noticeMigrationAlertMsgs, alertMsg)
+				} else {
+					infoMsg := fmt.Sprintf("Config migration: moved %s to %s", migration.OldPath, migration.NewPath)
+					noticeMigrationInfoMsgs = append(noticeMigrationInfoMsgs, infoMsg)
+				}
 			}
+		} else {
+			noticeMigrationInfoMsgs = append(noticeMigrationInfoMsgs, "Config migration: migration already completed")
 		}
-	} else {
-		noticeMigrationInfoMsgs = append(noticeMigrationInfoMsgs, "Config migration: migration already completed")
 	}
 
 	if config.UseNoticeFiles != nil {
@@ -1093,19 +1098,18 @@ func (config *Config) Commit() error {
 
 	// Migrate from old config fields. This results in files being moved under
 	// a config specified data root directory.
+	if migrateFromLegacyFields && needMigration {
 
-	// If unset, set MigrateDataStoreDirectory to the previous default value for
-	// DataStoreDirectory to ensure that datastore files are migrated.
-	if config.MigrateDataStoreDirectory == "" {
-		wd, err := os.Getwd()
-		if err != nil {
-			return errors.Trace(err)
+		// If unset, set MigrateDataStoreDirectory to the previous default value for
+		// DataStoreDirectory to ensure that datastore files are migrated.
+		if config.MigrateDataStoreDirectory == "" {
+			wd, err := os.Getwd()
+			if err != nil {
+				return errors.Trace(err)
+			}
+			NoticeInfo("MigrateDataStoreDirectory unset, using working directory %s", wd)
+			config.MigrateDataStoreDirectory = wd
 		}
-		NoticeInfo("MigrateDataStoreDirectory unset, using working directory %s", wd)
-		config.MigrateDataStoreDirectory = wd
-	}
-
-	if needMigration {
 
 		// Move files that exist at legacy file paths under the data root
 		// directory.

psiphon/config_test.go

@@ -104,7 +104,7 @@ func TestConfigTestSuite(t *testing.T) {
 func (suite *ConfigTestSuite) Test_LoadConfig_BasicGood() {
 	config, err := LoadConfig(suite.confStubBlob)
 	if err == nil {
-		err = config.Commit()
+		err = config.Commit(false)
 	}
 	suite.Nil(err, "a basic config should succeed")
 }
@@ -129,7 +129,7 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 			`{"f1": 11, "f2": "two", "DataRootDirectory" : %s}`,
 			suite.testDirectory)))
 	if err == nil {
-		err = config.Commit()
+		err = config.Commit(false)
 	}
 	suite.NotNil(err, "JSON with none of our fields should fail")
 
@@ -141,7 +141,7 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 		testObjJSON, _ = json.Marshal(testObj)
 		config, err = LoadConfig(testObjJSON)
 		if err == nil {
-			err = config.Commit()
+			err = config.Commit(false)
 		}
 		suite.NotNil(err, "JSON with one of our required fields missing should fail: %s", field)
 
@@ -151,7 +151,7 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 		testObjJSON, _ = json.Marshal(testObj)
 		config, err = LoadConfig(testObjJSON)
 		if err == nil {
-			err = config.Commit()
+			err = config.Commit(false)
 		}
 		suite.NotNil(err, "JSON with one of our required fields with the wrong type should fail: %s", field)
 
@@ -161,7 +161,7 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 		testObjJSON, _ = json.Marshal(testObj)
 		config, err = LoadConfig(testObjJSON)
 		if err == nil {
-			err = config.Commit()
+			err = config.Commit(false)
 		}
 		suite.NotNil(err, "JSON with one of our required fields set to null should fail: %s", field)
 
@@ -171,7 +171,7 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 		testObjJSON, _ = json.Marshal(testObj)
 		config, err = LoadConfig(testObjJSON)
 		if err == nil {
-			err = config.Commit()
+			err = config.Commit(false)
 		}
 		suite.NotNil(err, "JSON with one of our required fields set to an empty string should fail: %s", field)
 	}
@@ -184,7 +184,7 @@ func (suite *ConfigTestSuite) Test_LoadConfig_BadJson() {
 		testObjJSON, _ = json.Marshal(testObj)
 		config, err = LoadConfig(testObjJSON)
 		if err == nil {
-			err = config.Commit()
+			err = config.Commit(false)
 		}
 		suite.NotNil(err, "JSON with one of our optional fields with the wrong type should fail: %s", field)
 	}
@@ -205,14 +205,14 @@ func (suite *ConfigTestSuite) Test_LoadConfig_GoodJson() {
 	testObjJSON, _ = json.Marshal(testObj)
 	config, err := LoadConfig(testObjJSON)
 	if err == nil {
-		err = config.Commit()
+		err = config.Commit(false)
 	}
 	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
 	config, err = LoadConfig(suite.confStubBlob)
 	if err == nil {
-		err = config.Commit()
+		err = config.Commit(false)
 	}
 	suite.Nil(err, "JSON with all good values for required and optional fields should succeed")
 
@@ -224,7 +224,7 @@ func (suite *ConfigTestSuite) Test_LoadConfig_GoodJson() {
 	testObjJSON, _ = json.Marshal(testObj)
 	config, err = LoadConfig(testObjJSON)
 	if err == nil {
-		err = config.Commit()
+		err = config.Commit(false)
 	}
 	suite.Nil(err, "JSON with null for optional values should succeed")
 }
@@ -402,7 +402,7 @@ func LoadConfigMigrateTest(oslDirChildrenPreMigration []FileTree, oslDirChildren
 	}
 
 	// Commit config, this is where file migration happens
-	err = config.Commit()
+	err = config.Commit(true)
 	if err != nil {
 		suite.T().Fatal("Error committing config:", err)
 		return

psiphon/controller_test.go

@@ -561,7 +561,7 @@ func controllerRun(t *testing.T, runConfig *controllerRunConfig) {
 	}
 
 	// All config fields should be set before calling Commit.
-	err = config.Commit()
+	err = config.Commit(false)
 	if err != nil {
 		t.Fatalf("error committing configuration file: %s", err)
 	}

psiphon/dataStoreRecovery_test.go

@@ -66,7 +66,7 @@ func TestBoltResiliency(t *testing.T) {
 	if err != nil {
 		t.Fatalf("LoadConfig failed: %s", err)
 	}
-	err = clientConfig.Commit()
+	err = clientConfig.Commit(false)
 	if err != nil {
 		t.Fatalf("Commit failed: %s", err)
 	}

psiphon/dialParameters_test.go

@@ -71,7 +71,7 @@ func runDialParametersAndReplay(t *testing.T, tunnelProtocol string) {
 		NetworkIDGetter:      new(testNetworkGetter),
 	}
 
-	err = clientConfig.Commit()
+	err = clientConfig.Commit(false)
 	if err != nil {
 		t.Fatalf("error committing configuration file: %s", err)
 	}

psiphon/exchange_test.go

@@ -82,7 +82,7 @@ func TestServerEntryExchange(t *testing.T) {
 	if err != nil {
 		t.Fatalf("LoadConfig failed: %s", err)
 	}
-	err = config.Commit()
+	err = config.Commit(false)
 	if err != nil {
 		t.Fatalf("Commit failed: %s", err)
 	}

psiphon/feedback.go

@@ -107,7 +107,7 @@ func SendFeedback(configJson, diagnosticsJson, b64EncodedPublicKey, uploadServer
 	if err != nil {
 		return errors.Trace(err)
 	}
-	err = config.Commit()
+	err = config.Commit(true)
 	if err != nil {
 		return errors.Trace(err)
 	}

psiphon/limitProtocols_test.go

@@ -109,7 +109,7 @@ func TestLimitTunnelProtocols(t *testing.T) {
 
 	clientConfig.DataRootDirectory = testDataDirName
 
-	err = clientConfig.Commit()
+	err = clientConfig.Commit(false)
 	if err != nil {
 		t.Fatalf("error committing configuration file: %s", err)
 	}