Creating JwksFetcher interface and impl v2 by nickrmc83 · Pull Request #4242 · envoyproxy/envoy

nickrmc83 · 2018-08-24T11:12:08Z

JwksFetcher wraps up HTTP acquisition of JWKS strings converting them into a concrete type on the way.
JwksFetcher is reusable so can be used in a wider context.

Tests updated and fixed where necessary.
Description:
We are in the process of implementing a new Envoy filter based on the design presented here and wish to re-use existing logic in the jwt_authn filter. We've split out the logic we're interested in into a new class called JwksFetcher. Later PRs will re-use the split out logic an OpenID Connect filter.

This is the second crack at this PR (see #4225 which went horribly wrong after following the DCO rebase guidelines).

Risk Level:
Medium
Testing:
Add replacement and additional unit tests for the logic that's been moved.
Docs Changes:
None
Release Notes:
[Optional Fixes #Issue]
[Optional Deprecated:]

JwksFetcher wraps up HTTP acquisition of JWKS strings converting them into a concrete type on the way. JwksFetcher is reusable so can be used in a wider context. Tests updated and fixed where necessary. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

- Changes from PR review. - Added missing tests for JwksFetcher::cancel(). - Added comment and TODO around a potential bug in jwt_authn::authenticator. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

source/extensions/filters/http/common/jwks_fetcher.h

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

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

nickrmc83 · 2018-08-24T11:23:01Z

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

    return;
  }

+  // TODO: Cross-platform-wise the below unix_timestamp code is wrong as the


@qiwzhang note comment I've added here.

nickrmc83 · 2018-08-24T11:23:32Z

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

-  if (jwks_data_->getJwksObj() != nullptr && !jwks_data_->isExpired()) {
+  auto jwks_obj = jwks_data_->getJwksObj();
+  if (jwks_obj != nullptr && !jwks_data_->isExpired()) {
+    // TODO: It would seem there's a window of error whereby if the JWT issuer


@qiwzhang note the comment I've added here.

Can you assign a TODO owner, e.g. TODO(nickrmc83)?

nickrmc83 · 2018-08-24T11:24:28Z

@qiwzhang @lizan I've started this off again and closed the old PR due to my lack of git foo. I've transported all the original comments here. Apologies

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

test/extensions/filters/http/common/jwks_fetcher_test.cc

qiwzhang · 2018-08-24T16:51:00Z

test/extensions/filters/http/common/test_common.h

@@ -3,7 +3,7 @@
 namespace Envoy {
 namespace Extensions {
 namespace HttpFilters {
-namespace JwtAuthn {


It seems that data are still specific to jwt_authn. so the file name is still better call "jwt_authn_common"

My thinking here was that all JWT related test vectors should reside together especially as they all use the same signing keys. Happy to move if necessary but some of the variables are used in the jwks_fetcher_test

Agreed with @qiwzhang, still JWT specific.

qiwzhang

Looks pretty good. Only some small comments.

qiwzhang · 2018-08-24T16:52:50Z

LGTM

source/extensions/filters/http/common/jwks_fetcher.cc

source/extensions/filters/http/common/jwks_fetcher.h

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

- Removed unused response_message variable - use of std::make_unique - Fix enum inline with style guide - Added missing param documentation - Added comments re: nbf and exp JWT fields and their default values - Used reference instead of pointer for non-null function param Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

bazel/repositories.bzl

lizan · 2018-08-30T08:25:51Z

test/extensions/filters/http/common/test_common.h

@@ -3,7 +3,7 @@
 namespace Envoy {
 namespace Extensions {
 namespace HttpFilters {
-namespace JwtAuthn {


Agreed with @qiwzhang, still JWT specific.

test/extensions/filters/http/common/jwks_fetcher_test.cc

test/extensions/filters/http/jwt_authn/authenticator_test.cc

The most major change is to swap JwksFetcher creation to be just-in-time using a lambda callback. Moved test_common.h from test/extensions/filters/http/common back to test/extensions/filters/http/jwt_authn. Most others changes are minor. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

nickrmc83 · 2018-08-30T11:13:55Z

@qiwzhang @lizan I think this is now done with all nits picked. There are some outstanding comments from me that I'd like either of you to resolve specifically regarding the comments I've added to the code for potential areas to be addressed as well as an explanation about why there's a new abseil dependency.

lizan

LGTM modulo one comment, thanks!

bazel/repositories.bzl

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

lizan · 2018-08-30T20:51:06Z

fix format?

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

lizan · 2018-08-30T21:41:25Z

bazel/repositories.bzl

@@ -465,8 +465,8 @@ def _com_google_absl():
        actual = "@com_google_absl//absl/debugging:symbolize",
    )



run tools/check_format.py fix, it is complaining about trailing whitespace on this line.

Check_format.py made some unwanted changes to certain lines in baxel/repositories.bzl that had not been modified. I've reverted those lines in the hope they will take. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

source/extensions/filters/http/common/jwks_fetcher.cc

htuch · 2018-09-04T17:32:02Z

source/extensions/filters/http/common/jwks_fetcher.cc

+
+  void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver& receiver) {
+    ENVOY_LOG(trace, "{}", __func__);
+    receiver_ = &receiver;


What if receiver_ is already set? What is the contract on multiple fetches?

~~@htuch There's a description of the use of the Jwksetcher interface just before the definition in the header file. Would you like further commentary closer to the fetch() method?~~

Just seen your later comment :)

Can you ASSERT here that recever_ == nullptr, i.e. we catch in debug builds violations of the contract.

htuch · 2018-09-04T17:33:27Z

source/extensions/filters/http/common/jwks_fetcher.h

+/**
+ * JwksFetcher interface can be used to retrieve remote JWKS
+ * (https://tools.ietf.org/html/rfc7517) data structures returning a concrete,
+ * type-safe representation. An instance of this interface is designed to


Ah, you answer the above question here. This is a pretty weird pattern; I would call it out by explicitly naming the class/interface JwksSingleUseFetcher or something like that.

@htuch I've improved the description of the functionality in 8fa1dc8. I've also made this class re-usable but designed to retrieve a single JWKS at a time.

Mainly small fixes and better documentation. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch

Looks good! Just a few more comments and this should be ready to go.

htuch · 2018-09-04T23:02:56Z

source/extensions/filters/http/common/jwks_fetcher.h

+  virtual void cancel() PURE;
+
+  /*
+   * Retrieve a JWKS resource from a remote HTTP host.


Retrieve a JWKS resource from a remote HTTP host. At most one outstanding request may be in-flight, i.e. from the invocation of `fetch()` until either a callback or `cancel()` is invoked, no additional `fetch()` may be issued.

Done in 2d18a01

htuch · 2018-09-04T23:04:25Z

source/extensions/filters/http/common/jwks_fetcher.h

+  class JwksReceiver {
+  public:
+    enum class Failure {
+      Unknown,


Please comment these reason values.

htuch · 2018-09-04T23:05:12Z

source/extensions/filters/http/common/jwks_fetcher.cc

+
+  void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver& receiver) {
+    ENVOY_LOG(trace, "{}", __func__);
+    receiver_ = &receiver;


Can you ASSERT here that recever_ == nullptr, i.e. we catch in debug builds violations of the contract.

htuch · 2018-09-04T23:09:51Z

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

+  }
+  // If the exp claim does *not* appear in the JWT then the exp field is defaulted
+  // to 0.
+  if (jwt_.exp_ && jwt_.exp_ < unix_timestamp) {


Prefer maintaining jwt_.exp_ > 0 as before for clarity.

htuch · 2018-09-04T23:10:38Z

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

-  if (jwks_data_->getJwksObj() != nullptr && !jwks_data_->isExpired()) {
+  auto jwks_obj = jwks_data_->getJwksObj();
+  if (jwks_obj != nullptr && !jwks_data_->isExpired()) {
+    // TODO: It would seem there's a window of error whereby if the JWT issuer


Can you assign a TODO owner, e.g. TODO(nickrmc83)?

source/extensions/filters/http/common/jwks_fetcher.cc

Changes from final PR review to include missing unit testand fix commenting Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

This should fix any use-after-free errors Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch · 2018-09-06T23:38:23Z

@nickrmc83 is this now ready for another pass? I.e. coverage fixed etc.

nickrmc83 · 2018-09-07T05:44:54Z

@htuch This is now ready.

htuch

LGTM, thanks! One last tiny nit and we can ship.

htuch · 2018-09-07T16:22:14Z

test/extensions/filters/http/common/jwks_fetcher_test.cc

+   * Expectations and assertions should be made on onJwksSuccessImpl in place
+   * of onJwksSuccess.
+   */
+  void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { onJwksSuccessImpl(jwks); }


Nit: can you change this to pass Jwks& and deref the unique_ptr in args? In general, it doesn't make a lot of sense to pass unique_ptr by ref.

I've changed onJwksSuccessImpl to receive a const Jwks* to avoid the unique_ptr pass-by-reference.

We now pass by const ptr. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch

Needs to merge master to resolve a conflict.

test/extensions/filters/http/common/jwks_fetcher_test.cc

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

test/extensions/filters/http/common/jwks_fetcher_test.cc

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

qiwzhang · 2018-09-12T16:10:06Z

LGTM

test/extensions/filters/http/common/mock.h

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

nickrmc83 · 2018-09-13T12:07:40Z

@htuch I think this is done with the following caveats

My local check_format run shows no errors and the CI failure is giving me no hints.
The failing ipv6 looks like an intermittent issue completely outside of the changes in this PR.

Can you advise on the format error?

htuch · 2018-09-13T17:52:09Z

@nickrmc83 are you using check_format outside of Docker? If so, maybe you have a version mismatch? Ideally you just run ./ci/run_envoy_docker.sh './ci/do_ci.sh fix_format' and you should have the same behavior as CI.

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch

Thanks! Good to merge.

Nick A. Smith added 2 commits August 24, 2018 11:24

Changes from original PR review.

d3fbad5

- Changes from PR review. - Added missing tests for JwksFetcher::cancel(). - Added comment and TODO around a potential bug in jwt_authn::authenticator. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

nickrmc83 requested a review from lizan as a code owner August 24, 2018 11:12

nickrmc83 commented Aug 24, 2018

View reviewed changes

Fixing bazel/repositories.bzl formatting

c90ebf7

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

qiwzhang reviewed Aug 24, 2018

View reviewed changes

test/extensions/filters/http/common/jwks_fetcher_test.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Aug 24, 2018

View reviewed changes

lizan self-assigned this Aug 24, 2018

lizan reviewed Aug 24, 2018

View reviewed changes

Nick A. Smith added 2 commits August 28, 2018 21:13

check-format fixes

6dbc828

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

lizan reviewed Aug 30, 2018

View reviewed changes

bazel/repositories.bzl Show resolved Hide resolved

Added comment describing the dependency on abseil time

498c858

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

More style fixing :(

af9175e

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

lizan previously approved these changes Aug 30, 2018

View reviewed changes

Another go at fixing format of bazel/repositories.bzl

51c4d1b

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

nickrmc83 dismissed lizan’s stale review via 51c4d1b August 30, 2018 21:39

lizan reviewed Aug 30, 2018

View reviewed changes

Run tools/check_format.py fix

f477905

Check_format.py made some unwanted changes to certain lines in baxel/repositories.bzl that had not been modified. I've reverted those lines in the hope they will take. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

nickrmc83 mentioned this pull request Aug 31, 2018

api config: Refactor jwt_auth filter config #3428

Closed

lizan previously approved these changes Sep 3, 2018

View reviewed changes

htuch suggested changes Sep 4, 2018

View reviewed changes

Fixed from PR review

8fa1dc8

Mainly small fixes and better documentation. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

nickrmc83 dismissed lizan’s stale review via 8fa1dc8 September 4, 2018 20:25

htuch reviewed Sep 4, 2018

View reviewed changes

Nick A. Smith added 4 commits September 5, 2018 06:28

Added missing unit tests, tidying

2d18a01

Changes from final PR review to include missing unit testand fix commenting Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

Added state machine for handling immediate JwksFetch returns.

1d9281e

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

Style fixes

9d277be

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

Changed JwtAuthN::Authenticator to re-use a JwksFetcher.

4c77200

This should fix any use-after-free errors Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch suggested changes Sep 7, 2018

View reviewed changes

Refactored unique_ptr pass by reference

4bb61d2

We now pass by const ptr. Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch previously approved these changes Sep 7, 2018

View reviewed changes

htuch reviewed Sep 7, 2018

View reviewed changes

test/extensions/filters/http/common/jwks_fetcher_test.cc Outdated Show resolved Hide resolved

Change of const pointer to const reference inline with policy.

8c1c294

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

nickrmc83 dismissed htuch’s stale review via 8c1c294 September 10, 2018 11:51

nickrmc83 and others added 2 commits September 10, 2018 14:56

Merge branch 'master' into master

489c52d

Fixing bazel/repository_locations.bzl

17ddf46

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

qiwzhang reviewed Sep 10, 2018

View reviewed changes

test/extensions/filters/http/common/jwks_fetcher_test.cc Outdated Show resolved Hide resolved

qiwzhang reviewed Sep 10, 2018

View reviewed changes

test/extensions/filters/http/common/jwks_fetcher_test.cc Outdated Show resolved Hide resolved

Refactoring location of mocks for re-usability

8038924

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch reviewed Sep 12, 2018

View reviewed changes

test/extensions/filters/http/common/mock.h Outdated Show resolved Hide resolved

Nick A. Smith added 2 commits September 12, 2018 21:38

Moved Mock constructors from header to translation unit

1457fb1

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

Fixing formatting of BUILD file

f95e51e

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

Fixing format of t/e/f/h/c/BUILD

18b6b9d

Signed-off-by: Nick A. Smith <nick.a.smith@thales-esecurity.com>

htuch approved these changes Sep 14, 2018

View reviewed changes

htuch merged commit b0ff481 into envoyproxy:master Sep 14, 2018

		@@ -465,8 +465,8 @@ def _com_google_absl():
		actual = "@com_google_absl//absl/debugging:symbolize",
		)

Conversation

nickrmc83 commented Aug 24, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

nickrmc83 commented Aug 24, 2018

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!

qiwzhang left a comment

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Aug 24, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

nickrmc83 commented Aug 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

lizan commented Aug 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nickrmc83 Sep 4, 2018 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

htuch 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!

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

nickrmc83 commented Aug 30, 2018 •

edited

Loading

nickrmc83 Sep 4, 2018 •

edited

Loading