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.

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
11 changes: 11 additions & 0 deletions api/envoy/extensions/filters/http/jwt_authn/v3/config.proto
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,17 @@ message JwtProvider {
// exp: 1501281058
//
string payload_in_metadata = 9;

// The two below fields define the amount of slack in seconds that will be used
// when determining if a JWT is valid or has expired. Validity is determined by
// the formula: VALID_FROM("iat") + nbf_slack <= NOW <= VALID_TO("exp") + 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.

s/iat/nbf

Copy link
Author

Choose a reason for hiding this comment

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

Still seeing this:

source/extensions/filters/http/jwt_authn/authenticator.cc:172:59: error: no member named 'nbf_slack' in 'envoy::extensions::filters::http::jwt_authn::v3::JwtProvider'
const uint32_t nbf_slack = jwks_data_->getJwtProvider().nbf_slack();
~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
source/extensions/filters/http/jwt_authn/authenticator.cc:173:59: error: no member named 'exp_slack' in 'envoy::extensions::filters::http::jwt_authn::v3::JwtProvider'
const uint32_t exp_slack = jwks_data_->getJwtProvider().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.

Hmm, make sure this modified proto files are compiled. by

bazel build //api/...

for docker run: you need to run this command "bazel.api"

Copy link
Author

Choose a reason for hiding this comment

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

// which aims to provide a remedy in the following case:
// Some load balancer performinc OIDC authentication receives a request and determines that the
// existing access token is valid (for another 1 second). It forwards the request to Istio Ingressgateway
// and subsequently to some pod with an envoy sidecar. Meanwhile, 1 second has passed and when envoy checks
// the token it finds that it has expired.
uint32 nbf_slack = 10;
uint32 exp_slack = 11;
}

// This message specifies how to fetch JWKS from remote and how to cache it.
Expand Down
8 changes: 4 additions & 4 deletions bazel/repository_locations.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -183,10 +183,10 @@ REPOSITORY_LOCATIONS = dict(
urls = ["https://github.com/msgpack/msgpack-c/releases/download/cpp-3.2.1/msgpack-3.2.1.tar.gz"],
),
com_github_google_jwt_verify = dict(
sha256 = "d422a6eadd4bcdd0f9b122cd843a4015f8b18aebea6e1deb004bd4d401a8ef92",
strip_prefix = "jwt_verify_lib-40e2cc938f4bcd059a97dc6c73f59ecfa5a71bac",
# 2020-02-11
urls = ["https://github.com/google/jwt_verify_lib/archive/40e2cc938f4bcd059a97dc6c73f59ecfa5a71bac.tar.gz"],
sha256 = "21b9fd9fb8714cb199a823a4c01cf6665bdd42b62137348707dee51714797dfc",
strip_prefix = "jwt_verify_lib-b5b3b4ed8611b1eea8764845381e60becc7b0b43",
# 2020-04-17
urls = ["https://github.com/google/jwt_verify_lib/archive/b5b3b4ed8611b1eea8764845381e60becc7b0b43.tar.gz"],
),
com_github_nodejs_http_parser = dict(
sha256 = "8fa0ab8770fd8425a9b431fdbf91623c4d7a9cdb842b9339289bd2b0b01b0d3d",
Expand Down
1 change: 1 addition & 0 deletions source/extensions/filters/http/jwt_authn/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ envoy_cc_library(
deps = [
":extractor_lib",
":jwks_cache_lib",
"@com_google_absl//absl/time",
"//include/envoy/server:filter_config_interface",
"//include/envoy/stats:stats_macros",
"//source/common/http:message_lib",
Expand Down
39 changes: 23 additions & 16 deletions source/extensions/filters/http/jwt_authn/authenticator.cc
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,33 +155,38 @@ 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 the issuer is configured or not.
jwks_data_ = provider_ ? jwks_cache_.findByProvider(provider_.value())
: jwks_cache_.findByIssuer(jwt_->iss_);
// isIssuerSpecified() check already make sure the issuer is in the cache.
ASSERT(jwks_data_ != nullptr);

// Check "exp" claim.
const uint64_t unix_timestamp =
std::chrono::duration_cast<std::chrono::seconds>(timeSource().systemTime().time_since_epoch())
.count();
// The two below fields define the amount of slack in seconds that will be used
// when determining if a JWT is valid or has expired. Validity is determined by
// the formula: VALID_FROM("iat") + nbf_slack <= NOW <= VALID_TO("exp") + exp_slack
// which aims to provide a remedy in the following case:
// Some load balancer performinc OIDC authentication receives a request and determines that the
// existing access token is valid (for another 1 second). It forwards the request to Istio Ingressgateway
// and subsequently to some pod with an envoy sidecar. Meanwhile, 1 second has passed and when envoy checks
// the token it finds that it has expired.
const uint64_t now = absl::ToUnixSeconds(absl::Now());
Copy link
Member

Choose a reason for hiding this comment

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

Don't use absl::Now but use time from timeSource().systemTime(), this will make test easier by providing mock time.

const uint32_t nbf_slack = jwks_data_->getJwtProvider().nbf_slack();
const uint32_t exp_slack = jwks_data_->getJwtProvider().exp_slack();

// If the nbf claim does *not* appear in the JWT, then the nbf field is defaulted
// to 0.
if (jwt_->nbf_ > unix_timestamp) {
if (now < jwt_->nbf_ + nbf_slack) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should it be

now < nbf_ - nbf_slack

you want the jwt to be ready ahead of nbf time. Is that right?

Copy link
Author

Choose a reason for hiding this comment

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

Reflecting on that idea, I think that any use cases for this come down to machines running with incorrect system time and I don't see why we should account for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, then

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 (0 < jwt_->exp_ && now > jwt_->exp_ + exp_slack) {
doneWithStatus(Status::JwtExpired);
return;
}

// Check the issuer is configured or not.
jwks_data_ = provider_ ? jwks_cache_.findByProvider(provider_.value())
: jwks_cache_.findByIssuer(jwt_->iss_);
// isIssuerSpecified() check already make sure the issuer is in the cache.
ASSERT(jwks_data_ != nullptr);

// Check if audience is allowed
bool is_allowed = check_audience_ ? check_audience_->areAudiencesAllowed(jwt_->audiences_)
: jwks_data_->areAudiencesAllowed(jwt_->audiences_);
Expand Down Expand Up @@ -237,7 +244,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::verifyJwtWithoutTimeChecking(*jwt_, *jwks_data_->getJwksObj());
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to get the latest jwt_verify_lib here:

By change its SHA, and sha256.

the sha256 sum is by: downloading that .tar.gz file, and run sha256sum on the file.

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 get the current .tar.gz file. As I understand, the file
https://github.com/google/jwt_verify_lib/archive/40e2cc938f4bcd059a97dc6c73f59ecfa5a71bac.tar.gz

is outdated. But how do I get the new one?

Copy link
Contributor

Choose a reason for hiding this comment

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

the new sha is: b5b3b4ed8611b1eea8764845381e60becc7b0b43

the file is https://github.com/google/jwt_verify_lib/archive/b5b3b4ed8611b1eea8764845381e60becc7b0b43.tar.gz

sha256 is: 21b9fd9fb8714cb199a823a4c01cf6665bdd42b62137348707dee51714797dfc

Copy link
Contributor

Choose a reason for hiding this comment

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

please also update the date in the comment field

if (status != Status::Ok) {
doneWithStatus(status);
return;
Expand Down
2 changes: 2 additions & 0 deletions source/extensions/filters/http/jwt_authn/authenticator.h
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