Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,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 <deprecated>`
Expand Down
17 changes: 17 additions & 0 deletions source/common/http/utility.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint32_t> 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) {
/**
Expand Down
9 changes: 9 additions & 0 deletions source/common/http/utility.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<uint32_t> 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
Expand Down
22 changes: 3 additions & 19 deletions source/common/tracing/http_tracer_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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";

Expand Down Expand Up @@ -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(), "-"));
Expand Down
15 changes: 12 additions & 3 deletions source/extensions/filters/http/jwt_authn/filter.cc
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@ namespace HttpFilters {
namespace JwtAuthn {

namespace {

constexpr absl::string_view InvalidTokenErrorString = ", error=\"invalid_token\"";
constexpr uint32_t MaximumUriLength = 256;
Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
access_control_request_method_handle(Http::CustomHeaders::get().AccessControlRequestMethod);
Http::RegisterCustomInlineHeader<Http::CustomInlineHeaderRegistry::Type::RequestHeaders>
Expand Down Expand Up @@ -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_);
Expand Down Expand Up @@ -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();
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/http/jwt_authn/filter.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ class Filter : public Http::StreamDecoderFilter,
FilterConfigSharedPtr config_;
// Verify context for current request.
ContextSharedPtr context_;

std::string original_uri_;
};

} // namespace JwtAuthn
Expand Down
25 changes: 25 additions & 0 deletions test/common/http/utility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
1 change: 1 addition & 0 deletions test/common/tracing/http_tracer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Expand Down
19 changes: 17 additions & 2 deletions test/extensions/filters/http/jwt_authn/filter_integration_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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());
}
Expand Down Expand Up @@ -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();
}
Expand All @@ -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();
}

Expand Down