-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Changes from all commits
488f1ae
4b973ea
6d27fff
c2f082c
e1f734e
0fc50c9
e1c7689
662d7d9
829a14a
f45601f
da966bb
0ce07e5
056b6fc
abf60db
39e6dc0
3263dbe
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -153,33 +153,41 @@ 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 = std::chrono::time_point_cast<std::chrono::seconds>(timeSource().systemTime()) | ||
.time_since_epoch() | ||
.count(); | ||
|
||
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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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_); | ||
|
@@ -237,7 +245,8 @@ 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()); | ||
if (status != Status::Ok) { | ||
doneWithStatus(status); | ||
return; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/iat/nbf
There was a problem hiding this comment.
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();
There was a problem hiding this comment.
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"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing the error as output from
https://circleci.com/gh/envoyproxy/envoy/338165?utm_campaign=workflow-failed&utm_medium=email&utm_source=notification