caddyhttp: Add MatchWithError to replace SetVar hack (#6596)

* caddyhttp: Add `MatchWithError` to replace SetVar hack

* Error in IP matchers on TLS handshake not complete

* Use MatchWithError everywhere possible

* Move implementations to MatchWithError versions

* Looser interface checking to allow fallback

* CEL factories can return RequestMatcherWithError

* Clarifying comment since it's subtle that an err is returned

* Return 425 Too Early status in IP matchers

* Keep AnyMatch signature the same for now

* Apparently Deprecated can't be all-uppercase to get IDE linting

* Linter

Author: francislavoie

caddyconfig/httpcaddyfile/httptype.go

@@ -1466,9 +1466,9 @@ func (st *ServerType) compileEncodedMatcherSets(sblock serverBlock) ([]caddy.Mod
 
 	// iterate each pairing of host and path matchers and
 	// put them into a map for JSON encoding
-	var matcherSets []map[string]caddyhttp.RequestMatcher
+	var matcherSets []map[string]caddyhttp.RequestMatcherWithError
 	for _, mp := range matcherPairs {
-		matcherSet := make(map[string]caddyhttp.RequestMatcher)
+		matcherSet := make(map[string]caddyhttp.RequestMatcherWithError)
 		if len(mp.hostm) > 0 {
 			matcherSet["host"] = mp.hostm
 		}
@@ -1527,12 +1527,17 @@ func parseMatcherDefinitions(d *caddyfile.Dispenser, matchers map[string]caddy.M
 		if err != nil {
 			return err
 		}
-		rm, ok := unm.(caddyhttp.RequestMatcher)
-		if !ok {
-			return fmt.Errorf("matcher module '%s' is not a request matcher", matcherName)
+
+		if rm, ok := unm.(caddyhttp.RequestMatcherWithError); ok {
+			matchers[definitionName][matcherName] = caddyconfig.JSON(rm, nil)
+			return nil
 		}
-		matchers[definitionName][matcherName] = caddyconfig.JSON(rm, nil)
-		return nil
+		// nolint:staticcheck
+		if rm, ok := unm.(caddyhttp.RequestMatcher); ok {
+			matchers[definitionName][matcherName] = caddyconfig.JSON(rm, nil)
+			return nil
+		}
+		return fmt.Errorf("matcher module '%s' is not a request matcher", matcherName)
 	}
 
 	// if the next token is quoted, we can assume it's not a matcher name
@@ -1576,7 +1581,7 @@ func parseMatcherDefinitions(d *caddyfile.Dispenser, matchers map[string]caddy.M
 	return nil
 }
 
-func encodeMatcherSet(matchers map[string]caddyhttp.RequestMatcher) (caddy.ModuleMap, error) {
+func encodeMatcherSet(matchers map[string]caddyhttp.RequestMatcherWithError) (caddy.ModuleMap, error) {
 	msEncoded := make(caddy.ModuleMap)
 	for matcherName, val := range matchers {
 		jsonBytes, err := json.Marshal(val)

modules/caddyhttp/caddyhttp.go

@@ -36,10 +36,26 @@ func init() {
 // RequestMatcher is a type that can match to a request.
 // A route matcher MUST NOT modify the request, with the
 // only exception being its context.
+//
+// Deprecated: Matchers should now implement RequestMatcherWithError.
+// You may remove any interface guards for RequestMatcher
+// but keep your Match() methods for backwards compatibility.
 type RequestMatcher interface {
 	Match(*http.Request) bool
 }
 
+// RequestMatcherWithError is like RequestMatcher but can return an error.
+// An error during matching will abort the request middleware chain and
+// invoke the error middleware chain.
+//
+// This will eventually replace RequestMatcher. Matcher modules
+// should implement both interfaces, and once all modules have
+// been updated to use RequestMatcherWithError, the RequestMatcher
+// interface may eventually be dropped.
+type RequestMatcherWithError interface {
+	MatchWithError(*http.Request) (bool, error)
+}
+
 // Handler is like http.Handler except ServeHTTP may return an error.
 //
 // If any handler encounters an error, it should be returned for proper

modules/caddyhttp/celmatcher.go

@@ -202,17 +202,25 @@ func (m *MatchExpression) Provision(ctx caddy.Context) error {
 
 // Match returns true if r matches m.
 func (m MatchExpression) Match(r *http.Request) bool {
+	match, err := m.MatchWithError(r)
+	if err != nil {
+		SetVar(r.Context(), MatcherErrorVarKey, err)
+	}
+	return match
+}
+
+// MatchWithError returns true if r matches m.
+func (m MatchExpression) MatchWithError(r *http.Request) (bool, error) {
 	celReq := celHTTPRequest{r}
 	out, _, err := m.prg.Eval(celReq)
 	if err != nil {
 		m.log.Error("evaluating expression", zap.Error(err))
-		SetVar(r.Context(), MatcherErrorVarKey, err)
-		return false
+		return false, err
 	}
 	if outBool, ok := out.Value().(bool); ok {
-		return outBool
+		return outBool, nil
 	}
-	return false
+	return false, nil
 }
 
 // UnmarshalCaddyfile implements caddyfile.Unmarshaler.
@@ -380,7 +388,7 @@ type CELLibraryProducer interface {
 // limited set of function signatures. For strong type validation you may need
 // to provide a custom macro which does a more detailed analysis of the CEL
 // literal provided to the macro as an argument.
-func CELMatcherImpl(macroName, funcName string, matcherDataTypes []*cel.Type, fac CELMatcherFactory) (cel.Library, error) {
+func CELMatcherImpl(macroName, funcName string, matcherDataTypes []*cel.Type, fac any) (cel.Library, error) {
 	requestType := cel.ObjectType("http.Request")
 	var macro parser.Macro
 	switch len(matcherDataTypes) {
@@ -424,7 +432,11 @@ func CELMatcherImpl(macroName, funcName string, matcherDataTypes []*cel.Type, fa
 }
 
 // CELMatcherFactory converts a constant CEL value into a RequestMatcher.
-type CELMatcherFactory func(data ref.Val) (RequestMatcher, error)
+// Deprecated: Use CELMatcherWithErrorFactory instead.
+type CELMatcherFactory = func(data ref.Val) (RequestMatcher, error)
+
+// CELMatcherWithErrorFactory converts a constant CEL value into a RequestMatcherWithError.
+type CELMatcherWithErrorFactory = func(data ref.Val) (RequestMatcherWithError, error)
 
 // matcherCELLibrary is a simplistic configurable cel.Library implementation.
 type matcherCELLibrary struct {
@@ -452,7 +464,7 @@ func (lib *matcherCELLibrary) ProgramOptions() []cel.ProgramOption {
 // that takes a single argument, and optimizes the implementation to precompile
 // the matcher and return a function that references the precompiled and
 // provisioned matcher.
-func CELMatcherDecorator(funcName string, fac CELMatcherFactory) interpreter.InterpretableDecorator {
+func CELMatcherDecorator(funcName string, fac any) interpreter.InterpretableDecorator {
 	return func(i interpreter.Interpretable) (interpreter.Interpretable, error) {
 		call, ok := i.(interpreter.InterpretableCall)
 		if !ok {
@@ -481,35 +493,92 @@ func CELMatcherDecorator(funcName string, fac CELMatcherFactory) interpreter.Int
 			// and matcher provisioning should be handled at dynamically.
 			return i, nil
 		}
-		matcher, err := fac(matcherData.Value())
-		if err != nil {
-			return nil, err
+
+		if factory, ok := fac.(CELMatcherWithErrorFactory); ok {
+			matcher, err := factory(matcherData.Value())
+			if err != nil {
+				return nil, err
+			}
+			return interpreter.NewCall(
+				i.ID(), funcName, funcName+"_opt",
+				[]interpreter.Interpretable{reqAttr},
+				func(args ...ref.Val) ref.Val {
+					// The request value, guaranteed to be of type celHTTPRequest
+					celReq := args[0]
+					// If needed this call could be changed to convert the value
+					// to a *http.Request using CEL's ConvertToNative method.
+					httpReq := celReq.Value().(celHTTPRequest)
+					match, err := matcher.MatchWithError(httpReq.Request)
+					if err != nil {
+						return types.WrapErr(err)
+					}
+					return types.Bool(match)
+				},
+			), nil
 		}
-		return interpreter.NewCall(
-			i.ID(), funcName, funcName+"_opt",
-			[]interpreter.Interpretable{reqAttr},
-			func(args ...ref.Val) ref.Val {
-				// The request value, guaranteed to be of type celHTTPRequest
-				celReq := args[0]
-				// If needed this call could be changed to convert the value
-				// to a *http.Request using CEL's ConvertToNative method.
-				httpReq := celReq.Value().(celHTTPRequest)
-				return types.Bool(matcher.Match(httpReq.Request))
-			},
-		), nil
+
+		if factory, ok := fac.(CELMatcherFactory); ok {
+			matcher, err := factory(matcherData.Value())
+			if err != nil {
+				return nil, err
+			}
+			return interpreter.NewCall(
+				i.ID(), funcName, funcName+"_opt",
+				[]interpreter.Interpretable{reqAttr},
+				func(args ...ref.Val) ref.Val {
+					// The request value, guaranteed to be of type celHTTPRequest
+					celReq := args[0]
+					// If needed this call could be changed to convert the value
+					// to a *http.Request using CEL's ConvertToNative method.
+					httpReq := celReq.Value().(celHTTPRequest)
+					if m, ok := matcher.(RequestMatcherWithError); ok {
+						match, err := m.MatchWithError(httpReq.Request)
+						if err != nil {
+							return types.WrapErr(err)
+						}
+						return types.Bool(match)
+					}
+					return types.Bool(matcher.Match(httpReq.Request))
+				},
+			), nil
+		}
+
+		return nil, fmt.Errorf("invalid matcher factory, must be CELMatcherFactory or CELMatcherWithErrorFactory: %T", fac)
 	}
 }
 
 // CELMatcherRuntimeFunction creates a function binding for when the input to the matcher
 // is dynamically resolved rather than a set of static constant values.
-func CELMatcherRuntimeFunction(funcName string, fac CELMatcherFactory) functions.BinaryOp {
+func CELMatcherRuntimeFunction(funcName string, fac any) functions.BinaryOp {
 	return func(celReq, matcherData ref.Val) ref.Val {
-		matcher, err := fac(matcherData)
-		if err != nil {
-			return types.WrapErr(err)
+		if factory, ok := fac.(CELMatcherWithErrorFactory); ok {
+			matcher, err := factory(matcherData)
+			if err != nil {
+				return types.WrapErr(err)
+			}
+			httpReq := celReq.Value().(celHTTPRequest)
+			match, err := matcher.MatchWithError(httpReq.Request)
+			if err != nil {
+				return types.WrapErr(err)
+			}
+			return types.Bool(match)
+		}
+		if factory, ok := fac.(CELMatcherFactory); ok {
+			matcher, err := factory(matcherData)
+			if err != nil {
+				return types.WrapErr(err)
+			}
+			httpReq := celReq.Value().(celHTTPRequest)
+			if m, ok := matcher.(RequestMatcherWithError); ok {
+				match, err := m.MatchWithError(httpReq.Request)
+				if err != nil {
+					return types.WrapErr(err)
+				}
+				return types.Bool(match)
+			}
+			return types.Bool(matcher.Match(httpReq.Request))
 		}
-		httpReq := celReq.Value().(celHTTPRequest)
-		return types.Bool(matcher.Match(httpReq.Request))
+		return types.NewErr("CELMatcherRuntimeFunction invalid matcher factory: %T", fac)
 	}
 }
 
@@ -733,9 +802,9 @@ const MatcherNameCtxKey = "matcher_name"
 
 // Interface guards
 var (
-	_ caddy.Provisioner     = (*MatchExpression)(nil)
-	_ RequestMatcher        = (*MatchExpression)(nil)
-	_ caddyfile.Unmarshaler = (*MatchExpression)(nil)
-	_ json.Marshaler        = (*MatchExpression)(nil)
-	_ json.Unmarshaler      = (*MatchExpression)(nil)
+	_ caddy.Provisioner       = (*MatchExpression)(nil)
+	_ RequestMatcherWithError = (*MatchExpression)(nil)
+	_ caddyfile.Unmarshaler   = (*MatchExpression)(nil)
+	_ json.Marshaler          = (*MatchExpression)(nil)
+	_ json.Unmarshaler        = (*MatchExpression)(nil)
 )

modules/caddyhttp/celmatcher_test.go

@@ -489,7 +489,11 @@ func TestMatchExpressionMatch(t *testing.T) {
 				}
 			}
 
-			if tc.expression.Match(req) != tc.wantResult {
+			matches, err := tc.expression.MatchWithError(req)
+			if err != nil {
+				t.Errorf("MatchExpression.Match() error = %v", err)
+			}
+			if matches != tc.wantResult {
 				t.Errorf("MatchExpression.Match() expected to return '%t', for expression : '%s'", tc.wantResult, tc.expression.Expr)
 			}
 		})
@@ -532,7 +536,7 @@ func BenchmarkMatchExpressionMatch(b *testing.B) {
 			}
 			b.ResetTimer()
 			for i := 0; i < b.N; i++ {
-				tc.expression.Match(req)
+				tc.expression.MatchWithError(req)
 			}
 		})
 	}