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

Fix bug determining jwt validity due to incorrect computation of system timestamp and provide configuration option to allow for timely slack in token validity #10753

Closed
wants to merge 16 commits into from
Closed

Fix bug determining jwt validity due to incorrect computation of system timestamp and provide configuration option to allow for timely slack in token validity #10753

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
488f1ae
check jwt exp using abseil
Apr 13, 2020
4b973ea
add slack
Apr 13, 2020
6d27fff
check token validity with slack
Apr 14, 2020
c2f082c
add fields exp_slack, nbf_slack to filter config and check token vali…
fscz Apr 15, 2020
e1f734e
change nbf_slack and exp_slack to uint32 and fix some bugs
fscz Apr 16, 2020
0fc50c9
fix field index
fscz Apr 16, 2020
e1c7689
fix nonsense
fscz Apr 16, 2020
662d7d9
remove class variable
fscz Apr 16, 2020
829a14a
remove whitespace
fscz Apr 16, 2020
f45601f
bump jwt_verify_lib version
fscz Apr 16, 2020
da966bb
bump jwt_verify_lib
fscz Apr 17, 2020
0ce07e5
try fix get proto fields
fscz Apr 17, 2020
056b6fc
remove some whitespace
fscz Apr 17, 2020
abf60db
swap abseil with timesource.systemtime
fscz Apr 17, 2020
39e6dc0
fix get timesource
fscz Apr 18, 2020
3263dbe
fix format
fscz May 7, 2020
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
1 change: 1 addition & 0 deletions source/extensions/filters/http/jwt_authn/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ envoy_cc_library(
"//source/common/config:datasource_lib",
"//source/common/protobuf:utility_lib",
"@envoy_api//envoy/extensions/filters/http/jwt_authn/v3:pkg_cc_proto",
"@com_google_absl//absl/time",
],
)

Expand Down
16 changes: 9 additions & 7 deletions source/extensions/filters/http/jwt_authn/authenticator.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@
#include "jwt_verify_lib/jwt.h"
#include "jwt_verify_lib/verify.h"

#include "absl/time/clock.h"

using ::google::jwt_verify::CheckAudience;
using ::google::jwt_verify::Status;

Expand Down Expand Up @@ -153,14 +155,14 @@ void AuthenticatorImpl::startVerify() {
return;
}

// TODO(qiwzhang): Cross-platform-wise the below unix_timestamp code is wrong as the
// epoch is not guaranteed to be defined as the unix epoch. We should use
// the abseil time functionality instead or use the jwt_verify_lib to check
// the validity of a JWT.
// Check "exp" claim.
const uint64_t unix_timestamp =
std::chrono::duration_cast<std::chrono::seconds>(timeSource().systemTime().time_since_epoch())
.count();
// We use the current system time and allow for up to 5 seconds of slack.
// Consider the following case:
// AWS ALB receives a request and determines that the existing access token is valid
// (for another 1 seconds), forwards the request to Istio Ingressgateway and subsequently
// to some pod with an envoy sidecar. Meanwhile, 0.1 seconds have passed and when envoy checks
// the token it finds that it has expired.
const uint64_t unix_timestamp = absl::ToUnixSeconds(absl::Now()) - 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

I am fine with change to use absl::ToUnixSeconds(absl::Now()).

But I don't like "- 5" part, it seems like a hack to solve your particular problem

BTW, even you passed this check, there is another check in verifyJwt which is calling this code here
https://github.com/google/jwt_verify_lib/blob/master/src/verify.cc#L163

Copy link
Author

Choose a reason for hiding this comment

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

The specific amount of slack is debatable. However, I don't think there is any doubt that there is a general need for some slack. Whatever part of an authn system (be it AWS ALB or https://istio.io/blog/2019/app-identity-and-access-adapter/) that refreshes access tokens has to check if the current token is still valid. Finally, istio-proxy validates the token again and the time between these two validation events is the time needed as slack.

Copy link
Author

Choose a reason for hiding this comment

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

Could we make the amount configurable?

Copy link
Contributor

@qiwzhang qiwzhang Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, I like this idea. Let us make it configurable from filter config. Please see my other comments.

// If the nbf claim does *not* appear in the JWT, then the nbf field is defaulted
// to 0.
if (jwt_->nbf_ > unix_timestamp) {
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/http/jwt_authn/authenticator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@
#include "jwt_verify_lib/check_audience.h"
#include "jwt_verify_lib/status.h"

#include "absl/time/clock.h"
Copy link
Member

Choose a reason for hiding this comment

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

Do we have any regression or unit/integration tests for this?

Copy link
Author

Choose a reason for hiding this comment

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

Tests? Jeeze. What roughnecked thinking. No, seriously. This is my first contribution here and I wasn't actually planning on going through by myself. Anyways, I don't mind at all doing it but need to get set up with a proper build environment to do real work. The coming weekend seems a good time to do so.

Copy link
Author

Choose a reason for hiding this comment

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

Ah heck, here I am working already.

Copy link
Contributor

Choose a reason for hiding this comment

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

This header is not needed any more. please remove it


namespace Envoy {
namespace Extensions {
namespace HttpFilters {
Expand Down