Enable NoticeClientAddress for in-proxy protocols

- For indirect protocols, ClientAddress will report port 0
- Add server.sshClient.getClientIP helper

psiphon/config.go

@@ -514,7 +514,9 @@ type Config struct {
 	EmitServerAlerts bool `json:",omitempty"`
 
 	// EmitClientAddress indicates whether to emit the client's public network
-	// address, IP and port, as seen by the server.
+	// address, IP and port, as seen by the server. For non-direct protocols,
+	// such as FRONTED and INPROXY, where the client isn't connecting
+	// directly to the server, the reported port number is 0.
 	EmitClientAddress bool `json:",omitempty"`
 
 	// RateLimits specify throttling configuration for the tunnel.

psiphon/server/api.go

@@ -221,6 +221,10 @@ func handshakeAPIRequestHandler(
 			return nil, errors.Trace(err)
 		}
 
+		if net.ParseIP(inproxyClientIP) == nil {
+			return nil, errors.TraceNew("invalid inproxy client IP")
+		}
+
 		inproxyClientGeoIPData = support.GeoIPService.Lookup(inproxyClientIP)
 		clientGeoIPData = inproxyClientGeoIPData
 
@@ -352,20 +356,8 @@ func handshakeAPIRequestHandler(
 	var encodedServerList []string
 	if !sshClient.getDisableDiscovery() {
 
-		clientIP := ""
-		if sshClient.isInproxyTunnelProtocol {
-			clientIP = inproxyClientIP
-		} else if sshClient.peerAddr != nil {
-			clientIP, _, _ = net.SplitHostPort(sshClient.peerAddr.String())
-
-		}
-
-		IP := net.ParseIP(clientIP)
-		if IP == nil {
-			return nil, errors.TraceNew("invalid client IP")
-		}
-
-		encodedServerList = support.discovery.DiscoverServers(IP)
+		encodedServerList = support.discovery.DiscoverServers(
+			net.ParseIP(sshClient.getClientIP()))
 	}
 
 	// When the client indicates that it used an out-of-date server entry for
@@ -408,22 +400,23 @@ func handshakeAPIRequestHandler(
 		isMobile)
 
 	clientAddress := ""
-	if sshClient.isInproxyTunnelProtocol {
+	if protocol.TunnelProtocolIsDirect(sshClient.tunnelProtocol) {
 
-		// ClientAddress not supported for in-proxy tunnel protocols:
-		//
-		// - We don't want to return the address of the direct peer, the
-		//   in-proxy proxy;
-		// - The known  port number will correspond to the in-proxy proxy
-		//   source address, not the client;
-		// - While we assume that the the original client IP from the broker
-		//   is representative for geolocation, an actual direct connection
-		//   to the Psiphon server from the client may route differently and
-		//   use a different IP address.
-
-		clientAddress = ""
-	} else if sshClient.peerAddr != nil {
 		clientAddress = sshClient.peerAddr.String()
+
+	} else {
+
+		// Limitations for indirect protocols:
+		//
+		// - The peer port number will not correspond to the original client's
+		//   source port; 0 is reported instead.
+		//
+		// - For in-proxy protocols, while we assume that the the original
+		//   client IP from the broker is representative for geolocation, an
+		//   actual direct connection to the Psiphon server from the client
+		//   may route differently and use a different IP address.
+
+		clientAddress = net.JoinHostPort(sshClient.getClientIP(), "0")
 	}
 
 	var marshaledTacticsPayload []byte

psiphon/server/discovery.go

@@ -155,6 +155,10 @@ func (d *Discovery) DiscoverServers(clientIP net.IP) []string {
 	d.RLock()
 	defer d.RUnlock()
 
+	if clientIP == nil {
+		return []string{}
+	}
+
 	servers := d.discovery.SelectServers(clientIP)
 
 	encodedServerEntries := make([]string, 0)

psiphon/server/tunnelServer.go

@@ -1811,7 +1811,7 @@ func (sshServer *sshServer) handleClient(
 		}
 	}
 
-	sshClient := newSshClient(
+	sshClient, err := newSshClient(
 		sshServer,
 		sshListener,
 		tunnelProtocol,
@@ -1820,6 +1820,12 @@ func (sshServer *sshServer) handleClient(
 		replayedServerPacketManipulation,
 		peerAddr,
 		peerGeoIPData)
+	if err != nil {
+		conn.Close()
+		log.WithTraceFields(LogFields{"error": err}).Warning(
+			"newSshClient failed")
+		return
+	}
 
 	// sshClient.run _must_ call onSSHHandshakeFinished to release the semaphore:
 	// in any error case; or, as soon as the SSH handshake phase has successfully
@@ -1924,6 +1930,7 @@ type sshClient struct {
 	replayedServerPacketManipulation     bool
 	peerAddr                             net.Addr
 	peerGeoIPData                        GeoIPData
+	clientIP                             string
 	clientGeoIPData                      GeoIPData
 	sessionID                            string
 	isFirstTunnelInSession               bool
@@ -2291,7 +2298,7 @@ func newSshClient(
 	serverPacketManipulation string,
 	replayedServerPacketManipulation bool,
 	peerAddr net.Addr,
-	peerGeoIPData GeoIPData) *sshClient {
+	peerGeoIPData GeoIPData) (*sshClient, error) {
 
 	runCtx, stopRunning := context.WithCancel(context.Background())
 
@@ -2308,6 +2315,7 @@ func newSshClient(
 		serverPacketManipulation:         serverPacketManipulation,
 		replayedServerPacketManipulation: replayedServerPacketManipulation,
 		peerAddr:                         peerAddr,
+		peerGeoIPData:                    peerGeoIPData,
 		isFirstTunnelInSession:           true,
 		qualityMetrics:                   newQualityMetrics(),
 		tcpPortForwardLRU:                common.NewLRUConns(),
@@ -2322,17 +2330,35 @@ func newSshClient(
 	client.tcpTrafficState.availablePortForwardCond = sync.NewCond(new(sync.Mutex))
 	client.udpTrafficState.availablePortForwardCond = sync.NewCond(new(sync.Mutex))
 
-	// In the case of in-proxy tunnel protocols, clientGeoIPData is not set
-	// until the original client IP is relayed from the broker during the
-	// handshake. In other cases, clientGeoIPData is the peerGeoIPData
-	// (this includes fronted meek).
+	if peerAddr == nil {
+		return nil, errors.TraceNew("missing peerAddr")
+	}
+	peerIP, _, err := net.SplitHostPort(peerAddr.String())
+	if err != nil {
+		return nil, errors.Trace(err)
+	}
+	if net.ParseIP(peerIP) == nil {
+		return nil, errors.TraceNew("invalid peerIP")
+	}
+
+	// In the case of in-proxy tunnel protocols, clientIP and clientGeoIPData
+	// are not set until the original client IP is relayed from the broker
+	// during the handshake. In other cases, clientGeoIPData is the
+	// peerGeoIPData (this includes fronted meek).
 
-	client.peerGeoIPData = peerGeoIPData
 	if !client.isInproxyTunnelProtocol {
+		client.clientIP = peerIP
 		client.clientGeoIPData = peerGeoIPData
 	}
 
-	return client
+	return client, nil
+}
+
+// getClientP gets sshClient.clientIP. See getClientGeoIPData comment.
+func (sshClient *sshClient) getClientIP() string {
+	sshClient.Lock()
+	defer sshClient.Unlock()
+	return sshClient.clientIP
 }
 
 // getClientGeoIPData gets sshClient.clientGeoIPData. Use this helper when
@@ -4106,9 +4132,11 @@ func (sshClient *sshClient) setHandshakeState(
 
 		if sshClient.isInproxyTunnelProtocol {
 
-			// Set the client GeoIP data to the value obtained using the
-			// original client IP, from the broker, in the handshake. Also
-			// update the GeoIP session hash to use the client GeoIP data.
+			// Set the client IP and GeoIP data to the value obtained using
+			// the original client IP, from the broker, in the handshake.
+			// Also update the GeoIP session hash to use the client GeoIP data.
+
+			sshClient.clientIP = sshClient.handshakeState.inproxyClientIP
 
 			sshClient.clientGeoIPData =
 				sshClient.handshakeState.inproxyClientGeoIPData

psiphon/serverApi.go

@@ -267,10 +267,8 @@ func (serverContext *ServerContext) doHandshakeRequest(ignoreStatsRegexps bool)
 		return errors.Trace(err)
 	}
 
-	// Limitation: ClientAddress is not supported for in-proxy tunnel
-	// protocols; see comment in server.handshakeAPIRequestHandler.
 	if serverContext.tunnel.config.EmitClientAddress &&
-		!protocol.TunnelProtocolUsesInproxy(serverContext.tunnel.dialParams.TunnelProtocol) {
+		handshakeResponse.ClientAddress != "" {
 
 		NoticeClientAddress(handshakeResponse.ClientAddress)
 	}