Browse Source

DNS resolver fixes

- Ensure no DNS dial parameters are reported in metrics or diagnostics when
  the dial address is actually an IP address.

- In ResolveIP, check for an IP address hostname input and return before
  initializing default parameters or updating network state.
Rod Hynes 3 years ago
parent
commit
5d41f51f8b
2 changed files with 26 additions and 17 deletions
  1. 12 12
      psiphon/common/resolver/resolver.go
  2. 14 5
      psiphon/dialParameters.go

+ 12 - 12
psiphon/common/resolver/resolver.go

@@ -496,6 +496,18 @@ func (r *Resolver) ResolveIP(
 	// ResolveIP does _not_ lock r.mutex for the lifetime of the function, to
 	// ensure many ResolveIP calls can run concurrently.
 
+	// If the hostname is already an IP address, just return that. For
+	// metrics, this does not count as a resolve, as the caller may invoke
+	// ResolveIP for all dials.
+	IP := net.ParseIP(hostname)
+	if IP != nil {
+		return []net.IP{IP}, nil
+	}
+
+	// Count all resolves of an actual domain, including cached and
+	// pre-resolved cases.
+	r.updateMetricResolves()
+
 	// Call updateNetworkState immediately before resolving, as a best effort
 	// to ensure that system DNS servers and IPv6 routing network state
 	// reflects the current network. updateNetworkState locks the Resolver
@@ -517,18 +529,6 @@ func (r *Resolver) ResolveIP(
 		}
 	}
 
-	// If the hostname is already an IP address, just return that. For
-	// metrics, this does not count as a resolve, as the caller may invoke
-	// ResolveIP for all dials.
-	IP := net.ParseIP(hostname)
-	if IP != nil {
-		return []net.IP{IP}, nil
-	}
-
-	// Count all resolves of an actual domain, including cached and
-	// pre-resolved cases.
-	r.updateMetricResolves()
-
 	// When PreresolvedIPAddress is set, tactics parameters determined the IP address
 	// in this case.
 	if params.PreresolvedIPAddress != "" {

+ 14 - 5
psiphon/dialParameters.go

@@ -721,8 +721,17 @@ func MakeDialParameters(
 		}
 	}
 
-	useResolver := protocol.TunnelProtocolUsesFrontedMeek(dialParams.TunnelProtocol) ||
-		dialParams.ConjureAPIRegistration
+	// Initialize dialParams.ResolveParameters for dials that will resolve
+	// domain names, which currently includes fronted meek and Conjure API
+	// registration, where the dial address is not an IP address.
+	//
+	// dialParams.ResolveParameters must be nil when the dial address is an IP
+	// address to ensure that no DNS dial parameters are reported in metrics
+	// or diagnostics when when no domain is resolved.
+
+	useResolver := (protocol.TunnelProtocolUsesFrontedMeek(dialParams.TunnelProtocol) ||
+		dialParams.ConjureAPIRegistration) &&
+		net.ParseIP(dialParams.MeekFrontingDialAddress) == nil
 
 	if (!isReplay || !replayResolveParameters) && useResolver {
 
@@ -885,9 +894,9 @@ func MakeDialParameters(
 	// Initialize Dial/MeekConfigs to be passed to the corresponding dialers.
 
 	// Custom ResolveParameters are set only when useResolver is true, but
-	// DialConfig.ResolveIP is wired up unconditionally, so that we fail over
-	// to resolving, but without custom parameters, in case of a
-	// misconfigured or miscoded case.
+	// DialConfig.ResolveIP is required and wired up unconditionally. Any
+	// misconfigured or miscoded domain dial cases will use default
+	// ResolveParameters.
 	//
 	// ResolveIP will use the networkID obtained above, as it will be used
 	// almost immediately, instead of incurring the overhead of calling