Fix: handle omitted tactics configuration

- Previously used zero value of tactics.Server,
  which returned invalid tactics payloads to the
  client.

- Now explicitly checks for configuration loading;
  when not loaded, don't return tactics payloads
  in handshakes and don't handle tactics requests.

psiphon/common/tactics/tactics.go

@@ -241,6 +241,13 @@ type Server struct {
 		Tactics Tactics
 	}
 
+	// When no tactics configuration file is provided, there will be no
+	// request key material or default tactics, and the server will not
+	// support tactics. The loaded flag, set to true only when a configuration
+	// file has been successfully loaded, provides an explict check for this
+	// condition (vs., say, checking for a zero-value Server).
+	loaded bool
+
 	logger                common.Logger
 	logFieldFormatter     common.APIParameterLogFieldFormatter
 	apiParameterValidator common.APIParameterValidator
@@ -443,6 +450,8 @@ func NewServer(
 			server.DefaultTactics = newServer.DefaultTactics
 			server.FilteredTactics = newServer.FilteredTactics
 
+			server.loaded = true
+
 			return nil
 		})
 
@@ -574,20 +583,18 @@ func (server *Server) initLookups() {
 	}
 }
 
-// GetTacticsPayload assembles and returns a tactics payload
-// for a client with the specified GeoIP, API parameter, and
-// speed test attributes.
+// GetTacticsPayload assembles and returns a tactics payload for a client with
+// the specified GeoIP, API parameter, and speed test attributes.
 //
-// The speed test samples are expected to be in apiParams,
-// as is the stored tactics tag.
+// The speed test samples are expected to be in apiParams, as is the stored
+// tactics tag.
 //
-// GetTacticsPayload will always return a payload for any
-// client. When the client's stored tactics tag is identical
-// to the assembled tactics, the Payload.Tactics is nil.
+// Unless no tactics configuration was loaded, GetTacticsPayload will always
+// return a payload for any client. When the client's stored tactics tag is
+// identical to the assembled tactics, the Payload.Tactics is nil.
 //
-// Elements of the returned Payload, e.g., tactics parameters,
-// will point to data in DefaultTactics and FilteredTactics
-// and must not be modifed.
+// Elements of the returned Payload, e.g., tactics parameters, will point to
+// data in DefaultTactics and FilteredTactics and must not be modifed.
 func (server *Server) GetTacticsPayload(
 	geoIPData common.GeoIPData,
 	apiParams common.APIParameters) (*Payload, error) {
@@ -595,6 +602,11 @@ func (server *Server) GetTacticsPayload(
 	server.ReloadableFile.RLock()
 	defer server.ReloadableFile.RUnlock()
 
+	if !server.loaded {
+		// No tactics configuration was loaded.
+		return nil, nil
+	}
+
 	tactics := server.DefaultTactics.clone()
 
 	var aggregatedValues map[string]int
@@ -887,6 +899,15 @@ func (server *Server) HandleEndPoint(
 	w http.ResponseWriter,
 	r *http.Request) bool {
 
+	server.ReloadableFile.RLock()
+	loaded := server.loaded
+	server.ReloadableFile.RUnlock()
+
+	if !loaded {
+		// No tactics configuration was loaded.
+		return false
+	}
+
 	switch endPoint {
 	case SPEED_TEST_END_POINT:
 		server.handleSpeedTestRequest(geoIPData, w, r)
@@ -965,6 +986,9 @@ func (server *Server) handleTacticsRequest(
 	}
 
 	tacticsPayload, err := server.GetTacticsPayload(geoIPData, apiParams)
+	if err == nil && tacticsPayload == nil {
+		err = common.ContextError(errors.New("unexpected missing tactics payload"))
+	}
 	if err != nil {
 		server.logger.WithContextFields(
 			common.LogFields{"error": err}).Warning("failed to get tactics")

psiphon/server/api.go

@@ -242,28 +242,33 @@ func handshakeAPIRequestHandler(
 		return nil, common.ContextError(err)
 	}
 
-	marshaledTacticsPayload, err := json.Marshal(tacticsPayload)
-	if err != nil {
-		return nil, common.ContextError(err)
-	}
+	var marshaledTacticsPayload []byte
 
-	// Log a metric when new tactics are issued. Logging here indicates that
-	// the handshake tactics mechansim is active; but logging for every
-	// handshake creates unneccesary log data.
+	if tacticsPayload != nil {
 
-	if len(tacticsPayload.Tactics) > 0 {
+		marshaledTacticsPayload, err = json.Marshal(tacticsPayload)
+		if err != nil {
+			return nil, common.ContextError(err)
+		}
 
-		logFields := getRequestLogFields(
-			tactics.TACTICS_METRIC_EVENT_NAME,
-			geoIPData,
-			authorizedAccessTypes,
-			params,
-			handshakeRequestParams)
+		// Log a metric when new tactics are issued. Logging here indicates that
+		// the handshake tactics mechansim is active; but logging for every
+		// handshake creates unneccesary log data.
 
-		logFields[tactics.NEW_TACTICS_TAG_LOG_FIELD_NAME] = tacticsPayload.Tag
-		logFields[tactics.IS_TACTICS_REQUEST_LOG_FIELD_NAME] = false
+		if len(tacticsPayload.Tactics) > 0 {
 
-		log.LogRawFieldsWithTimestamp(logFields)
+			logFields := getRequestLogFields(
+				tactics.TACTICS_METRIC_EVENT_NAME,
+				geoIPData,
+				authorizedAccessTypes,
+				params,
+				handshakeRequestParams)
+
+			logFields[tactics.NEW_TACTICS_TAG_LOG_FIELD_NAME] = tacticsPayload.Tag
+			logFields[tactics.IS_TACTICS_REQUEST_LOG_FIELD_NAME] = false
+
+			log.LogRawFieldsWithTimestamp(logFields)
+		}
 	}
 
 	// The log comes _after_ SetClientHandshakeState, in case that call rejects

psiphon/server/server_test.go

@@ -384,7 +384,7 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 
 	// Enable tactics when the test protocol is meek. Both the client and the
 	// server will be configured to support tactics. The client config will be
-	// set with a nonfunctional config so thatthe tactics request must
+	// set with a nonfunctional config so that the tactics request must
 	// succeed, overriding the nonfunctional values, for the tunnel to
 	// establish.
 
@@ -435,11 +435,17 @@ func runServer(t *testing.T, runConfig *runServerConfig) {
 		t, trafficRulesFilename, propagationChannelID, accessType,
 		runConfig.requireAuthorization, runConfig.denyTrafficRules)
 
-	tacticsConfigFilename := filepath.Join(testDataDirName, "tactics_config.json")
-	paveTacticsConfigFile(
-		t, tacticsConfigFilename,
-		tacticsRequestPublicKey, tacticsRequestPrivateKey, tacticsRequestObfuscatedKey,
-		propagationChannelID)
+	var tacticsConfigFilename string
+
+	// Only pave the tactics config when tactics are required. This exercises the
+	// case where the tactics config is omitted.
+	if doTactics {
+		tacticsConfigFilename = filepath.Join(testDataDirName, "tactics_config.json")
+		paveTacticsConfigFile(
+			t, tacticsConfigFilename,
+			tacticsRequestPublicKey, tacticsRequestPrivateKey, tacticsRequestObfuscatedKey,
+			propagationChannelID)
+	}
 
 	var serverConfig map[string]interface{}
 	json.Unmarshal(serverConfigJSON, &serverConfig)