Skip to content

rbac: add matching range of destination ports #17356

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.

Sign up for GitHub

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

Merged
htuch merged 14 commits into from
Aug 1, 2021
Merged

rbac: add matching range of destination ports #17356

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c7ad00d
rbac: add matcher for matching range of destination ports
zhxie Jul 14, 2021
4a72a94
test: add test for RBAC port range matching
zhxie Jul 14, 2021
f571b4b
rbac: fix format in destination port range matcher
zhxie Jul 15, 2021
4d615da
test: add test coverage for RBAC port range matching
zhxie Jul 16, 2021
33c9d44
docs: add RBAC destination_port_range release note
zhxie Jul 19, 2021
90cacf3
Merge branch 'main' into rbac-dport-range
zhxie Jul 20, 2021
af9846e
test: remove unused range in RBAC test
zhxie Jul 26, 2021
1e9c882
test: add comment for destination_port_range test in RBAC
zhxie Jul 26, 2021
e545ebe
Merge branch 'envoyproxy:main' into rbac-dport-range
zhxie Jul 26, 2021
f97bdf1
docs: rearrange order
zhxie Jul 26, 2021
a4740b4
rbac: check destination port range validation
zhxie Jul 30, 2021
4b99701
test: add test for validating RBAC rules
zhxie Jul 30, 2021
d51155f
rbac: fix grammar
zhxie Jul 30, 2021
bbcf5b6
ci: kick ci
zhxie Jul 31, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions api/envoy/config/rbac/v3/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ api_proto_package(
"//envoy/config/core/v3:pkg",
"//envoy/config/route/v3:pkg",
"//envoy/type/matcher/v3:pkg",
"//envoy/type/v3:pkg",
"@com_github_cncf_udpa//udpa/annotations:pkg",
"@com_google_googleapis//google/api/expr/v1alpha1:checked_proto",
"@com_google_googleapis//google/api/expr/v1alpha1:syntax_proto",
Expand Down
6 changes: 5 additions & 1 deletion api/envoy/config/rbac/v3/rbac.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import "envoy/config/route/v3/route_components.proto";
import "envoy/type/matcher/v3/metadata.proto";
import "envoy/type/matcher/v3/path.proto";
import "envoy/type/matcher/v3/string.proto";
import "envoy/type/v3/range.proto";

import "google/api/expr/v1alpha1/checked.proto";
import "google/api/expr/v1alpha1/syntax.proto";
Expand Down Expand Up @@ -145,7 +146,7 @@ message Policy {
}

// Permission defines an action (or actions) that a principal can take.
// [#next-free-field: 11]
// [#next-free-field: 12]
message Permission {
option (udpa.annotations.versioning).previous_message_type = "envoy.config.rbac.v2.Permission";

Expand Down Expand Up @@ -185,6 +186,9 @@ message Permission {
// A port number that describes the destination port connecting to.
uint32 destination_port = 6 [(validate.rules).uint32 = {lte: 65535}];

// A port number range that describes a range of destination ports connecting to.
type.v3.Int32Range destination_port_range = 11;
Comment thread
htuch marked this conversation as resolved.

// Metadata that describes additional information about the action.
type.matcher.v3.MetadataMatcher metadata = 7;

Expand Down
1 change: 1 addition & 0 deletions api/envoy/config/rbac/v4alpha/BUILD
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion api/envoy/config/rbac/v4alpha/rbac.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Removed Config or Runtime
New Features
------------
* http: added :ref:`string_match <envoy_v3_api_field_config.route.v3.HeaderMatcher.string_match>` in the header matcher.
* rbac: added :ref:`destination_port_range <envoy_v3_api_field_config.rbac.v3.Permission.destination_port_range>` for matching range of destination ports.

Deprecated
----------
Expand Down
1 change: 1 addition & 0 deletions generated_api_shadow/envoy/config/rbac/v3/BUILD
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion generated_api_shadow/envoy/config/rbac/v3/rbac.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions generated_api_shadow/envoy/config/rbac/v4alpha/BUILD
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 5 additions & 1 deletion generated_api_shadow/envoy/config/rbac/v4alpha/rbac.proto
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 14 additions & 0 deletions source/extensions/filters/common/rbac/matchers.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@ MatcherConstSharedPtr Matcher::create(const envoy::config::rbac::v3::Permission&
IPMatcher::Type::DownstreamLocal);
case envoy::config::rbac::v3::Permission::RuleCase::kDestinationPort:
return std::make_shared<const PortMatcher>(permission.destination_port());
case envoy::config::rbac::v3::Permission::RuleCase::kDestinationPortRange:
return std::make_shared<const PortRangeMatcher>(permission.destination_port_range());
case envoy::config::rbac::v3::Permission::RuleCase::kAny:
return std::make_shared<const AlwaysMatcher>();
case envoy::config::rbac::v3::Permission::RuleCase::kMetadata:
Expand Down Expand Up @@ -159,6 +161,18 @@ bool PortMatcher::matches(const Network::Connection&, const Envoy::Http::Request
return ip && ip->port() == port_;
}

bool PortRangeMatcher::matches(const Network::Connection&, const Envoy::Http::RequestHeaderMap&,
const StreamInfo::StreamInfo& info) const {
const Envoy::Network::Address::Ip* ip =
info.downstreamAddressProvider().localAddress().get()->ip();
if (ip) {
const auto port = ip->port();
return start_ <= port && port < end_;
} else {
return false;
}
}

bool AuthenticatedMatcher::matches(const Network::Connection& connection,
const Envoy::Http::RequestHeaderMap&,
const StreamInfo::StreamInfo&) const {
Expand Down
13 changes: 13 additions & 0 deletions source/extensions/filters/common/rbac/matchers.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,19 @@ class PortMatcher : public Matcher {
const uint32_t port_;
};

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

@htuch htuch Jul 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zhxie zhxie Jul 29, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@htuch htuch Jul 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


bool matches(const Network::Connection&, const Envoy::Http::RequestHeaderMap&,
const StreamInfo::StreamInfo& info) const override;

private:
const uint32_t start_;
const uint32_t end_;
};

/**
* Matches the principal name as described in the peer certificate. Uses the URI SAN first. If that
* field is not present, uses the subject instead.
Expand Down
42 changes: 42 additions & 0 deletions test/extensions/filters/common/rbac/matchers_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
#include "envoy/config/route/v3/route_components.pb.h"
#include "envoy/type/matcher/v3/metadata.pb.h"

#include "source/common/network/address_impl.h"
#include "source/common/network/utility.h"
#include "source/extensions/filters/common/expr/evaluator.h"
#include "source/extensions/filters/common/rbac/matchers.h"
Expand Down Expand Up @@ -101,6 +102,15 @@ TEST(OrMatcher, Permission_Set) {

checkMatcher(RBAC::OrMatcher(set), false, conn, headers, info);

perm = set.add_rules();
envoy::type::v3::Int32Range range;
range.set_start(123);
range.set_end(456);
perm->mutable_destination_port_range()->set_start(123);
perm->mutable_destination_port_range()->set_end(456);
Comment on lines +110 to +111

@adisuissa adisuissa Jul 24, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why not copy range into destination_port_range?

@zhxie zhxie Jul 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's right. Unused variable has been removed.


checkMatcher(RBAC::OrMatcher(set), false, conn, headers, info);

perm = set.add_rules();
perm->set_any(true);

Expand Down Expand Up @@ -233,6 +243,38 @@ TEST(PortMatcher, PortMatcher) {
checkMatcher(PortMatcher(456), false, conn, headers, info);
}

TEST(PortRangeMatcher, PortRangeMatcher) {

@adisuissa adisuissa Jul 24, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add a short comment stating what the test validates.

@zhxie zhxie Jul 26, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, comment added.

Envoy::Network::MockConnection conn;
Envoy::Http::TestRequestHeaderMapImpl headers;
NiceMock<StreamInfo::MockStreamInfo> info;
Envoy::Network::Address::InstanceConstSharedPtr addr =
Envoy::Network::Utility::parseInternetAddress("1.2.3.4", 456, false);
info.downstream_address_provider_->setLocalAddress(addr);

envoy::type::v3::Int32Range range;
range.set_start(123);
range.set_end(789);
checkMatcher(PortRangeMatcher(range), true, conn, headers, info);

range.set_start(456);
range.set_end(789);
checkMatcher(PortRangeMatcher(range), true, conn, headers, info);

range.set_start(123);
range.set_end(456);
checkMatcher(PortRangeMatcher(range), false, conn, headers, info);

range.set_start(12);
range.set_end(34);
checkMatcher(PortRangeMatcher(range), false, conn, headers, info);

NiceMock<StreamInfo::MockStreamInfo> info2;
Envoy::Network::Address::InstanceConstSharedPtr addr2 =
std::make_shared<const Envoy::Network::Address::PipeInstance>("test");
info2.downstream_address_provider_->setLocalAddress(addr2);
checkMatcher(PortRangeMatcher(range), false, conn, headers, info2);
}

TEST(AuthenticatedMatcher, uriSanPeerCertificate) {
Envoy::Network::MockConnection conn;
auto ssl = std::make_shared<Ssl::MockConnectionInfo>();
Expand Down