Merge branch 'main' into feat/envoy-billing-pipeline

Author: JoseSzycho

internal/controller/trafficprotectionpolicy_controller.go

@@ -6,6 +6,7 @@ import (
 	"context"
 	"encoding/json"
 	"fmt"
+	"slices"
 	"sort"
 	"strings"
 
@@ -1229,7 +1230,7 @@ func (r *TrafficProtectionPolicyReconciler) getCorazaDirectivesForTrafficProtect
 		secRuleEngine = "Off"
 	}
 
-	directives := r.Config.Gateway.Coraza.RouteBaseDirectives
+	directives := slices.Clone(r.Config.Gateway.Coraza.RouteBaseDirectives)
 
 	directives = append(directives, fmt.Sprintf("SecRuleEngine %s", secRuleEngine))
 

internal/controller/trafficprotectionpolicy_controller_test.go

@@ -992,6 +992,36 @@ func TestGetCorazaDirectivesForTrafficProtectionPolicy(t *testing.T) {
 	}
 }
 
+func TestGetCorazaDirectivesDoesNotAliasRouteBaseDirectives(t *testing.T) {
+	base := make([]string, 2, 8)
+	base[0] = "Include @crs-setup-conf"
+	base[1] = "Include @recommended-conf"
+
+	operatorConfig := config.NetworkServicesOperator{
+		Gateway: config.GatewayConfig{
+			Coraza: config.CorazaConfig{RouteBaseDirectives: base},
+		},
+	}
+	reconciler := &TrafficProtectionPolicyReconciler{Config: operatorConfig}
+
+	enforce := reconciler.getCorazaDirectivesForTrafficProtectionPolicy(&policyContext{ptr.To(
+		newTrafficProtectionPolicy("default", "tpp-enforce", func(tpp *networkingv1alpha.TrafficProtectionPolicy) {
+			tpp.Spec.Mode = networkingv1alpha.TrafficProtectionPolicyEnforce
+		}),
+	)})
+	assert.Contains(t, enforce, "SecRuleEngine On")
+
+	reconciler.getCorazaDirectivesForTrafficProtectionPolicy(&policyContext{ptr.To(
+		newTrafficProtectionPolicy("default", "tpp-observe"),
+	)})
+
+	assert.Contains(t, enforce, "SecRuleEngine On",
+		"second policy leaked into the first via a shared RouteBaseDirectives backing array")
+	assert.EqualValues(t, []string{"Include @crs-setup-conf", "Include @recommended-conf"},
+		reconciler.Config.Gateway.Coraza.RouteBaseDirectives,
+		"shared base directives were mutated")
+}
+
 // nolint:unparam
 func newTrafficProtectionPolicy(
 	namespace,

internal/extensionserver/cmd/run.go

@@ -142,13 +142,33 @@ func buildLocalReplyConfig(cfg config.ErrorPageConfig, log *slog.Logger) mutate.
 		}
 	}
 
-	return mutate.LocalReplyConfig{
+	lrConfig := mutate.LocalReplyConfig{
 		Disabled:      !cfg.Enabled,
 		MinStatusCode: cfg.MinStatusCode,
 		RuntimeKey:    cfg.RuntimeKey,
 		BodyHTML:      body,
 		ContentType:   cfg.ContentType,
 	}
+
+	// Validate before the config can reach the data plane. Escaping already makes
+	// the body Envoy-safe, so this is defense-in-depth: if the escaper or its
+	// allowlist ever regresses, fail loud here and fall back to a known-safe body
+	// instead of NACKing the xDS update fleet-wide on the failOpen:false hook.
+	if err := mutate.ValidateLocalReplyConfig(&lrConfig); err != nil {
+		if body != extassets.DefaultError5xxHTML {
+			log.Error("error-page body override failed validation; using embedded default",
+				"path", cfg.BodyPath, "err", err)
+			lrConfig.BodyHTML = extassets.DefaultError5xxHTML
+			err = mutate.ValidateLocalReplyConfig(&lrConfig)
+		}
+		if err != nil {
+			log.Error("embedded default error page failed validation; disabling branded error page",
+				"err", err)
+			lrConfig.Disabled = true
+		}
+	}
+
+	return lrConfig
 }
 
 // run starts the extension server with the given options. It owns cache startup,

internal/extensionserver/mutate/localreply.go

@@ -2,6 +2,7 @@ package mutate
 
 import (
 	"fmt"
+	"strings"
 
 	accesslogv3 "github.com/envoyproxy/go-control-plane/envoy/config/accesslog/v3"
 	corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3"
@@ -146,10 +147,114 @@ func buildLocalReplyConfig(cfg *LocalReplyConfig) *hcmv3.LocalReplyConfig {
 				ContentType: cfg.ContentType,
 				Format: &corev3.SubstitutionFormatString_TextFormatSource{
 					TextFormatSource: &corev3.DataSource{
-						Specifier: &corev3.DataSource_InlineString{InlineString: cfg.BodyHTML},
+						Specifier: &corev3.DataSource_InlineString{InlineString: escapeEnvoyFormatLiterals(cfg.BodyHTML)},
 					},
 				},
 			},
 		}},
 	}
 }
