Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor JWT matching logic #20578

Open
htuch opened this issue Mar 30, 2022 · 7 comments
Open

Refactor JWT matching logic #20578

htuch opened this issue Mar 30, 2022 · 7 comments
Assignees
Labels
area/jwt_authn area/matching enhancement Feature requests. Not bugs or questions. help wanted Needs help!

Comments

@htuch
Copy link
Member

htuch commented Mar 30, 2022

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 htuch added enhancement Feature requests. Not bugs or questions. triage Issue requires triage labels Mar 30, 2022
@htuch
Copy link
Member Author

htuch commented Mar 30, 2022

CC @qiwzhang @envoyproxy/api-shepherds

@tpetkov-VMW
Copy link
Contributor

I can try to do it

@wbpcode wbpcode added area/matching and removed triage Issue requires triage labels Mar 30, 2022
@qiwzhang
Copy link
Contributor

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

@htuch
Copy link
Member Author

htuch commented Mar 30, 2022

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

@fredrikgh
Copy link

@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
Copy link
Contributor

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
Copy link
Member Author

htuch commented Jul 1, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/jwt_authn area/matching enhancement Feature requests. Not bugs or questions. help wanted Needs help!
Projects
None yet
Development

No branches or pull requests

5 participants