Redact sensitive data from diagnostic notices

- Never log server IP addresses. Servers are
  now identified by a short ID derived from the
  server entry tag.

- A regex is used to strip any remaining IP
  addresses from notice text. This covers cases
  such as "net" package error messages containing
  addresses.

- EmitDiagnosticNetworkParameters introduces a
  second level of detail for EmitDiagnosticNotices.
  By default, EmitDiagnosticNotices will not
  log dial parameters or resource URLs and error
  messages that may contain domain names are
  redacted. This data may be logged by enabling the
  new flag.

ConsoleClient/main.go

@@ -161,22 +161,22 @@ func main() {
 	// Handle required config file parameter
 
 	// EmitDiagnosticNotices is set by LoadConfig; force to true
-	// an emit diagnostics when LoadConfig-related errors occur.
+	// and emit diagnostics when LoadConfig-related errors occur.
 
 	if configFilename == "" {
-		psiphon.SetEmitDiagnosticNotices(true)
+		psiphon.SetEmitDiagnosticNotices(true, false)
 		psiphon.NoticeError("configuration file is required")
 		os.Exit(1)
 	}
 	configFileContents, err := ioutil.ReadFile(configFilename)
 	if err != nil {
-		psiphon.SetEmitDiagnosticNotices(true)
+		psiphon.SetEmitDiagnosticNotices(true, false)
 		psiphon.NoticeError("error loading configuration file: %s", err)
 		os.Exit(1)
 	}
 	config, err := psiphon.LoadConfig(configFileContents)
 	if err != nil {
-		psiphon.SetEmitDiagnosticNotices(true)
+		psiphon.SetEmitDiagnosticNotices(true, false)
 		psiphon.NoticeError("error processing configuration file: %s", err)
 		os.Exit(1)
 	}
@@ -191,7 +191,7 @@ func main() {
 		tunDeviceFile, err := configurePacketTunnel(
 			config, tunDevice, tunBindInterface, tunPrimaryDNS, tunSecondaryDNS)
 		if err != nil {
-			psiphon.SetEmitDiagnosticNotices(true)
+			psiphon.SetEmitDiagnosticNotices(true, false)
 			psiphon.NoticeError("error configuring packet tunnel: %s", err)
 			os.Exit(1)
 		}
@@ -202,7 +202,7 @@ func main() {
 
 	err = config.Commit()
 	if err != nil {
-		psiphon.SetEmitDiagnosticNotices(true)
+		psiphon.SetEmitDiagnosticNotices(true, false)
 		psiphon.NoticeError("error loading configuration file: %s", err)
 		os.Exit(1)
 	}

psiphon/LookupIP.go

@@ -62,7 +62,9 @@ func LookupIP(ctx context.Context, host string, config *DialConfig) ([]net.IP, e
 			return ips, err
 		}
 
-		NoticeAlert("retry resolve host %s: %s", host, err)
+		if GetEmitNetworkParameters() {
+			NoticeAlert("retry resolve host %s: %s", host, err)
+		}
 
 		return bindLookupIP(ctx, host, dnsServer, config)
 	}
@@ -72,6 +74,11 @@ func LookupIP(ctx context.Context, host string, config *DialConfig) ([]net.IP, e
 		return nil, common.ContextError(err)
 	}
 
+	// Remove domain names from "net" error messages.
+	if !GetEmitNetworkParameters() {
+		err = RedactNetError(err)
+	}
+
 	ips := make([]net.IP, len(addrs))
 	for i, addr := range addrs {
 		ips[i] = addr.IP
@@ -116,7 +123,7 @@ func bindLookupIP(
 		copy(ipv6[:], ipAddr.To16())
 		domain = syscall.AF_INET6
 	} else {
-		return nil, common.ContextError(fmt.Errorf("invalid IP address for dns server: %s", ipAddr.String()))
+		return nil, common.ContextError(errors.New("invalid IP address for DNS server"))
 	}
 
 	socketFd, err := syscall.Socket(domain, syscall.SOCK_DGRAM, 0)
@@ -127,7 +134,7 @@ func bindLookupIP(
 	_, err = config.DeviceBinder.BindToDevice(socketFd)
 	if err != nil {
 		syscall.Close(socketFd)
-		return nil, common.ContextError(fmt.Errorf("BindToDevice failed: %s", err))
+		return nil, common.ContextError(fmt.Errorf("BindToDevice failed with %s", err))
 	}
 
 	// Connect socket to the server's IP address

psiphon/LookupIP_nobind.go

@@ -41,6 +41,11 @@ func LookupIP(ctx context.Context, host string, config *DialConfig) ([]net.IP, e
 		return nil, common.ContextError(err)
 	}
 
+	// Remove domain names from "net" error messages.
+	if !GetEmitNetworkParameters() {
+		err = RedactNetError(err)
+	}
+
 	ips := make([]net.IP, len(addrs))
 	for i, addr := range addrs {
 		ips[i] = addr.IP

psiphon/TCPConn.go

@@ -84,7 +84,9 @@ func DialTCP(
 	if config.FragmentorConfig.MayFragment() {
 		conn = fragmentor.NewConn(
 			config.FragmentorConfig,
-			func(message string) { NoticeInfo(message) },
+			func(message string) {
+				NoticeFragmentor(config.DiagnosticID, message)
+			},
 			conn)
 	}
 

psiphon/TCPConn_bind.go

@@ -117,7 +117,7 @@ func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, er
 			copy(ipv6[:], ipAddr.To16())
 			domain = syscall.AF_INET6
 		} else {
-			lastErr = common.ContextError(fmt.Errorf("invalid IP address: %s", ipAddr.String()))
+			lastErr = common.ContextError(errors.New("invalid IP address"))
 			continue
 		}
 		if domain == syscall.AF_INET {
@@ -142,7 +142,7 @@ func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, er
 			_, err = config.DeviceBinder.BindToDevice(socketFD)
 			if err != nil {
 				syscall.Close(socketFD)
-				lastErr = common.ContextError(fmt.Errorf("BindToDevice failed: %s", err))
+				lastErr = common.ContextError(fmt.Errorf("BindToDevice failed with %s", err))
 				continue
 			}
 		}

psiphon/TCPConn_nobind.go

@@ -40,6 +40,11 @@ func tcpDial(ctx context.Context, addr string, config *DialConfig) (net.Conn, er
 
 	conn, err := dialer.DialContext(ctx, "tcp", addr)
 
+	// Remove domain names from "net" error messages.
+	if !GetEmitNetworkParameters() {
+		err = RedactNetError(err)
+	}
+
 	if err != nil {
 		return nil, common.ContextError(err)
 	}

psiphon/common/fragmentor/fragmentor.go

@@ -259,14 +259,7 @@ func (c *Conn) Write(buffer []byte) (int, error) {
 	var notice bytes.Buffer
 
 	if emitNotice {
-		remoteAddrStr := "(nil)"
-		remoteAddr := c.Conn.RemoteAddr()
-		if remoteAddr != nil {
-			remoteAddrStr = remoteAddr.String()
-		}
-		fmt.Fprintf(&notice,
-			"fragment %s %d bytes:",
-			remoteAddrStr, len(buffer))
+		fmt.Fprintf(&notice, "fragment %d bytes:", len(buffer))
 	}
 
 	for iterations := 0; len(buffer) > 0; iterations += 1 {

psiphon/common/protocol/serverEntry.go

@@ -155,6 +155,18 @@ func (fields ServerEntryFields) SetTag(tag string) {
 	fields["isLocalDerivedTag"] = true
 }
 
+func (fields ServerEntryFields) GetDiagnosticID() string {
+	tag, ok := fields["tag"]
+	if !ok {
+		return ""
+	}
+	tagStr, ok := tag.(string)
+	if !ok {
+		return ""
+	}
+	return TagToDiagnosticID(tagStr)
+}
+
 func (fields ServerEntryFields) GetIPAddress() string {
 	ipAddress, ok := fields["ipAddress"]
 	if !ok {
@@ -507,6 +519,10 @@ func (serverEntry *ServerEntry) HasSignature() bool {
 	return serverEntry.Signature != ""
 }
 
+func (serverEntry *ServerEntry) GetDiagnosticID() string {
+	return TagToDiagnosticID(serverEntry.Tag)
+}
+
 // GenerateServerEntryTag creates a server entry tag value that is
 // cryptographically derived from the IP address and web server secret in a
 // way that is difficult to reverse the IP address value from the tag or
@@ -521,6 +537,18 @@ func GenerateServerEntryTag(ipAddress, webServerSecret string) string {
 	return base64.StdEncoding.EncodeToString(h.Sum(nil))
 }
 
+// TagToDiagnosticID returns a prefix of the server entry tag that should be
+// sufficient to uniquely identify servers in diagnostics, while also being
+// more human readable than emitting the full tag. The tag is used as the base
+// of the diagnostic ID as it doesn't leak the server IP address in diagnostic
+// output.
+func TagToDiagnosticID(tag string) string {
+	if len(tag) < 8 {
+		return "<unknown>"
+	}
+	return tag[:8]
+}
+
 // EncodeServerEntry returns a string containing the encoding of
 // a ServerEntry following Psiphon conventions.
 func EncodeServerEntry(serverEntry *ServerEntry) (string, error) {

psiphon/config.go

@@ -447,10 +447,16 @@ type Config struct {
 
 	// EmitDiagnosticNotices indicates whether to output notices containing
 	// detailed information about the Psiphon session. As these notices may
-	// contain sensitive network information, they should not be insecurely
-	// distributed or displayed to users. Default is off.
+	// contain sensitive information, they should not be insecurely distributed
+	// or displayed to users. Default is off.
 	EmitDiagnosticNotices bool
 
+	// EmitDiagnosticNetworkParameters indicates whether to include network
+	// parameters in diagnostic notices. As these parameters are sensitive
+	// circumvention network information, they should not be insecurely
+	// distributed or displayed to users. Default is off.
+	EmitDiagnosticNetworkParameters bool
+
 	// RateLimits specify throttling configuration for the tunnel.
 	RateLimits common.RateLimits
 
@@ -596,10 +602,11 @@ func (config *Config) IsCommitted() bool {
 // not be reflected in internal data structures.
 func (config *Config) Commit() error {
 
-	// Do SetEmitDiagnosticNotices first, to ensure config file errors are emitted.
-
+	// Do SetEmitDiagnosticNotices first, to ensure config file errors are
+	// emitted.
 	if config.EmitDiagnosticNotices {
-		SetEmitDiagnosticNotices(true)
+		SetEmitDiagnosticNotices(
+			true, config.EmitDiagnosticNetworkParameters)
 	}
 
 	// Promote legacy fields.

psiphon/controller.go

@@ -292,7 +292,7 @@ func (controller *Controller) TerminateNextActiveTunnel() {
 	tunnel := controller.getNextActiveTunnel()
 	if tunnel != nil {
 		controller.SignalTunnelFailure(tunnel)
-		NoticeInfo("terminated tunnel: %s", tunnel.dialParams.ServerEntry.IpAddress)
+		NoticeInfo("terminated tunnel: %s", tunnel.dialParams.ServerEntry.GetDiagnosticID())
 	}
 }
 
@@ -643,7 +643,7 @@ loop:
 			}
 
 		case failedTunnel := <-controller.failedTunnels:
-			NoticeAlert("tunnel failed: %s", failedTunnel.dialParams.ServerEntry.IpAddress)
+			NoticeAlert("tunnel failed: %s", failedTunnel.dialParams.ServerEntry.GetDiagnosticID())
 			controller.terminateTunnel(failedTunnel)
 
 			// Clear the reference to this tunnel before calling startEstablishing,
@@ -699,7 +699,7 @@ loop:
 
 				if err != nil {
 					NoticeAlert("failed to activate %s: %s",
-						connectedTunnel.dialParams.ServerEntry.IpAddress, err)
+						connectedTunnel.dialParams.ServerEntry.GetDiagnosticID(), err)
 					discardTunnel = true
 				} else {
 					// It's unlikely that registerTunnel will fail, since only this goroutine
@@ -707,7 +707,7 @@ loop:
 					// expected.
 					if !controller.registerTunnel(connectedTunnel) {
 						NoticeAlert("failed to register %s: %s",
-							connectedTunnel.dialParams.ServerEntry.IpAddress, err)
+							connectedTunnel.dialParams.ServerEntry.GetDiagnosticID(), err)
 						discardTunnel = true
 					}
 				}
@@ -732,7 +732,7 @@ loop:
 			}
 
 			NoticeActiveTunnel(
-				connectedTunnel.dialParams.ServerEntry.IpAddress,
+				connectedTunnel.dialParams.ServerEntry.GetDiagnosticID(),
 				connectedTunnel.dialParams.TunnelProtocol,
 				connectedTunnel.dialParams.ServerEntry.SupportsSSHAPIRequests())
 
@@ -843,7 +843,7 @@ func (controller *Controller) SignalTunnelFailure(tunnel *Tunnel) {
 
 // discardTunnel disposes of a successful connection that is no longer required.
 func (controller *Controller) discardTunnel(tunnel *Tunnel) {
-	NoticeInfo("discard tunnel: %s", tunnel.dialParams.ServerEntry.IpAddress)
+	NoticeInfo("discard tunnel: %s", tunnel.dialParams.ServerEntry.GetDiagnosticID())
 	// TODO: not calling PromoteServerEntry, since that would rank the
 	// discarded tunnel before fully active tunnels. Can a discarded tunnel
 	// be promoted (since it connects), but with lower rank than all active
@@ -866,7 +866,7 @@ func (controller *Controller) registerTunnel(tunnel *Tunnel) bool {
 		if activeTunnel.dialParams.ServerEntry.IpAddress ==
 			tunnel.dialParams.ServerEntry.IpAddress {
 
-			NoticeAlert("duplicate tunnel: %s", tunnel.dialParams.ServerEntry.IpAddress)
+			NoticeAlert("duplicate tunnel: %s", tunnel.dialParams.ServerEntry.GetDiagnosticID())
 			return false
 		}
 	}
@@ -1901,7 +1901,7 @@ loop:
 			// Silently skip the candidate in this case. Otherwise, emit error.
 			if err != nil {
 				NoticeInfo("failed to select protocol for %s: %s",
-					candidateServerEntry.serverEntry.IpAddress, err)
+					candidateServerEntry.serverEntry.GetDiagnosticID(), err)
 			}
 
 			// Unblock other candidates immediately when server affinity
@@ -2008,7 +2008,7 @@ loop:
 			}
 
 			NoticeInfo("failed to connect to %s: %s",
-				candidateServerEntry.serverEntry.IpAddress, err)
+				candidateServerEntry.serverEntry.GetDiagnosticID(), err)
 
 			continue
 		}

psiphon/dataStore.go

@@ -192,10 +192,6 @@ func StoreServerEntry(serverEntryFields protocol.ServerEntryFields, replaceIfExi
 		update := !exists || replaceIfExists || newer
 
 		if !update {
-			// Disabling this notice, for now, as it generates too much noise
-			// in diagnostics with clients that always submit embedded servers
-			// to the core on each run.
-			// NoticeInfo("ignored update for server %s", serverEntry.IpAddress)
 			return nil
 		}
 
@@ -242,7 +238,7 @@ func StoreServerEntry(serverEntryFields protocol.ServerEntryFields, replaceIfExi
 			return common.ContextError(err)
 		}
 
-		NoticeInfo("updated server %s", serverEntryFields.GetIPAddress())
+		NoticeInfo("updated server %s", serverEntryFields.GetDiagnosticID())
 
 		return nil
 	})
@@ -506,7 +502,7 @@ func newTargetServerEntryIterator(config *Config, isTactics bool) (bool, *Server
 		targetServerEntry:            serverEntry,
 	}
 
-	NoticeInfo("using TargetServerEntry: %s", serverEntry.IpAddress)
+	NoticeInfo("using TargetServerEntry: %s", serverEntry.GetDiagnosticID())
 
 	return false, iterator, nil
 }

psiphon/dialParameters.go

@@ -558,6 +558,7 @@ func MakeDialParameters(
 	// Initialize Dial/MeekConfigs to be passed to the corresponding dialers.
 
 	dialParams.dialConfig = &DialConfig{
+		DiagnosticID:                  serverEntry.GetDiagnosticID(),
 		UpstreamProxyURL:              config.UpstreamProxyURL,
 		CustomHeaders:                 dialCustomHeaders,
 		DeviceBinder:                  config.deviceBinder,
@@ -574,6 +575,7 @@ func MakeDialParameters(
 	if protocol.TunnelProtocolUsesMeek(dialParams.TunnelProtocol) {
 
 		dialParams.meekConfig = &MeekConfig{
+			DiagnosticID:                  serverEntry.GetDiagnosticID(),
 			ClientParameters:              config.clientParameters,
 			DialAddress:                   dialParams.MeekDialAddress,
 			UseQUIC:                       protocol.TunnelProtocolUsesFrontedMeekQUIC(dialParams.TunnelProtocol),
@@ -623,7 +625,7 @@ func (dialParams *DialParameters) Succeeded() {
 		return
 	}
 
-	NoticeInfo("Set dial parameters for %s", dialParams.ServerEntry.IpAddress)
+	NoticeInfo("Set dial parameters for %s", dialParams.ServerEntry.GetDiagnosticID())
 	err := SetDialParameters(dialParams.ServerEntry.IpAddress, dialParams.NetworkID, dialParams)
 	if err != nil {
 		NoticeAlert("SetDialParameters failed: %s", err)
@@ -648,7 +650,7 @@ func (dialParams *DialParameters) Failed(config *Config) {
 		!config.GetClientParametersSnapshot().WeightedCoinFlip(
 			parameters.ReplayRetainFailedProbability) {
 
-		NoticeInfo("Delete dial parameters for %s", dialParams.ServerEntry.IpAddress)
+		NoticeInfo("Delete dial parameters for %s", dialParams.ServerEntry.GetDiagnosticID())
 		err := DeleteDialParameters(dialParams.ServerEntry.IpAddress, dialParams.NetworkID)
 		if err != nil {
 			NoticeAlert("DeleteDialParameters failed: %s", err)

psiphon/meekConn.go

@@ -65,6 +65,9 @@ const (
 // MeekConfig specifies the behavior of a MeekConn
 type MeekConfig struct {
 
+	// DiagnosticID is the server ID to record in any diagnostics notices.
+	DiagnosticID string
+
 	// ClientParameters is the active set of client parameters to use
 	// for the meek dial.
 	ClientParameters *parameters.ClientParameters
@@ -348,7 +351,7 @@ func DialMeek(
 		cachedTLSDialer = newCachedTLSDialer(preConn, tlsDialer)
 
 		if IsTLSConnUsingHTTP2(preConn) {
-			NoticeInfo("negotiated HTTP/2 for %s", meekConfig.DialAddress)
+			NoticeInfo("negotiated HTTP/2 for %s", meekConfig.DiagnosticID)
 			transport = &http2.Transport{
 				DialTLS: func(network, addr string, _ *tls.Config) (net.Conn, error) {
 					return cachedTLSDialer.dial(network, addr)

psiphon/memory_test/memory_test.go

@@ -85,7 +85,7 @@ func runMemoryTest(t *testing.T, testMode int) {
 	}
 	defer os.RemoveAll(testDataDirName)
 
-	psiphon.SetEmitDiagnosticNotices(true)
+	psiphon.SetEmitDiagnosticNotices(true, true)
 
 	configJSON, err := ioutil.ReadFile("../controller_test.config")
 	if err != nil {

psiphon/net.go

@@ -45,6 +45,9 @@ const DNS_PORT = 53
 // of a Psiphon dialer (TCPDial, UDPDial, MeekDial, etc.)
 type DialConfig struct {
 
+	// DiagnosticID is the server ID to record in any diagnostics notices.
+	DiagnosticID string
+
 	// UpstreamProxyURL specifies a proxy to connect through.
 	// E.g., "http://proxyhost:8080"
 	//       "socks5://user:password@proxyhost:1080"

psiphon/notice.go

@@ -35,7 +35,8 @@ import (
 )
 
 type noticeLogger struct {
-	logDiagnostics             int32
+	emitDiagnostics            int32
+	emitNetworkParameters      int32
 	mutex                      sync.Mutex
 	writer                     io.Writer
 	homepageFilename           string
@@ -53,23 +54,40 @@ var singletonNoticeLogger = noticeLogger{
 	writer: os.Stderr,
 }
 
-// SetEmitDiagnosticNotices toggles whether diagnostic notices
-// are emitted. Diagnostic notices contain potentially sensitive
-// circumvention network information; only enable this in environments
-// where notices are handled securely (for example, don't include these
-// notices in log files which users could post to public forums).
-func SetEmitDiagnosticNotices(enable bool) {
-	if enable {
-		atomic.StoreInt32(&singletonNoticeLogger.logDiagnostics, 1)
+// SetEmitDiagnosticNotices toggles whether diagnostic notices are emitted;
+// and whether to include circumvention network parameters in diagnostics.
+//
+// Diagnostic notices contain potentially sensitive user information; and
+// sensitive circumvention network parameters, when enabled. Only enable this
+// in environments where notices are handled securely (for example, don't
+// include these notices in log files which users could post to public
+// forums).
+func SetEmitDiagnosticNotices(
+	emitDiagnostics bool, emitNetworkParameters bool) {
+
+	if emitDiagnostics {
+		atomic.StoreInt32(&singletonNoticeLogger.emitDiagnostics, 1)
+	} else {
+		atomic.StoreInt32(&singletonNoticeLogger.emitDiagnostics, 0)
+	}
+
+	if emitNetworkParameters {
+		atomic.StoreInt32(&singletonNoticeLogger.emitNetworkParameters, 1)
 	} else {
-		atomic.StoreInt32(&singletonNoticeLogger.logDiagnostics, 0)
+		atomic.StoreInt32(&singletonNoticeLogger.emitNetworkParameters, 0)
 	}
 }
 
 // GetEmitDiagnoticNotices returns the current state
 // of emitting diagnostic notices.
 func GetEmitDiagnoticNotices() bool {
-	return atomic.LoadInt32(&singletonNoticeLogger.logDiagnostics) == 1
+	return atomic.LoadInt32(&singletonNoticeLogger.emitDiagnostics) == 1
+}
+
+// GetEmitNetworkParameters returns the current state
+// of emitting network parameters.
+func GetEmitNetworkParameters() bool {
+	return atomic.LoadInt32(&singletonNoticeLogger.emitNetworkParameters) == 1
 }
 
 // SetNoticeWriter sets a target writer to receive notices. By default,
@@ -188,7 +206,7 @@ const (
 // outputNotice encodes a notice in JSON and writes it to the output writer.
 func (nl *noticeLogger) outputNotice(noticeType string, noticeFlags uint32, args ...interface{}) {
 
-	if (noticeFlags&noticeIsDiagnostic != 0) && atomic.LoadInt32(&nl.logDiagnostics) != 1 {
+	if (noticeFlags&noticeIsDiagnostic != 0) && !GetEmitDiagnoticNotices() {
 		return
 	}
 
@@ -218,6 +236,11 @@ func (nl *noticeLogger) outputNotice(noticeType string, noticeFlags uint32, args
 			fmt.Sprintf("marshal notice failed: %s", common.ContextError(err)))
 	}
 
+	// Ensure direct server IPs are not exposed in notices. The "net" package,
+	// and possibly other 3rd party packages, will include destination addresses
+	// in I/O error messages.
+	output = StripIPAddresses(output)
+
 	nl.mutex.Lock()
 	defer nl.mutex.Unlock()
 
@@ -404,78 +427,81 @@ func NoticeAvailableEgressRegions(regions []string) {
 func noticeWithDialParameters(noticeType string, dialParams *DialParameters) {
 
 	args := []interface{}{
-		"ipAddress", dialParams.ServerEntry.IpAddress,
+		"serverID", dialParams.ServerEntry.GetDiagnosticID(),
 		"region", dialParams.ServerEntry.Region,
 		"protocol", dialParams.TunnelProtocol,
 		"isReplay", dialParams.IsReplay,
 	}
 
-	if dialParams.SelectedSSHClientVersion {
-		args = append(args, "SSHClientVersion", dialParams.SSHClientVersion)
-	}
+	if GetEmitNetworkParameters() {
 
-	if dialParams.UpstreamProxyType != "" {
-		args = append(args, "upstreamProxyType", dialParams.UpstreamProxyType)
-	}
+		if dialParams.SelectedSSHClientVersion {
+			args = append(args, "SSHClientVersion", dialParams.SSHClientVersion)
+		}
 
-	if dialParams.UpstreamProxyCustomHeaderNames != nil {
-		args = append(args, "upstreamProxyCustomHeaderNames", strings.Join(dialParams.UpstreamProxyCustomHeaderNames, ","))
-	}
+		if dialParams.UpstreamProxyType != "" {
+			args = append(args, "upstreamProxyType", dialParams.UpstreamProxyType)
+		}
 
-	if dialParams.MeekDialAddress != "" {
-		args = append(args, "meekDialAddress", dialParams.MeekDialAddress)
-	}
+		if dialParams.UpstreamProxyCustomHeaderNames != nil {
+			args = append(args, "upstreamProxyCustomHeaderNames", strings.Join(dialParams.UpstreamProxyCustomHeaderNames, ","))
+		}
 
-	meekResolvedIPAddress := dialParams.MeekResolvedIPAddress.Load().(string)
-	if meekResolvedIPAddress != "" {
-		args = append(args, "meekResolvedIPAddress", meekResolvedIPAddress)
-	}
+		if dialParams.MeekDialAddress != "" {
+			args = append(args, "meekDialAddress", dialParams.MeekDialAddress)
+		}
 
-	if dialParams.MeekSNIServerName != "" {
-		args = append(args, "meekSNIServerName", dialParams.MeekSNIServerName)
-	}
+		meekResolvedIPAddress := dialParams.MeekResolvedIPAddress.Load().(string)
+		if meekResolvedIPAddress != "" {
+			args = append(args, "meekResolvedIPAddress", meekResolvedIPAddress)
+		}
 
-	if dialParams.MeekHostHeader != "" {
-		args = append(args, "meekHostHeader", dialParams.MeekHostHeader)
-	}
+		if dialParams.MeekSNIServerName != "" {
+			args = append(args, "meekSNIServerName", dialParams.MeekSNIServerName)
+		}
 
-	// MeekTransformedHostName is meaningful when meek is used, which is when MeekDialAddress != ""
-	if dialParams.MeekDialAddress != "" {
-		args = append(args, "meekTransformedHostName", dialParams.MeekTransformedHostName)
-	}
+		if dialParams.MeekHostHeader != "" {
+			args = append(args, "meekHostHeader", dialParams.MeekHostHeader)
+		}
 
-	if dialParams.SelectedUserAgent {
-		args = append(args, "userAgent", dialParams.UserAgent)
-	}
+		// MeekTransformedHostName is meaningful when meek is used, which is when MeekDialAddress != ""
+		if dialParams.MeekDialAddress != "" {
+			args = append(args, "meekTransformedHostName", dialParams.MeekTransformedHostName)
+		}
 
-	if dialParams.SelectedTLSProfile {
-		args = append(args, "TLSProfile", dialParams.TLSProfile)
-		args = append(args, "TLSVersion", dialParams.TLSVersion)
-	}
+		if dialParams.SelectedUserAgent {
+			args = append(args, "userAgent", dialParams.UserAgent)
+		}
 
-	if dialParams.DialPortNumber != "" {
-		args = append(args, "dialPortNumber", dialParams.DialPortNumber)
-	}
+		if dialParams.SelectedTLSProfile {
+			args = append(args, "TLSProfile", dialParams.TLSProfile)
+			args = append(args, "TLSVersion", dialParams.TLSVersion)
+		}
 
-	if dialParams.QUICVersion != "" {
-		args = append(args, "QUICVersion", dialParams.QUICVersion)
-	}
+		if dialParams.DialPortNumber != "" {
+			args = append(args, "dialPortNumber", dialParams.DialPortNumber)
+		}
 
-	if dialParams.QUICDialSNIAddress != "" {
-		args = append(args, "QUICDialSNIAddress", dialParams.QUICDialSNIAddress)
-	}
+		if dialParams.QUICVersion != "" {
+			args = append(args, "QUICVersion", dialParams.QUICVersion)
+		}
 
-	if dialParams.DialConnMetrics != nil {
-		metrics := dialParams.DialConnMetrics.GetMetrics()
-		for name, value := range metrics {
-			args = append(args, name, value)
+		if dialParams.QUICDialSNIAddress != "" {
+			args = append(args, "QUICDialSNIAddress", dialParams.QUICDialSNIAddress)
 		}
-	}
 
-	if dialParams.ObfuscatedSSHConnMetrics != nil {
-		metrics := dialParams.ObfuscatedSSHConnMetrics.GetMetrics()
-		for name, value := range metrics {
-			args = append(args, name, value)
+		if dialParams.DialConnMetrics != nil {
+			metrics := dialParams.DialConnMetrics.GetMetrics()
+			for name, value := range metrics {
+				args = append(args, name, value)
+			}
+		}
+
+		if dialParams.ObfuscatedSSHConnMetrics != nil {
+			metrics := dialParams.ObfuscatedSSHConnMetrics.GetMetrics()
+			for name, value := range metrics {
+				args = append(args, name, value)
+			}
 		}
 	}
 
@@ -505,10 +531,10 @@ func NoticeRequestedTactics(dialParams *DialParameters) {
 }
 
 // NoticeActiveTunnel is a successful connection that is used as an active tunnel for port forwarding
-func NoticeActiveTunnel(ipAddress, protocol string, isTCS bool) {
+func NoticeActiveTunnel(serverID, protocol string, isTCS bool) {
 	singletonNoticeLogger.outputNotice(
 		"ActiveTunnel", noticeIsDiagnostic,
-		"ipAddress", ipAddress,
+		"serverID", serverID,
 		"protocol", protocol,
 		"isTCS", isTCS)
 }
@@ -642,25 +668,24 @@ func NoticeClientUpgradeDownloaded(filename string) {
 }
 
 // NoticeBytesTransferred reports how many tunneled bytes have been
-// transferred since the last NoticeBytesTransferred, for the tunnel
-// to the server at ipAddress. This is not a diagnostic notice: the
-// user app has requested this notice with EmitBytesTransferred for
-// functionality such as traffic display; and this frequent notice
-// is not intended to be included with feedback.
-func NoticeBytesTransferred(ipAddress string, sent, received int64) {
+// transferred since the last NoticeBytesTransferred. This is not a diagnostic
+// notice: the user app has requested this notice with EmitBytesTransferred
+// for functionality such as traffic display; and this frequent notice is not
+// intended to be included with feedback.
+func NoticeBytesTransferred(serverID string, sent, received int64) {
 	singletonNoticeLogger.outputNotice(
 		"BytesTransferred", 0,
+		"serverID", serverID,
 		"sent", sent,
 		"received", received)
 }
 
 // NoticeTotalBytesTransferred reports how many tunneled bytes have been
-// transferred in total up to this point, for the tunnel to the server
-// at ipAddress. This is a diagnostic notice.
-func NoticeTotalBytesTransferred(ipAddress string, sent, received int64) {
+// transferred in total up to this point. This is a diagnostic notice.
+func NoticeTotalBytesTransferred(serverID string, sent, received int64) {
 	singletonNoticeLogger.outputNotice(
 		"TotalBytesTransferred", noticeIsDiagnostic,
-		"ipAddress", ipAddress,
+		"serverID", serverID,
 		"sent", sent,
 		"received", received)
 }
@@ -701,6 +726,9 @@ func NoticeExiting() {
 
 // NoticeRemoteServerListResourceDownloadedBytes reports remote server list download progress.
 func NoticeRemoteServerListResourceDownloadedBytes(url string, bytes int64) {
+	if !GetEmitNetworkParameters() {
+		url = "<url>"
+	}
 	singletonNoticeLogger.outputNotice(
 		"RemoteServerListResourceDownloadedBytes", noticeIsDiagnostic,
 		"url", url,
@@ -710,6 +738,9 @@ func NoticeRemoteServerListResourceDownloadedBytes(url string, bytes int64) {
 // NoticeRemoteServerListResourceDownloaded indicates that a remote server list download
 // completed successfully.
 func NoticeRemoteServerListResourceDownloaded(url string) {
+	if !GetEmitNetworkParameters() {
+		url = "<url>"
+	}
 	singletonNoticeLogger.outputNotice(
 		"RemoteServerListResourceDownloaded", noticeIsDiagnostic,
 		"url", url)
@@ -757,10 +788,10 @@ func NoticeNetworkID(networkID string) {
 		"NetworkID", 0, "ID", networkID)
 }
 
-func NoticeLivenessTest(ipAddress string, metrics *livenessTestMetrics, success bool) {
+func NoticeLivenessTest(serverID string, metrics *livenessTestMetrics, success bool) {
 	singletonNoticeLogger.outputNotice(
 		"LivenessTest", noticeIsDiagnostic,
-		"ipAddress", ipAddress,
+		"serverID", serverID,
 		"metrics", metrics,
 		"success", success)
 }
@@ -779,6 +810,13 @@ func NoticeEstablishTunnelTimeout(timeout time.Duration) {
 		"timeout", timeout)
 }
 
+func NoticeFragmentor(serverID string, message string) {
+	singletonNoticeLogger.outputNotice(
+		"Fragmentor", noticeIsDiagnostic,
+		"serverID", serverID,
+		"message", message)
+}
+
 type repetitiveNoticeState struct {
 	message string
 	repeats int

psiphon/server/server_test.go

@@ -80,7 +80,7 @@ func TestMain(m *testing.M) {
 	}
 	defer os.RemoveAll(testDataDirName)
 
-	psiphon.SetEmitDiagnosticNotices(true)
+	psiphon.SetEmitDiagnosticNotices(true, true)
 
 	mockWebServerURL, mockWebServerExpectedResponse = runMockWebServer()
 

psiphon/serverApi.go

@@ -32,7 +32,6 @@ import (
 	"net"
 	"net/http"
 	"net/url"
-	"regexp"
 	"strconv"
 	"strings"
 
@@ -616,13 +615,6 @@ func RecordRemoteServerListStat(
 		config, datastorePersistentStatTypeRemoteServerList, remoteServerListStatJson)
 }
 
-// failedTunnelErrStripAddressRegex strips IPv4 address [and optional port]
-// strings from "net" package I/O error messages. This is to avoid
-// inadvertently recording direct server IPs via error message logs, and to
-// reduce the error space due to superfluous source port data.
-var failedTunnelErrStripAddressRegex = regexp.MustCompile(
-	`(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}(:(6553[0-5]|655[0-2][0-9]\d|65[0-4](\d){2}|6[0-4](\d){3}|[1-5](\d){4}|[1-9](\d){0,3}))?`)
-
 // RecordFailedTunnelStat records metrics for a failed tunnel dial, including
 // dial parameters and error condition (tunnelErr).
 //
@@ -647,7 +639,13 @@ func RecordFailedTunnelStat(
 	params["server_entry_tag"] = dialParams.ServerEntry.Tag
 	params["last_connected"] = lastConnected
 	params["client_failed_timestamp"] = common.TruncateTimestampToHour(common.GetCurrentTimestamp())
-	params["tunnel_error"] = failedTunnelErrStripAddressRegex.ReplaceAllString(tunnelErr.Error(), "<address>")
+
+	// Ensure direct server IPs are not exposed in logs. The "net" package, and
+	// possibly other 3rd party packages, will include destination addresses in
+	// I/O error messages.
+	tunnelError := StripIPAddressesString(tunnelErr.Error())
+
+	params["tunnel_error"] = tunnelError
 
 	failedTunnelStatJson, err := json.Marshal(params)
 	if err != nil {

psiphon/tunnel.go

@@ -174,7 +174,7 @@ func (tunnel *Tunnel) Activate(
 	if !tunnel.config.DisableApi {
 		NoticeInfo(
 			"starting server context for %s",
-			tunnel.dialParams.ServerEntry.IpAddress)
+			tunnel.dialParams.ServerEntry.GetDiagnosticID())
 
 		// Call NewServerContext in a goroutine, as it blocks on a network operation,
 		// the handshake request, and would block shutdown. If the shutdown signal is
@@ -224,7 +224,7 @@ func (tunnel *Tunnel) Activate(
 		if result.err != nil {
 			return common.ContextError(
 				fmt.Errorf("error starting server context for %s: %s",
-					tunnel.dialParams.ServerEntry.IpAddress, result.err))
+					tunnel.dialParams.ServerEntry.GetDiagnosticID(), result.err))
 		}
 
 		serverContext = result.serverContext
@@ -829,7 +829,7 @@ func dialTunnel(
 				// Skip notice when cancelling.
 				if baseCtx.Err() == nil {
 					NoticeLivenessTest(
-						dialParams.ServerEntry.IpAddress, metrics, err == nil)
+						dialParams.ServerEntry.GetDiagnosticID(), metrics, err == nil)
 				}
 			}
 		}
@@ -1101,14 +1101,14 @@ func (tunnel *Tunnel) operateTunnel(tunnelOwner TunnelOwner) {
 
 			if lastTotalBytesTransferedTime.Add(noticePeriod).Before(monotime.Now()) {
 				NoticeTotalBytesTransferred(
-					tunnel.dialParams.ServerEntry.IpAddress, totalSent, totalReceived)
+					tunnel.dialParams.ServerEntry.GetDiagnosticID(), totalSent, totalReceived)
 				lastTotalBytesTransferedTime = monotime.Now()
 			}
 
 			// Only emit the frequent BytesTransferred notice when tunnel is not idle.
 			if tunnel.config.EmitBytesTransferred && (sent > 0 || received > 0) {
 				NoticeBytesTransferred(
-					tunnel.dialParams.ServerEntry.IpAddress, sent, received)
+					tunnel.dialParams.ServerEntry.GetDiagnosticID(), sent, received)
 			}
 
 			// Once the tunnel has connected, activated, and successfully
@@ -1147,7 +1147,7 @@ func (tunnel *Tunnel) operateTunnel(tunnelOwner TunnelOwner) {
 			// Note: no mutex on portForwardFailureTotal; only referenced here
 			tunnel.totalPortForwardFailures++
 			NoticeInfo("port forward failures for %s: %d",
-				tunnel.dialParams.ServerEntry.IpAddress,
+				tunnel.dialParams.ServerEntry.GetDiagnosticID(),
 				tunnel.totalPortForwardFailures)
 
 			// If the underlying Conn has closed (meek and other plugin protocols may close
@@ -1202,7 +1202,7 @@ func (tunnel *Tunnel) operateTunnel(tunnelOwner TunnelOwner) {
 
 	// Always emit a final NoticeTotalBytesTransferred
 	NoticeTotalBytesTransferred(
-		tunnel.dialParams.ServerEntry.IpAddress, totalSent, totalReceived)
+		tunnel.dialParams.ServerEntry.GetDiagnosticID(), totalSent, totalReceived)
 
 	if err == nil {
 		NoticeInfo("shutdown operate tunnel")
@@ -1216,7 +1216,7 @@ func (tunnel *Tunnel) operateTunnel(tunnelOwner TunnelOwner) {
 
 	} else {
 		NoticeAlert("operate tunnel error for %s: %s",
-			tunnel.dialParams.ServerEntry.IpAddress, err)
+			tunnel.dialParams.ServerEntry.GetDiagnosticID(), err)
 		tunnelOwner.SignalTunnelFailure(tunnel)
 	}
 }
@@ -1312,7 +1312,7 @@ func sendStats(tunnel *Tunnel) bool {
 	err := tunnel.serverContext.DoStatusRequest(tunnel)
 	if err != nil {
 		NoticeAlert("DoStatusRequest failed for %s: %s",
-			tunnel.dialParams.ServerEntry.IpAddress, err)
+			tunnel.dialParams.ServerEntry.GetDiagnosticID(), err)
 	}
 
 	return err == nil

psiphon/utils.go

@@ -27,8 +27,10 @@ import (
 	"net"
 	"net/url"
 	"os"
+	"regexp"
 	"runtime"
 	"runtime/debug"
+	"strings"
 	"syscall"
 	"time"
 
@@ -111,6 +113,56 @@ func IsAddressInUseError(err error) bool {
 	return false
 }
 
+var stripIPv4AddressRegex = regexp.MustCompile(
+	`(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\.(25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)){3}(:(6553[0-5]|655[0-2][0-9]\d|65[0-4](\d){2}|6[0-4](\d){3}|[1-5](\d){4}|[1-9](\d){0,3}))?`)
+
+// StripIPAddresses returns a copy of the input with all IP addresses [and
+// optional ports] replaced  by "<address>". This is intended to be used to
+// strip addresses from "net" package I/O error messages and otherwise avoid
+// inadvertently recording direct server IPs via error message logs; and, in
+// metrics, to reduce the error space due to superfluous source port data.
+//
+// Limitation: only strips IPv4 addresses.
+func StripIPAddresses(b []byte) []byte {
+	// TODO: IPv6 support
+	return stripIPv4AddressRegex.ReplaceAll(b, []byte("<address>"))
+}
+
+// StripIPAddressesString is StripIPAddresses for strings.
+func StripIPAddressesString(s string) string {
+	// TODO: IPv6 support
+	return stripIPv4AddressRegex.ReplaceAllString(s, "<address>")
+}
+
+// RedactNetError removes network address information from a "net" package
+// error message. Addresses may be domains or IP addresses.
+//
+// Limitations: some non-address error context can be lost; this function
+// makes assumptions about how the Go "net" package error messages are
+// formatted and will fail to redact network addresses if this assumptions
+// become untrue.
+func RedactNetError(err error) error {
+
+	// Example "net" package error messages:
+	//
+	// - lookup <domain>: no such host
+	// - lookup <domain>: No address associated with hostname
+	// - dial tcp <address>: connectex: No connection could be made because the target machine actively refused it
+	// - write tcp <address>-><address>: write: connection refused
+
+	if err == nil {
+		return err
+	}
+
+	errstr := err.Error()
+	index := strings.Index(errstr, ":")
+	if index == -1 {
+		return err
+	}
+
+	return errors.New(errstr[index:])
+}
+
 // SyncFileWriter wraps a file and exposes an io.Writer. At predefined
 // steps, the file is synced (flushed to disk) while writing.
 type SyncFileWriter struct {