From 4ecead5c0407dcfa30c0899ff6a64d48a74c424b Mon Sep 17 00:00:00 2001 From: AJ Heller Date: Thu, 16 Nov 2023 17:51:34 -0800 Subject: [PATCH 1/6] [EventEngine] Clarify API: callback cancellation and thread safety (#35009) Closes #35009 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35009 from drfloob:improve-ee-API-docs 996e7d3148700996966625737b2a57e912bf400a PiperOrigin-RevId: 583219509 --- include/grpc/event_engine/event_engine.h | 32 +++++++++++++++--------- 1 file changed, 20 insertions(+), 12 deletions(-) diff --git a/include/grpc/event_engine/event_engine.h b/include/grpc/event_engine/event_engine.h index 2130f7feb1a..4beca657625 100644 --- a/include/grpc/event_engine/event_engine.h +++ b/include/grpc/event_engine/event_engine.h @@ -79,7 +79,7 @@ namespace experimental { /// /// /// Blocking EventEngine Callbacks -/// ----------------------------- +/// ------------------------------ /// /// Doing blocking work in EventEngine callbacks is generally not advisable. /// While gRPC's default EventEngine implementations have some capacity to scale @@ -90,6 +90,15 @@ namespace experimental { /// *Best Practice* : Occasional blocking work may be fine, but we do not /// recommend running a mostly blocking workload in EventEngine threads. /// +/// +/// Thread-safety guarantees +/// ------------------------ +/// +/// All EventEngine methods are guaranteed to be thread-safe, no external +/// synchronization is required to call any EventEngine method. Please note that +/// this does not apply to application callbacks, which may be run concurrently; +/// application state synchronization must be managed by the application. +/// //////////////////////////////////////////////////////////////////////////////// class EventEngine : public std::enable_shared_from_this { public: @@ -405,8 +414,8 @@ class EventEngine : public std::enable_shared_from_this { /// Asynchronously executes a task as soon as possible. /// - /// \a Closures scheduled with \a Run cannot be cancelled. The \a closure will - /// not be deleted after it has been run, ownership remains with the caller. + /// \a Closures passed to \a Run cannot be cancelled. The \a closure will not + /// be deleted after it has been run, ownership remains with the caller. /// /// Implementations must not execute the closure in the calling thread before /// \a Run returns. For example, if the caller must release a lock before the @@ -415,9 +424,9 @@ class EventEngine : public std::enable_shared_from_this { virtual void Run(Closure* closure) = 0; /// Asynchronously executes a task as soon as possible. /// - /// \a Closures scheduled with \a Run cannot be cancelled. Unlike the - /// overloaded \a Closure alternative, the absl::AnyInvocable version's \a - /// closure will be deleted by the EventEngine after the closure has been run. + /// \a Closures passed to \a Run cannot be cancelled. Unlike the overloaded \a + /// Closure alternative, the absl::AnyInvocable version's \a closure will be + /// deleted by the EventEngine after the closure has been run. /// /// This version of \a Run may be less performant than the \a Closure version /// in some scenarios. This overload is useful in situations where performance @@ -453,13 +462,12 @@ class EventEngine : public std::enable_shared_from_this { absl::AnyInvocable closure) = 0; /// Request cancellation of a task. /// - /// If the associated closure has already been scheduled to run, it will not - /// be cancelled, and this function will return false. + /// If the associated closure cannot be cancelled for any reason, this + /// function will return false. /// - /// If the associated closure has not been scheduled to run, it will be - /// cancelled, and this method will return true. The associated - /// absl::AnyInvocable or \a Closure* will not be called. If the closure type - /// was an absl::AnyInvocable, it will be destroyed before the method returns. + /// If the associated closure can be cancelled, the associated callback will + /// never be run, and this method will return true. If the callback type was + /// an absl::AnyInvocable, it will be destroyed before the method returns. virtual bool Cancel(TaskHandle handle) = 0; }; From 42284424acd2ba4ba649599984592f5d2eade919 Mon Sep 17 00:00:00 2001 From: AJ Heller Date: Fri, 17 Nov 2023 13:21:21 -0800 Subject: [PATCH 2/6] [EventEngine] Produce better error logs on `bind` failures (#35004) Seeing a rare flake where a fixture address is already in use. This PR adds sockaddr->string conversion for the address, in case some of this address is being truncated before attempting to bind. It's very odd to see this exact-time-based address string generated twice in succession. `E1116 21:34:56.431768483 17 chttp2_server.cc:1051] UNKNOWN:No address added out of total 1 resolved for 'unix-abstract:grpc_fullstack_test.%00.17.974.661825339.14468088171934000544.7442035989947845277' {created_time:"2023-11-16T21:34:56.431729283+00:00", children:[FAILED_PRECONDITION:Error in bind: Address already in use]} ` https://source.cloud.google.com/results/invocations/48ee2e26-9341-46ec-9761-35939b73f3f8/targets/%2F%2Ftest%2Fcore%2Fend2end:retry_transparent_max_concurrent_streams_test@poller%3Dpoll/log Closes #35004 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35004 from drfloob:better-error-on-bind-failure d12d5f86326fdf411fed51093998e09121535c39 PiperOrigin-RevId: 583472016 --- .../posix_engine/posix_engine_listener_utils.cc | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/core/lib/event_engine/posix_engine/posix_engine_listener_utils.cc b/src/core/lib/event_engine/posix_engine/posix_engine_listener_utils.cc index 8c2745c02b2..ed2321b2971 100644 --- a/src/core/lib/event_engine/posix_engine/posix_engine_listener_utils.cc +++ b/src/core/lib/event_engine/posix_engine/posix_engine_listener_utils.cc @@ -25,6 +25,8 @@ #include "absl/cleanup/cleanup.h" #include "absl/status/status.h" +#include "absl/strings/str_cat.h" +#include "absl/strings/str_replace.h" #include #include @@ -44,8 +46,6 @@ #include // IWYU pragma: keep #include // IWYU pragma: keep #include // IWYU pragma: keep - -#include "absl/strings/str_cat.h" #endif namespace grpc_event_engine { @@ -176,8 +176,16 @@ absl::Status PrepareSocket(const PosixTcpOptions& options, GRPC_FD_SERVER_LISTENER_USAGE, options)); if (bind(fd, socket.addr.address(), socket.addr.size()) < 0) { + auto sockaddr_str = ResolvedAddressToString(socket.addr); + if (!sockaddr_str.ok()) { + gpr_log(GPR_ERROR, "Could not convert sockaddr to string: %s", + sockaddr_str.status().ToString().c_str()); + sockaddr_str = ""; + } + sockaddr_str = absl::StrReplaceAll(*sockaddr_str, {{"\0", "@"}}); return absl::FailedPreconditionError( - absl::StrCat("Error in bind: ", std::strerror(errno))); + absl::StrCat("Error in bind for address '", *sockaddr_str, + "': ", std::strerror(errno))); } if (listen(fd, GetMaxAcceptQueueSize()) < 0) { From 667def75058ffab2b76abaaeb106fbf59e944a24 Mon Sep 17 00:00:00 2001 From: gRPC Team Bot Date: Mon, 27 Nov 2023 21:27:21 +0000 Subject: [PATCH 3/6] Internal change COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35043 from gtcooke94:deprecate_old_crl_apis 003057a93c2686411be4c02bec38fcad4fa7e85d PiperOrigin-RevId: 585744149 --- include/grpc/grpc_security.h | 5 ++++- .../posix_engine/tcp_socket_utils.cc | 3 +-- .../grpc/_cython/_cygrpc/aio/server.pyx.pxi | 2 ++ test/cpp/interop/xds_stats_watcher.cc | 21 +++++++++++++++++-- test/cpp/interop/xds_stats_watcher_test.cc | 7 +------ 5 files changed, 27 insertions(+), 11 deletions(-) diff --git a/include/grpc/grpc_security.h b/include/grpc/grpc_security.h index 54dc862d260..d099ff62933 100644 --- a/include/grpc/grpc_security.h +++ b/include/grpc/grpc_security.h @@ -894,7 +894,10 @@ GRPCAPI void grpc_tls_credentials_options_set_identity_cert_name( GRPCAPI void grpc_tls_credentials_options_set_cert_request_type( grpc_tls_credentials_options* options, grpc_ssl_client_certificate_request_type type); -/** + +/** Deprecated in favor of grpc_tls_credentials_options_set_crl_provider. The + * crl provider interface provides a significantly more flexible approach to + * using CRLs. See gRFC A69 for details. * EXPERIMENTAL API - Subject to change * * If set, gRPC will read all hashed x.509 CRL files in the directory and diff --git a/src/core/lib/event_engine/posix_engine/tcp_socket_utils.cc b/src/core/lib/event_engine/posix_engine/tcp_socket_utils.cc index 845e48cdf41..c5b2277d6a7 100644 --- a/src/core/lib/event_engine/posix_engine/tcp_socket_utils.cc +++ b/src/core/lib/event_engine/posix_engine/tcp_socket_utils.cc @@ -75,6 +75,7 @@ int AdjustValue(int default_value, int min_value, int max_value, return *actual_value; } +#ifdef GRPC_POSIX_SOCKET_UTILS_COMMON // The default values for TCP_USER_TIMEOUT are currently configured to be in // line with the default values of KEEPALIVE_TIMEOUT as proposed in // https://github.com/grpc/proposal/blob/master/A18-tcp-user-timeout.md */ @@ -83,8 +84,6 @@ int kDefaultServerUserTimeoutMs = 20000; bool kDefaultClientUserTimeoutEnabled = false; bool kDefaultServerUserTimeoutEnabled = true; -#ifdef GRPC_POSIX_SOCKET_UTILS_COMMON - absl::Status ErrorForFd( int fd, const experimental::EventEngine::ResolvedAddress& addr) { if (fd >= 0) return absl::OkStatus(); diff --git a/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi b/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi index 508196e0a41..a21c1ec0082 100644 --- a/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi +++ b/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi @@ -470,6 +470,7 @@ async def _finish_handler_with_stream_responses(RPCState rpc_state, """ cdef object async_response_generator cdef object response_message + install_context_from_request_call_event_aio(rpc_state) if inspect.iscoroutinefunction(stream_handler): # Case 1: Coroutine async handler - using reader-writer API @@ -523,6 +524,7 @@ async def _finish_handler_with_stream_responses(RPCState rpc_state, rpc_state.metadata_sent = True rpc_state.status_sent = True await execute_batch(rpc_state, finish_ops, loop) + uninstall_context() async def _handle_unary_unary_rpc(object method_handler, diff --git a/test/cpp/interop/xds_stats_watcher.cc b/test/cpp/interop/xds_stats_watcher.cc index d6cb3037b8a..1f2852575a3 100644 --- a/test/cpp/interop/xds_stats_watcher.cc +++ b/test/cpp/interop/xds_stats_watcher.cc @@ -51,6 +51,19 @@ std::unordered_set ToLowerCase( return result; } +bool HasNonEmptyMetadata( + const std::map& + metadata_by_peer) { + for (const auto& entry : metadata_by_peer) { + for (const auto& rpc_metadata : entry.second.rpc_metadata()) { + if (rpc_metadata.metadata_size() > 0) { + return true; + } + } + } + return false; +} + } // namespace XdsStatsWatcher::XdsStatsWatcher(int start_id, int end_id, @@ -113,8 +126,12 @@ LoadBalancerStatsResponse XdsStatsWatcher::WaitForRpcStatsResponse( [this] { return rpcs_needed_ == 0; }); response.mutable_rpcs_by_peer()->insert(rpcs_by_peer_.begin(), rpcs_by_peer_.end()); - response.mutable_metadatas_by_peer()->insert(metadata_by_peer_.begin(), - metadata_by_peer_.end()); + // Return metadata if at least one RPC had relevant metadata. Note that empty + // entries would be returned for RCPs with no relevant metadata in this case. + if (HasNonEmptyMetadata(metadata_by_peer_)) { + response.mutable_metadatas_by_peer()->insert(metadata_by_peer_.begin(), + metadata_by_peer_.end()); + } auto& response_rpcs_by_method = *response.mutable_rpcs_by_method(); for (const auto& rpc_by_type : rpcs_by_type_) { std::string method_name; diff --git a/test/cpp/interop/xds_stats_watcher_test.cc b/test/cpp/interop/xds_stats_watcher_test.cc index 0909b49bd1b..3d2f951c246 100644 --- a/test/cpp/interop/xds_stats_watcher_test.cc +++ b/test/cpp/interop/xds_stats_watcher_test.cc @@ -152,7 +152,7 @@ TEST(XdsStatsWatcherTest, WaitForRpcStatsResponseReturnsAll) { watcher.WaitForRpcStatsResponse(0).DebugString()); } -TEST(XdsStatsWatcherTest, WaitForRpcStatsResponseIgnoresMetadata) { +TEST(XdsStatsWatcherTest, WaitForRpcStatsResponseExcludesMetadata) { XdsStatsWatcher watcher(0, 3, {}); // RPC had metadata - but watcher should ignore it watcher.RpcCompleted(BuildCallResult(0), "peer1", @@ -163,11 +163,6 @@ TEST(XdsStatsWatcherTest, WaitForRpcStatsResponseIgnoresMetadata) { {{"k1", "v5"}, {"k2", "v6"}, {"k3", "v7"}}); LoadBalancerStatsResponse expected; expected.mutable_rpcs_by_peer()->insert({{"peer1", 2}, {"peer2", 1}}); - // There will still be an empty metadata collection for each RPC - expected.mutable_metadatas_by_peer()->insert({ - {"peer1", BuildMetadatas({{}, {}})}, - {"peer2", BuildMetadatas({{}})}, - }); (*expected.mutable_rpcs_by_method())["UnaryCall"] .mutable_rpcs_by_peer() ->insert({{"peer1", 2}, {"peer2", 1}}); From 79c9a67dee5bbbd65031296097994e222b885d22 Mon Sep 17 00:00:00 2001 From: aeitzman Date: Mon, 27 Nov 2023 16:46:39 -0800 Subject: [PATCH 4/6] [core/security] Adding metrics header to sts request for external account credentials (#34661) Closes #34661 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/34661 from aeitzman:metrics 9f53992ed4c8aef4f753e9c1ec3d65cb7eab1dba PiperOrigin-RevId: 585796526 --- .../aws_external_account_credentials.cc | 4 + .../aws_external_account_credentials.h | 4 + .../external/external_account_credentials.cc | 48 ++++---- .../external/external_account_credentials.h | 4 + .../file_external_account_credentials.cc | 4 + .../file_external_account_credentials.h | 4 + .../url_external_account_credentials.cc | 4 + .../url_external_account_credentials.h | 4 + test/core/security/credentials_test.cc | 111 ++++++++++++++++-- 9 files changed, 158 insertions(+), 29 deletions(-) diff --git a/src/core/lib/security/credentials/external/aws_external_account_credentials.cc b/src/core/lib/security/credentials/external/aws_external_account_credentials.cc index 68ddaaeb903..cd7d117b28e 100644 --- a/src/core/lib/security/credentials/external/aws_external_account_credentials.cc +++ b/src/core/lib/security/credentials/external/aws_external_account_credentials.cc @@ -525,4 +525,8 @@ void AwsExternalAccountCredentials::FinishRetrieveSubjectToken( } } +absl::string_view AwsExternalAccountCredentials::CredentialSourceType() { + return "aws"; +} + } // namespace grpc_core diff --git a/src/core/lib/security/credentials/external/aws_external_account_credentials.h b/src/core/lib/security/credentials/external/aws_external_account_credentials.h index e8adc02452f..a3a41236615 100644 --- a/src/core/lib/security/credentials/external/aws_external_account_credentials.h +++ b/src/core/lib/security/credentials/external/aws_external_account_credentials.h @@ -24,6 +24,8 @@ #include #include +#include "absl/strings/string_view.h" + #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/http/httpcli.h" @@ -72,6 +74,8 @@ class AwsExternalAccountCredentials final : public ExternalAccountCredentials { void AddMetadataRequestHeaders(grpc_http_request* request); + absl::string_view CredentialSourceType() override; + std::string audience_; OrphanablePtr http_request_; diff --git a/src/core/lib/security/credentials/external/external_account_credentials.cc b/src/core/lib/security/credentials/external/external_account_credentials.cc index 93228dbca4e..3ffd2acb0ef 100644 --- a/src/core/lib/security/credentials/external/external_account_credentials.cc +++ b/src/core/lib/security/credentials/external/external_account_credentials.cc @@ -26,6 +26,7 @@ #include "absl/status/status.h" #include "absl/status/statusor.h" +#include "absl/strings/escaping.h" #include "absl/strings/match.h" #include "absl/strings/numbers.h" #include "absl/strings/str_cat.h" @@ -53,7 +54,6 @@ #include "src/core/lib/security/credentials/external/file_external_account_credentials.h" #include "src/core/lib/security/credentials/external/url_external_account_credentials.h" #include "src/core/lib/security/util/json_util.h" -#include "src/core/lib/slice/b64.h" #include "src/core/lib/uri/uri_parser.h" #define EXTERNAL_ACCOUNT_CREDENTIALS_GRANT_TYPE \ @@ -271,6 +271,20 @@ std::string ExternalAccountCredentials::debug_string() { grpc_oauth2_token_fetcher_credentials::debug_string()); } +std::string ExternalAccountCredentials::MetricsHeaderValue() { + return absl::StrFormat( + "gl-cpp/unknown auth/%s google-byoid-sdk source/%s sa-impersonation/%v " + "config-lifetime/%v", + grpc_version_string(), CredentialSourceType(), + !options_.service_account_impersonation_url.empty(), + options_.service_account_impersonation.token_lifetime_seconds != + IMPERSONATED_CRED_DEFAULT_LIFETIME_IN_SECONDS); +} + +absl::string_view ExternalAccountCredentials::CredentialSourceType() { + return "unknown"; +} + // The token fetching flow: // 1. Retrieve subject token - Subclass's RetrieveSubjectToken() gets called // and the subject token is received in OnRetrieveSubjectTokenInternal(). @@ -317,27 +331,21 @@ void ExternalAccountCredentials::ExchangeToken( } grpc_http_request request; memset(&request, 0, sizeof(grpc_http_request)); - grpc_http_header* headers = nullptr; - if (!options_.client_id.empty() && !options_.client_secret.empty()) { - request.hdr_count = 2; - headers = static_cast( - gpr_malloc(sizeof(grpc_http_header) * request.hdr_count)); - headers[0].key = gpr_strdup("Content-Type"); - headers[0].value = gpr_strdup("application/x-www-form-urlencoded"); + const bool add_authorization_header = + !options_.client_id.empty() && !options_.client_secret.empty(); + request.hdr_count = add_authorization_header ? 3 : 2; + auto* headers = static_cast( + gpr_malloc(sizeof(grpc_http_header) * request.hdr_count)); + headers[0].key = gpr_strdup("Content-Type"); + headers[0].value = gpr_strdup("application/x-www-form-urlencoded"); + headers[1].key = gpr_strdup("x-goog-api-client"); + headers[1].value = gpr_strdup(MetricsHeaderValue().c_str()); + if (add_authorization_header) { std::string raw_cred = absl::StrFormat("%s:%s", options_.client_id, options_.client_secret); - char* encoded_cred = - grpc_base64_encode(raw_cred.c_str(), raw_cred.length(), 0, 0); - std::string str = absl::StrFormat("Basic %s", std::string(encoded_cred)); - headers[1].key = gpr_strdup("Authorization"); - headers[1].value = gpr_strdup(str.c_str()); - gpr_free(encoded_cred); - } else { - request.hdr_count = 1; - headers = static_cast( - gpr_malloc(sizeof(grpc_http_header) * request.hdr_count)); - headers[0].key = gpr_strdup("Content-Type"); - headers[0].value = gpr_strdup("application/x-www-form-urlencoded"); + std::string str = absl::StrFormat("Basic %s", absl::Base64Escape(raw_cred)); + headers[2].key = gpr_strdup("Authorization"); + headers[2].value = gpr_strdup(str.c_str()); } request.hdrs = headers; std::vector body_parts; diff --git a/src/core/lib/security/credentials/external/external_account_credentials.h b/src/core/lib/security/credentials/external/external_account_credentials.h index bcbd33b272b..13749db8567 100644 --- a/src/core/lib/security/credentials/external/external_account_credentials.h +++ b/src/core/lib/security/credentials/external/external_account_credentials.h @@ -101,6 +101,10 @@ class ExternalAccountCredentials HTTPRequestContext* ctx, const Options& options, std::function cb) = 0; + virtual absl::string_view CredentialSourceType(); + + std::string MetricsHeaderValue(); + private: // This method implements the common token fetch logic and it will be called // when grpc_oauth2_token_fetcher_credentials request a new access token. diff --git a/src/core/lib/security/credentials/external/file_external_account_credentials.cc b/src/core/lib/security/credentials/external/file_external_account_credentials.cc index e70131bfeef..0fb3ce5091d 100644 --- a/src/core/lib/security/credentials/external/file_external_account_credentials.cc +++ b/src/core/lib/security/credentials/external/file_external_account_credentials.cc @@ -137,4 +137,8 @@ void FileExternalAccountCredentials::RetrieveSubjectToken( cb(std::string(content), absl::OkStatus()); } +absl::string_view FileExternalAccountCredentials::CredentialSourceType() { + return "file"; +} + } // namespace grpc_core diff --git a/src/core/lib/security/credentials/external/file_external_account_credentials.h b/src/core/lib/security/credentials/external/file_external_account_credentials.h index 40f0e9f23f4..c8cf12177bd 100644 --- a/src/core/lib/security/credentials/external/file_external_account_credentials.h +++ b/src/core/lib/security/credentials/external/file_external_account_credentials.h @@ -23,6 +23,8 @@ #include #include +#include "absl/strings/string_view.h" + #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/iomgr/error.h" #include "src/core/lib/security/credentials/external/external_account_credentials.h" @@ -44,6 +46,8 @@ class FileExternalAccountCredentials final : public ExternalAccountCredentials { HTTPRequestContext* ctx, const Options& options, std::function cb) override; + absl::string_view CredentialSourceType() override; + // Fields of credential source std::string file_; std::string format_type_; diff --git a/src/core/lib/security/credentials/external/url_external_account_credentials.cc b/src/core/lib/security/credentials/external/url_external_account_credentials.cc index 0e4cb8afba1..4b977c045ee 100644 --- a/src/core/lib/security/credentials/external/url_external_account_credentials.cc +++ b/src/core/lib/security/credentials/external/url_external_account_credentials.cc @@ -240,4 +240,8 @@ void UrlExternalAccountCredentials::FinishRetrieveSubjectToken( } } +absl::string_view UrlExternalAccountCredentials::CredentialSourceType() { + return "url"; +} + } // namespace grpc_core diff --git a/src/core/lib/security/credentials/external/url_external_account_credentials.h b/src/core/lib/security/credentials/external/url_external_account_credentials.h index 71b734e591e..9331412d321 100644 --- a/src/core/lib/security/credentials/external/url_external_account_credentials.h +++ b/src/core/lib/security/credentials/external/url_external_account_credentials.h @@ -24,6 +24,8 @@ #include #include +#include "absl/strings/string_view.h" + #include "src/core/lib/gprpp/orphanable.h" #include "src/core/lib/gprpp/ref_counted_ptr.h" #include "src/core/lib/http/httpcli.h" @@ -48,6 +50,8 @@ class UrlExternalAccountCredentials final : public ExternalAccountCredentials { HTTPRequestContext* ctx, const Options& options, std::function cb) override; + absl::string_view CredentialSourceType() override; + static void OnRetrieveSubjectToken(void* arg, grpc_error_handle error); void OnRetrieveSubjectTokenInternal(grpc_error_handle error); diff --git a/test/core/security/credentials_test.cc b/test/core/security/credentials_test.cc index f0c0543275c..ad58ccd394f 100644 --- a/test/core/security/credentials_test.cc +++ b/test/core/security/credentials_test.cc @@ -2127,12 +2127,12 @@ void validate_external_account_creds_token_exchage_request( // Check the rest of the request. GPR_ASSERT(strcmp(host, "foo.com:5555") == 0); GPR_ASSERT(strcmp(path, "/token") == 0); - GPR_ASSERT(request->hdr_count == 2); + GPR_ASSERT(request->hdr_count == 3); GPR_ASSERT(strcmp(request->hdrs[0].key, "Content-Type") == 0); GPR_ASSERT( strcmp(request->hdrs[0].value, "application/x-www-form-urlencoded") == 0); - GPR_ASSERT(strcmp(request->hdrs[1].key, "Authorization") == 0); - GPR_ASSERT(strcmp(request->hdrs[1].value, + GPR_ASSERT(strcmp(request->hdrs[2].key, "Authorization") == 0); + GPR_ASSERT(strcmp(request->hdrs[2].value, "Basic Y2xpZW50X2lkOmNsaWVudF9zZWNyZXQ=") == 0); } @@ -2155,12 +2155,12 @@ void validate_external_account_creds_token_exchage_request_with_url_encode( // Check the rest of the request. GPR_ASSERT(strcmp(host, "foo.com:5555") == 0); GPR_ASSERT(strcmp(path, "/token_url_encode") == 0); - GPR_ASSERT(request->hdr_count == 2); + GPR_ASSERT(request->hdr_count == 3); GPR_ASSERT(strcmp(request->hdrs[0].key, "Content-Type") == 0); GPR_ASSERT( strcmp(request->hdrs[0].value, "application/x-www-form-urlencoded") == 0); - GPR_ASSERT(strcmp(request->hdrs[1].key, "Authorization") == 0); - GPR_ASSERT(strcmp(request->hdrs[1].value, + GPR_ASSERT(strcmp(request->hdrs[2].key, "Authorization") == 0); + GPR_ASSERT(strcmp(request->hdrs[2].value, "Basic Y2xpZW50X2lkOmNsaWVudF9zZWNyZXQ=") == 0); } @@ -2311,12 +2311,18 @@ void validate_aws_external_account_creds_token_exchage_request( // Check the rest of the request. GPR_ASSERT(strcmp(host, "foo.com:5555") == 0); GPR_ASSERT(strcmp(path, "/token") == 0); - GPR_ASSERT(request->hdr_count == 2); + GPR_ASSERT(request->hdr_count == 3); GPR_ASSERT(strcmp(request->hdrs[0].key, "Content-Type") == 0); GPR_ASSERT( strcmp(request->hdrs[0].value, "application/x-www-form-urlencoded") == 0); - GPR_ASSERT(strcmp(request->hdrs[1].key, "Authorization") == 0); - GPR_ASSERT(strcmp(request->hdrs[1].value, + GPR_ASSERT(strcmp(request->hdrs[1].key, "x-goog-api-client") == 0); + EXPECT_EQ( + request->hdrs[1].value, + absl::StrFormat("gl-cpp/unknown auth/%s google-byoid-sdk source/aws " + "sa-impersonation/false config-lifetime/false", + grpc_version_string())); + GPR_ASSERT(strcmp(request->hdrs[2].key, "Authorization") == 0); + GPR_ASSERT(strcmp(request->hdrs[2].value, "Basic Y2xpZW50X2lkOmNsaWVudF9zZWNyZXQ=") == 0); } @@ -2385,6 +2391,8 @@ class TestExternalAccountCredentials final : public ExternalAccountCredentials { std::vector scopes) : ExternalAccountCredentials(std::move(options), std::move(scopes)) {} + std::string GetMetricsValue() { return MetricsHeaderValue(); } + protected: void RetrieveSubjectToken( HTTPRequestContext* /*ctx*/, const Options& /*options*/, @@ -2393,6 +2401,91 @@ class TestExternalAccountCredentials final : public ExternalAccountCredentials { } }; +TEST(CredentialsTest, TestExternalAccountCredsMetricsHeader) { + Json credential_source = Json::FromString(""); + TestExternalAccountCredentials::ServiceAccountImpersonation + service_account_impersonation; + service_account_impersonation.token_lifetime_seconds = 3600; + TestExternalAccountCredentials::Options options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "", // service_account_impersonation_url; + service_account_impersonation, // service_account_impersonation; + "https://foo.com:5555/token", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + "", // workforce_pool_user_project; + }; + TestExternalAccountCredentials creds(options, {}); + + EXPECT_EQ( + creds.GetMetricsValue(), + absl::StrFormat("gl-cpp/unknown auth/%s google-byoid-sdk source/unknown " + "sa-impersonation/false config-lifetime/false", + grpc_version_string())); +} + +TEST(CredentialsTest, + TestExternalAccountCredsMetricsHeaderWithServiceAccountImpersonation) { + Json credential_source = Json::FromString(""); + TestExternalAccountCredentials::ServiceAccountImpersonation + service_account_impersonation; + service_account_impersonation.token_lifetime_seconds = 3600; + TestExternalAccountCredentials::Options options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "https://foo.com:5555/service_account_impersonation", // service_account_impersonation_url; + service_account_impersonation, // service_account_impersonation; + "https://foo.com:5555/token", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + "", // workforce_pool_user_project; + }; + TestExternalAccountCredentials creds(options, {}); + + EXPECT_EQ( + creds.GetMetricsValue(), + absl::StrFormat("gl-cpp/unknown auth/%s google-byoid-sdk source/unknown " + "sa-impersonation/true config-lifetime/false", + grpc_version_string())); +} + +TEST(CredentialsTest, TestExternalAccountCredsMetricsHeaderWithConfigLifetime) { + Json credential_source = Json::FromString(""); + TestExternalAccountCredentials::ServiceAccountImpersonation + service_account_impersonation; + service_account_impersonation.token_lifetime_seconds = 5000; + TestExternalAccountCredentials::Options options = { + "external_account", // type; + "audience", // audience; + "subject_token_type", // subject_token_type; + "https://foo.com:5555/service_account_impersonation", // service_account_impersonation_url; + service_account_impersonation, // service_account_impersonation; + "https://foo.com:5555/token", // token_url; + "https://foo.com:5555/token_info", // token_info_url; + credential_source, // credential_source; + "quota_project_id", // quota_project_id; + "client_id", // client_id; + "client_secret", // client_secret; + "", // workforce_pool_user_project; + }; + TestExternalAccountCredentials creds(options, {}); + + EXPECT_EQ( + creds.GetMetricsValue(), + absl::StrFormat("gl-cpp/unknown auth/%s google-byoid-sdk source/unknown " + "sa-impersonation/true config-lifetime/true", + grpc_version_string())); +} + TEST(CredentialsTest, TestExternalAccountCredsSuccess) { ExecCtx exec_ctx; Json credential_source = Json::FromString(""); From d47a264991cf52cf5ee48ea85047e04005d5b3fe Mon Sep 17 00:00:00 2001 From: Gregory Cooke Date: Tue, 28 Nov 2023 11:48:50 -0800 Subject: [PATCH 5/6] [Security - Documentation] Mark cpp crl_directory method as deprecated (#35128) The c-core API was marked as deprecated, also mark the cpp api as deprecated Closes #35128 COPYBARA_INTEGRATE_REVIEW=https://github.com/grpc/grpc/pull/35128 from gtcooke94:deprecate_cpp_crl_directory 56717d020c3a8b36996878b8f953968b99205e2f PiperOrigin-RevId: 586057092 --- include/grpcpp/security/tls_credentials_options.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/grpcpp/security/tls_credentials_options.h b/include/grpcpp/security/tls_credentials_options.h index 341d3a857bf..7ad5b088568 100644 --- a/include/grpcpp/security/tls_credentials_options.h +++ b/include/grpcpp/security/tls_credentials_options.h @@ -99,8 +99,9 @@ class TlsCredentialsOptions { // verifiers other than the host name verifier is used. void set_check_call_host(bool check_call_host); - // TODO(zhenlian): This is an experimental API is likely to change in the - // future. Before de-experiementalizing, verify the API is up to date. + // Deprecated in favor of set_crl_provider. The + // crl provider interface provides a significantly more flexible approach to + // using CRLs. See gRFC A69 for details. // If set, gRPC will read all hashed x.509 CRL files in the directory and // enforce the CRL files on all TLS handshakes. Only supported for OpenSSL // version > 1.1. From 5ac8442432950c5c11a39904bb1f381829b7ae0c Mon Sep 17 00:00:00 2001 From: Craig Tiller Date: Wed, 29 Nov 2023 09:41:04 -0800 Subject: [PATCH 6/6] [copybara] omit bot from autofix (#35141) --- .github/workflows/pr-auto-fix.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/pr-auto-fix.yaml b/.github/workflows/pr-auto-fix.yaml index 573de52d9cb..2374a5e0e0d 100644 --- a/.github/workflows/pr-auto-fix.yaml +++ b/.github/workflows/pr-auto-fix.yaml @@ -48,7 +48,7 @@ jobs: with: script: | // If you'd like not to run this code on your commits, add your github user id here: - NO_AUTOFIX_USERS = [] + NO_AUTOFIX_USERS = ["copybara-service"] const { owner, repo } = context.repo if (NO_AUTOFIX_USERS.includes(context.actor)) { console.log('Cancelling');