rbac: add matching range of destination ports by zhxie · Pull Request #17356 · envoyproxy/envoy

zhxie · 2021-07-15T03:29:03Z

Signed-off-by: Xie Zhihao zhihao.xie@intel.com

Commit Message: rbac: add matching range of destination ports
Additional Description: use one destination port range rule to cover thousands of port rules in RBAC permissions, adding convenience for writing configuration and making matching quicker
Risk Level: Low
Testing: Unit
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A
Fixes #16039

repokitteh-read-only · 2021-07-15T03:29:06Z

Hi @zhxie, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17356 was opened by zhxie.

see: more, trace.

repokitteh-read-only · 2021-07-15T03:29:10Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #17356 was opened by zhxie.

see: more, trace.

leyao-daily · 2021-07-15T05:02:43Z

please rebase and sign all your commits with git commit -s to pass DCO check.

adisuissa

Thanks for working on this!
Left a few API questions.

api/envoy/config/rbac/v3/rbac.proto

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

adisuissa

/lgtm api

api/envoy/config/rbac/v3/rbac.proto

zhxie · 2021-07-20T05:35:49Z

/retest

repokitteh-read-only · 2021-07-20T05:35:52Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17356 (comment) was created by @zhxie.

see: more, trace.

htuch · 2021-07-23T17:50:24Z

@adisuissa could you also take a look at the first pass for implementation review when you're back? Thanks!

adisuissa

Implementation and tests look good overall, thanks!
Left a couple of nits.

adisuissa · 2021-07-24T20:05:34Z

test/extensions/filters/common/rbac/matchers_test.cc

+  perm->mutable_destination_port_range()->set_start(123);
+  perm->mutable_destination_port_range()->set_end(456);


nit: why not copy range into destination_port_range?

That's right. Unused variable has been removed.

adisuissa · 2021-07-24T20:06:25Z

test/extensions/filters/common/rbac/matchers_test.cc

  checkMatcher(PortMatcher(456), false, conn, headers, info);
 }

