From 91dc01d36b698ffdbb55b252bdc1de7804f82cfa Mon Sep 17 00:00:00 2001 From: Oliver Liu Date: Fri, 26 Feb 2021 10:27:37 -0800 Subject: [PATCH 1/3] jwt_authn: fix a bug where JWT with wrong issuer is allowed in allow_missing case --- docs/root/version_history/current.rst | 10 ++++++++++ .../extensions/filters/http/jwt_authn/verifier.cc | 15 +++++++-------- .../filters/http/jwt_authn/all_verifier_test.cc | 13 +++++++++++-- .../filters/http/jwt_authn/group_verifier_test.cc | 6 +++--- 4 files changed, 31 insertions(+), 13 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0a1de40e6e03d..1af71a190c5b5 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -13,6 +13,16 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. +* buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1. +* fault injection: stop counting as active fault after delay elapsed. Previously fault injection filter continues to count the injected delay as an active fault even after it has elapsed. This produces incorrect output statistics and impacts the max number of consecutive faults allowed (e.g., for long-lived streams). This change decreases the active fault count when the delay fault is the only active and has gone finished. +* filter_chain: fix filter chain matching with the server name as the case-insensitive way. +* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false. +* grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream. +* http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. +* http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. +* jwt_authn: reject requests with a proper error if JWT has the wrong issuer when allow_missing is used. Before this change, the requests are accepted. +* listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled. * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration. Removed Config or Runtime diff --git a/source/extensions/filters/http/jwt_authn/verifier.cc b/source/extensions/filters/http/jwt_authn/verifier.cc index cb920c1a5477c..83361c8edd7b8 100644 --- a/source/extensions/filters/http/jwt_authn/verifier.cc +++ b/source/extensions/filters/http/jwt_authn/verifier.cc @@ -301,17 +301,16 @@ class AnyVerifierImpl : public BaseGroupVerifierImpl { // Then wait for all children to be done. if (++completion_state.number_completed_children_ == verifiers_.size()) { // Aggregate all children status into a final status. - // JwtMissing should be treated differently than other failure status - // since it simply means there is not Jwt token for the required provider. - // If there is a failure status other than JwtMissing in the children, - // it should be used as the final status. + // JwtMissed and JwtUnknownIssuer should be treated differently than other errors. + // JwtMissed means not Jwt token for the required provider. + // JwtUnknownIssuer means wrong issuer for the required provider. Status final_status = Status::JwtMissed; for (const auto& it : verifiers_) { - // If a Jwt is extracted from a location not specified by the required provider, - // the authenticator returns JwtUnknownIssuer. It should be treated the same as - // JwtMissed. + // Prefer errors which are not JwtMissed nor JwtUnknownIssuer. + // Prefer JwtUnknownIssuer between JwtMissed and JwtUnknownIssuer. Status child_status = context.getCompletionState(it.get()).status_; - if (child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) { + if ((child_status != Status::JwtMissed && child_status != Status::JwtUnknownIssuer) || + final_status == Status::JwtMissed) { final_status = child_status; } } diff --git a/test/extensions/filters/http/jwt_authn/all_verifier_test.cc b/test/extensions/filters/http/jwt_authn/all_verifier_test.cc index 08cea40b92513..ba198bb922a5f 100644 --- a/test/extensions/filters/http/jwt_authn/all_verifier_test.cc +++ b/test/extensions/filters/http/jwt_authn/all_verifier_test.cc @@ -197,7 +197,7 @@ TEST_F(SingleAllowMissingInOrListTest, BadJwt) { } TEST_F(SingleAllowMissingInOrListTest, MissingIssToken) { - EXPECT_CALL(mock_cb_, onComplete(Status::Ok)); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, ES256WithoutIssToken}}; context_ = Verifier::createContext(headers, parent_span_, &mock_cb_); verifier_->verify(context_); @@ -471,6 +471,15 @@ TEST_F(AllowMissingInOrListTest, OtherGoodJwt) { EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kOtherHeader)); } +TEST_F(AllowMissingInOrListTest, WrongIssuer) { + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); + auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, OtherGoodToken}}; + context_ = Verifier::createContext(headers, parent_span_, &mock_cb_); + verifier_->verify(context_); + // x-other JWT should be ignored. + EXPECT_THAT(headers, JwtOutputFailedOrIgnore(kOtherHeader)); +} + TEST_F(AllowMissingInOrListTest, BadAndGoodJwts) { EXPECT_CALL(mock_cb_, onComplete(Status::JwtVerificationFail)); auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, NonExistKidToken}, @@ -589,7 +598,7 @@ TEST_F(AllowMissingInAndOfOrListTest, TwoGoodJwts) { } TEST_F(AllowMissingInAndOfOrListTest, GoodAndBadJwts) { - EXPECT_CALL(mock_cb_, onComplete(Status::Ok)); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); // Use the token with example.com issuer for x-other. auto headers = Http::TestRequestHeaderMapImpl{{kExampleHeader, GoodToken}, {kOtherHeader, GoodToken}}; diff --git a/test/extensions/filters/http/jwt_authn/group_verifier_test.cc b/test/extensions/filters/http/jwt_authn/group_verifier_test.cc index ab14d6243cb09..0ccbe3aac770f 100644 --- a/test/extensions/filters/http/jwt_authn/group_verifier_test.cc +++ b/test/extensions/filters/http/jwt_authn/group_verifier_test.cc @@ -582,8 +582,8 @@ TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButFailed) { callbacks_["other_provider"](Status::JwtExpired); } -// Test RequiresAny with two providers and allow_missing, but OK -TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButOk) { +// Test RequiresAny with two providers and allow_missing, but one returns JwtUnknownIssuer +TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButUnknownIssuer) { TestUtility::loadFromYaml(RequiresAnyConfig, proto_config_); proto_config_.mutable_rules(0) ->mutable_requires() @@ -592,7 +592,7 @@ TEST_F(GroupVerifierTest, TestRequiresAnyWithAllowMissingButOk) { ->mutable_allow_missing(); createAsyncMockAuthsAndVerifier(std::vector{"example_provider", "other_provider"}); - EXPECT_CALL(mock_cb_, onComplete(Status::Ok)); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtUnknownIssuer)); auto headers = Http::TestRequestHeaderMapImpl{}; context_ = Verifier::createContext(headers, parent_span_, &mock_cb_); From b430c831e0f016196308ff2d8faeaafa9299aa03 Mon Sep 17 00:00:00 2001 From: Oliver Liu Date: Fri, 26 Feb 2021 10:41:21 -0800 Subject: [PATCH 2/3] Small fix --- docs/root/version_history/current.rst | 9 --------- 1 file changed, 9 deletions(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 1af71a190c5b5..d535a7849048a 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -13,16 +13,7 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* -* active http health checks: properly handles HTTP/2 GOAWAY frames from the upstream. Previously a GOAWAY frame due to a graceful listener drain could cause improper failed health checks due to streams being refused by the upstream on a connection that is going away. To revert to old GOAWAY handling behavior, set the runtime feature `envoy.reloadable_features.health_check.graceful_goaway_handling` to false. -* buffer: tighten network connection read and write buffer high watermarks in preparation to more careful enforcement of read limits. Buffer high-watermark is now set to the exact configured value; previously it was set to value + 1. -* fault injection: stop counting as active fault after delay elapsed. Previously fault injection filter continues to count the injected delay as an active fault even after it has elapsed. This produces incorrect output statistics and impacts the max number of consecutive faults allowed (e.g., for long-lived streams). This change decreases the active fault count when the delay fault is the only active and has gone finished. -* filter_chain: fix filter chain matching with the server name as the case-insensitive way. -* grpc-web: fix local reply and non-proto-encoded gRPC response handling for small response bodies. This fix can be temporarily reverted by setting `envoy.reloadable_features.grpc_web_fix_non_proto_encoded_response_handling` to false. -* grpc_http_bridge: the downstream HTTP status is now correctly set for trailers-only responses from the upstream. -* http: disallowing "host:" in request_headers_to_add for behavioral consistency with rejecting :authority header. This behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_host_like_authority` to false. -* http: reverting a behavioral change where upstream connect timeouts were temporarily treated differently from other connection failures. The change back to the original behavior can be temporarily reverted by setting `envoy.reloadable_features.treat_upstream_connect_timeout_as_connect_failure` to false. * jwt_authn: reject requests with a proper error if JWT has the wrong issuer when allow_missing is used. Before this change, the requests are accepted. -* listener: prevent crashing when an unknown listener config proto is received and debug logging is enabled. * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration. Removed Config or Runtime From b45ff34ccc3276c77dbd6a30e9f6101ef6f65bff Mon Sep 17 00:00:00 2001 From: Oliver Liu Date: Fri, 26 Feb 2021 10:49:21 -0800 Subject: [PATCH 3/3] Small fix. --- VERSION | 2 +- docs/root/version_history/current.rst | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/VERSION b/VERSION index 3cf57a7b61c64..511a76e6faf83 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.17.1-dev +1.17.1 diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index d535a7849048a..15c5b4b5ce9e9 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -1,5 +1,5 @@ -1.17.1 (Pending) -================ +1.17.1 (February 25, 2021) +========================== Incompatible Behavior Changes -----------------------------