From 7fed8f541ed9cb97eb1196b6eb9eb87b88449924 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rafa=C5=82=20Augustyniak?= Date: Wed, 15 Mar 2023 17:22:38 -0400 Subject: [PATCH 1/4] core: log caught exceptions (#26102) Commit Message: https://github.com/envoyproxy/envoy/pull/26010 replaces obj-c with Swift. Swift interop with C++ does not support all c++ features: all uncaught c++ exceptions become UB in Swift. Adding logging of C++ exceptions to see whether we have any. If yes, we may need to rethink whether we want to migrate to Swift. Additional Description: Risk Level: Low Testing: Docs Changes: Release Notes: Platform Specific Features: [Optional Runtime guard:] [Optional Fixes #Issue] [Optional Fixes commit #PR or SHA] [Optional Deprecated:] [Optional [API Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):] Signed-off-by: Rafal Augustyniak --- mobile/library/objective-c/EnvoyEngineImpl.mm | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/mobile/library/objective-c/EnvoyEngineImpl.mm b/mobile/library/objective-c/EnvoyEngineImpl.mm index 3cfb0d5f1a5f..20a9fc8f3e2b 100644 --- a/mobile/library/objective-c/EnvoyEngineImpl.mm +++ b/mobile/library/objective-c/EnvoyEngineImpl.mm @@ -530,8 +530,7 @@ - (int)runWithConfig:(EnvoyConfiguration *)config logLevel:(NSString *)logLevel options->setConcurrency(1); return reinterpret_cast(_engineHandle)->run(std::move(options)); } @catch (NSException *exception) { - NSLog(@"[Envoy] exception caught: %@", exception); - [NSNotificationCenter.defaultCenter postNotificationName:@"EnvoyError" object:self]; + [self logException:exception]; return kEnvoyFailure; } } @@ -545,8 +544,7 @@ - (int)runWithYAML:(NSString *)yaml @try { return (int)run_engine(_engineHandle, yaml.UTF8String, logLevel.UTF8String, ""); } @catch (NSException *exception) { - NSLog(@"[Envoy] exception caught: %@", exception); - [NSNotificationCenter.defaultCenter postNotificationName:@"EnvoyError" object:self]; + [self logException:exception]; return kEnvoyFailure; } } @@ -609,4 +607,16 @@ - (void)terminateNotification:(NSNotification *)notification { terminate_engine(_engineHandle, /* release */ false); } +- (void)logException:(NSException *)exception { + NSLog(@"[Envoy] exception caught: %@", exception); + + NSString *message = [NSString stringWithFormat:@"%@;%@;%@", exception.name, exception.reason, + exception.callStackSymbols.description]; + ENVOY_LOG_EVENT_TO_LOGGER(Envoy::Logger::Registry::getLog(Envoy::Logger::Id::misc), error, + "handled_cxx_exception", [message UTF8String]); + + [NSNotificationCenter.defaultCenter postNotificationName:@"EnvoyHandledCXXException" + object:exception]; +} + @end From 987bf5043191f3b3b71eddea4928cdccfec4331f Mon Sep 17 00:00:00 2001 From: code Date: Thu, 16 Mar 2023 11:07:59 +0800 Subject: [PATCH 2/4] api: new field to the FilterConfig to disable a filter in specific route or vh (#25927) Risk Level: low. Testing: n/a. Signed-off-by: wbpcode --- api/envoy/config/route/v3/route_components.proto | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/api/envoy/config/route/v3/route_components.proto b/api/envoy/config/route/v3/route_components.proto index 786481ccd237..fdef07a2f31b 100644 --- a/api/envoy/config/route/v3/route_components.proto +++ b/api/envoy/config/route/v3/route_components.proto @@ -2384,4 +2384,18 @@ message FilterConfig { // not support the specified filter, it may ignore the map entry rather // than rejecting the config. bool is_optional = 2; + + // If true, the filter is disabled in the route or virtual host and the ``config`` field is ignored. + // + // .. note:: + // + // This field will take effect when the request arrive and filter chain is created for the request. + // If initial route is selected for the request and a filter is disabled in the initial route, then + // the filter will not be added to the filter chain. + // And if the request is mutated later and re-match to another route, the disabled filter by the + // initial route will not be added back to the filter chain because the filter chain is already + // created and it is too late to change the chain. + // + // This field only make sense for the downstream HTTP filters for now. + bool disabled = 3; } From 25c383b3879877170cf6b3a823085026d305181e Mon Sep 17 00:00:00 2001 From: John Howard Date: Thu, 16 Mar 2023 06:20:12 -0700 Subject: [PATCH 3/4] listener filters: improve logging levels (#26108) Currently, we have a generic "new connection accepted" at debug level, but "set to {}" at trace. This ends up being the inversion of what we typically want; we know any new connection will call the listener filter, but what is interesting is knowing the result. This PR simply inverts the scopes, so we have the same amount of logs at `debug` but get more information. Signed-off-by: John Howard --- .../filters/listener/http_inspector/http_inspector.cc | 4 ++-- .../extensions/filters/listener/original_dst/original_dst.cc | 4 ++-- .../filters/listener/tls_inspector/tls_inspector.cc | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/source/extensions/filters/listener/http_inspector/http_inspector.cc b/source/extensions/filters/listener/http_inspector/http_inspector.cc index 6410e654a5e5..c9b10974bc22 100644 --- a/source/extensions/filters/listener/http_inspector/http_inspector.cc +++ b/source/extensions/filters/listener/http_inspector/http_inspector.cc @@ -50,7 +50,7 @@ Network::FilterStatus Filter::onData(Network::ListenerFilterBuffer& buffer) { } Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { - ENVOY_LOG(debug, "http inspector: new connection accepted"); + ENVOY_LOG(trace, "http inspector: new connection accepted"); const Network::ConnectionSocket& socket = cb.socket(); @@ -136,7 +136,7 @@ void Filter::done(bool success) { // TODO(yxue): use detected protocol from http inspector and support h2c token in HCM protocol = Http::Utility::AlpnNames::get().Http2c; } - ENVOY_LOG(trace, "http inspector: set application protocol to {}", protocol); + ENVOY_LOG(debug, "http inspector: set application protocol to {}", protocol); cb_->socket().setRequestedApplicationProtocols({protocol}); } else { diff --git a/source/extensions/filters/listener/original_dst/original_dst.cc b/source/extensions/filters/listener/original_dst/original_dst.cc index a9b36d35aeb1..7f8c63f5f271 100644 --- a/source/extensions/filters/listener/original_dst/original_dst.cc +++ b/source/extensions/filters/listener/original_dst/original_dst.cc @@ -17,7 +17,7 @@ Network::Address::InstanceConstSharedPtr OriginalDstFilter::getOriginalDst(Netwo } Network::FilterStatus OriginalDstFilter::onAccept(Network::ListenerFilterCallbacks& cb) { - ENVOY_LOG(debug, "original_dst: new connection accepted"); + ENVOY_LOG(trace, "original_dst: new connection accepted"); Network::ConnectionSocket& socket = cb.socket(); if (socket.addressType() == Network::Address::Type::Ip) { @@ -63,7 +63,7 @@ Network::FilterStatus OriginalDstFilter::onAccept(Network::ListenerFilterCallbac } } #endif - ENVOY_LOG(trace, "original_dst: set destination to {}", original_local_address->asString()); + ENVOY_LOG(debug, "original_dst: set destination to {}", original_local_address->asString()); // Restore the local address to the original one. socket.connectionInfoProvider().restoreLocalAddress(original_local_address); diff --git a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc index e051d28083f1..fb78a4fcafd1 100644 --- a/source/extensions/filters/listener/tls_inspector/tls_inspector.cc +++ b/source/extensions/filters/listener/tls_inspector/tls_inspector.cc @@ -83,7 +83,7 @@ Filter::Filter(const ConfigSharedPtr& config) : config_(config), ssl_(config_->n } Network::FilterStatus Filter::onAccept(Network::ListenerFilterCallbacks& cb) { - ENVOY_LOG(debug, "tls inspector: new connection accepted"); + ENVOY_LOG(trace, "tls inspector: new connection accepted"); cb_ = &cb; return Network::FilterStatus::StopIteration; From 4e6956f77ce9685237f9a523e0aa59ef5f171455 Mon Sep 17 00:00:00 2001 From: schonbachler Date: Thu, 16 Mar 2023 15:01:11 +0100 Subject: [PATCH 4/4] oauth filter: make IdToken and RefreshToken cookie names customizable (#24828) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Schönbächler --- .../filters/http/oauth2/v3/oauth.proto | 9 ++++ changelogs/current.yaml | 3 ++ .../http/http_filters/oauth2_filter.rst | 3 +- .../extensions/filters/http/oauth2/filter.cc | 29 ++++++------- .../extensions/filters/http/oauth2/filter.h | 12 ++++-- .../filters/http/oauth2/config_test.cc | 2 + .../filters/http/oauth2/filter_test.cc | 43 +++++++++++++++++-- .../http/oauth2/oauth_integration_test.cc | 3 +- 8 files changed, 78 insertions(+), 26 deletions(-) diff --git a/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto b/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto index eec7a0565c63..7c933d8726fa 100644 --- a/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto +++ b/api/envoy/extensions/filters/http/oauth2/v3/oauth.proto @@ -22,6 +22,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // message OAuth2Credentials { + // [#next-free-field: 6] message CookieNames { // Cookie name to hold OAuth bearer token value. When the authentication server validates the // client and returns an authorization token back to the OAuth filter, no matter what format @@ -38,6 +39,14 @@ message OAuth2Credentials { // Cookie name to hold OAuth expiry value. Defaults to ``OauthExpires``. string oauth_expires = 3 [(validate.rules).string = {well_known_regex: HTTP_HEADER_NAME ignore_empty: true}]; + + // Cookie name to hold the id token. Defaults to ``IdToken``. + string id_token = 4 + [(validate.rules).string = {well_known_regex: HTTP_HEADER_NAME ignore_empty: true}]; + + // Cookie name to hold the refresh token. Defaults to ``RefreshToken``. + string refresh_token = 5 + [(validate.rules).string = {well_known_regex: HTTP_HEADER_NAME ignore_empty: true}]; } // The client_id to be used in the authorize calls. This value will be URL encoded when sent to the OAuth server. diff --git a/changelogs/current.yaml b/changelogs/current.yaml index 9e0b4d4d8e93..76f47233b83d 100644 --- a/changelogs/current.yaml +++ b/changelogs/current.yaml @@ -119,6 +119,9 @@ new_features: - area: access_log change: | enhanced observability into local close for :ref:`%RESPONSE_CODE_DETAILS% `. +- area: oauth filter + change: | + extended :ref:`cookie_names ` to allow overriding (default) cookie names (``IdToken``, ``RefreshToken``) set by the filter. - area: tracing change: | allow :ref:`grpc_service ` to be optional. This enables a means to disable collection of traces. diff --git a/docs/root/configuration/http/http_filters/oauth2_filter.rst b/docs/root/configuration/http/http_filters/oauth2_filter.rst index 187753b77b45..ad20273eda46 100644 --- a/docs/root/configuration/http/http_filters/oauth2_filter.rst +++ b/docs/root/configuration/http/http_filters/oauth2_filter.rst @@ -28,7 +28,8 @@ The OAuth filter's flow involves: :ref:`hmac_secret ` to assist in encoding. * The filter calls ``continueDecoding()`` to unblock the filter chain. -* The filter sets ``IdToken`` and ``RefreshToken`` cookies if they are provided by Identity provider along with ``AccessToken``. +* The filter sets ``IdToken`` and ``RefreshToken`` cookies if they are provided by Identity provider along with ``AccessToken``. These cookie names + can be customized by setting :ref:`cookie_names `. When the authn server validates the client and returns an authorization token back to the OAuth filter, no matter what format that token is, if diff --git a/source/extensions/filters/http/oauth2/filter.cc b/source/extensions/filters/http/oauth2/filter.cc index 6033d5c4433a..b770f922883f 100644 --- a/source/extensions/filters/http/oauth2/filter.cc +++ b/source/extensions/filters/http/oauth2/filter.cc @@ -34,20 +34,9 @@ namespace { Http::RegisterCustomInlineHeader authorization_handle(Http::CustomHeaders::get().Authorization); -// Deleted OauthHMAC cookie. -constexpr const char* SignoutCookieValue = +constexpr const char* CookieDeleteFormatString = "{}=deleted; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT"; -// Deleted BearerToken cookie. -constexpr const char* SignoutBearerTokenValue = - "{}=deleted; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT"; - -constexpr absl::string_view SignoutIdTokenValue = - "IdToken=deleted; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT"; - -constexpr absl::string_view SignoutRefreshTokenValue = - "RefreshToken=deleted; path=/; expires=Thu, 01 Jan 1970 00:00:00 GMT"; - constexpr const char* CookieTailFormatString = ";version=1;path=/;Max-Age={};secure"; constexpr const char* CookieTailHttpOnlyFormatString = @@ -189,7 +178,8 @@ void OAuth2CookieValidator::setParams(const Http::RequestHeaderMap& headers, const std::string& secret) { const auto& cookies = Http::Utility::parseCookies(headers, [this](absl::string_view key) -> bool { return key == cookie_names_.oauth_expires_ || key == cookie_names_.bearer_token_ || - key == cookie_names_.oauth_hmac_ || key == cookie_names_.id_token_; + key == cookie_names_.oauth_hmac_ || key == cookie_names_.id_token_ || + key == cookie_names_.refresh_token_; }); expires_ = findValue(cookies, cookie_names_.oauth_expires_); @@ -444,12 +434,16 @@ Http::FilterHeadersStatus OAuth2Filter::signOutUser(const Http::RequestHeaderMap const std::string new_path = absl::StrCat(headers.getSchemeValue(), "://", host_, "/"); response_headers->addReferenceKey( Http::Headers::get().SetCookie, - fmt::format(SignoutCookieValue, config_->cookieNames().oauth_hmac_)); + fmt::format(CookieDeleteFormatString, config_->cookieNames().oauth_hmac_)); response_headers->addReferenceKey( Http::Headers::get().SetCookie, - fmt::format(SignoutBearerTokenValue, config_->cookieNames().bearer_token_)); - response_headers->addReferenceKey(Http::Headers::get().SetCookie, SignoutIdTokenValue); - response_headers->addReferenceKey(Http::Headers::get().SetCookie, SignoutRefreshTokenValue); + fmt::format(CookieDeleteFormatString, config_->cookieNames().bearer_token_)); + response_headers->addReferenceKey( + Http::Headers::get().SetCookie, + fmt::format(CookieDeleteFormatString, config_->cookieNames().id_token_)); + response_headers->addReferenceKey( + Http::Headers::get().SetCookie, + fmt::format(CookieDeleteFormatString, config_->cookieNames().refresh_token_)); response_headers->setLocation(new_path); decoder_callbacks_->encodeHeaders(std::move(response_headers), true, SIGN_OUT); @@ -543,6 +537,7 @@ void OAuth2Filter::addResponseCookies(Http::ResponseHeaderMap& headers, } } } + void OAuth2Filter::sendUnauthorizedResponse() { config_->stats().oauth_failure_.inc(); decoder_callbacks_->sendLocalReply(Http::Code::Unauthorized, UnauthorizedBodyMessage, nullptr, diff --git a/source/extensions/filters/http/oauth2/filter.h b/source/extensions/filters/http/oauth2/filter.h index 46cc996d95d3..fc3eaab4f1d7 100644 --- a/source/extensions/filters/http/oauth2/filter.h +++ b/source/extensions/filters/http/oauth2/filter.h @@ -102,14 +102,17 @@ struct CookieNames { CookieNames(const envoy::extensions::filters::http::oauth2::v3::OAuth2Credentials::CookieNames& cookie_names) : CookieNames(cookie_names.bearer_token(), cookie_names.oauth_hmac(), - cookie_names.oauth_expires()) {} + cookie_names.oauth_expires(), cookie_names.id_token(), + cookie_names.refresh_token()) {} CookieNames(const std::string& bearer_token, const std::string& oauth_hmac, - const std::string& oauth_expires) + const std::string& oauth_expires, const std::string& id_token, + const std::string& refresh_token) : bearer_token_(bearer_token.empty() ? "BearerToken" : bearer_token), oauth_hmac_(oauth_hmac.empty() ? "OauthHMAC" : oauth_hmac), - oauth_expires_(oauth_expires.empty() ? "OauthExpires" : oauth_expires), id_token_(IdToken), - refresh_token_(RefreshToken) {} + oauth_expires_(oauth_expires.empty() ? OauthExpires : oauth_expires), + id_token_(id_token.empty() ? IdToken : id_token), + refresh_token_(refresh_token.empty() ? RefreshToken : refresh_token) {} const std::string bearer_token_; const std::string oauth_hmac_; @@ -117,6 +120,7 @@ struct CookieNames { const std::string id_token_; const std::string refresh_token_; + static constexpr absl::string_view OauthExpires = "OauthExpires"; static constexpr absl::string_view IdToken = "IdToken"; static constexpr absl::string_view RefreshToken = "RefreshToken"; }; diff --git a/test/extensions/filters/http/oauth2/config_test.cc b/test/extensions/filters/http/oauth2/config_test.cc index a6cbf3ac3a10..a44afe3fbd4a 100644 --- a/test/extensions/filters/http/oauth2/config_test.cc +++ b/test/extensions/filters/http/oauth2/config_test.cc @@ -91,6 +91,8 @@ TEST(ConfigTest, CreateFilter) { bearer_token: BearerToken oauth_hmac: OauthHMAC oauth_expires: OauthExpires + id_token: IdToken + refresh_token: RefreshToken authorization_endpoint: https://oauth.com/oauth/authorize/ redirect_uri: "%REQ(x-forwarded-proto)%://%REQ(:authority)%/callback" redirect_path_matcher: diff --git a/test/extensions/filters/http/oauth2/filter_test.cc b/test/extensions/filters/http/oauth2/filter_test.cc index d334ba2ee23f..8cf6b8125821 100644 --- a/test/extensions/filters/http/oauth2/filter_test.cc +++ b/test/extensions/filters/http/oauth2/filter_test.cc @@ -787,12 +787,14 @@ TEST_F(OAuth2Test, OAuthOptionsRequestAndContinue_oauth_header_passthrough_fix) // Validates the behavior of the cookie validator. TEST_F(OAuth2Test, CookieValidator) { - expectValidCookies(CookieNames{"BearerToken", "OauthHMAC", "OauthExpires"}); + expectValidCookies( + CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}); } // Validates the behavior of the cookie validator with custom cookie names. TEST_F(OAuth2Test, CookieValidatorWithCustomNames) { - expectValidCookies(CookieNames{"CustomBearerToken", "CustomOauthHMAC", "CustomOauthExpires"}); + expectValidCookies(CookieNames{"CustomBearerToken", "CustomOauthHMAC", "CustomOauthExpires", + "CustomIdToken", "CustomRefreshToken"}); } // Validates the behavior of the cookie validator when the expires_at value is not a valid integer. @@ -810,7 +812,8 @@ TEST_F(OAuth2Test, CookieValidatorInvalidExpiresAt) { }; auto cookie_validator = std::make_shared( - test_time_, CookieNames{"BearerToken", "OauthHMAC", "OauthExpires"}); + test_time_, + CookieNames{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", "RefreshToken"}); cookie_validator->setParams(request_headers, "mock-secret"); EXPECT_TRUE(cookie_validator->hmacIsValid()); @@ -1204,6 +1207,40 @@ TEST_F(OAuth2Test, OAuthTestFullFlowPostWithParameters) { filter_->finishGetAccessTokenFlow(); } +/** + * Testing oauth response after tokens are set. + * + * Expected behavior: cookies are set. + */ +TEST_F(OAuth2Test, OAuthAccessTokenSucessWithTokens) { + + // Set SystemTime to a fixed point so we get consistent HMAC encodings between test runs. + test_time_.setSystemTime(SystemTime(std::chrono::seconds(0))); + + // Expected response after the callback is complete. + Http::TestRequestHeaderMapImpl expected_headers{ + {Http::Headers::get().Status.get(), "302"}, + {Http::Headers::get().SetCookie.get(), + "OauthHMAC=" + "OTBhMzEwNjk4YzJiNjIxMTcwMTE0ZDE2NjUyNjIyNmI1YmE0Y2NhNTQ3ZWYwZGYzZTNhYjEwNzhmZmQyZGY4Zg==;" + "version=1;path=/;Max-Age=600;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), + "OauthExpires=600;version=1;path=/;Max-Age=600;secure;HttpOnly"}, + {Http::Headers::get().SetCookie.get(), + "BearerToken=access_code;version=1;path=/;Max-Age=600;secure"}, + {Http::Headers::get().SetCookie.get(), + "IdToken=some-id-token;version=1;path=/;Max-Age=600;secure"}, + {Http::Headers::get().SetCookie.get(), + "RefreshToken=some-refresh-token;version=1;path=/;Max-Age=600;secure"}, + {Http::Headers::get().Location.get(), ""}, + }; + + EXPECT_CALL(decoder_callbacks_, encodeHeaders_(HeaderMapEqualRef(&expected_headers), true)); + + filter_->onGetAccessTokenSuccess("access_code", "some-id-token", "some-refresh-token", + std::chrono::seconds(600)); +} + TEST_F(OAuth2Test, OAuthBearerTokenFlowFromHeader) { Http::TestRequestHeaderMapImpl request_headers{ {Http::Headers::get().Path.get(), "/test?role=bearer"}, diff --git a/test/extensions/filters/http/oauth2/oauth_integration_test.cc b/test/extensions/filters/http/oauth2/oauth_integration_test.cc index 5fbc0df6ee71..3d5c8196dd64 100644 --- a/test/extensions/filters/http/oauth2/oauth_integration_test.cc +++ b/test/extensions/filters/http/oauth2/oauth_integration_test.cc @@ -297,7 +297,8 @@ name: oauth codec_client_->close(); } - const CookieNames default_cookie_names_{"BearerToken", "OauthHMAC", "OauthExpires"}; + const CookieNames default_cookie_names_{"BearerToken", "OauthHMAC", "OauthExpires", "IdToken", + "RefreshToken"}; envoy::config::listener::v3::Listener listener_config_; std::string listener_name_{"http"}; FakeHttpConnectionPtr lds_connection_;