+
+// envoyBodyAllowedCommands is the allowlist of Envoy substitution command
+// operators permitted to pass through unescaped in an error-page body. It is an
+// allowlist, not a grammar match: only operators we actually template are
+// preserved, because any *other* command-shaped token (e.g. a literal
+// "%COMPLETE%" in an override page) is not a valid Envoy operator and would
+// re-trigger the exact listener rejection this escaper exists to prevent
+// (issue #243). Keep entries longest-first so matching is unambiguous.
+var envoyBodyAllowedCommands = []string{
+	"%RESPONSE_CODE%",
+}
+
+// matchAllowedCommand returns the allowlisted command operator that prefixes s,
+// or "" if none does.
+func matchAllowedCommand(s string) string {
+	for _, cmd := range envoyBodyAllowedCommands {
+		if strings.HasPrefix(s, cmd) {
+			return cmd
+		}
+	}
+	return ""
+}
+
+// escapeEnvoyFormatLiterals makes an arbitrary string safe to embed as the
+// text_format of an Envoy SubstitutionFormatString. Envoy parses that string as
+// a format template, so any bare '%' is read as the start of a command operator
+// and an unrecognized one is rejected — which, on the failOpen:false downstream
+// hook, NACKs the whole listener xDS update fleet-wide (see issue #243: the
+// branded error page's CSS "height: 100%" took the listener down).
+//
+// Bare '%' are escaped to '%%' (Envoy's literal-percent escape) while
+// already-escaped '%%' and allowlisted operators (envoyBodyAllowedCommands,
+// e.g. the intended %RESPONSE_CODE%) are preserved unchanged. Every other
+// command-shaped token is treated as a literal and escaped, so an unrecognized
+// operator can never survive into the pushed config. The transform is
+// idempotent.
+func escapeEnvoyFormatLiterals(s string) string {
+	var b strings.Builder
+	b.Grow(len(s))
+	for i := 0; i < len(s); {
+		if s[i] != '%' {
+			b.WriteByte(s[i])
+			i++
+			continue
+		}
+		if i+1 < len(s) && s[i+1] == '%' {
+			b.WriteString("%%")
+			i += 2
+			continue
+		}
+		if cmd := matchAllowedCommand(s[i:]); cmd != "" {
+			b.WriteString(cmd)
+			i += len(cmd)
+			continue
+		}
+		b.WriteString("%%")
+		i++
+	}
+	return b.String()
+}
+
+// assertEnvoyFormatSafe returns an error if s still contains a bare '%' that
+// Envoy would misparse as an incomplete or unrecognized command operator. It is
+// the inverse invariant of escapeEnvoyFormatLiterals and backs the startup
+// validation in ValidateLocalReplyConfig.
+func assertEnvoyFormatSafe(s string) error {
+	for i := 0; i < len(s); {
+		if s[i] != '%' {
+			i++
+			continue
+		}
+		if i+1 < len(s) && s[i+1] == '%' {
+			i += 2
+			continue
+		}
+		if cmd := matchAllowedCommand(s[i:]); cmd != "" {
+			i += len(cmd)
+			continue
+		}
+		return fmt.Errorf("bare %% at byte offset %d", i)
+	}
+	return nil
+}
+
+// ValidateLocalReplyConfig assembles the local_reply_config and asserts it is
+// safe to push, so a malformed body is caught in the operator process at startup
+// instead of NACKing the xDS update fleet-wide on the failOpen:false downstream
+// hook (issue #243). It round-trips the config through proto marshalling and
+// verifies the injected body carries no bare '%'. A disabled or empty config is
+// vacuously valid (injection is a no-op).
+func ValidateLocalReplyConfig(cfg *LocalReplyConfig) error {
+	if cfg.Disabled || cfg.BodyHTML == "" {
+		return nil
+	}
+	lrc := buildLocalReplyConfig(cfg)
+	if _, err := anypb.New(lrc); err != nil {
+		return fmt.Errorf("marshal local_reply_config: %w", err)
+	}
+	body := lrc.GetMappers()[0].GetBodyFormatOverride().GetTextFormatSource().GetInlineString()
+	if err := assertEnvoyFormatSafe(body); err != nil {
+		return fmt.Errorf("assembled error-page body is not Envoy-safe: %w", err)
+	}
+	return nil
+}

internal/extensionserver/mutate/localreply_test.go

