Skip to content
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

oauth2: set accept header on access_token request #14538

Merged
merged 6 commits into from
Jan 21, 2021
Merged

oauth2: set accept header on access_token request #14538

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
4d9fa70
Fix OAuth2 client not setting accept header
riptl Dec 29, 2020
7dc4095
Safer OAuth2 token request header test
riptl Dec 30, 2020
d09c10e
oauth2: comment explaining why we set Accept header
riptl Jan 11, 2021
9f718ed
Merge remote-tracking branch 'origin/master' into oauth2-accept-json
riptl Jan 11, 2021
8143105
check for access_token req content type header
riptl Jan 11, 2021
1104afa
check for access token request method
riptl Jan 11, 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
8 changes: 7 additions & 1 deletion source/extensions/filters/http/oauth2/oauth_client.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,13 @@ class OAuth2ClientImpl : public OAuth2Client, Logger::Loggable<Logger::Id::upstr
Http::RequestMessagePtr createPostRequest() {
auto request = Http::Utility::prepareHeaders(uri_);
request->headers().setReferenceMethod(Http::Headers::get().MethodValues.Post);
request->headers().setContentType(Http::Headers::get().ContentTypeValues.FormUrlEncoded);
request->headers().setReferenceContentType(
Http::Headers::get().ContentTypeValues.FormUrlEncoded);
// Use the Accept header to ensure the Access Token Response is returned as JSON.
// Some authorization servers return other encodings (e.g. FormUrlEncoded) in the absence of the
// Accept header. RFC 6749 Section 5.1 defines the media type to be JSON, so this is safe.
request->headers().setReference(Http::CustomHeaders::get().Accept,
Http::Headers::get().ContentTypeValues.Json);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm do we want to make this configurable but on by default?

cc: @williamsfu99

Copy link
Member

Choose a reason for hiding this comment

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

cc: @jparise

Copy link
Member

@dio dio Jan 3, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

As a note, the current client expects (to accept/parse) JSON payload from the server:

envoy/source/extensions/filters/http/oauth2/oauth_client.cc

Line 81 in 4d9fa70

MessageUtil::loadFromJson(response_body, response, ProtobufMessage::getNullValidationVisitor());

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm do we want to make this configurable but on by default?

@rgs1 If Envoy is aiming for strict RFC 6749 compliance, I don't think it needs to be configurable. I'm not aware of a version of the OAuth 2.0 standard that allows FormUrlEncoded access token responses but I don't have much experience with OAuth to be honest.

https://tools.ietf.org/html/rfc6749#section-4.1.4 states:

If the access token request is valid and authorized, the
authorization server issues an access token and optional refresh
token as described in Section 5.1.

https://tools.ietf.org/html/rfc6749#section-5.1 states:

The authorization server issues an access token and optional refresh token, and constructs the response by adding the following parameters to the entity-body of the HTTP response with a 200 (OK) status code:
[ ... ]
The parameters are included in the entity-body of the HTTP response using the "application/json" media type as defined by [RFC4627].

I just noticed this also means Envoy's code was never really wrong to begin with. It's reasonable to expect a JSON response if the RFC says so, but of course setting the Accept header is nicer. GitHub just happened to violate the RFC by defaulting to FormUrlEncoded instead of JSON.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks for the reply. Mind adding a summary of the above with those links into the code as a comment?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rgs1 Done

return request;
}
};
Expand Down
20 changes: 14 additions & 6 deletions test/extensions/filters/http/oauth2/oauth_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,12 +79,20 @@ TEST_F(OAuth2ClientTest, RequestAccessTokenSuccess) {
mock_response->body().add(json);

EXPECT_CALL(cm_.thread_local_cluster_.async_client_, send_(_, _, _))
.WillRepeatedly(
Invoke([&](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks& cb,
const Http::AsyncClient::RequestOptions&) -> Http::AsyncClient::Request* {
callbacks_.push_back(&cb);
return &request_;
}));
.WillRepeatedly(Invoke([&](Http::RequestMessagePtr& message, Http::AsyncClient::Callbacks& cb,
const Http::AsyncClient::RequestOptions&)
-> Http::AsyncClient::Request* {
EXPECT_EQ(Http::Headers::get().MethodValues.Post,
message->headers().Method()->value().getStringView());
EXPECT_EQ(Http::Headers::get().ContentTypeValues.FormUrlEncoded,
message->headers().ContentType()->value().getStringView());
EXPECT_TRUE(
!message->headers().get(Http::CustomHeaders::get().Accept).empty() &&
message->headers().get(Http::CustomHeaders::get().Accept)[0]->value().getStringView() ==
Http::Headers::get().ContentTypeValues.Json);
callbacks_.push_back(&cb);
return &request_;
}));

client_->setCallbacks(*mock_callbacks_);
client_->asyncGetAccessToken("a", "b", "c", "d");
Expand Down