diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index e2a7c10912ec9..ae56b87d4190b 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -17,6 +17,8 @@ Bug Fixes --------- *Changes expected to improve the state of the world and are unlikely to have negative effects* +* jwt_authn: unauthorized responses now correctly include a `www-authenticate` header. + Removed Config or Runtime ------------------------- *Normally occurs at the end of the* :ref:`deprecation period ` diff --git a/source/common/http/utility.cc b/source/common/http/utility.cc index 2ee0b2f7fce34..efb04740b2527 100644 --- a/source/common/http/utility.cc +++ b/source/common/http/utility.cc @@ -751,6 +751,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 64d85613ee1a3..17ad96cbc724e 100644 --- a/source/common/http/utility.h +++ b/source/common/http/utility.h @@ -372,6 +372,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 32c3a5224aa50..9f3c8ca1e48f4 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 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 49cf81f341e27..b5565ae07a925 100644 --- a/test/common/http/utility_test.cc +++ b/test/common/http/utility_test.cc @@ -1296,6 +1296,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 c94dee161313b..316f204adfcd9 100644 --- a/test/common/tracing/http_tracer_impl_test.cc +++ b/test/common/tracing/http_tracer_impl_test.cc @@ -717,6 +717,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 8fb7f918e46a9..40bb22278a729 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) { 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) { @@ -158,6 +161,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) { @@ -177,6 +183,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()); } @@ -424,6 +434,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(); } @@ -444,7 +457,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(); }