Skip to content

jwt_authn: implementation of www-authenticate header #16216

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
snowp merged 24 commits into from
Jul 7, 2021
Merged

jwt_authn: implementation of www-authenticate header #16216

Show file tree
Hide file tree
Changes from 8 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
17 changes: 17 additions & 0 deletions source/common/http/utility.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -751,6 +751,23 @@ const std::string& Utility::getProtocolString(const Protocol protocol) {
NOT_REACHED_GCOVR_EXCL_LINE;
}

const std::string Utility::buildOriginalUri(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());

@qiwzhang qiwzhang May 3, 2021

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"?

@cyran-ryan cyran-ryan May 4, 2021

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.

@snowp snowp Jun 9, 2021

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?

@snowp snowp Jun 14, 2021

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 (path.length() > max_path_length) {
path = path.substr(0, max_path_length);

@snowp snowp Jun 14, 2021

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.

What happens in practice if we truncate the path? Will consumers handle this gracefully? Maybe we should increment some stat at least to indicate that this is happening?

@cyran-ryan cyran-ryan Jun 14, 2021

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.

So specifically, this code is based on how the http_tracing filter gets the original URL back from the request headers:

envoy/source/common/tracing/http_tracer_impl.cc

Line 39 in 99ff096

static std::string buildUrl(const Http::RequestHeaderMap& request_headers,

I'm fine with simplifying it if we understand the reduction in functionality we get by removing the extra checks. However, since this is in the common library and while this filter uses it in a way that the getEnvoyOriginalPath() check is not necessary, it may be necessary in the general case.

Truncating the path would return an "incomplete" URI. There's no defined length limit for URIs in the HTTP standard, though in practice most browser will balk at anything over 2048 characters. Consumers could set an arbitrarily high max_path_length or perhaps a flag could be added to say, I do not want truncation, if it matters to consumers. I do agree it would be nice to surface the fact that the truncation happened,

@snowp snowp Jun 14, 2021

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.

If we're copying the implementation from another place to put it into common code, can we have that other location also use this shared code? The jwt filter will never see the original host header at the time this is called due to the call to buildUrl being in decodeHeaders, but if this is being used by other things I agree we should be incorporating the original host header (and ideally document this behavior as part of the docstring for this function).

How about we make the trunaction flag optional (absl::optional<uint32_t>) and don't do truncation in this case unless required? Then the tracer lib can call this and preserve the trunacation behavior there while this new usage doesn't truncate until we know that this is necessary

@snowp snowp Jun 14, 2021

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.

Oh and I would love to see unit tests testing this helper directly instead of via extensions, e.g. something in test/common/http/utility_test

@cyran-ryan cyran-ryan Jun 15, 2021

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.

I agree on all points! I'll be working on this today

}

return absl::StrCat(request_headers.getForwardedProtoValue(), "://",
Comment thread
lizan marked this conversation as resolved.
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
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
lizan marked this conversation as resolved.
* @param request headers from the original request
* @param length to truncate the constructed URI's path
*/
const std::string buildOriginalUri(const Http::RequestHeaderMap& request_headers,
Comment thread
cyran-ryan marked this conversation as resolved.
Outdated
const uint32_t max_path_length);
Comment thread
cyran-ryan marked this conversation as resolved.
Outdated

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

namespace {

const absl::string_view InvalidTokenErrorString = "invalid_token";
const uint32_t MaximumUriLength = 256;

@lizan lizan May 18, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

constexpr

@cyran-ryan cyran-ryan May 24, 2021

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.

Done

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, ", error=\"", InvalidTokenErrorString, "\"");

@snowp snowp Jun 9, 2021

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.

Maybe make InvalidTokenErroString contain the full error= string so that we don't have to concat this every time?

@cyran-ryan cyran-ryan Jun 9, 2021

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.

We can do that, but we'll still need to append a ", " to the value before the InvalidTokenErrorString

@cyran-ryan cyran-ryan Jun 9, 2021

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.

Wait, scratch that. I see what you mean now.

}
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
View file
Open in desktop
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
15 changes: 13 additions & 2 deletions test/extensions/filters/http/jwt_authn/filter_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ namespace JwtAuthn {
namespace {

const char HeaderToFilterStateFilterName[] = "envoy.filters.http.header_to_filter_state_for_test";

const Http::LowerCaseString WwwAuthenticateHeader("www-authenticate");

@lizan lizan May 27, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This will lead static-initialization fiasco hence flaky test, use a function with CONSTRUCT_ON_FIRST_USE.

@cyran-ryan cyran-ryan Jun 1, 2021

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.

Swapped it over to the same value we use in the filter itself

// 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 +140,9 @@ TEST_P(LocalJwksIntegrationTest, ExpiredToken) {
ASSERT_TRUE(response->waitForEndStream());
ASSERT_TRUE(response->complete());
EXPECT_EQ("401", response->headers().getStatusValue());
EXPECT_EQ(1, response->headers().get(WwwAuthenticateHeader).size());
EXPECT_EQ("Bearer realm=\"http://host/\", error=\"invalid_token\"",
response->headers().get(WwwAuthenticateHeader)[0]->value().getStringView());
}

TEST_P(LocalJwksIntegrationTest, MissingToken) {
Expand All @@ -158,6 +161,8 @@ 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(WwwAuthenticateHeader)[0]->value().getStringView());
}

TEST_P(LocalJwksIntegrationTest, ExpiredTokenHeadReply) {
Expand All @@ -177,6 +182,9 @@ 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(WwwAuthenticateHeader)[0]->value().getStringView());

EXPECT_NE("0", response->headers().getContentLengthValue());
EXPECT_THAT(response->body(), ::testing::IsEmpty());
}
Expand Down Expand Up @@ -424,6 +432,8 @@ 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(WwwAuthenticateHeader)[0]->value().getStringView());

cleanup();
}
Expand All @@ -444,7 +454,8 @@ 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(WwwAuthenticateHeader)[0]->value().getStringView());
cleanup();
}

Expand Down