+TEST(PortRangeMatcher, PortRangeMatcher) {


Please add a short comment stating what the test validates.

Ok, comment added.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

adisuissa

Thanks, LGTM.
@htuch for the next pass

htuch · 2021-07-29T04:02:06Z

source/extensions/filters/common/rbac/matchers.h

+class PortRangeMatcher : public Matcher {
+public:
+  PortRangeMatcher(const ::envoy::type::v3::Int32Range& range)
+      : start_(range.start()), end_(range.end()) {}


Arguably we should reject negative and OOB values here for ports.

To validate the range of ports, we are better to add a validation rule in RBAC proto.

envoy/api/envoy/config/rbac/v3/rbac.proto

Lines 189 to 190 in f97bdf1

// A port number range that describes a range of destination ports connecting to.

type.v3.Int32Range destination_port_range = 11;

But the type type.v3.Int32Range does not have any validation rules, and only certain types have validation rules that is located in validate.proto, which belongs to another repo envoyproxy/protoc-gen-validate. As a result, we are not easy to reject invalid or illegal values.

In addition, type.v3.Int32Range is also used in destination port matching in listener filter, but the ListenerFilterDstPortMatcher does not check its input.

envoy/api/envoy/config/listener/v3/listener_components.proto

Lines 330 to 332 in f97bdf1

// Match destination port. Particularly, the match evaluation must use the recovered local port if

// the owning listener filter is after :ref:`an original_dst listener filter <config_listener_filters_original_dst>`.

type.v3.Int32Range destination_port_range = 5;

envoy/source/common/network/filter_matcher.h

Lines 40 to 46 in f97bdf1

/**

* Destination port matcher.

*/

class ListenerFilterDstPortMatcher final : public ListenerFilterMatcher {

public:

explicit ListenerFilterDstPortMatcher(const ::envoy::type::v3::Int32Range& range)

: start_(range.start()), end_(range.end()) {}

So I think maybe we do not have to add a check?

Yes, this is a limit of PGV annotations and proto typing, I don't think it can be done cleanly at the proto annotation level. I think both ListenerFilterDstPortMatcher and your code could share a common utility that at config ingestion time, checks the port range and throws if invalid; this is the next best thing.

htuch

LGTM modulo small ask.
/wait

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie · 2021-07-30T08:36:41Z

/retest

repokitteh-read-only · 2021-07-30T08:36:44Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17356 (comment) was created by @zhxie.

see: more, trace.

zhxie · 2021-07-30T11:36:42Z

/retest

repokitteh-read-only · 2021-07-30T11:36:45Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17356 (comment) was created by @zhxie.

see: more, trace.

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

htuch

LGTM, thanks!

use one destination port range rule to cover thousands of port rules in RBAC permissions, adding convenience for writing configuration and making matching quicker Risk Level: Low Testing: Unit Docs Changes: N/A Release Notes: Added Platform Specific Features: N/A Fixes envoyproxy#16039 Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

repokitteh-read-only bot added the api label Jul 15, 2021

repokitteh-read-only bot assigned adisuissa Jul 15, 2021

zhxie force-pushed the rbac-dport-range branch 2 times, most recently from 9b62ded to f4a7842 Compare July 15, 2021 12:45

adisuissa reviewed Jul 15, 2021

View reviewed changes

api/envoy/config/rbac/v3/rbac.proto Show resolved Hide resolved

zhxie added 5 commits July 19, 2021 09:10

rbac: add matcher for matching range of destination ports

c7ad00d

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add test for RBAC port range matching

4a72a94

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

rbac: fix format in destination port range matcher

f571b4b

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add test coverage for RBAC port range matching

4d615da

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

docs: add RBAC destination_port_range release note

33c9d44

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie force-pushed the rbac-dport-range branch from 9b0b76b to 33c9d44 Compare July 19, 2021 05:44

zhxie changed the title ~~[WIP] rbac: add matching range of destination ports~~ rbac: add matching range of destination ports Jul 19, 2021

zhxie marked this pull request as ready for review July 19, 2021 07:19

adisuissa reviewed Jul 19, 2021

View reviewed changes

api/envoy/config/rbac/v3/rbac.proto Show resolved Hide resolved

repokitteh-read-only bot removed the api label Jul 19, 2021

Merge branch 'main' into rbac-dport-range

90cacf3

zhxie requested review from adisuissa and htuch July 21, 2021 01:37

adisuissa reviewed Jul 24, 2021

View reviewed changes

htuch self-assigned this Jul 25, 2021

zhxie added 2 commits July 26, 2021 10:23

test: remove unused range in RBAC test

af9846e

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add comment for destination_port_range test in RBAC

1e9c882

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie force-pushed the rbac-dport-range branch from 41b6e39 to 1e9c882 Compare July 26, 2021 02:24

Merge branch 'envoyproxy:main' into rbac-dport-range

e545ebe

docs: rearrange order

f97bdf1

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie requested a review from adisuissa July 27, 2021 02:11

adisuissa previously approved these changes Jul 27, 2021

View reviewed changes

htuch reviewed Jul 29, 2021

View reviewed changes

htuch suggested changes Jul 29, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Jul 29, 2021

zhxie added 3 commits July 30, 2021 10:04

rbac: check destination port range validation

a4740b4

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

test: add test for validating RBAC rules

4b99701

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

rbac: fix grammar

d51155f

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

zhxie dismissed adisuissa’s stale review via d51155f July 30, 2021 04:12

repokitteh-read-only bot removed the waiting label Jul 30, 2021

zhxie requested a review from htuch July 30, 2021 11:37

ci: kick ci

bbcf5b6

Signed-off-by: Xie Zhihao <zhihao.xie@intel.com>

htuch approved these changes Aug 1, 2021

View reviewed changes

htuch merged commit 0d2418e into envoyproxy:main Aug 1, 2021

zhxie deleted the rbac-dport-range branch September 7, 2022 08:27

		perm->mutable_destination_port_range()->set_start(123);
		perm->mutable_destination_port_range()->set_end(456);

	// A port number range that describes a range of destination ports connecting to.
	type.v3.Int32Range destination_port_range = 11;

	// Match destination port. Particularly, the match evaluation must use the recovered local port if
	// the owning listener filter is after :ref:`an original_dst listener filter <config_listener_filters_original_dst>`.
	type.v3.Int32Range destination_port_range = 5;

	/**
	* Destination port matcher.
	*/
	class ListenerFilterDstPortMatcher final : public ListenerFilterMatcher {
	public:
	explicit ListenerFilterDstPortMatcher(const ::envoy::type::v3::Int32Range& range)
	: start_(range.start()), end_(range.end()) {}

Conversation

zhxie commented Jul 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Jul 15, 2021

Uh oh!

repokitteh-read-only bot commented Jul 15, 2021

Uh oh!

leyao-daily commented Jul 15, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zhxie commented Jul 20, 2021

Uh oh!

repokitteh-read-only bot commented Jul 20, 2021

Uh oh!

htuch commented Jul 23, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

adisuissa Jul 24, 2021

Choose a reason for hiding this comment

Uh oh!

zhxie Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

adisuissa Jul 24, 2021

Choose a reason for hiding this comment

Uh oh!

zhxie Jul 26, 2021

Choose a reason for hiding this comment

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

htuch Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

zhxie Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

htuch Jul 29, 2021

Choose a reason for hiding this comment

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

zhxie commented Jul 30, 2021

Uh oh!

repokitteh-read-only bot commented Jul 30, 2021

Uh oh!

zhxie commented Jul 30, 2021

Uh oh!

repokitteh-read-only bot commented Jul 30, 2021

Uh oh!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

zhxie commented Jul 15, 2021 •

edited

Loading