diff --git a/src/core/lib/security/authorization/matchers.cc b/src/core/lib/security/authorization/matchers.cc index e4262d99588..3798caa5b07 100644 --- a/src/core/lib/security/authorization/matchers.cc +++ b/src/core/lib/security/authorization/matchers.cc @@ -105,7 +105,7 @@ std::unique_ptr AuthorizationMatcher::Create( std::move(principal.header_matcher)); case Rbac::Principal::RuleType::kPath: return absl::make_unique( - std::move(principal.string_matcher.value())); + std::move(principal.string_matcher)); case Rbac::Principal::RuleType::kMetadata: return absl::make_unique(principal.invert); } @@ -184,14 +184,14 @@ bool AuthenticatedAuthorizationMatcher::Matches( // Connection is not authenticated. return false; } - if (!matcher_.has_value()) { + if (matcher_.string_matcher().empty()) { // Allows any authenticated user. return true; } std::vector uri_sans = args.GetUriSans(); if (!uri_sans.empty()) { for (const auto& uri : uri_sans) { - if (matcher_->Match(uri)) { + if (matcher_.Match(uri)) { return true; } } @@ -199,12 +199,12 @@ bool AuthenticatedAuthorizationMatcher::Matches( std::vector dns_sans = args.GetDnsSans(); if (!dns_sans.empty()) { for (const auto& dns : dns_sans) { - if (matcher_->Match(dns)) { + if (matcher_.Match(dns)) { return true; } } } - return matcher_->Match(args.GetSubject()); + return matcher_.Match(args.GetSubject()); } bool ReqServerNameAuthorizationMatcher::Matches(const EvaluateArgs&) const { diff --git a/src/core/lib/security/authorization/matchers.h b/src/core/lib/security/authorization/matchers.h index a64d6b20b15..6c66a00879e 100644 --- a/src/core/lib/security/authorization/matchers.h +++ b/src/core/lib/security/authorization/matchers.h @@ -153,13 +153,13 @@ class PortAuthorizationMatcher : public AuthorizationMatcher { // or DNS SAN in that order, otherwise uses subject field. class AuthenticatedAuthorizationMatcher : public AuthorizationMatcher { public: - explicit AuthenticatedAuthorizationMatcher(absl::optional auth) + explicit AuthenticatedAuthorizationMatcher(StringMatcher auth) : matcher_(std::move(auth)) {} bool Matches(const EvaluateArgs& args) const override; private: - const absl::optional matcher_; + const StringMatcher matcher_; }; // Perform a match against the request server from the client's connection diff --git a/src/core/lib/security/authorization/rbac_policy.cc b/src/core/lib/security/authorization/rbac_policy.cc index f43eefc1dc0..e0185ff6585 100644 --- a/src/core/lib/security/authorization/rbac_policy.cc +++ b/src/core/lib/security/authorization/rbac_policy.cc @@ -278,7 +278,7 @@ Rbac::Principal Rbac::Principal::MakeAnyPrincipal() { } Rbac::Principal Rbac::Principal::MakeAuthenticatedPrincipal( - absl::optional string_matcher) { + StringMatcher string_matcher) { Principal principal; principal.type = Principal::RuleType::kPrincipalName; principal.string_matcher = std::move(string_matcher); @@ -398,7 +398,7 @@ std::string Rbac::Principal::ToString() const { case RuleType::kAny: return "any"; case RuleType::kPrincipalName: - return absl::StrFormat("principal_name=%s", string_matcher->ToString()); + return absl::StrFormat("principal_name=%s", string_matcher.ToString()); case RuleType::kSourceIp: return absl::StrFormat("source_ip=%s", ip.ToString()); case RuleType::kDirectRemoteIp: @@ -408,7 +408,7 @@ std::string Rbac::Principal::ToString() const { case RuleType::kHeader: return absl::StrFormat("header=%s", header_matcher.ToString()); case RuleType::kPath: - return absl::StrFormat("path=%s", string_matcher->ToString()); + return absl::StrFormat("path=%s", string_matcher.ToString()); case RuleType::kMetadata: return absl::StrFormat("%smetadata", invert ? "invert " : ""); default: diff --git a/src/core/lib/security/authorization/rbac_policy.h b/src/core/lib/security/authorization/rbac_policy.h index df26ba56cdd..f59713e6367 100644 --- a/src/core/lib/security/authorization/rbac_policy.h +++ b/src/core/lib/security/authorization/rbac_policy.h @@ -113,8 +113,7 @@ struct Rbac { std::vector> principals); static Principal MakeNotPrincipal(Principal principal); static Principal MakeAnyPrincipal(); - static Principal MakeAuthenticatedPrincipal( - absl::optional string_matcher); + static Principal MakeAuthenticatedPrincipal(StringMatcher string_matcher); static Principal MakeSourceIpPrincipal(CidrRange ip); static Principal MakeDirectRemoteIpPrincipal(CidrRange ip); static Principal MakeRemoteIpPrincipal(CidrRange ip); @@ -132,7 +131,7 @@ struct Rbac { RuleType type = RuleType::kAnd; HeaderMatcher header_matcher; - absl::optional string_matcher; + StringMatcher string_matcher; CidrRange ip; // For type kAnd/kOr/kNot. For kNot type, the vector will have only one // element. diff --git a/test/core/security/authorization_matchers_test.cc b/test/core/security/authorization_matchers_test.cc index 975e97e178d..08e1ebc9a5c 100644 --- a/test/core/security/authorization_matchers_test.cc +++ b/test/core/security/authorization_matchers_test.cc @@ -456,7 +456,11 @@ TEST_F(AuthorizationMatchersTest, args_.AddPropertyToAuthContext(GRPC_TRANSPORT_SECURITY_TYPE_PROPERTY_NAME, GRPC_SSL_TRANSPORT_SECURITY_TYPE); EvaluateArgs args = args_.MakeEvaluateArgs(); - AuthenticatedAuthorizationMatcher matcher(/*auth=*/absl::nullopt); + AuthenticatedAuthorizationMatcher matcher( + StringMatcher::Create(StringMatcher::Type::kExact, + /*matcher=*/"", + /*case_sensitive=*/false) + .value()); EXPECT_TRUE(matcher.Matches(args)); } diff --git a/test/core/security/rbac_translator_test.cc b/test/core/security/rbac_translator_test.cc index 1290ae3a0ca..0e44fa855ac 100644 --- a/test/core/security/rbac_translator_test.cc +++ b/test/core/security/rbac_translator_test.cc @@ -24,12 +24,10 @@ namespace { MATCHER_P3(EqualsPrincipalName, expected_matcher_type, expected_matcher_value, is_regex, "") { return arg->type == Rbac::Principal::RuleType::kPrincipalName && - arg->string_matcher.value().type() == expected_matcher_type && - is_regex - ? arg->string_matcher.value().regex_matcher()->pattern() == + arg->string_matcher.type() == expected_matcher_type && is_regex + ? arg->string_matcher.regex_matcher()->pattern() == expected_matcher_value - : arg->string_matcher.value().string_matcher() == - expected_matcher_value; + : arg->string_matcher.string_matcher() == expected_matcher_value; } MATCHER_P3(EqualsPath, expected_matcher_type, expected_matcher_value, is_regex, diff --git a/test/cpp/end2end/BUILD b/test/cpp/end2end/BUILD index 55b92c37e6f..e59d371b80c 100644 --- a/test/cpp/end2end/BUILD +++ b/test/cpp/end2end/BUILD @@ -855,13 +855,6 @@ grpc_cc_test( grpc_cc_test( name = "sdk_authz_end2end_test", srcs = ["sdk_authz_end2end_test.cc"], - data = [ - "//src/core/tsi/test_creds:ca.pem", - "//src/core/tsi/test_creds:client.key", - "//src/core/tsi/test_creds:client.pem", - "//src/core/tsi/test_creds:server1.key", - "//src/core/tsi/test_creds:server1.pem", - ], external_deps = [ "gtest", ], diff --git a/test/cpp/end2end/sdk_authz_end2end_test.cc b/test/cpp/end2end/sdk_authz_end2end_test.cc index 09d06f71956..35b95c492ce 100644 --- a/test/cpp/end2end/sdk_authz_end2end_test.cc +++ b/test/cpp/end2end/sdk_authz_end2end_test.cc @@ -22,7 +22,6 @@ #include #include -#include "src/core/lib/iomgr/load_file.h" #include "src/core/lib/security/credentials/fake/fake_credentials.h" #include "src/cpp/client/secure_credentials.h" #include "src/cpp/server/secure_server_credentials.h" @@ -36,52 +35,19 @@ namespace grpc { namespace testing { namespace { -constexpr char kCaCertPath[] = "src/core/tsi/test_creds/ca.pem"; -constexpr char kServerCertPath[] = "src/core/tsi/test_creds/server1.pem"; -constexpr char kServerKeyPath[] = "src/core/tsi/test_creds/server1.key"; -constexpr char kClientCertPath[] = "src/core/tsi/test_creds/client.pem"; -constexpr char kClientKeyPath[] = "src/core/tsi/test_creds/client.key"; - constexpr char kMessage[] = "Hello"; -std::string ReadFile(const char* file_path) { - grpc_slice slice; - GPR_ASSERT( - GRPC_LOG_IF_ERROR("load_file", grpc_load_file(file_path, 0, &slice))); - std::string file_contents(grpc_core::StringViewFromSlice(slice)); - grpc_slice_unref(slice); - return file_contents; -} - class SdkAuthzEnd2EndTest : public ::testing::Test { protected: SdkAuthzEnd2EndTest() : server_address_( - absl::StrCat("localhost:", grpc_pick_unused_port_or_die())) { - std::string root_cert = ReadFile(kCaCertPath); - std::string identity_cert = ReadFile(kServerCertPath); - std::string private_key = ReadFile(kServerKeyPath); - std::vector - server_identity_key_cert_pairs = {{private_key, identity_cert}}; - grpc::experimental::TlsServerCredentialsOptions server_options( - std::make_shared( - root_cert, server_identity_key_cert_pairs)); - server_options.watch_root_certs(); - server_options.watch_identity_key_cert_pairs(); - server_options.set_cert_request_type( - GRPC_SSL_REQUEST_CLIENT_CERTIFICATE_AND_VERIFY); - server_creds_ = grpc::experimental::TlsServerCredentials(server_options); - std::vector - channel_identity_key_cert_pairs = { - {ReadFile(kClientKeyPath), ReadFile(kClientCertPath)}}; - grpc::experimental::TlsChannelCredentialsOptions channel_options; - channel_options.set_certificate_provider( - std::make_shared( - ReadFile(kCaCertPath), channel_identity_key_cert_pairs)); - channel_options.watch_identity_key_cert_pairs(); - channel_options.watch_root_certs(); - channel_creds_ = grpc::experimental::TlsCredentials(channel_options); - } + absl::StrCat("localhost:", grpc_pick_unused_port_or_die())), + server_creds_( + std::shared_ptr(new SecureServerCredentials( + grpc_fake_transport_security_server_credentials_create()))), + channel_creds_( + std::shared_ptr(new SecureChannelCredentials( + grpc_fake_transport_security_credentials_create()))) {} ~SdkAuthzEnd2EndTest() override { server_->Shutdown(); } @@ -124,8 +90,6 @@ class SdkAuthzEnd2EndTest : public ::testing::Test { std::shared_ptr BuildChannel() { ChannelArguments args; - // Override target name for host name check - args.SetSslTargetNameOverride("foo.test.google.fr"); return ::grpc::CreateCustomChannel(server_address_, channel_creds_, args); } @@ -371,9 +335,16 @@ TEST_F( " \"name\": \"authz\"," " \"allow_rules\": [" " {" - " \"name\": \"allow_mtls\"," + " \"name\": \"allow_echo\"," " \"source\": {" - " \"principals\": [\"*\"]" + " \"principals\": [" + " \"foo\"" + " ]" + " }," + " \"request\": {" + " \"paths\": [" + " \"*/Echo\"" + " ]" " }" " }" " ]" @@ -389,29 +360,6 @@ TEST_F( EXPECT_TRUE(resp.message().empty()); } -TEST_F(SdkAuthzEnd2EndTest, - StaticInitAllowsRpcRequestWithPrincipalsFieldOnAuthenticatedConnection) { - std::string policy = - "{" - " \"name\": \"authz\"," - " \"allow_rules\": [" - " {" - " \"name\": \"allow_mtls\"," - " \"source\": {" - " \"principals\": [\"*\"]" - " }" - " }" - " ]" - "}"; - InitServer(CreateStaticAuthzPolicyProvider(policy)); - auto channel = BuildChannel(); - ClientContext context; - grpc::testing::EchoResponse resp; - grpc::Status status = SendRpc(channel, &context, &resp); - EXPECT_TRUE(status.ok()); - EXPECT_EQ(resp.message(), kMessage); -} - TEST_F(SdkAuthzEnd2EndTest, FileWatcherInitAllowsRpcRequestNoMatchInDenyMatchInAllow) { std::string policy =