From 09b2cbcf4d839adec91b189fea549d64a69e0595 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 4 Nov 2024 18:18:50 -0500 Subject: [PATCH] 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 --- caddyconfig/httpcaddyfile/httptype.go | 21 +- modules/caddyhttp/caddyhttp.go | 16 ++ modules/caddyhttp/celmatcher.go | 135 ++++++++--- modules/caddyhttp/celmatcher_test.go | 8 +- modules/caddyhttp/fileserver/matcher.go | 45 ++-- modules/caddyhttp/fileserver/matcher_test.go | 17 +- modules/caddyhttp/ip_matchers.go | 64 +++-- modules/caddyhttp/matchers.go | 219 ++++++++++++------ modules/caddyhttp/matchers_test.go | 53 +++-- .../caddyhttp/reverseproxy/healthchecks.go | 2 +- .../caddyhttp/reverseproxy/httptransport.go | 4 +- .../caddyhttp/reverseproxy/reverseproxy.go | 13 +- modules/caddyhttp/routes.go | 97 ++++++-- modules/caddyhttp/vars.go | 38 ++- modules/caddytls/connpolicy.go | 6 +- modules/caddytls/ondemand.go | 2 +- 16 files changed, 537 insertions(+), 203 deletions(-) diff --git a/caddyconfig/httpcaddyfile/httptype.go b/caddyconfig/httpcaddyfile/httptype.go index 704424ac..c169b92a 100644 --- a/caddyconfig/httpcaddyfile/httptype.go +++ b/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) diff --git a/modules/caddyhttp/caddyhttp.go b/modules/caddyhttp/caddyhttp.go index e1e71f4a..aacafc92 100644 --- a/modules/caddyhttp/caddyhttp.go +++ b/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 diff --git a/modules/caddyhttp/celmatcher.go b/modules/caddyhttp/celmatcher.go index 2a03ebba..3d118ea7 100644 --- a/modules/caddyhttp/celmatcher.go +++ b/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) } - httpReq := celReq.Value().(celHTTPRequest) - return types.Bool(matcher.Match(httpReq.Request)) + 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)) + } + 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) ) diff --git a/modules/caddyhttp/celmatcher_test.go b/modules/caddyhttp/celmatcher_test.go index 26491b7c..a7e91529 100644 --- a/modules/caddyhttp/celmatcher_test.go +++ b/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) } }) } diff --git a/modules/caddyhttp/fileserver/matcher.go b/modules/caddyhttp/fileserver/matcher.go index 28f7b89b..c4290749 100644 --- a/modules/caddyhttp/fileserver/matcher.go +++ b/modules/caddyhttp/fileserver/matcher.go @@ -173,7 +173,7 @@ func (m *MatchFile) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { func (MatchFile) CELLibrary(ctx caddy.Context) (cel.Library, error) { requestType := cel.ObjectType("http.Request") - matcherFactory := func(data ref.Val) (caddyhttp.RequestMatcher, error) { + matcherFactory := func(data ref.Val) (caddyhttp.RequestMatcherWithError, error) { values, err := caddyhttp.CELValueToMapStrList(data) if err != nil { return nil, err @@ -313,12 +313,22 @@ func (m MatchFile) Validate() error { // - http.matchers.file.type: file or directory // - http.matchers.file.remainder: Portion remaining after splitting file path (if configured) func (m MatchFile) Match(r *http.Request) bool { + match, err := m.selectFile(r) + if err != nil { + // nolint:staticcheck + caddyhttp.SetVar(r.Context(), caddyhttp.MatcherErrorVarKey, err) + } + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchFile) MatchWithError(r *http.Request) (bool, error) { return m.selectFile(r) } // selectFile chooses a file according to m.TryPolicy by appending // the paths in m.TryFiles to m.Root, with placeholder replacements. -func (m MatchFile) selectFile(r *http.Request) (matched bool) { +func (m MatchFile) selectFile(r *http.Request) (bool, error) { repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer) root := filepath.Clean(repl.ReplaceAll(m.Root, ".")) @@ -330,7 +340,7 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) { if c := m.logger.Check(zapcore.ErrorLevel, "use of unregistered filesystem"); c != nil { c.Write(zap.String("fs", fsName)) } - return false + return false, nil } type matchCandidate struct { fullpath, relative, splitRemainder string @@ -421,15 +431,18 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) { switch m.TryPolicy { case "", tryPolicyFirstExist: for _, pattern := range m.TryFiles { + // If the pattern is a status code, emit an error, + // which short-circuits the middleware pipeline and + // writes an HTTP error response. if err := parseErrorCode(pattern); err != nil { - caddyhttp.SetVar(r.Context(), caddyhttp.MatcherErrorVarKey, err) - return + return false, err } + candidates := makeCandidates(pattern) for _, c := range candidates { if info, exists := m.strictFileExists(fileSystem, c.fullpath); exists { setPlaceholders(c, info) - return true + return true, nil } } } @@ -450,10 +463,10 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) { } } if largestInfo == nil { - return false + return false, nil } setPlaceholders(largest, largestInfo) - return true + return true, nil case tryPolicySmallestSize: var smallestSize int64 @@ -471,10 +484,10 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) { } } if smallestInfo == nil { - return false + return false, nil } setPlaceholders(smallest, smallestInfo) - return true + return true, nil case tryPolicyMostRecentlyMod: var recent matchCandidate @@ -491,13 +504,13 @@ func (m MatchFile) selectFile(r *http.Request) (matched bool) { } } if recentInfo == nil { - return false + return false, nil } setPlaceholders(recent, recentInfo) - return true + return true, nil } - return + return false, nil } // parseErrorCode checks if the input is a status @@ -703,7 +716,7 @@ const ( // Interface guards var ( - _ caddy.Validator = (*MatchFile)(nil) - _ caddyhttp.RequestMatcher = (*MatchFile)(nil) - _ caddyhttp.CELLibraryProducer = (*MatchFile)(nil) + _ caddy.Validator = (*MatchFile)(nil) + _ caddyhttp.RequestMatcherWithError = (*MatchFile)(nil) + _ caddyhttp.CELLibraryProducer = (*MatchFile)(nil) ) diff --git a/modules/caddyhttp/fileserver/matcher_test.go b/modules/caddyhttp/fileserver/matcher_test.go index 95eeb821..b6697b9d 100644 --- a/modules/caddyhttp/fileserver/matcher_test.go +++ b/modules/caddyhttp/fileserver/matcher_test.go @@ -130,7 +130,10 @@ func TestFileMatcher(t *testing.T) { req := &http.Request{URL: u} repl := caddyhttp.NewTestReplacer(req) - result := m.Match(req) + result, err := m.MatchWithError(req) + if err != nil { + t.Errorf("Test %d: unexpected error: %v", i, err) + } if result != tc.matched { t.Errorf("Test %d: expected match=%t, got %t", i, tc.matched, result) } @@ -240,7 +243,10 @@ func TestPHPFileMatcher(t *testing.T) { req := &http.Request{URL: u} repl := caddyhttp.NewTestReplacer(req) - result := m.Match(req) + result, err := m.MatchWithError(req) + if err != nil { + t.Errorf("Test %d: unexpected error: %v", i, err) + } if result != tc.matched { t.Errorf("Test %d: expected match=%t, got %t", i, tc.matched, result) } @@ -389,7 +395,12 @@ func TestMatchExpressionMatch(t *testing.T) { ctx := context.WithValue(req.Context(), caddy.ReplacerCtxKey, repl) req = req.WithContext(ctx) - if tc.expression.Match(req) != tc.wantResult { + matches, err := tc.expression.MatchWithError(req) + if err != nil { + t.Errorf("MatchExpression.Match() error = %v", err) + return + } + if matches != tc.wantResult { t.Errorf("MatchExpression.Match() expected to return '%t', for expression : '%s'", tc.wantResult, tc.expression.Expr) } diff --git a/modules/caddyhttp/ip_matchers.go b/modules/caddyhttp/ip_matchers.go index 99eb39df..5e0b356e 100644 --- a/modules/caddyhttp/ip_matchers.go +++ b/modules/caddyhttp/ip_matchers.go @@ -108,7 +108,7 @@ func (MatchRemoteIP) CELLibrary(ctx caddy.Context) (cel.Library, error) { // internal data type of the MatchPath value. []*cel.Type{cel.ListType(cel.StringType)}, // function to convert a constant list of strings to a MatchPath instance. - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) strList, err := data.ConvertToNative(refStringList) if err != nil { @@ -145,9 +145,23 @@ func (m *MatchRemoteIP) Provision(ctx caddy.Context) error { // Match returns true if r matches m. func (m MatchRemoteIP) Match(r *http.Request) bool { - if r.TLS != nil && !r.TLS.HandshakeComplete { - return false // if handshake is not finished, we infer 0-RTT that has not verified remote IP; could be spoofed + match, err := m.MatchWithError(r) + if err != nil { + SetVar(r.Context(), MatcherErrorVarKey, err) } + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchRemoteIP) MatchWithError(r *http.Request) (bool, error) { + // if handshake is not finished, we infer 0-RTT that has + // not verified remote IP; could be spoofed, so we throw + // HTTP 425 status to tell the client to try again after + // the handshake is complete + if r.TLS != nil && !r.TLS.HandshakeComplete { + return false, Error(http.StatusTooEarly, fmt.Errorf("TLS handshake not complete, remote IP cannot be verified")) + } + address := r.RemoteAddr clientIP, zoneID, err := parseIPZoneFromString(address) if err != nil { @@ -155,7 +169,7 @@ func (m MatchRemoteIP) Match(r *http.Request) bool { c.Write(zap.Error(err)) } - return false + return false, nil } matches, zoneFilter := matchIPByCidrZones(clientIP, zoneID, m.cidrs, m.zones) if !matches && !zoneFilter { @@ -163,7 +177,7 @@ func (m MatchRemoteIP) Match(r *http.Request) bool { c.Write(zap.String("zone", zoneID)) } } - return matches + return matches, nil } // CaddyModule returns the Caddy module information. @@ -207,7 +221,7 @@ func (MatchClientIP) CELLibrary(ctx caddy.Context) (cel.Library, error) { // internal data type of the MatchPath value. []*cel.Type{cel.ListType(cel.StringType)}, // function to convert a constant list of strings to a MatchPath instance. - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) strList, err := data.ConvertToNative(refStringList) if err != nil { @@ -238,20 +252,34 @@ func (m *MatchClientIP) Provision(ctx caddy.Context) error { // Match returns true if r matches m. func (m MatchClientIP) Match(r *http.Request) bool { - if r.TLS != nil && !r.TLS.HandshakeComplete { - return false // if handshake is not finished, we infer 0-RTT that has not verified remote IP; could be spoofed + match, err := m.MatchWithError(r) + if err != nil { + SetVar(r.Context(), MatcherErrorVarKey, err) } + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchClientIP) MatchWithError(r *http.Request) (bool, error) { + // if handshake is not finished, we infer 0-RTT that has + // not verified remote IP; could be spoofed, so we throw + // HTTP 425 status to tell the client to try again after + // the handshake is complete + if r.TLS != nil && !r.TLS.HandshakeComplete { + return false, Error(http.StatusTooEarly, fmt.Errorf("TLS handshake not complete, remote IP cannot be verified")) + } + address := GetVar(r.Context(), ClientIPVarKey).(string) clientIP, zoneID, err := parseIPZoneFromString(address) if err != nil { m.logger.Error("getting client IP", zap.Error(err)) - return false + return false, nil } matches, zoneFilter := matchIPByCidrZones(clientIP, zoneID, m.cidrs, m.zones) if !matches && !zoneFilter { m.logger.Debug("zone ID from client IP did not match", zap.String("zone", zoneID)) } - return matches + return matches, nil } func provisionCidrsZonesFromRanges(ranges []string) ([]*netip.Prefix, []string, error) { @@ -326,13 +354,13 @@ func matchIPByCidrZones(clientIP netip.Addr, zoneID string, cidrs []*netip.Prefi // Interface guards var ( - _ RequestMatcher = (*MatchRemoteIP)(nil) - _ caddy.Provisioner = (*MatchRemoteIP)(nil) - _ caddyfile.Unmarshaler = (*MatchRemoteIP)(nil) - _ CELLibraryProducer = (*MatchRemoteIP)(nil) + _ RequestMatcherWithError = (*MatchRemoteIP)(nil) + _ caddy.Provisioner = (*MatchRemoteIP)(nil) + _ caddyfile.Unmarshaler = (*MatchRemoteIP)(nil) + _ CELLibraryProducer = (*MatchRemoteIP)(nil) - _ RequestMatcher = (*MatchClientIP)(nil) - _ caddy.Provisioner = (*MatchClientIP)(nil) - _ caddyfile.Unmarshaler = (*MatchClientIP)(nil) - _ CELLibraryProducer = (*MatchClientIP)(nil) + _ RequestMatcherWithError = (*MatchClientIP)(nil) + _ caddy.Provisioner = (*MatchClientIP)(nil) + _ caddyfile.Unmarshaler = (*MatchClientIP)(nil) + _ CELLibraryProducer = (*MatchClientIP)(nil) ) diff --git a/modules/caddyhttp/matchers.go b/modules/caddyhttp/matchers.go index 93a36237..25fdc1fe 100644 --- a/modules/caddyhttp/matchers.go +++ b/modules/caddyhttp/matchers.go @@ -296,6 +296,12 @@ func (m MatchHost) Provision(_ caddy.Context) error { // Match returns true if r matches m. func (m MatchHost) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchHost) MatchWithError(r *http.Request) (bool, error) { reqHost, _, err := net.SplitHostPort(r.Host) if err != nil { // OK; probably didn't have a port @@ -315,7 +321,7 @@ func (m MatchHost) Match(r *http.Request) bool { return m[i] >= reqHost }) if pos < len(m) && m[pos] == reqHost { - return true + return true, nil } } @@ -346,13 +352,13 @@ outer: continue outer } } - return true + return true, nil } else if strings.EqualFold(reqHost, host) { - return true + return true, nil } } - return false + return false, nil } // CELLibrary produces options that expose this matcher for use in CEL @@ -366,7 +372,7 @@ func (MatchHost) CELLibrary(ctx caddy.Context) (cel.Library, error) { "host", "host_match_request_list", []*cel.Type{cel.ListType(cel.StringType)}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) strList, err := data.ConvertToNative(refStringList) if err != nil { @@ -411,6 +417,12 @@ func (m MatchPath) Provision(_ caddy.Context) error { // Match returns true if r matches m. func (m MatchPath) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchPath) MatchWithError(r *http.Request) (bool, error) { // Even though RFC 9110 says that path matching is case-sensitive // (https://www.rfc-editor.org/rfc/rfc9110.html#section-4.2.3), // we do case-insensitive matching to mitigate security issues @@ -436,7 +448,7 @@ func (m MatchPath) Match(r *http.Request) bool { // special case: whole path is wildcard; this is unnecessary // as it matches all requests, which is the same as no matcher if matchPattern == "*" { - return true + return true, nil } // Clean the path, merge doubled slashes, etc. @@ -464,7 +476,7 @@ func (m MatchPath) Match(r *http.Request) bool { if strings.Contains(matchPattern, "%") { reqPathForPattern := CleanPath(r.URL.EscapedPath(), mergeSlashes) if m.matchPatternWithEscapeSequence(reqPathForPattern, matchPattern) { - return true + return true, nil } // doing prefix/suffix/substring matches doesn't make sense @@ -483,7 +495,7 @@ func (m MatchPath) Match(r *http.Request) bool { strings.HasPrefix(matchPattern, "*") && strings.HasSuffix(matchPattern, "*") { if strings.Contains(reqPathForPattern, matchPattern[1:len(matchPattern)-1]) { - return true + return true, nil } continue } @@ -495,7 +507,7 @@ func (m MatchPath) Match(r *http.Request) bool { // treat it as a fast suffix match if strings.HasPrefix(matchPattern, "*") { if strings.HasSuffix(reqPathForPattern, matchPattern[1:]) { - return true + return true, nil } continue } @@ -504,7 +516,7 @@ func (m MatchPath) Match(r *http.Request) bool { // treat it as a fast prefix match if strings.HasSuffix(matchPattern, "*") { if strings.HasPrefix(reqPathForPattern, matchPattern[:len(matchPattern)-1]) { - return true + return true, nil } continue } @@ -515,10 +527,10 @@ func (m MatchPath) Match(r *http.Request) bool { // because we can't handle it anyway matches, _ := path.Match(matchPattern, reqPathForPattern) if matches { - return true + return true, nil } } - return false + return false, nil } func (MatchPath) matchPatternWithEscapeSequence(escapedPath, matchPath string) bool { @@ -642,7 +654,7 @@ func (MatchPath) CELLibrary(ctx caddy.Context) (cel.Library, error) { // internal data type of the MatchPath value. []*cel.Type{cel.ListType(cel.StringType)}, // function to convert a constant list of strings to a MatchPath instance. - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) strList, err := data.ConvertToNative(refStringList) if err != nil { @@ -677,6 +689,12 @@ func (MatchPathRE) CaddyModule() caddy.ModuleInfo { // Match returns true if r matches m. func (m MatchPathRE) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchPathRE) MatchWithError(r *http.Request) (bool, error) { repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer) // Clean the path, merges doubled slashes, etc. @@ -684,7 +702,7 @@ func (m MatchPathRE) Match(r *http.Request) bool { // the path matcher. See #4407 cleanedPath := cleanPath(r.URL.Path) - return m.MatchRegexp.Match(cleanedPath, repl) + return m.MatchRegexp.Match(cleanedPath, repl), nil } // CELLibrary produces options that expose this matcher for use in CEL @@ -698,7 +716,7 @@ func (MatchPathRE) CELLibrary(ctx caddy.Context) (cel.Library, error) { "path_regexp", "path_regexp_request_string", []*cel.Type{cel.StringType}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { pattern := data.(types.String) matcher := MatchPathRE{MatchRegexp{ Name: ctx.Value(MatcherNameCtxKey).(string), @@ -715,7 +733,7 @@ func (MatchPathRE) CELLibrary(ctx caddy.Context) (cel.Library, error) { "path_regexp", "path_regexp_request_string_string", []*cel.Type{cel.StringType, cel.StringType}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) params, err := data.ConvertToNative(refStringList) if err != nil { @@ -764,7 +782,13 @@ func (m *MatchMethod) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // Match returns true if r matches m. func (m MatchMethod) Match(r *http.Request) bool { - return slices.Contains(m, r.Method) + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchMethod) MatchWithError(r *http.Request) (bool, error) { + return slices.Contains(m, r.Method), nil } // CELLibrary produces options that expose this matcher for use in CEL @@ -778,7 +802,7 @@ func (MatchMethod) CELLibrary(_ caddy.Context) (cel.Library, error) { "method", "method_request_list", []*cel.Type{cel.ListType(cel.StringType)}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) strList, err := data.ConvertToNative(refStringList) if err != nil { @@ -823,10 +847,17 @@ func (m *MatchQuery) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // Match returns true if r matches m. An empty m matches an empty query string. func (m MatchQuery) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +// An empty m matches an empty query string. +func (m MatchQuery) MatchWithError(r *http.Request) (bool, error) { // If no query keys are configured, this only // matches an empty query string. if len(m) == 0 { - return len(r.URL.Query()) == 0 + return len(r.URL.Query()) == 0, nil } repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer) @@ -843,7 +874,7 @@ func (m MatchQuery) Match(r *http.Request) bool { // "Relying on parser alignment for security is doomed." Overall conclusion is that // splitting on & and rejecting ; in key=value pairs is safer than accepting raw ;. // We regard the Go team's decision as sound and thus reject malformed query strings. - return false + return false, nil } // Count the amount of matched keys, to ensure we AND @@ -854,7 +885,7 @@ func (m MatchQuery) Match(r *http.Request) bool { param = repl.ReplaceAll(param, "") paramVal, found := parsed[param] if !found { - return false + return false, nil } for _, v := range vals { v = repl.ReplaceAll(v, "") @@ -864,7 +895,7 @@ func (m MatchQuery) Match(r *http.Request) bool { } } } - return matchedKeys == len(m) + return matchedKeys == len(m), nil } // CELLibrary produces options that expose this matcher for use in CEL @@ -878,7 +909,7 @@ func (MatchQuery) CELLibrary(_ caddy.Context) (cel.Library, error) { "query", "query_matcher_request_map", []*cel.Type{CELTypeJSON}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { mapStrListStr, err := CELValueToMapStrList(data) if err != nil { return nil, err @@ -940,8 +971,14 @@ func (m *MatchHeader) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // Match returns true if r matches m. func (m MatchHeader) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchHeader) MatchWithError(r *http.Request) (bool, error) { repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer) - return matchHeaders(r.Header, http.Header(m), r.Host, repl) + return matchHeaders(r.Header, http.Header(m), r.Host, repl), nil } // CELLibrary produces options that expose this matcher for use in CEL @@ -956,7 +993,7 @@ func (MatchHeader) CELLibrary(_ caddy.Context) (cel.Library, error) { "header", "header_matcher_request_map", []*cel.Type{CELTypeJSON}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { mapStrListStr, err := CELValueToMapStrList(data) if err != nil { return nil, err @@ -1075,6 +1112,12 @@ func (m *MatchHeaderRE) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // Match returns true if r matches m. func (m MatchHeaderRE) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchHeaderRE) MatchWithError(r *http.Request) (bool, error) { for field, rm := range m { actualFieldVals := getHeaderFieldVals(r.Header, field, r.Host) match := false @@ -1087,10 +1130,10 @@ func (m MatchHeaderRE) Match(r *http.Request) bool { } } if !match { - return false + return false, nil } } - return true + return true, nil } // Provision compiles m's regular expressions. @@ -1126,7 +1169,7 @@ func (MatchHeaderRE) CELLibrary(ctx caddy.Context) (cel.Library, error) { "header_regexp", "header_regexp_request_string_string", []*cel.Type{cel.StringType, cel.StringType}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) params, err := data.ConvertToNative(refStringList) if err != nil { @@ -1149,7 +1192,7 @@ func (MatchHeaderRE) CELLibrary(ctx caddy.Context) (cel.Library, error) { "header_regexp", "header_regexp_request_string_string_string", []*cel.Type{cel.StringType, cel.StringType, cel.StringType}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) params, err := data.ConvertToNative(refStringList) if err != nil { @@ -1187,31 +1230,37 @@ func (MatchProtocol) CaddyModule() caddy.ModuleInfo { // Match returns true if r matches m. func (m MatchProtocol) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchProtocol) MatchWithError(r *http.Request) (bool, error) { switch string(m) { case "grpc": - return strings.HasPrefix(r.Header.Get("content-type"), "application/grpc") + return strings.HasPrefix(r.Header.Get("content-type"), "application/grpc"), nil case "https": - return r.TLS != nil + return r.TLS != nil, nil case "http": - return r.TLS == nil + return r.TLS == nil, nil case "http/1.0": - return r.ProtoMajor == 1 && r.ProtoMinor == 0 + return r.ProtoMajor == 1 && r.ProtoMinor == 0, nil case "http/1.0+": - return r.ProtoAtLeast(1, 0) + return r.ProtoAtLeast(1, 0), nil case "http/1.1": - return r.ProtoMajor == 1 && r.ProtoMinor == 1 + return r.ProtoMajor == 1 && r.ProtoMinor == 1, nil case "http/1.1+": - return r.ProtoAtLeast(1, 1) + return r.ProtoAtLeast(1, 1), nil case "http/2": - return r.ProtoMajor == 2 + return r.ProtoMajor == 2, nil case "http/2+": - return r.ProtoAtLeast(2, 0) + return r.ProtoAtLeast(2, 0), nil case "http/3": - return r.ProtoMajor == 3 + return r.ProtoMajor == 3, nil case "http/3+": - return r.ProtoAtLeast(3, 0) + return r.ProtoAtLeast(3, 0), nil } - return false + return false, nil } // UnmarshalCaddyfile implements caddyfile.Unmarshaler. @@ -1238,7 +1287,7 @@ func (MatchProtocol) CELLibrary(_ caddy.Context) (cel.Library, error) { "protocol", "protocol_request_string", []*cel.Type{cel.StringType}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { protocolStr, ok := data.(types.String) if !ok { return nil, errors.New("protocol argument was not a string") @@ -1258,16 +1307,22 @@ func (MatchTLS) CaddyModule() caddy.ModuleInfo { // Match returns true if r matches m. func (m MatchTLS) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchTLS) MatchWithError(r *http.Request) (bool, error) { if r.TLS == nil { - return false + return false, nil } if m.HandshakeComplete != nil { if (!*m.HandshakeComplete && r.TLS.HandshakeComplete) || (*m.HandshakeComplete && !r.TLS.HandshakeComplete) { - return false + return false, nil } } - return true + return true, nil } // UnmarshalCaddyfile parses Caddyfile tokens for this matcher. Syntax: @@ -1337,7 +1392,15 @@ func (m *MatchNot) Provision(ctx caddy.Context) error { for _, modMap := range matcherSets.([]map[string]any) { var ms MatcherSet for _, modIface := range modMap { - ms = append(ms, modIface.(RequestMatcher)) + if mod, ok := modIface.(RequestMatcherWithError); ok { + ms = append(ms, mod) + continue + } + if mod, ok := modIface.(RequestMatcher); ok { + ms = append(ms, mod) + continue + } + return fmt.Errorf("module is not a request matcher: %T", modIface) } m.MatcherSets = append(m.MatcherSets, ms) } @@ -1348,12 +1411,24 @@ func (m *MatchNot) Provision(ctx caddy.Context) error { // the embedded matchers, false is returned if any of its matcher // sets return true. func (m MatchNot) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. Since this matcher +// negates the embedded matchers, false is returned if any of its +// matcher sets return true. +func (m MatchNot) MatchWithError(r *http.Request) (bool, error) { for _, ms := range m.MatcherSets { - if ms.Match(r) { - return false + matches, err := ms.MatchWithError(r) + if err != nil { + return false, err + } + if matches { + return false, nil } } - return true + return true, nil } // MatchRegexp is an embedable type for matching @@ -1469,7 +1544,7 @@ func (mre *MatchRegexp) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // ParseCaddyfileNestedMatcher parses the Caddyfile tokens for a nested // matcher set, and returns its raw module map value. func ParseCaddyfileNestedMatcherSet(d *caddyfile.Dispenser) (caddy.ModuleMap, error) { - matcherMap := make(map[string]RequestMatcher) + matcherMap := make(map[string]any) // in case there are multiple instances of the same matcher, concatenate // their tokens (we expect that UnmarshalCaddyfile should be able to @@ -1494,11 +1569,15 @@ func ParseCaddyfileNestedMatcherSet(d *caddyfile.Dispenser) (caddy.ModuleMap, er if err != nil { return nil, err } - rm, ok := unm.(RequestMatcher) - if !ok { - return nil, fmt.Errorf("matcher module '%s' is not a request matcher", matcherName) + if rm, ok := unm.(RequestMatcherWithError); ok { + matcherMap[matcherName] = rm + continue } - matcherMap[matcherName] = rm + if rm, ok := unm.(RequestMatcher); ok { + matcherMap[matcherName] = rm + continue + } + return nil, fmt.Errorf("matcher module '%s' is not a request matcher", matcherName) } // we should now have a functional matcher, but we also @@ -1524,24 +1603,28 @@ const regexpPlaceholderPrefix = "http.regexp" // holds an optional error emitted from a request matcher, // to short-circuit the handler chain, since matchers cannot // return errors via the RequestMatcher interface. +// +// Deprecated: Matchers should implement RequestMatcherWithError +// which can return an error directly, instead of smuggling it +// through the vars map. const MatcherErrorVarKey = "matchers.error" // Interface guards var ( - _ RequestMatcher = (*MatchHost)(nil) - _ caddy.Provisioner = (*MatchHost)(nil) - _ RequestMatcher = (*MatchPath)(nil) - _ RequestMatcher = (*MatchPathRE)(nil) - _ caddy.Provisioner = (*MatchPathRE)(nil) - _ RequestMatcher = (*MatchMethod)(nil) - _ RequestMatcher = (*MatchQuery)(nil) - _ RequestMatcher = (*MatchHeader)(nil) - _ RequestMatcher = (*MatchHeaderRE)(nil) - _ caddy.Provisioner = (*MatchHeaderRE)(nil) - _ RequestMatcher = (*MatchProtocol)(nil) - _ RequestMatcher = (*MatchNot)(nil) - _ caddy.Provisioner = (*MatchNot)(nil) - _ caddy.Provisioner = (*MatchRegexp)(nil) + _ RequestMatcherWithError = (*MatchHost)(nil) + _ caddy.Provisioner = (*MatchHost)(nil) + _ RequestMatcherWithError = (*MatchPath)(nil) + _ RequestMatcherWithError = (*MatchPathRE)(nil) + _ caddy.Provisioner = (*MatchPathRE)(nil) + _ RequestMatcherWithError = (*MatchMethod)(nil) + _ RequestMatcherWithError = (*MatchQuery)(nil) + _ RequestMatcherWithError = (*MatchHeader)(nil) + _ RequestMatcherWithError = (*MatchHeaderRE)(nil) + _ caddy.Provisioner = (*MatchHeaderRE)(nil) + _ RequestMatcherWithError = (*MatchProtocol)(nil) + _ RequestMatcherWithError = (*MatchNot)(nil) + _ caddy.Provisioner = (*MatchNot)(nil) + _ caddy.Provisioner = (*MatchRegexp)(nil) _ caddyfile.Unmarshaler = (*MatchHost)(nil) _ caddyfile.Unmarshaler = (*MatchPath)(nil) diff --git a/modules/caddyhttp/matchers_test.go b/modules/caddyhttp/matchers_test.go index 05eaade5..f7be6909 100644 --- a/modules/caddyhttp/matchers_test.go +++ b/modules/caddyhttp/matchers_test.go @@ -158,7 +158,10 @@ func TestHostMatcher(t *testing.T) { t.Errorf("Test %d %v: provisioning failed: %v", i, tc.match, err) } - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d %v: Expected %t, got %t for '%s'", i, tc.match, tc.expect, actual, tc.input) continue @@ -430,7 +433,10 @@ func TestPathMatcher(t *testing.T) { ctx := context.WithValue(req.Context(), caddy.ReplacerCtxKey, repl) req = req.WithContext(ctx) - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d %v: Expected %t, got %t for '%s'", i, tc.match, tc.expect, actual, tc.input) continue @@ -451,7 +457,10 @@ func TestPathMatcherWindows(t *testing.T) { req = req.WithContext(ctx) match := MatchPath{"*.php"} - matched := match.Match(req) + matched, err := match.MatchWithError(req) + if err != nil { + t.Errorf("Expected no error, but got: %v", err) + } if !matched { t.Errorf("Expected to match; should ignore trailing dots and spaces") } @@ -555,7 +564,10 @@ func TestPathREMatcher(t *testing.T) { req = req.WithContext(ctx) addHTTPVarsToReplacer(repl, req, httptest.NewRecorder()) - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d [%v]: Expected %t, got %t for input '%s'", i, tc.match.Pattern, tc.expect, actual, tc.input) @@ -691,7 +703,10 @@ func TestHeaderMatcher(t *testing.T) { ctx := context.WithValue(req.Context(), caddy.ReplacerCtxKey, repl) req = req.WithContext(ctx) - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d %v: Expected %t, got %t for '%s'", i, tc.match, tc.expect, actual, tc.input) continue @@ -818,7 +833,10 @@ func TestQueryMatcher(t *testing.T) { repl.Set("http.vars.debug", "1") repl.Set("http.vars.key", "somekey") req = req.WithContext(ctx) - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d %v: Expected %t, got %t for '%s'", i, tc.match, tc.expect, actual, tc.input) continue @@ -887,7 +905,10 @@ func TestHeaderREMatcher(t *testing.T) { req = req.WithContext(ctx) addHTTPVarsToReplacer(repl, req, httptest.NewRecorder()) - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d [%v]: Expected %t, got %t for input '%s'", i, tc.match, tc.expect, actual, tc.input) @@ -927,7 +948,7 @@ func BenchmarkHeaderREMatcher(b *testing.B) { req = req.WithContext(ctx) addHTTPVarsToReplacer(repl, req, httptest.NewRecorder()) for run := 0; run < b.N; run++ { - match.Match(req) + match.MatchWithError(req) } } @@ -998,7 +1019,10 @@ func TestVarREMatcher(t *testing.T) { tc.input.ServeHTTP(httptest.NewRecorder(), req, emptyHandler) - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d [%v]: Expected %t, got %t for input '%s'", i, tc.match, tc.expect, actual, tc.input) @@ -1123,7 +1147,10 @@ func TestNotMatcher(t *testing.T) { ctx := context.WithValue(req.Context(), caddy.ReplacerCtxKey, repl) req = req.WithContext(ctx) - actual := tc.match.Match(req) + actual, err := tc.match.MatchWithError(req) + if err != nil { + t.Errorf("Test %d %v: matching failed: %v", i, tc.match, err) + } if actual != tc.expect { t.Errorf("Test %d %+v: Expected %t, got %t for: host=%s path=%s'", i, tc.match, tc.expect, actual, tc.host, tc.path) continue @@ -1155,7 +1182,7 @@ func BenchmarkLargeHostMatcher(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - matcher.Match(req) + matcher.MatchWithError(req) } } @@ -1169,7 +1196,7 @@ func BenchmarkHostMatcherWithoutPlaceholder(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - match.Match(req) + match.MatchWithError(req) } } @@ -1187,6 +1214,6 @@ func BenchmarkHostMatcherWithPlaceholder(b *testing.B) { b.ResetTimer() for i := 0; i < b.N; i++ { - match.Match(req) + match.MatchWithError(req) } } diff --git a/modules/caddyhttp/reverseproxy/healthchecks.go b/modules/caddyhttp/reverseproxy/healthchecks.go index 1735e45a..f0ffee5b 100644 --- a/modules/caddyhttp/reverseproxy/healthchecks.go +++ b/modules/caddyhttp/reverseproxy/healthchecks.go @@ -72,7 +72,7 @@ type HealthChecks struct { // health checks (that is, health checks which occur in a // background goroutine independently). type ActiveHealthChecks struct { - // DEPRECATED: Use 'uri' instead. This field will be removed. TODO: remove this field + // Deprecated: Use 'uri' instead. This field will be removed. TODO: remove this field Path string `json:"path,omitempty"` // The URI (path and query) to use for health checks diff --git a/modules/caddyhttp/reverseproxy/httptransport.go b/modules/caddyhttp/reverseproxy/httptransport.go index 2b4c3f09..910033ca 100644 --- a/modules/caddyhttp/reverseproxy/httptransport.go +++ b/modules/caddyhttp/reverseproxy/httptransport.go @@ -545,11 +545,11 @@ type TLSConfig struct { // Certificate authority module which provides the certificate pool of trusted certificates CARaw json.RawMessage `json:"ca,omitempty" caddy:"namespace=tls.ca_pool.source inline_key=provider"` - // DEPRECATED: Use the `ca` field with the `tls.ca_pool.source.inline` module instead. + // Deprecated: Use the `ca` field with the `tls.ca_pool.source.inline` module instead. // Optional list of base64-encoded DER-encoded CA certificates to trust. RootCAPool []string `json:"root_ca_pool,omitempty"` - // DEPRECATED: Use the `ca` field with the `tls.ca_pool.source.file` module instead. + // Deprecated: Use the `ca` field with the `tls.ca_pool.source.file` module instead. // List of PEM-encoded CA certificate files to add to the same trust // store as RootCAPool (or root_ca_pool in the JSON). RootCAPEMFiles []string `json:"root_ca_pem_files,omitempty"` diff --git a/modules/caddyhttp/reverseproxy/reverseproxy.go b/modules/caddyhttp/reverseproxy/reverseproxy.go index 1250eae6..5ef37a4e 100644 --- a/modules/caddyhttp/reverseproxy/reverseproxy.go +++ b/modules/caddyhttp/reverseproxy/reverseproxy.go @@ -496,7 +496,7 @@ func (h *Handler) proxyLoopIteration(r *http.Request, origReq *http.Request, w h if proxyErr == nil { proxyErr = caddyhttp.Error(http.StatusServiceUnavailable, errNoUpstream) } - if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r) { + if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r, h.logger) { return true, proxyErr } return false, proxyErr @@ -562,7 +562,7 @@ func (h *Handler) proxyLoopIteration(r *http.Request, origReq *http.Request, w h h.countFailure(upstream) // if we've tried long enough, break - if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r) { + if !h.LoadBalancing.tryAgain(h.ctx, start, retries, proxyErr, r, h.logger) { return true, proxyErr } @@ -1089,7 +1089,7 @@ func (h *Handler) finalizeResponse( // If true is returned, it has already blocked long enough before // the next retry (i.e. no more sleeping is needed). If false is // returned, the handler should stop trying to proxy the request. -func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int, proxyErr error, req *http.Request) bool { +func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int, proxyErr error, req *http.Request, logger *zap.Logger) bool { // no retries are configured if lb.TryDuration == 0 && lb.Retries == 0 { return false @@ -1124,7 +1124,12 @@ func (lb LoadBalancing) tryAgain(ctx caddy.Context, start time.Time, retries int return false } - if !lb.RetryMatch.AnyMatch(req) { + match, err := lb.RetryMatch.AnyMatchWithError(req) + if err != nil { + logger.Error("error matching request for retry", zap.Error(err)) + return false + } + if !match { return false } } diff --git a/modules/caddyhttp/routes.go b/modules/caddyhttp/routes.go index 939d01e5..ccb5f251 100644 --- a/modules/caddyhttp/routes.go +++ b/modules/caddyhttp/routes.go @@ -254,18 +254,13 @@ func wrapRoute(route Route) Middleware { nextCopy := next // route must match at least one of the matcher sets - if !route.MatcherSets.AnyMatch(req) { + matches, err := route.MatcherSets.AnyMatchWithError(req) + if err != nil { // allow matchers the opportunity to short circuit // the request and trigger the error handling chain - err, ok := GetVar(req.Context(), MatcherErrorVarKey).(error) - if ok { - // clear out the error from context, otherwise - // it will cascade to the error routes (#4916) - SetVar(req.Context(), MatcherErrorVarKey, nil) - // return the matcher's error - return err - } - + return err + } + if !matches { // call the next handler, and skip this one, // since the matcher didn't match return nextCopy.ServeHTTP(rw, req) @@ -341,19 +336,58 @@ func wrapMiddleware(ctx caddy.Context, mh MiddlewareHandler, metrics *Metrics) M // MatcherSet is a set of matchers which // must all match in order for the request // to be matched successfully. -type MatcherSet []RequestMatcher +type MatcherSet []any // Match returns true if the request matches all // matchers in mset or if there are no matchers. func (mset MatcherSet) Match(r *http.Request) bool { for _, m := range mset { - if !m.Match(r) { - return false + if me, ok := m.(RequestMatcherWithError); ok { + match, _ := me.MatchWithError(r) + if !match { + return false + } + continue } + if me, ok := m.(RequestMatcher); ok { + if !me.Match(r) { + return false + } + continue + } + return false } return true } +// MatchWithError returns true if r matches m. +func (mset MatcherSet) MatchWithError(r *http.Request) (bool, error) { + for _, m := range mset { + if me, ok := m.(RequestMatcherWithError); ok { + match, err := me.MatchWithError(r) + if err != nil || !match { + return match, err + } + continue + } + if me, ok := m.(RequestMatcher); ok { + if !me.Match(r) { + // for backwards compatibility + err, ok := GetVar(r.Context(), MatcherErrorVarKey).(error) + if ok { + // clear out the error from context since we've consumed it + SetVar(r.Context(), MatcherErrorVarKey, nil) + return false, err + } + return false, nil + } + continue + } + return false, fmt.Errorf("matcher is not a RequestMatcher or RequestMatcherWithError: %#v", m) + } + return true, nil +} + // RawMatcherSets is a group of matcher sets // in their raw, JSON form. type RawMatcherSets []caddy.ModuleMap @@ -366,25 +400,50 @@ type MatcherSets []MatcherSet // AnyMatch returns true if req matches any of the // matcher sets in ms or if there are no matchers, // in which case the request always matches. +// +// Deprecated: Use AnyMatchWithError instead. func (ms MatcherSets) AnyMatch(req *http.Request) bool { for _, m := range ms { - if m.Match(req) { - return true + match, err := m.MatchWithError(req) + if err != nil { + SetVar(req.Context(), MatcherErrorVarKey, err) + return false + } + if match { + return match } } return len(ms) == 0 } +// AnyMatchWithError returns true if req matches any of the +// matcher sets in ms or if there are no matchers, in which +// case the request always matches. If any matcher returns +// an error, we cut short and return the error. +func (ms MatcherSets) AnyMatchWithError(req *http.Request) (bool, error) { + for _, m := range ms { + match, err := m.MatchWithError(req) + if err != nil || match { + return match, err + } + } + return len(ms) == 0, nil +} + // FromInterface fills ms from an 'any' value obtained from LoadModule. func (ms *MatcherSets) FromInterface(matcherSets any) error { for _, matcherSetIfaces := range matcherSets.([]map[string]any) { var matcherSet MatcherSet for _, matcher := range matcherSetIfaces { - reqMatcher, ok := matcher.(RequestMatcher) - if !ok { - return fmt.Errorf("decoded module is not a RequestMatcher: %#v", matcher) + if m, ok := matcher.(RequestMatcherWithError); ok { + matcherSet = append(matcherSet, m) + continue } - matcherSet = append(matcherSet, reqMatcher) + if m, ok := matcher.(RequestMatcher); ok { + matcherSet = append(matcherSet, m) + continue + } + return fmt.Errorf("decoded module is not a RequestMatcher or RequestMatcherWithError: %#v", matcher) } *ms = append(*ms, matcherSet) } diff --git a/modules/caddyhttp/vars.go b/modules/caddyhttp/vars.go index 77e06e3c..7ab891fc 100644 --- a/modules/caddyhttp/vars.go +++ b/modules/caddyhttp/vars.go @@ -166,8 +166,14 @@ func (m *VarsMatcher) UnmarshalCaddyfile(d *caddyfile.Dispenser) error { // Match matches a request based on variables in the context, // or placeholders if the key is not a variable. func (m VarsMatcher) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m VarsMatcher) MatchWithError(r *http.Request) (bool, error) { if len(m) == 0 { - return true + return true, nil } vars := r.Context().Value(VarsCtxKey).(map[string]any) @@ -200,11 +206,11 @@ func (m VarsMatcher) Match(r *http.Request) bool { varStr = fmt.Sprintf("%v", vv) } if varStr == matcherValExpanded { - return true + return true, nil } } } - return false + return false, nil } // CELLibrary produces options that expose this matcher for use in CEL @@ -219,7 +225,7 @@ func (VarsMatcher) CELLibrary(_ caddy.Context) (cel.Library, error) { "vars", "vars_matcher_request_map", []*cel.Type{CELTypeJSON}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { mapStrListStr, err := CELValueToMapStrList(data) if err != nil { return nil, err @@ -294,6 +300,12 @@ func (m MatchVarsRE) Provision(ctx caddy.Context) error { // Match returns true if r matches m. func (m MatchVarsRE) Match(r *http.Request) bool { + match, _ := m.MatchWithError(r) + return match +} + +// MatchWithError returns true if r matches m. +func (m MatchVarsRE) MatchWithError(r *http.Request) (bool, error) { vars := r.Context().Value(VarsCtxKey).(map[string]any) repl := r.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer) for key, val := range m { @@ -322,10 +334,10 @@ func (m MatchVarsRE) Match(r *http.Request) bool { valExpanded := repl.ReplaceAll(varStr, "") if match := val.Match(valExpanded, repl); match { - return match + return match, nil } } - return false + return false, nil } // CELLibrary produces options that expose this matcher for use in CEL @@ -340,7 +352,7 @@ func (MatchVarsRE) CELLibrary(ctx caddy.Context) (cel.Library, error) { "vars_regexp", "vars_regexp_request_string_string", []*cel.Type{cel.StringType, cel.StringType}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) params, err := data.ConvertToNative(refStringList) if err != nil { @@ -363,7 +375,7 @@ func (MatchVarsRE) CELLibrary(ctx caddy.Context) (cel.Library, error) { "vars_regexp", "vars_regexp_request_string_string_string", []*cel.Type{cel.StringType, cel.StringType, cel.StringType}, - func(data ref.Val) (RequestMatcher, error) { + func(data ref.Val) (RequestMatcherWithError, error) { refStringList := reflect.TypeOf([]string{}) params, err := data.ConvertToNative(refStringList) if err != nil { @@ -435,8 +447,10 @@ func SetVar(ctx context.Context, key string, value any) { // Interface guards var ( - _ MiddlewareHandler = (*VarsMiddleware)(nil) - _ caddyfile.Unmarshaler = (*VarsMiddleware)(nil) - _ RequestMatcher = (*VarsMatcher)(nil) - _ caddyfile.Unmarshaler = (*VarsMatcher)(nil) + _ MiddlewareHandler = (*VarsMiddleware)(nil) + _ caddyfile.Unmarshaler = (*VarsMiddleware)(nil) + _ RequestMatcherWithError = (*VarsMatcher)(nil) + _ caddyfile.Unmarshaler = (*VarsMatcher)(nil) + _ RequestMatcherWithError = (*MatchVarsRE)(nil) + _ caddyfile.Unmarshaler = (*MatchVarsRE)(nil) ) diff --git a/modules/caddytls/connpolicy.go b/modules/caddytls/connpolicy.go index f415fffa..46afc669 100644 --- a/modules/caddytls/connpolicy.go +++ b/modules/caddytls/connpolicy.go @@ -535,21 +535,21 @@ type ClientAuthentication struct { CARaw json.RawMessage `json:"ca,omitempty" caddy:"namespace=tls.ca_pool.source inline_key=provider"` ca CA - // DEPRECATED: Use the `ca` field with the `tls.ca_pool.source.inline` module instead. + // Deprecated: Use the `ca` field with the `tls.ca_pool.source.inline` module instead. // A list of base64 DER-encoded CA certificates // against which to validate client certificates. // Client certs which are not signed by any of // these CAs will be rejected. TrustedCACerts []string `json:"trusted_ca_certs,omitempty"` - // DEPRECATED: Use the `ca` field with the `tls.ca_pool.source.file` module instead. + // Deprecated: Use the `ca` field with the `tls.ca_pool.source.file` module instead. // TrustedCACertPEMFiles is a list of PEM file names // from which to load certificates of trusted CAs. // Client certificates which are not signed by any of // these CA certificates will be rejected. TrustedCACertPEMFiles []string `json:"trusted_ca_certs_pem_files,omitempty"` - // DEPRECATED: This field is deprecated and will be removed in + // Deprecated: This field is deprecated and will be removed in // a future version. Please use the `validators` field instead // with the tls.client_auth.verifier.leaf module instead. // diff --git a/modules/caddytls/ondemand.go b/modules/caddytls/ondemand.go index 066473cd..0970234c 100644 --- a/modules/caddytls/ondemand.go +++ b/modules/caddytls/ondemand.go @@ -42,7 +42,7 @@ func init() { // to your application whether a particular domain is allowed // to have a certificate issued for it. type OnDemandConfig struct { - // DEPRECATED. WILL BE REMOVED SOON. Use 'permission' instead with the `http` module. + // Deprecated. WILL BE REMOVED SOON. Use 'permission' instead with the `http` module. Ask string `json:"ask,omitempty"` // REQUIRED. A module that will determine whether a