Fix: validation with cross-tactics references

psiphon/common/parameters/packetman.go

@@ -49,7 +49,7 @@ func (specs PacketManipulationSpecs) Validate() error {
 			return errors.TraceNew("missing spec name")
 		}
 		if ok, _ := specNames[spec.Name]; ok {
-			return errors.TraceNew("duplicate spec name")
+			return errors.Tracef("duplicate spec name: %s", spec.Name)
 		}
 		specNames[spec.Name] = true
 
@@ -88,7 +88,7 @@ func (manipulations ProtocolPacketManipulations) Validate(specs PacketManipulati
 
 		for _, specName := range specNames {
 			if ok, _ := validSpecNames[specName]; !ok {
-				return errors.TraceNew("invalid spec name")
+				return errors.Tracef("invalid spec name: %s", specName)
 			}
 		}
 	}

psiphon/common/tactics/tactics.go

@@ -498,7 +498,24 @@ func (server *Server) Validate() error {
 		}
 	}
 
-	validateTactics := func(tactics *Tactics, isDefault bool) error {
+	// validateTactics validates either the defaultTactics, when filteredTactics
+	// is nil, or the filteredTactics otherwise. In the second case,
+	// defaultTactics must be passed in to validate filtered tactics references
+	// to default tactics parameters, such as CustomTLSProfiles or
+	// PacketManipulationSpecs.
+	//
+	// Limitation: references must point to the default tactics or the filtered
+	// tactics itself; referring to parameters in a previous filtered tactics is
+	// not suported.
+
+	validateTactics := func(defaultTactics, filteredTactics *Tactics) error {
+
+		tactics := defaultTactics
+		validatingDefault := true
+		if filteredTactics != nil {
+			tactics = filteredTactics
+			validatingDefault = false
+		}
 
 		// Allow "" for 0, even though ParseDuration does not.
 		var d time.Duration
@@ -511,14 +528,14 @@ func (server *Server) Validate() error {
 		}
 
 		if d <= 0 {
-			if isDefault {
+			if validatingDefault {
 				return errors.TraceNew("invalid duration")
 			}
 			// For merging logic, Normalize any 0 duration to "".
 			tactics.TTL = ""
 		}
 
-		if (isDefault && tactics.Probability == 0.0) ||
+		if (validatingDefault && tactics.Probability == 0.0) ||
 			tactics.Probability < 0.0 ||
 			tactics.Probability > 1.0 {
 
@@ -530,7 +547,15 @@ func (server *Server) Validate() error {
 			return errors.Trace(err)
 		}
 
-		_, err = params.Set("", false, tactics.Parameters)
+		applyParameters := []map[string]interface{}{
+			defaultTactics.Parameters,
+		}
+		if filteredTactics != nil {
+			applyParameters = append(
+				applyParameters, filteredTactics.Parameters)
+		}
+
+		_, err = params.Set("", false, applyParameters...)
 		if err != nil {
 			return errors.Trace(err)
 		}
@@ -557,14 +582,14 @@ func (server *Server) Validate() error {
 		return nil
 	}
 
-	err := validateTactics(&server.DefaultTactics, true)
+	err := validateTactics(&server.DefaultTactics, nil)
 	if err != nil {
 		return errors.Tracef("invalid default tactics: %s", err)
 	}
 
 	for i, filteredTactics := range server.FilteredTactics {
 
-		err := validateTactics(&filteredTactics.Tactics, false)
+		err := validateTactics(&server.DefaultTactics, &filteredTactics.Tactics)
 
 		if err == nil {
 			err = validateRange(filteredTactics.Filter.SpeedTestRTTMilliseconds)

psiphon/common/tactics/tactics_test.go

@@ -42,8 +42,12 @@ func TestTactics(t *testing.T) {
 
 	// Server tactics configuration
 
-	// Long and short region lists test both map and slice lookups
-	// Repeated median aggregation tests aggregation memoization
+	// Long and short region lists test both map and slice lookups.
+	//
+	// Repeated median aggregation tests aggregation memoization.
+	//
+	// The test-packetman-spec tests a reference between a filter tactics
+	// and default tactics.
 
 	tacticsConfigTemplate := `
     {
@@ -54,7 +58,8 @@ func TestTactics(t *testing.T) {
         "TTL" : "1s",
         "Probability" : %0.1f,
         "Parameters" : {
-          "NetworkLatencyMultiplier" : %0.1f
+          "NetworkLatencyMultiplier" : %0.1f,
+          "ServerPacketManipulationSpecs" : [{"Name": "test-packetman-spec", "PacketSpecs": [["TCP-flags S"]]}]
         }
       },
       "FilteredTactics" : [
@@ -97,6 +102,16 @@ func TestTactics(t *testing.T) {
               "ConnectionWorkerPoolSize" : %d
             }
           }
+        },
+        {
+          "Filter" : {
+            "Regions": ["R7"]
+          },
+          "Tactics" : {
+            "Parameters" : {
+              "ServerProtocolPacketManipulations": {"All" : ["test-packetman-spec"]}
+            }
+          }
         }
       ]
     }