Oauth2 resource param by sudeeptoroy · Pull Request #15147 · envoyproxy/envoy

sudeeptoroy · 2021-02-23T07:23:11Z

Commit Message: added support for resource param in authorization request
Risk Level: low
Testing: unit test added
Docs Changes: added
Release Notes: na
Platform Specific Features: na
Fixes #15124

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

repokitteh-read-only · 2021-02-23T07:23:18Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #15147 was opened by sudeeptoroy.

see: more, trace.

Fixed a compilation with test cases Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com> Signed-off-by: sudeeptoroy <sudeeptoroy@users.noreply.github.com>

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

moderation · 2021-02-25T19:55:18Z

Oauth2 codeowners @rgs1 @derekargueta @snowp

rgs1 · 2021-02-25T19:56:37Z

cc: @williamsfu99

snowp · 2021-02-25T21:28:13Z

api/envoy/extensions/filters/http/oauth2/v3alpha/oauth.proto

+
+  // Optional resource parameter for authorization request
+  // RFC: https://tools.ietf.org/html/rfc8707
+  string resource = 10;


The RFC seems to allow multiple values here, should this be a repeated field?

Multiple "resource" parameters MAY be used to indicate that the requested token is intended to be used at multiple resources.

@snowp I will convert this to a repeated field.

@snowp Its up for review.

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

mattklein123 · 2021-03-01T18:07:47Z

/api lgtm

mattklein123 · 2021-03-01T18:09:14Z

/lgtm api

snowp · 2021-03-01T18:29:42Z

source/extensions/filters/http/oauth2/filter.cc

