diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 0d67d7b220380..bcb576b4e5df4 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -47,6 +47,7 @@ Bug Fixes * hot_restart: fix double counting of `server.seconds_until_first_ocsp_response_expiring` and `server.days_until_first_cert_expiring` during hot-restart. This stat was only incorrect until the parent process terminated. * http: port stripping now works for CONNECT requests, though the port will be restored if the CONNECT request is sent upstream. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.strip_port_from_connect`` to false. * http: raise max configurable max_request_headers_kb limit to 8192 KiB (8MiB) from 96 KiB in http connection manager. +* jwt_authn: unauthorized responses now correctly include a `www-authenticate` header. * listener: fix the crash which could happen when the ongoing filter chain only listener update is followed by the listener removal or full listener update. * udp: limit each UDP listener to read maxmium 6000 packets per event loop. This behavior can be temporarily reverted by setting ``envoy.reloadable_features.udp_per_event_loop_read_limit`` to false. * validation: fix an issue that causes TAP sockets to panic during config validation mode. diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 58ac1b6015f50..be776112dd26d 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -761,6 +761,23 @@ const std::string& Utility::getProtocolString(const Protocol protocol) { NOT_REACHED_GCOVR_EXCL_LINE; } +std::string Utility::buildOriginalUri(const Http::RequestHeaderMap& request_headers, + const absl::optional max_path_length) { + if (!request_headers.Path()) { + return ""; + } + absl::string_view path(request_headers.EnvoyOriginalPath() + ? request_headers.getEnvoyOriginalPathValue() + : request_headers.getPathValue()); + + if (max_path_length.has_value() && path.length() > max_path_length) { + path = path.substr(0, max_path_length.value()); + } + + return absl::StrCat(request_headers.getForwardedProtoValue(), "://", + request_headers.getHostValue(), path); +} + void Utility::extractHostPathFromUri(const absl::string_view& uri, absl::string_view& host, absl::string_view& path) { /** diff --git a/source/common/http/utility.h b/source/common/http/utility.h index 0cf5f612eb367..bab488ab2c0ce 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -388,6 +388,15 @@ bool sanitizeConnectionHeader(Http::RequestHeaderMap& headers); */ const std::string& getProtocolString(const Protocol p); +/** + * Constructs the original URI sent from the client from + * the request headers. + * @param request headers from the original request + * @param length to truncate the constructed URI's path + */ +std::string buildOriginalUri(const Http::RequestHeaderMap& request_headers, + absl::optional max_path_length); + /** * Extract host and path from a URI. The host may contain port. * This function doesn't validate if the URI is valid. It only parses the URI with following diff --git a/source/common/tracing/http_tracer_impl.cc b/source/common/tracing/http_tracer_impl.cc index 12aeace932b89..d478cdd14eb4f 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -36,23 +36,6 @@ static absl::string_view valueOrDefault(const Http::HeaderEntry* header, return header ? header->value().getStringView() : default_value; } -static std::string buildUrl(const Http::RequestHeaderMap& request_headers, - const uint32_t max_path_length) { - if (!request_headers.Path()) { - return ""; - } - absl::string_view path(request_headers.EnvoyOriginalPath() - ? request_headers.getEnvoyOriginalPathValue() - : request_headers.getPathValue()); - - if (path.length() > max_path_length) { - path = path.substr(0, max_path_length); - } - - return absl::StrCat(request_headers.getForwardedProtoValue(), "://", - request_headers.getHostValue(), path); -} - const std::string HttpTracerUtility::IngressOperation = "ingress"; const std::string HttpTracerUtility::EgressOperation = "egress"; @@ -159,8 +142,9 @@ void HttpTracerUtility::finalizeDownstreamSpan(Span& span, if (request_headers->RequestId()) { span.setTag(Tracing::Tags::get().GuidXRequestId, request_headers->getRequestIdValue()); } - span.setTag(Tracing::Tags::get().HttpUrl, - buildUrl(*request_headers, tracing_config.maxPathTagLength())); + span.setTag( + Tracing::Tags::get().HttpUrl, + Http::Utility::buildOriginalUri(*request_headers, tracing_config.maxPathTagLength())); span.setTag(Tracing::Tags::get().HttpMethod, request_headers->getMethodValue()); span.setTag(Tracing::Tags::get().DownstreamCluster, valueOrDefault(request_headers->EnvoyDownstreamServiceCluster(), "-")); diff --git a/source/extensions/filters/http/jwt_authn/filter.cc b/source/extensions/filters/http/jwt_authn/filter.cc index 14711ad2c80ff..c069e3a87b8d6 100644 --- a/source/extensions/filters/http/jwt_authn/filter.cc +++ b/source/extensions/filters/http/jwt_authn/filter.cc @@ -14,7 +14,8 @@ namespace HttpFilters { namespace JwtAuthn { namespace { - +constexpr absl::string_view InvalidTokenErrorString = ", error=\"invalid_token\""; +constexpr uint32_t MaximumUriLength = 256; Http::RegisterCustomInlineHeader access_control_request_method_handle(Http::CustomHeaders::get().AccessControlRequestMethod); Http::RegisterCustomInlineHeader @@ -85,6 +86,7 @@ Http::FilterHeadersStatus Filter::decodeHeaders(Http::RequestHeaderMap& headers, if (verifier == nullptr) { onComplete(Status::Ok); } else { + original_uri_ = Http::Utility::buildOriginalUri(headers, MaximumUriLength); // Verify the JWT token, onComplete() will be called when completed. context_ = Verifier::createContext(headers, decoder_callbacks_->activeSpan(), this); verifier->verify(context_); @@ -117,8 +119,15 @@ void Filter::onComplete(const Status& status) { status == Status::JwtAudienceNotAllowed ? Http::Code::Forbidden : Http::Code::Unauthorized; // return failure reason as message body decoder_callbacks_->sendLocalReply( - code, ::google::jwt_verify::getStatusString(status), nullptr, absl::nullopt, - generateRcDetails(::google::jwt_verify::getStatusString(status))); + code, ::google::jwt_verify::getStatusString(status), + [uri = this->original_uri_, status](Http::ResponseHeaderMap& headers) { + std::string value = absl::StrCat("Bearer realm=\"", uri, "\""); + if (status != Status::JwtMissed) { + absl::StrAppend(&value, InvalidTokenErrorString); + } + headers.setCopy(Http::Headers::get().WWWAuthenticate, value); + }, + absl::nullopt, generateRcDetails(::google::jwt_verify::getStatusString(status))); return; } stats_.allowed_.inc(); diff --git a/source/extensions/filters/http/jwt_authn/filter.h b/source/extensions/filters/http/jwt_authn/filter.h index f4d95f04d9004..e743324f8040a 100644 --- a/source/extensions/filters/http/jwt_authn/filter.h +++ b/source/extensions/filters/http/jwt_authn/filter.h @@ -49,6 +49,8 @@ class Filter : public Http::StreamDecoderFilter, FilterConfigSharedPtr config_; // Verify context for current request. ContextSharedPtr context_; + + std::string original_uri_; }; } // namespace JwtAuthn diff --git a/test/common/http/utility_test.cc b/test/common/http/utility_test.cc index 32149b467c395..9100dbb0b319b 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1313,6 +1313,31 @@ TEST(HttpUtility, TestRejectTeHeaderTooLong) { EXPECT_EQ(sanitized_headers, request_headers); } +TEST(HttpUtility, TestRejectUriWithNoPath) { + Http::TestRequestHeaderMapImpl request_headers_no_path = { + {":method", "GET"}, {":authority", "example.com"}, {"x-forwarded-proto", "http"}}; + EXPECT_EQ(Utility::buildOriginalUri(request_headers_no_path, {}), ""); +} + +TEST(HttpUtility, TestTruncateUri) { + Http::TestRequestHeaderMapImpl request_headers_truncated_path = {{":method", "GET"}, + {":path", "/hello_world"}, + {":authority", "example.com"}, + {"x-forwarded-proto", "http"}}; + EXPECT_EQ(Utility::buildOriginalUri(request_headers_truncated_path, 2), "http://example.com/h"); +} + +TEST(HttpUtility, TestUriUsesOriginalPath) { + Http::TestRequestHeaderMapImpl request_headers_truncated_path = { + {":method", "GET"}, + {":path", "/hello_world"}, + {":authority", "example.com"}, + {"x-forwarded-proto", "http"}, + {"x-envoy-original-path", "/goodbye_world"}}; + EXPECT_EQ(Utility::buildOriginalUri(request_headers_truncated_path, {}), + "http://example.com/goodbye_world"); +} + TEST(Url, ParsingFails) { Utility::Url url; EXPECT_FALSE(url.initialize("", false)); diff --git a/test/common/tracing/http_tracer_impl_test.cc b/test/common/tracing/http_tracer_impl_test.cc index 256270dffe70a..e3289d042f3f7 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -727,6 +727,7 @@ TEST(HttpNullTracerTest, BasicFunctionality) { ASSERT_EQ("", span_ptr->getBaggage("baggage_key")); ASSERT_EQ(span_ptr->getTraceIdAsHex(), ""); span_ptr->injectContext(request_headers); + span_ptr->log(SystemTime(), "fake_event"); EXPECT_NE(nullptr, span_ptr->spawnChild(config, "foo", SystemTime())); } @@ -829,6 +830,17 @@ TEST_F(HttpTracerImplTest, ChildUpstreamSpanTest) { stream_info_, config_); } +TEST_F(HttpTracerImplTest, MetadataCustomTagReturnsDefaultValue) { + envoy::type::tracing::v3::CustomTag::Metadata testing_metadata; + testing_metadata.mutable_metadata_key()->set_key("key"); + *testing_metadata.mutable_default_value() = "default_value"; + MetadataCustomTag tag("testing", testing_metadata); + StreamInfo::MockStreamInfo testing_info_; + Http::TestRequestHeaderMapImpl header_map_; + CustomTagContext context{&header_map_, testing_info_}; + EXPECT_EQ(tag.value(context), "default_value"); +} + } // namespace } // namespace Tracing } // namespace Envoy diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index 59b937dd8095f..9ea2b6a372409 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -21,7 +21,6 @@ namespace JwtAuthn { namespace { const char HeaderToFilterStateFilterName[] = "envoy.filters.http.header_to_filter_state_for_test"; - // This filter extracts a string header from "header" and // save it into FilterState as name "state" as read-only Router::StringAccessor. class HeaderToFilterStateFilter : public Http::PassThroughDecoderFilter { @@ -152,6 +151,10 @@ TEST_P(LocalJwksIntegrationTest, ExpiredToken) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); + EXPECT_EQ(1, response->headers().get(Http::Headers::get().WWWAuthenticate).size()); + EXPECT_EQ( + "Bearer realm=\"http://host/\", error=\"invalid_token\"", + response->headers().get(Http::Headers::get().WWWAuthenticate)[0]->value().getStringView()); } TEST_P(LocalJwksIntegrationTest, MissingToken) { @@ -170,6 +173,9 @@ TEST_P(LocalJwksIntegrationTest, MissingToken) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); + EXPECT_EQ( + "Bearer realm=\"http://host/\"", + response->headers().get(Http::Headers::get().WWWAuthenticate)[0]->value().getStringView()); } TEST_P(LocalJwksIntegrationTest, ExpiredTokenHeadReply) { @@ -189,6 +195,10 @@ TEST_P(LocalJwksIntegrationTest, ExpiredTokenHeadReply) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); + EXPECT_EQ( + "Bearer realm=\"http://host/\", error=\"invalid_token\"", + response->headers().get(Http::Headers::get().WWWAuthenticate)[0]->value().getStringView()); + EXPECT_NE("0", response->headers().getContentLengthValue()); EXPECT_THAT(response->body(), ::testing::IsEmpty()); } @@ -448,6 +458,9 @@ TEST_P(RemoteJwksIntegrationTest, FetchFailedJwks) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); + EXPECT_EQ( + "Bearer realm=\"http://host/\", error=\"invalid_token\"", + response->headers().get(Http::Headers::get().WWWAuthenticate)[0]->value().getStringView()); cleanup(); } @@ -468,7 +481,9 @@ TEST_P(RemoteJwksIntegrationTest, FetchFailedMissingCluster) { ASSERT_TRUE(response->waitForEndStream()); ASSERT_TRUE(response->complete()); EXPECT_EQ("401", response->headers().getStatusValue()); - + EXPECT_EQ( + "Bearer realm=\"http://host/\", error=\"invalid_token\"", + response->headers().get(Http::Headers::get().WWWAuthenticate)[0]->value().getStringView()); cleanup(); }