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

Jump to bottom

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 3 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
26 changes: 16 additions & 10 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 @@ -98,6 +100,10 @@ class AuthenticatorImpl : public Logger::Loggable<Logger::Id::jwt>,
const bool is_allow_failed_;
const bool is_allow_missing_;
TimeSource& time_source_;

// allow 5 seconds of slack when determining token expery
const uint64_t jwt_exp_slack = 5;
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.

It is better to config this slack time from filter config. If default is 0, it will not change existing behaviors.

We can add two fields:

exp_slack: // now <= exp + exp_slack
bnf_slack: // now >= bnf - bnf_slack

Copy link
Author

Choose a reason for hiding this comment

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

How do I use filter config?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. add new fields in Provider

  2. you can get provider() as

jwks_data_->getJwtProvider()

as in line

Copy link
Author

Choose a reason for hiding this comment

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

Working on it. Think I need a proper build environment. Is there some documentation to set it up?
Can't find it in https://github.com/envoyproxy/envoy/

Copy link
Contributor

Choose a reason for hiding this comment

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

You can check out this developer doc

The easiest way is to use docker

Copy link
Author

Choose a reason for hiding this comment

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

Ah. Good. Will do the docker thingy.

uint64_t now;
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.

not need to add anything to class members.

};

std::string AuthenticatorImpl::name() const {
Expand Down Expand Up @@ -153,23 +159,23 @@ 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.
now = absl::ToUnixSeconds(absl::Now());
// If the nbf claim does *not* appear in the JWT, then the nbf field is defaulted
// to 0.
if (jwt_->nbf_ > unix_timestamp) {
if (jwt_->nbf_ > now) {
doneWithStatus(Status::JwtNotYetValid);
return;
}
// If the exp claim does *not* appear in the JWT then the exp field is defaulted
// to 0.
if (jwt_->exp_ > 0 && jwt_->exp_ < unix_timestamp) {
if (jwt_->exp_ > 0 && jwt_->exp_ < now - jwt_exp_slack) {
doneWithStatus(Status::JwtExpired);
return;
}
Expand Down Expand Up @@ -237,7 +243,7 @@ void AuthenticatorImpl::onDestroy() {

// Verify with a specific public key.
void AuthenticatorImpl::verifyKey() {
const Status status = ::google::jwt_verify::verifyJwt(*jwt_, *jwks_data_->getJwksObj());
const Status status = ::google::jwt_verify::verifyJwt(*jwt_, *jwks_data_->getJwksObj(), now - jwt_exp_slack);
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need to double check the exp an bnf here. My suggestion is:

Copy link
Author

Choose a reason for hiding this comment

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

Err. Are you sure? jwt_verify_lib isn't an Istio thing, is it?

if (status != Status::Ok) {
doneWithStatus(status);
return;
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