diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index c300f0490dc70..cabb51af47995 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -15,6 +15,7 @@ Bug Fixes * 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. * overload: fix a bug that can cause use-after-free when one scaled timer disables another one with the same duration. +* jwt_authn: unauthorized responses now correctly include a `www-authenticate` header. Removed Config or Runtime ------------------------- diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index ff2cae86e2624..1c11e101303d5 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -775,6 +775,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 d677b097d86cb..492b7e7d47db3 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -371,6 +371,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 4b8e5e595fc98..eb6bd324de389 100644 --- a/source/common/tracing/http_tracer_impl.cc +++ b/source/common/tracing/http_tracer_impl.cc @@ -35,23 +35,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"; @@ -163,8 +146,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 343ebc1cbeff6..25a63bc109a14 100644 --- a/source/extensions/filters/http/jwt_authn/filter.cc +++ b/source/extensions/filters/http/jwt_authn/filter.cc @@ -16,7 +16,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 @@ -87,6 +88,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_); @@ -119,8 +121,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 732eb1ca913dd..c0316823d2d93 100644 --- a/source/extensions/filters/http/jwt_authn/filter.h +++ b/source/extensions/filters/http/jwt_authn/filter.h @@ -50,6 +50,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 1e51af4b5f99b..846da59adc136 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1245,6 +1245,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 b5a8a0df4970e..fedb3ca364e2f 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -739,6 +739,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())); } 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 bf0af7934ab3d..56aa1f158815f 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -23,7 +23,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 { @@ -140,6 +139,10 @@ TEST_P(LocalJwksIntegrationTest, ExpiredToken) { 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) { @@ -158,6 +161,9 @@ TEST_P(LocalJwksIntegrationTest, MissingToken) { 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) { @@ -177,6 +183,10 @@ TEST_P(LocalJwksIntegrationTest, ExpiredTokenHeadReply) { 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()); } @@ -424,6 +434,9 @@ TEST_P(RemoteJwksIntegrationTest, FetchFailedJwks) { 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(); } @@ -444,7 +457,9 @@ TEST_P(RemoteJwksIntegrationTest, FetchFailedMissingCluster) { 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(); }