Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d298dac
Adds the initial implementation of www-authenticate header
Apr 28, 2021
668491b
move uri code into http/common
Apr 28, 2021
2aa8dd9
return error field for processed tokens
May 3, 2021
6d2118a
Add testing for newly introduced headers
May 3, 2021
12d2c8e
fix format
May 3, 2021
f2a80f5
Modify the std::string to string_view and fix spelling error
May 7, 2021
b5681e9
Add singleton for WwwAuthenticate header
May 10, 2021
5d1dfd9
Remove extraneous header definition and use the existing one instead
May 10, 2021
25e4248
Merge remote-tracking branch 'upstream/main' into add_www_header
May 12, 2021
e2bed72
Merge remote-tracking branch 'upstream/main' into add_www_header
May 13, 2021
191c61c
Update source/common/http/utility.h
cyran-ryan May 19, 2021
7ee776f
Update source/common/http/utility.h
cyran-ryan May 19, 2021
84a311b
change const -> constexpr
May 19, 2021
a7f4854
fix return type
May 19, 2021
25c19f1
fix formatting issues
May 19, 2021
6b326ed
Merge remote-tracking branch 'upstream/main' into add_www_header
Jun 1, 2021
4ee3863
add release notes
Jun 1, 2021
89c8cb0
Fix static initalization issue
Jun 1, 2021
d2dd536
fix formatting issues
Jun 2, 2021
ef47927
Reduce concatnation of the error string
Jun 9, 2021
0b2c1a2
Move comma into error string as well
Jun 9, 2021
c9d5541
Merge remote-tracking branch 'upstream/main' into add_www_header
Jun 10, 2021
e4f1d82
Migrate the http tracing utility to use the new function and add unit…
Jun 16, 2021
5bed8e4
Add some test calls to increase code coverage
Jul 2, 2021
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
1 change: 1 addition & 0 deletions docs/root/version_history/current.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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.
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 @@ -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<uint32_t> max_path_length) {
if (!request_headers.Path()) {
return "";
}
absl::string_view path(request_headers.EnvoyOriginalPath()
? request_headers.getEnvoyOriginalPathValue()
: request_headers.getPathValue());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will we have case "envoy-origin-path" exists but not "path"?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears in the router config_impl, that if envoy-origin-path exists, it is set to the path value. So no, there should not be any possibility of envoy-origin-path existing but path not existing.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That code in the router_config runs after headers make it to the router, so I don't think this will have been set at this point. On the other hand I think the check for Path is unnecessary because getPathValue() should return an empty string in that case. Digging into it some more, original-path is only set in for certain route actions, so there might not be a point in reading it at this point at all?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping on this, I think we can simplify this logic


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 @@ -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<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 @@ -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";

Expand Down Expand Up @@ -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(), "-"));
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 @@ -14,7 +14,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 @@ -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_);
Expand Down Expand Up @@ -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();
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 @@ -49,6 +49,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 @@ -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));
Expand Down
12 changes: 12 additions & 0 deletions test/common/tracing/http_tracer_impl_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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()));
}
Expand Down Expand Up @@ -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
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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) {
Expand All @@ -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) {
Expand All @@ -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());
}
Expand Down Expand Up @@ -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();
}
Expand All @@ -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();
}

Expand Down