+    std::string h1 = "http://";
+    std::string h2 = "https://";
+
+    for (const auto& resource : resources_protos) {
+      if (resource.rfind(h1, 0) == 0 || resource.rfind(h2, 0) == 0) {


Can we extract the URL detection logic into its own function?

Currently this allocates two strings every time we do this which seems unnecessary. How about we rewrite this as
absl::StartsWith(resource, "http://") || absl::StartsWith(resource, "https://")?

snowp · 2021-03-01T18:30:14Z

source/extensions/filters/http/oauth2/filter.cc

+std::string encodeResourceList(const Protobuf::RepeatedPtrField<std::string>& resources_protos) {
+  std::string result = "";
+
+  if (!resources_protos.empty()) {


no need for this since in the empty case the for loop would terminate immediately

snowp · 2021-03-01T18:31:03Z

source/extensions/filters/http/oauth2/filter.h

  std::string tokenSecret() const { return secret_reader_->tokenSecret(); }
  FilterStats& stats() { return stats_; }
  const std::string& encodedAuthScopes() const { return encoded_auth_scopes_; }
+  const std::string encodedResources() const { return encoded_resources_; }


return by const & here

snowp · 2021-03-01T18:31:13Z

source/extensions/filters/http/oauth2/filter.cc

        config_->encodedAuthScopes(), escaped_redirect_uri, escaped_state);
-    response_headers->setLocation(new_url);
+
+    const std::string resource_param = config_->encodedResources();


use const ref here to avoid a copy

snowp · 2021-03-01T18:32:51Z

test/extensions/filters/http/oauth2/filter_test.cc

  // Auth_scopes was not set, should return default value.
  EXPECT_EQ(test_config_->encodedAuthScopes(), TEST_DEFAULT_SCOPE);

+  // resource by default is not set


nit: proper grammar/punctuation in comments

snowp · 2021-03-01T18:37:00Z

source/extensions/filters/http/oauth2/filter.cc

+    std::string h2 = "https://";
+
+    for (const auto& resource : resources_protos) {
+      if (resource.rfind(h1, 0) == 0 || resource.rfind(h2, 0) == 0) {


Looking at the RFC it seems like the resource must be an absolute URL, so why do we need this special handling? Is there a reason why we would not percent encode in all cases?

cc @williamsfu99 in case you can shed some light on the the spec here

resource may be in ARN format hence the special case.
"resource":"arn:aws:iam::123456789012:user/12345"

https://docs.aws.amazon.com/general/latest/gr/aws-arns-and-namespaces.html

@snowp @williamsfu99 Let me know what do you think about ARN resource. Also code is up for review again

I don't have a problem supporting the ARN resources, but it seems like we should percent encode it regardless in order to pass it as query param? Are there use cases in the wild where it expects these query parameters to not be URL encoded?

Hi @snowp , here is some information around URI and I agree that it should be url encoded.

absolute URI: The RFC does not mandate the format of the resource param. It's just an identifier and meant to identify the protected resource the client is requesting a token for.
The "resource" parameter URI value is an identifier representing the identity of the resource, which MAY be a locator that corresponds to a network-addressable location where the target resource is hosted.
definition of URI: https://tools.ietf.org/html/rfc3986#section-4.3

I guess, you are in agreement with this.

I will update the PR with URL encoding next.

@snowp its up for review

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

snowp

Thanks this looks good to me! Just a style nit

@rgs1 Do you wanna give this a quick look as well?

snowp · 2021-03-03T15:43:35Z

source/extensions/filters/http/oauth2/filter.cc

+    const std::string& resource_param = config_->encodedResources();
+
+    response_headers->setLocation(new_url + resource_param);


Just a small nit but could we rename encodedResources to encodedResourceQueryParams and then inline resource_param like so:

response_headers->setLocation(new_url + config_->encodedResourceQueryParams());

? I think that would read a bit cleaner

@snowp, I have done the changes. The macos tests are failing because go binary could not be installed on the test setup.
Let me know how to move forward from here.

code is up for review

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy · 2021-03-04T04:32:09Z

/retest

repokitteh-read-only · 2021-03-04T04:32:13Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15147 (comment) was created by @sudeeptoroy.

see: more, trace.

sudeeptoroy · 2021-03-04T13:34:56Z

/retest

repokitteh-read-only · 2021-03-04T13:34:59Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #15147 (comment) was created by @sudeeptoroy.

see: more, trace.

snowp

Thanks! Can you merge main to fix CI?

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy · 2021-03-05T03:55:13Z

Thanks! Can you merge main to fix CI?

Hi @snowp , the CI is fixed now. Requesting you to approve the merge.

snowp · 2021-03-05T18:17:43Z

@sudeeptoroy Sorry could you add a release note for this? Thanks!

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy · 2021-03-05T19:22:58Z

@sudeeptoroy Sorry could you add a release note for this? Thanks!

Hi @snowp, I have added a release note. Can you review it and let me know if wordings are fine?

snowp

Release note looks good besides one minor language suggestion! Thanks for your patience in iterating on this PR

snowp · 2021-03-05T21:26:44Z

docs/root/version_history/current.rst

 * json: introduced new JSON parser (https://github.com/nlohmann/json) to replace RapidJSON. The new parser is disabled by default. To test the new RapidJSON parser, enable the runtime feature `envoy.reloadable_features.remove_legacy_json`.
 * kill_request: :ref:`Kill Request <config_http_filters_kill_request>` Now supports bidirection killing.
 * log: added a new custom flag ``%j`` to the log pattern to print the actual message to log as JSON escaped string.
+* oauth filter: added the optional parameter :ref:`resources <envoy_v3_api_field_extensions.filters.http.oauth2.v3alpha.OAuth2Config.resources>`. Set this value to add multiple "resource" parameters in the Authorization request sent to the OAuth provider. This acts as an identifier representing protected resource the client is requesting a token for.


representing protected resource -> representing the protected resources

Hi @snowp , I have made the corresponding change.

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

snowp

Thanks!

added support for resource param in authorization request

5c1c426

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

repokitteh-read-only bot added the api label Feb 23, 2021

repokitteh-read-only bot assigned mattklein123 Feb 23, 2021

Update filter_test.cc

0e4849d

Fixed a compilation with test cases Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com> Signed-off-by: sudeeptoroy <sudeeptoroy@users.noreply.github.com>

sudeeptoroy force-pushed the oauth2_resource_param branch from cde5869 to 0e4849d Compare February 23, 2021 07:30

sudeeptoroy added 2 commits February 23, 2021 14:48

formatting changes

f10fddc

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

added empty method to check for emptiness of string

a41d062

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

snowp reviewed Feb 25, 2021

View reviewed changes

mattklein123 assigned snowp Feb 26, 2021

mattklein123 added the waiting label Feb 26, 2021

changes to allow repeated resources field

93364fc

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

repokitteh-read-only bot removed the waiting label Feb 26, 2021

sudeeptoroy added 3 commits February 27, 2021 11:11

formatting correction

bae2d4a

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

corrected the variable name

cd8beb0

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

corrected the test case

e8cbee4

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

mattklein123 removed their assignment Mar 1, 2021

repokitteh-read-only bot removed the api label Mar 1, 2021

snowp suggested changes Mar 1, 2021

View reviewed changes

review comments

05bf32d

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy requested a review from snowp March 2, 2021 09:44

snowp added the waiting:any label Mar 2, 2021

repokitteh-read-only bot removed the waiting:any label Mar 2, 2021

url encoding all resource param

d466930

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

snowp suggested changes Mar 3, 2021

View reviewed changes

snowp added the waiting label Mar 3, 2021

renamed encodedResources to encodedResourceQueryParams

4de33fa

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

repokitteh-read-only bot removed the waiting label Mar 3, 2021

fixed resources test

d3f8f3f

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy requested a review from snowp March 4, 2021 07:19

snowp previously approved these changes Mar 4, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/main' into oauth2_resource_param

bf6afd4

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

added release note

8236816

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

sudeeptoroy dismissed snowp’s stale review via 8236816 March 5, 2021 19:19

snowp suggested changes Mar 5, 2021

View reviewed changes

revision history review comment

a376282

Signed-off-by: Sudeepto Roy <sudeepto.roy@gmail.com>

snowp approved these changes Mar 8, 2021

View reviewed changes

snowp merged commit 471c609 into envoyproxy:main Mar 8, 2021

sudeeptoroy deleted the oauth2_resource_param branch March 8, 2021 15:50

		const std::string& resource_param = config_->encodedResources();

		response_headers->setLocation(new_url + resource_param);

Conversation

sudeeptoroy commented Feb 23, 2021

Uh oh!

repokitteh-read-only bot commented Feb 23, 2021

Uh oh!

moderation commented Feb 25, 2021

Uh oh!

rgs1 commented Feb 25, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattklein123 commented Mar 1, 2021

Uh oh!

mattklein123 commented Mar 1, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sudeeptoroy commented Mar 4, 2021

Uh oh!

repokitteh-read-only bot commented Mar 4, 2021

Uh oh!

sudeeptoroy commented Mar 4, 2021

Uh oh!

repokitteh-read-only bot commented Mar 4, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

sudeeptoroy commented Mar 5, 2021

Uh oh!

snowp commented Mar 5, 2021

Uh oh!

sudeeptoroy commented Mar 5, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sudeeptoroy Mar 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

sudeeptoroy Mar 6, 2021 •

edited

Loading