Chiebot-Mirror
/
grpc
mirror of https://github.com/grpc/grpc.git
8
0
Fork 0
Code Issues Projects Releases Wiki Activity

xDS: Rbac filter updates (#28568)

Browse Source
pull/27457/head
Yash Tibrewal 3 years ago committed by GitHub
parent 93733de253
commit 1caa3e8cfd
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 134 additions and 11 deletions
Whitespace
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Split View
Diff Options
Show Stats Download Patch File Download Diff File
  1. 24
      src/core/ext/filters/rbac/rbac_service_config_parser.cc
  2. 24
      src/core/ext/xds/xds_http_rbac_filter.cc
  3. 2
      src/proto/grpc/testing/xds/v3/metadata.proto
  4. 10
      test/core/ext/filters/rbac/rbac_service_config_parser_test.cc
  5. 85
      test/cpp/end2end/xds/xds_end2end_test.cc

24
src/core/ext/filters/rbac/rbac_service_config_parser.cc
Unescape View File

@ -262,8 +262,16 @@ Rbac::Permission ParsePermission(const Json::Object& permission_json,
permission = Rbac::Permission::MakeDestPortPermission(port);
} else if (ParseJsonObjectField(permission_json, "metadata", &inner_json,
error_list, /*required=*/false)) {
error_list->push_back(
GRPC_ERROR_CREATE_FROM_STATIC_STRING("Cannot handle metadata"));
std::vector<grpc_error_handle> metadata_error_list;
bool invert = false;
ParseJsonObjectField(*inner_json, "invert", &invert, &metadata_error_list,
/*required=*/false);
if (metadata_error_list.empty()) {
permission = Rbac::Permission::MakeMetadataPermission(invert);
} else {
error_list->push_back(
GRPC_ERROR_CREATE_FROM_VECTOR("metadata", &metadata_error_list));
}
} else if (ParseJsonObjectField(permission_json, "notRule", &inner_json,
error_list, /*required=*/false)) {
std::vector<grpc_error_handle> not_rule_error_list;
@ -430,8 +438,16 @@ Rbac::Principal ParsePrincipal(const Json::Object& principal_json,
}
} else if (ParseJsonObjectField(principal_json, "metadata", &inner_json,
error_list, /*required=*/false)) {
error_list->push_back(
GRPC_ERROR_CREATE_FROM_STATIC_STRING("Cannot handle metadata"));
std::vector<grpc_error_handle> metadata_error_list;
bool invert = false;
ParseJsonObjectField(*inner_json, "invert", &invert, &metadata_error_list,
/*required=*/false);
if (metadata_error_list.empty()) {
principal = Rbac::Principal::MakeMetadataPrincipal(invert);
} else {
error_list->push_back(
GRPC_ERROR_CREATE_FROM_VECTOR("metadata", &metadata_error_list));
}
} else if (ParseJsonObjectField(principal_json, "notId", &inner_json,
error_list, /*required=*/false)) {
std::vector<grpc_error_handle> not_rule_error_list;

24
src/core/ext/xds/xds_http_rbac_filter.cc
Unescape View File

@ -24,6 +24,7 @@
#include "envoy/config/route/v3/route_components.upb.h"
#include "envoy/extensions/filters/http/rbac/v3/rbac.upb.h"
#include "envoy/extensions/filters/http/rbac/v3/rbac.upbdefs.h"
#include "envoy/type/matcher/v3/metadata.upb.h"
#include "envoy/type/matcher/v3/path.upb.h"
#include "envoy/type/matcher/v3/regex.upb.h"
#include "envoy/type/matcher/v3/string.upb.h"
@ -181,6 +182,17 @@ Json ParseCidrRangeToJson(const envoy_config_core_v3_CidrRange* range) {
return json;
}
Json ParseMetadataMatcherToJson(
const envoy_type_matcher_v3_MetadataMatcher* metadata_matcher) {
Json::Object json;
// The fields "filter", "path" and "value" are irrelevant to gRPC as per
// https://github.com/grpc/proposal/blob/master/A41-xds-rbac.md and are not
// being parsed.
json.emplace("invert",
envoy_type_matcher_v3_MetadataMatcher_invert(metadata_matcher));
return json;
}
absl::StatusOr<Json> ParsePermissionToJson(
const envoy_config_rbac_v3_Permission* permission) {
Json::Object permission_json;
@ -251,9 +263,9 @@ absl::StatusOr<Json> ParsePermissionToJson(
"destinationPort",
envoy_config_rbac_v3_Permission_destination_port(permission));
} else if (envoy_config_rbac_v3_Permission_has_metadata(permission)) {
// Not parsing metadata even if its present since it is not relevant to
// gRPC.
permission_json.emplace("metadata", Json::Object());
permission_json.emplace(
"metadata", ParseMetadataMatcherToJson(
envoy_config_rbac_v3_Permission_metadata(permission)));
} else if (envoy_config_rbac_v3_Permission_has_not_rule(permission)) {
auto not_rule_json = ParsePermissionToJson(
envoy_config_rbac_v3_Permission_not_rule(permission));
@ -365,9 +377,9 @@ absl::StatusOr<Json> ParsePrincipalToJson(
}
principal_json.emplace("urlPath", std::move(*url_path_json));
} else if (envoy_config_rbac_v3_Principal_has_metadata(principal)) {
// Not parsing metadata even if its present since it is not relevant to
// gRPC.
principal_json.emplace("metadata", Json::Object());
principal_json.emplace(
"metadata", ParseMetadataMatcherToJson(
envoy_config_rbac_v3_Principal_metadata(principal)));
} else if (envoy_config_rbac_v3_Principal_has_not_id(principal)) {
auto not_id_json =
ParsePrincipalToJson(envoy_config_rbac_v3_Principal_not_id(principal));

2
src/proto/grpc/testing/xds/v3/metadata.proto
Unescape View File

@ -81,4 +81,6 @@ package envoy.type.matcher.v3;
// [#next-major-version: MetadataMatcher should use StructMatcher]
message MetadataMatcher {
// If true, the match result will be inverted.
bool invert = 4;
}

10
test/core/ext/filters/rbac/rbac_service_config_parser_test.cc
Unescape View File

@ -304,6 +304,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) {
" {\"urlPath\":{\"path\":{\"exact\":\"\"}}},\n"
" {\"destinationIp\":{\"addressPrefix\":\"::1\"}},\n"
" {\"destinationPort\":1234},\n"
" {\"metadata\":{\"invert\":true}},\n"
" {\"notRule\":{\"any\":true}},\n"
" {\"requestedServerName\":{\"exact\":\"\"}}\n"
" ],\n"
@ -318,6 +319,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsTypes) {
" {\"remoteIp\":{\"addressPrefix\":\"::1\"}},\n"
" {\"header\":{\"name\":\"name\", \"exactMatch\":\"\"}},\n"
" {\"urlPath\":{\"path\":{\"exact\":\"\"}}},\n"
" {\"metadata\":{\"invert\":true}},\n"
" {\"notId\":{\"any\":true}}\n"
" ]\n"
" }\n"
@ -362,6 +364,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) {
" {\"urlPath\":1234},\n"
" {\"destinationIp\":1234},\n"
" {\"destinationPort\":\"port\"},\n"
" {\"metadata\":1234},\n"
" {\"notRule\":1234},\n"
" {\"requestedServerName\":1234}\n"
" ],\n"
@ -375,6 +378,7 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) {
" {\"remoteIp\":1234},\n"
" {\"header\":1234},\n"
" {\"urlPath\":1234},\n"
" {\"metadata\":1234},\n"
" {\"notId\":1234}\n"
" ]\n"
" }\n"
@ -408,8 +412,10 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) {
"permissions\\[6\\]" CHILD_ERROR_TAG
"field:destinationPort error:type should be NUMBER.*"
"permissions\\[7\\]" CHILD_ERROR_TAG
"field:notRule error:type should be OBJECT.*"
"field:metadata error:type should be OBJECT.*"
"permissions\\[8\\]" CHILD_ERROR_TAG
"field:notRule error:type should be OBJECT.*"
"permissions\\[9\\]" CHILD_ERROR_TAG
"field:requestedServerName error:type should be OBJECT.*"
"principals\\[0\\]" CHILD_ERROR_TAG
"field:andIds error:type should be OBJECT.*"
@ -430,6 +436,8 @@ TEST(RbacServiceConfigParsingTest, VariousPermissionsAndPrincipalsBadTypes) {
"principals\\[8\\]" CHILD_ERROR_TAG
"field:urlPath error:type should be OBJECT.*"
"principals\\[9\\]" CHILD_ERROR_TAG
"field:metadata error:type should be OBJECT.*"
"principals\\[10\\]" CHILD_ERROR_TAG
"field:notId error:type should be OBJECT.*"));
GRPC_ERROR_UNREF(error);
}

85
test/cpp/end2end/xds/xds_end2end_test.cc
Unescape View File

@ -10465,6 +10465,33 @@ TEST_P(XdsRbacTestWithActionPermutations,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, MetadataPermissionAnyPrincipal) {
RBAC rbac;
auto* rules = rbac.mutable_rules();
rules->set_action(GetParam().rbac_action());
Policy policy;
policy.add_permissions()->mutable_metadata();
policy.add_principals()->set_any(true);
(*rules->mutable_policies())["policy"] = policy;
SetServerRbacPolicy(rbac);
backends_[0]->Start();
backends_[0]->notifier()->WaitOnServingStatusChange(
absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
grpc::StatusCode::OK);
SendRpc(
[this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW,
grpc::StatusCode::PERMISSION_DENIED);
// Test metadata with inverted match
policy.clear_permissions();
policy.add_permissions()->mutable_metadata()->set_invert(true);
(*rules->mutable_policies())["policy"] = policy;
SetServerRbacPolicy(rbac);
SendRpc([this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_DENY,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, ReqServerNamePermissionAnyPrincipal) {
RBAC rbac;
auto* rules = rbac.mutable_rules();
@ -10752,6 +10779,37 @@ TEST_P(XdsRbacTestWithActionPermutations,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionRemoteIpPrincipal) {
RBAC rbac;
auto* rules = rbac.mutable_rules();
rules->set_action(GetParam().rbac_action());
Policy policy;
auto* range = policy.add_principals()->mutable_remote_ip();
range->set_address_prefix(ipv6_only_ ? "::1" : "127.0.0.1");
range->mutable_prefix_len()->set_value(ipv6_only_ ? 128 : 32);
policy.add_permissions()->set_any(true);
(*rules->mutable_policies())["policy"] = policy;
SetServerRbacPolicy(rbac);
backends_[0]->Start();
backends_[0]->notifier()->WaitOnServingStatusChange(
absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
grpc::StatusCode::OK);
SendRpc([this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_DENY,
grpc::StatusCode::PERMISSION_DENIED);
// Change the policy itself for a negative test where there is no match.
policy.clear_principals();
range = policy.add_principals()->mutable_remote_ip();
range->set_address_prefix(ipv6_only_ ? "::2" : "127.0.0.2");
range->mutable_prefix_len()->set_value(ipv6_only_ ? 128 : 32);
(*rules->mutable_policies())["policy"] = policy;
SetServerRbacPolicy(rbac);
SendRpc(
[this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionAuthenticatedPrincipal) {
FakeCertificateProvider::CertDataMap fake1_cert_map = {
{"", {root_cert_, identity_pair_}}};
@ -10791,6 +10849,33 @@ TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionAuthenticatedPrincipal) {
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionMetadataPrincipal) {
RBAC rbac;
auto* rules = rbac.mutable_rules();
rules->set_action(GetParam().rbac_action());
Policy policy;
policy.add_principals()->mutable_metadata();
policy.add_permissions()->set_any(true);
(*rules->mutable_policies())["policy"] = policy;
SetServerRbacPolicy(rbac);
backends_[0]->Start();
backends_[0]->notifier()->WaitOnServingStatusChange(
absl::StrCat(ipv6_only_ ? "[::1]:" : "127.0.0.1:", backends_[0]->port()),
grpc::StatusCode::OK);
SendRpc(
[this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_ALLOW,
grpc::StatusCode::PERMISSION_DENIED);
// Test metadata with inverted match
policy.clear_principals();
policy.add_principals()->mutable_metadata()->set_invert(true);
(*rules->mutable_policies())["policy"] = policy;
SetServerRbacPolicy(rbac);
SendRpc([this]() { return CreateInsecureChannel(); }, {}, {},
/*test_expects_failure=*/GetParam().rbac_action() == RBAC_Action_DENY,
grpc::StatusCode::PERMISSION_DENIED);
}
TEST_P(XdsRbacTestWithActionPermutations, AnyPermissionNotIdPrincipal) {
RBAC rbac;
auto* rules = rbac.mutable_rules();

Write Preview
Loading…
Cancel
Save