jwt_authn: implementation of www-authenticate header by cyran-ryan · Pull Request #16216 · envoyproxy/envoy

cyran-ryan · 2021-04-28T17:58:24Z

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Commit Message: Adds www-authenticate header to jwt_authn responses
Additional Description: Returns a valid www-authenticate header when deny a request with a 401 Unauthorized response, see RFC 7235 and RFC 6750 for information on the spec and error codes this response returns
Risk Level: Medium
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Deprecated:]
[Optional API Considerations:]

repokitteh-read-only · 2021-04-28T17:58:29Z

Hi @cyran-ryan, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #16216 was opened by cyran-ryan.

see: more, trace.

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

qiwzhang · 2021-04-28T18:27:31Z

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

                      absl::StrJoin(absl::StrSplit(error_msg, ' '), "_"), "}");
 }

+std::string buildUri(const Http::RequestHeaderMap& request_headers,


buildOriginalUri(). This could be move to source/common/http/utility. other modules may need this

Agreed, I'll put up a change with that shortly

qiwzhang · 2021-04-28T18:29:02Z

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

-        code, ::google::jwt_verify::getStatusString(status), nullptr, absl::nullopt,
+        code, ::google::jwt_verify::getStatusString(status), 
+        [uri = this->original_uri_](Http::ResponseHeaderMap& headers) {
+          headers.setCopy(Http::LowerCaseString("www-authenticate"),


Is it OK to send the same response for "token missing" case?

So that's a tough one, it looks like from the OAuth 2.0 RFC (which seems to be the most analogous to this situation) the error field should be included if the token is rejected but not if it's missing. If we think that we just need to follow the 401 Unauthorized RFC, we just need realm to exist.

By looking at OAuth 2.0 RFC, for invalid token, server SHOULD add error=.

How about:
for missing we just add "realm=", but for invalid token we also add error=. e.g. "realm=URI, error=invalid_token"

Agreed! Working on that change today

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

qiwzhang · 2021-04-28T22:25:25Z

You need to add some unit-test or integration tests for the change.

cyran-ryan · 2021-04-29T19:25:58Z

You need to add some unit-test or integration tests for the change.

Would it be necessary to write an entirely new test for this? Or is it sufficient to add checks in the 401 response integration tests?

qiwzhang · 2021-04-29T20:56:13Z

You need to add some unit-test or integration tests for the change.

Would it be necessary to write an entirely new test for this? Or is it sufficient to add checks in the 401 response integration tests?

The latter is fine. As long as it cover both "missing" and "invalid_token" cases.

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

qiwzhang · 2021-05-03T20:50:58Z

source/common/http/utility.cc

+  }
+  absl::string_view path(request_headers.EnvoyOriginalPath()
+                             ? request_headers.getEnvoyOriginalPathValue()
+                             : request_headers.getPathValue());


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

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.

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?

Ping on this, I think we can simplify this logic

source/common/http/utility.cc

source/common/http/utility.h

qiwzhang · 2021-05-03T20:54:49Z

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

 namespace JwtAuthn {

 namespace {
+const std::string INVALID_TOKEN_ERROR_STRING = "invalid_token";


prefer not to use std::string in static, can we use string_view, or char[]

Swapped to string_view

qiwzhang · 2021-05-03T20:55:36Z

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

  if (verifier == nullptr) {
    onComplete(Status::Ok);
  } else {
+    original_uri_ = Http::Utility::buildOriginalUri(headers, 256);


Where is 256 coming from? Should we define it somewhere?

256 seemed to be a good length limit for URIs in the headers. There are no actual defined limits for how long headers can be, effectively though, many servers will limit the total amount of data headers can use to 8KiB or 16KiB. I'm up for discussion on how long we should allow the generated URI to be. And then yes, I think it's a good idea define the value rather than leaving it as a raw number.

It is fine to just const define it.

qiwzhang · 2021-05-03T20:58:10Z

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

+          if (status != Status::JwtMissed) {
+            absl::StrAppend(&value, ", error=\"", INVALID_TOKEN_ERROR_STRING, "\"");
+          }
+          headers.setCopy(Http::LowerCaseString("www-authenticate"), value);


maybe define this LowerCaseString as singleton?

How would you go about that? I've looked at a couple solutions but I run into issues with the fact that the filter can be constructed on multiple threads. We could do something with ThreadLocalStorage I suppose.

we can use this one directly

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

qiwzhang · 2021-05-10T21:28:00Z

source/common/http/headers.h

  const LowerCaseString Pragma{"pragma"};
  const LowerCaseString Referer{"referer"};
  const LowerCaseString Vary{"vary"};
+  const LowerCaseString WwwAuthenticate{"www-authenticate"};


You re-defined it. It is defined in line 215.

Whoops, I looked at the comment on my phone and didn't notice that. Fixing now

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

repokitteh-read-only · 2021-06-23T02:48:37Z

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

🐱

Caused by: a #16216 (comment) was created by @snowp.

see: more, trace.

snowp · 2021-06-23T13:52:15Z

Ok the tests passed but I think the refactor of the tracing library caused the coverage % to drop down. Do you think you could add some dummy call to the functions not covered by tests as per https://storage.googleapis.com/envoy-pr/e4f1d82/coverage/source/common/tracing/index.html ? Should be relatively easy and should make CI go green

cyran-ryan · 2021-06-29T17:42:34Z

Ok the tests passed but I think the refactor of the tracing library caused the coverage % to drop down. Do you think you could add some dummy call to the functions not covered by tests as per https://storage.googleapis.com/envoy-pr/e4f1d82/coverage/source/common/tracing/index.html ? Should be relatively easy and should make CI go green

That's understandable seeing as I removed a good chunk of covered code from those files. I'll work on these today.

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

snowp

Thanks!

/retest

repokitteh-read-only · 2021-07-06T14:10:39Z

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

🐱

Caused by: a #16216 (review) was submitted by @snowp.

see: more, trace.

cyran-ryan · 2021-07-06T19:00:36Z

Last question, we would like to cherrypick these changes to 1.18, and 1.17. This is for Istio to pull in for their release 1.9 and 1.10. Is this possible?

snowp · 2021-07-07T17:03:53Z

Current policy limits backports to security and stability fixes I'm afraid. You can try to post an issue about it to discuss, though my knee jerk reaction is that this doesn't qualify.

* main: listener: match rebalancer to listener IP family type (envoyproxy#16914) jwt_authn: implementation of www-authenticate header (envoyproxy#16216) listener: reset the file event in framework instead of listener filter doing itself (envoyproxy#17227) Small typo fix (envoyproxy#17247) Doc: Clarify request/response attributes are http-only (envoyproxy#17204) bazel/README.md: add aspell comment (envoyproxy#17072) docs: Fix broken URL links in HTTP upgrades doc (envoyproxy#17225) remove the wrong comment on test (envoyproxy#17233) upstream: allow clusters to skip waiting on warmup for initialization (envoyproxy#17179) JwtAuthn: support completing padding on forward jwt payload header (envoyproxy#16752) remove support for v2 UNSUPPORTED_REST_LEGACY (envoyproxy#16968) metrics service: fix wrong argument arrange on MetricsServiceSink (envoyproxy#17127) deps: update cel-cpp to 0.6.1 (envoyproxy#16293) Add ability to filter ConfigDump. (envoyproxy#16774) examples: fix Wasm example. (envoyproxy#17218) upstream: update host's socket factory when metadata is updated. (envoyproxy#16708) Signed-off-by: Garrett Bourg <bourg@squareup.com>

cyran-ryan · 2021-07-14T20:34:38Z

/backport

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info> Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: cyran-ryan <58446431+cyran-ryan@users.noreply.github.com>

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info> Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: cyran-ryan <58446431+cyran-ryan@users.noreply.github.com> Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info> Co-authored-by: Lizan Zhou <lizan@tetrate.io>

Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

… (envoyproxy#17441) Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

jwt_authn: implementation of www-authenticate header (envoyproxy#16216) (envoyproxy#17441)

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info> Co-authored-by: Lizan Zhou <lizan@tetrate.io>

cyran-ryan force-pushed the add_www_header branch from 6fcf4d9 to 28f4153 Compare April 28, 2021 18:00

cyran-ryan changed the title ~~jwt_authn: implementation of www-authenticate header~~ [wip] jwt_authn: implementation of www-authenticate header Apr 28, 2021

cyran-ryan mentioned this pull request Apr 28, 2021

"jwt_authn" Filter Should Include a Valid www-authenticate Header #16200

Closed

Adds the initial implementation of www-authenticate header

d298dac

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

cyran-ryan force-pushed the add_www_header branch from 28f4153 to d298dac Compare April 28, 2021 18:05

qiwzhang suggested changes Apr 28, 2021

View reviewed changes

move uri code into http/common

668491b

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

Ryan Sutton added 3 commits May 3, 2021 14:33

return error field for processed tokens

2aa8dd9

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

Add testing for newly introduced headers

6d2118a

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

fix format

12d2c8e

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

qiwzhang reviewed May 3, 2021

View reviewed changes

source/common/http/utility.cc Show resolved Hide resolved

qiwzhang reviewed May 3, 2021

View reviewed changes

source/common/http/utility.h Show resolved Hide resolved

qiwzhang reviewed May 3, 2021

View reviewed changes

qiwzhang reviewed May 4, 2021

View reviewed changes

Modify the std::string to string_view and fix spelling error

f2a80f5

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

qiwzhang previously approved these changes May 7, 2021

View reviewed changes

cyran-ryan marked this pull request as ready for review May 7, 2021 21:36

cyran-ryan requested a review from lizan as a code owner May 7, 2021 21:36

Add singleton for WwwAuthenticate header

b5681e9

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

cyran-ryan dismissed qiwzhang’s stale review via b5681e9 May 10, 2021 20:02

qiwzhang reviewed May 10, 2021

View reviewed changes

Remove extraneous header definition and use the existing one instead

5d1dfd9

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

antoniovicente assigned qiwzhang May 11, 2021

qiwzhang previously approved these changes May 11, 2021

View reviewed changes

snowp added the waiting label Jun 23, 2021

Add some test calls to increase code coverage

5bed8e4

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

cyran-ryan dismissed qiwzhang’s stale review via 5bed8e4 July 2, 2021 21:36

repokitteh-read-only bot removed the waiting label Jul 2, 2021

snowp approved these changes Jul 6, 2021

View reviewed changes

snowp merged commit 9fee580 into envoyproxy:main Jul 7, 2021

repokitteh-read-only bot added the backport/review Request to backport to stable releases label Jul 14, 2021

cyran-ryan mentioned this pull request Jul 14, 2021

Backport of www-authenticate header to 1.17 and 1.18 #17350

Closed

cyran-ryan mentioned this pull request Jul 20, 2021

[release/v1.18] jwt_authn: implementation of www-authenticate header (#16216) #17426

Merged

cyran-ryan mentioned this pull request Jul 21, 2021

[release/v1.17] jwt_authn: implementation of www-authenticate header (#16216) #17441

Merged

lizan added a commit that referenced this pull request Jul 28, 2021

jwt_authn: implementation of www-authenticate header (#16216) (#17426)

78ab00f

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info> Co-authored-by: Lizan Zhou <lizan@tetrate.io>

lizan added a commit that referenced this pull request Jul 28, 2021

jwt_authn: implementation of www-authenticate header (#16216) (#17441)

36c2277

Co-authored-by: Lizan Zhou <lizan@tetrate.io> Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info>

istio-testing added a commit to istio/envoy that referenced this pull request Aug 10, 2021

Merge pull request #329 from cyran-ryan/cherry-pick-www-auth

7cf32de

jwt_authn: implementation of www-authenticate header (envoyproxy#16216) (envoyproxy#17441)

leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021

jwt_authn: implementation of www-authenticate header (envoyproxy#16216)

1f777b0

Signed-off-by: Ryan Sutton <ryan.sutton@volunteers.acasi.info> Co-authored-by: Lizan Zhou <lizan@tetrate.io>

alyssawilk added backport/approved Approved backports to stable releases and removed backport/review Request to backport to stable releases labels Nov 29, 2021

Conversation

cyran-ryan commented Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Apr 28, 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!

qiwzhang commented Apr 28, 2021

Uh oh!

cyran-ryan commented Apr 29, 2021

Uh oh!

qiwzhang commented Apr 29, 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!

Uh oh!

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!

qiwzhang May 10, 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!

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jun 23, 2021

Uh oh!

snowp commented Jun 23, 2021

Uh oh!

cyran-ryan commented Jun 29, 2021

Uh oh!

snowp left a comment

Choose a reason for hiding this comment

Uh oh!

repokitteh-read-only bot commented Jul 6, 2021

Uh oh!

cyran-ryan commented Jul 6, 2021

Uh oh!

snowp commented Jul 7, 2021

Uh oh!

cyran-ryan commented Jul 14, 2021

Uh oh!

Reviewers

Assignees

Labels

cyran-ryan commented Apr 28, 2021 •

edited

Loading

qiwzhang May 10, 2021 •

edited

Loading