Refactor JWT matching logic

[htuch]

As per #20145, we have duplicative matching logic in core router and JWT filter. This seems a maintenance, testing and security concern. I think with some modest refactoring, we could move this match logic to a common library. This is a fairly straightforward refactoring I think (famous last words).

[htuch]

CC @qiwzhang @envoyproxy/api-shepherds

[tpetkov-VMW]

I can try to do it

[qiwzhang]

Sure, please go ahead. I can help with the review. Thanks

[htuch]

@tpetkov-VMW really appreciated, I've assigned the issue to you.

[fredrikgh]

@tpetkov-VMW would this also consolidate the matching types available so that jwt_authn supports all listed in RouteMatch? I was about to create an issue regarding metadata matching having no effect in jwt_authn rules, hence me asking.

[tpetkov-VMW]

Looks like the best solution will be to extend
api/envoy/type/matcher/v3/string.proto and put the relevant code in
source/common/common/matchers.h (cc)
However, with a simple patch to frame it, I'm getting a compile error, because api/envoy/type/matcher/v3/string.proto and
https://github.com/cncf/xds/blob/main/xds/type/matcher/v3/string.proto start to differ:

/source/common/common/matchers.h:123:47: error: no member named 'kPathSeparatedPrefix' in 'xds::type::matcher::v3::StringMatcher::MatchPatternCase'
    case StringMatcherType::MatchPatternCase::kPathSeparatedPrefix:
...
source/common/http/match_delegate/config.cc:267:34: note: in instantiation of function template specialization 'Envoy::Matcher::MatchTreeFactory<Envoy::Http::HttpMatchingData, Envoy::Http::Matching::HttpFilterActionContext>::create<xds::ty
pe::matcher::v3::Matcher>' requested here
    factory_cb = matcher_factory.create(proto_config.xds_matcher());

@htuch - Can you help out? The question is twofold.

• Is this the correct approach?
• If so, how do I go about patching the xds version?

Here's a patch below, that would trigger the error

diff --git a/api/envoy/type/matcher/v3/string.proto b/api/envoy/type/matcher/v3/string.proto
index efea6c0ab4..1ccdd5f1c7 100644
--- a/api/envoy/type/matcher/v3/string.proto
+++ b/api/envoy/type/matcher/v3/string.proto
@@ -61,6 +61,17 @@ message StringMatcher {
     //
     // * *abc* matches the value *xyz.abc.def*
     string contains = 7 [(validate.rules).string = {min_len: 1}];
+
+    // If specified, the route is a path-separated prefix rule meaning that the
+    // ``:path`` header (without the query string) must either exactly match the
+    // ``path_separated_prefix`` or have it as a prefix, followed by ``/``
+    //
+    // For example, ``/api/dev`` would match
+    // ``/api/dev``, ``/api/dev/``, ``/api/dev/v1``, and ``/api/dev?param=true``
+    // but would not match ``/api/developer``
+    //
+    // Expect the value to not contain ``?`` or ``#`` and not to end in ``/``
+    string path_separated_prefix = 8 [(validate.rules).string = {pattern: "^[^?#]+[^?#/]$"}];
   }

   // If true, indicates the exact/prefix/suffix/contains matching should be case insensitive. This
diff --git a/source/common/common/matchers.h b/source/common/common/matchers.h
index fb89efdc02..a30e2e68c4 100644
--- a/source/common/common/matchers.h
+++ b/source/common/common/matchers.h
@@ -120,6 +120,7 @@ public:
                  : absl::StrContains(value, matcher_.contains());
     case StringMatcherType::MatchPatternCase::kSafeRegex:
       return regex_->match(value);
+    case StringMatcherType::MatchPatternCase::kPathSeparatedPrefix:
     case StringMatcherType::MatchPatternCase::MATCH_PATTERN_NOT_SET:
       break;
     }

[htuch]

I'm not sure if the addition of just path_separated_prefix speaks fully to this issue. It might also be a bit too specific to add to StringMatcher.

Taking a step back, I think the way to wrap this in a sensible way is to have both RDS and JWT use generic matchers, where you have inherent sharing of code @snowp @kyessenov @qiwzhang