@@ -1,6 +1,7 @@
 package mutate
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
@@ -11,6 +12,8 @@ import (
 	routev3 "github.com/envoyproxy/go-control-plane/envoy/config/route/v3"
 	hcmv3 "github.com/envoyproxy/go-control-plane/envoy/extensions/filters/network/http_connection_manager/v3"
 	"google.golang.org/protobuf/types/known/anypb"
+
+	"go.datum.net/network-services-operator/internal/extensionserver/assets"
 )
 
 const (
@@ -218,3 +221,84 @@ func TestInjectLocalReplyConfig_NoOp(t *testing.T) {
 		})
 	}
 }
+
+func TestEscapeEnvoyFormatLiterals(t *testing.T) {
+	tests := []struct {
+		name string
+		in   string
+		want string
+	}{
+		{"empty", "", ""},
+		{"no percent", "<html>plain</html>", "<html>plain</html>"},
+		{"bare percent in css", "height: 100%;", "height: 100%%;"},
+		{"multiple bare percents", "120% 50% 0%", "120%% 50%% 0%%"},
+		{"preserve allowlisted command", "code %RESPONSE_CODE% end", "code %RESPONSE_CODE% end"},
+		{"escape non-allowlisted command", "%REQ(x-header)%", "%%REQ(x-header)%%"},
+		{"escape non-allowlisted command with length", "%REQ(x-header):10%", "%%REQ(x-header):10%%"},
+		{"escape command-shaped literal", "progress %COMPLETE% now", "progress %%COMPLETE%% now"},
+		{"already escaped stays escaped", "width: 100%%;", "width: 100%%;"},
+		{"mixed command and literal", "%RESPONSE_CODE% at 100%", "%RESPONSE_CODE% at 100%%"},
+		{"trailing bare percent", "50%", "50%%"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := escapeEnvoyFormatLiterals(tc.in)
+			assert.Equal(t, tc.want, got)
+			assert.Equal(t, got, escapeEnvoyFormatLiterals(got), "must be idempotent")
+		})
+	}
+}
+
+// TestEmbeddedDefaultPageIsEnvoySafe is the regression guard for issue #243:
+// the embedded default error page contains literal CSS percent signs
+// (e.g. "height: 100%") that Envoy would misparse as command operators and
+// reject the listener. After escaping, no bare '%' may remain and the intended
+// %RESPONSE_CODE% operator must survive.
+func TestEmbeddedDefaultPageIsEnvoySafe(t *testing.T) {
+	raw := assets.DefaultError5xxHTML
+	require.Contains(t, raw, "height: 100%;", "test premise: raw page has an unescaped percent")
+
+	escaped := escapeEnvoyFormatLiterals(raw)
+
+	assert.Contains(t, escaped, "height: 100%%;", "literal percent must be escaped")
+	assert.Equal(t, 1, strings.Count(escaped, "%RESPONSE_CODE%"), "command operator must be preserved exactly once")
+	assert.Equal(t, escaped, escapeEnvoyFormatLiterals(escaped), "escaping must be idempotent")
+
+	// The escaped body must satisfy the same invariant the startup validator
+	// asserts: no bare '%' Envoy could misparse as a command operator.
+	assert.NoError(t, assertEnvoyFormatSafe(escaped), "no bare percent may remain after escaping")
+}
+
+// TestValidateLocalReplyConfig covers the startup guard (issue #243): the
+// assembled config must validate for the embedded default, reject a body whose
+// bare '%' would reach Envoy unescaped, and no-op when injection is disabled.
+func TestValidateLocalReplyConfig(t *testing.T) {
+	base := testLocalReplyConfig()
+
+	t.Run("embedded default is valid", func(t *testing.T) {
+		cfg := *base
+		cfg.BodyHTML = assets.DefaultError5xxHTML
+		assert.NoError(t, ValidateLocalReplyConfig(&cfg))
+	})
+
+	t.Run("escaped body is valid", func(t *testing.T) {
+		cfg := *base
+		cfg.BodyHTML = "height: 100%;"
+		assert.NoError(t, ValidateLocalReplyConfig(&cfg))
+	})
+
+	t.Run("disabled is a no-op", func(t *testing.T) {
+		cfg := *base
+		cfg.Disabled = true
+		cfg.BodyHTML = "height: 100%;"
+		assert.NoError(t, ValidateLocalReplyConfig(&cfg))
+	})
+
+	t.Run("bare percent surviving into the body is rejected", func(t *testing.T) {
+		// buildLocalReplyConfig escapes, so this asserts the raw invariant the
+		// validator enforces on the injected body.
+		assert.Error(t, assertEnvoyFormatSafe("height: 100%;"))
+		assert.Error(t, assertEnvoyFormatSafe("%COMPLETE%"))
+		assert.NoError(t, assertEnvoyFormatSafe("code %RESPONSE_CODE% ok"))
+	})
+}