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

Handle validation of non expiring tokens in jwt_authn filter #4007

Merged
merged 2 commits into from
Aug 16, 2018
Merged

Handle validation of non expiring tokens in jwt_authn filter #4007

merged 2 commits into from
Aug 16, 2018

Conversation

vvinod1310
Copy link
Contributor

@vvinod1310 vvinod1310 commented Aug 1, 2018 •
edited
Loading

For an explanation of how to fill out the fields, please see the relevant section
in PULL_REQUESTS.md

Description: Handle validation of non expiring tokens in jwt_authn filter
Risk Level: Low
Testing: Added unit test to cover modification
Docs Changes: N/A
Release Notes: N/A

@vvinod1310 vvinod1310 requested a review from lizan as a code owner August 1, 2018 00:08
@ccaraman
Copy link
Contributor

ccaraman commented Aug 1, 2018

@vvinod1310 Please fix DCO https://github.com/envoyproxy/envoy/blob/dc03a9a4196204c13aaa192694acf67f3fc09ce8/CONTRIBUTING.md#dco-sign-your-work

And is there a test that can be added?

@lizan lizan self-assigned this Aug 1, 2018
lizan
lizan reviewed Aug 2, 2018
View reviewed changes
source/extensions/filters/http/jwt_authn/authenticator.cc Outdated
@@ -119,7 +119,7 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback
const auto unix_timestamp = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::system_clock::now().time_since_epoch())
.count();
if (jwt_.exp_ < unix_timestamp) {
if (jwt_.exp_ > 0 && jwt_.exp_ < unix_timestamp) {
Copy link
Member

Choose a reason for hiding this comment

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

@qiwzhang does jwt_verify_lib assign 0 to exp_ when "exp" is not in the JWT?

Even if so, "0" is valid "exp", perhaps we can make exp_ an absl::optional<uint64_t>?

Copy link
Contributor Author

@vvinod1310 vvinod1310 Aug 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@ccaraman Fixed DCO and added a test thanks!
@lizan jwt_verify_lib assigns 0 to exp_ if not specified (Reference: https://github.com/google/jwt_verify_lib/blob/b06ba30f5d2049bcabd6b4ae8bb8f3518fb18b44/src/jwt.cc#L104)

Copy link
Member

Choose a reason for hiding this comment

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

OK thanks, can you add a comment to clarify the 0 case?

@stale
Copy link

stale bot commented Aug 10, 2018

This pull request has been automatically marked as stale because it has not had activity in the last 7 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@stale stale bot added the stale stalebot believes this issue/PR has not been touched recently label Aug 10, 2018
@vvinod1310
Copy link
Contributor Author

vvinod1310 commented Aug 10, 2018 •
edited
Loading

Hi,

Could I request for this change to be reviewed and merged if appropriate?

Many thanks!

@stale stale bot removed the stale stalebot believes this issue/PR has not been touched recently label Aug 10, 2018
@mattklein123
Copy link
Member

@lizan @qiwzhang PTAL

lizan
lizan reviewed Aug 10, 2018
View reviewed changes
Copy link
Member

@lizan lizan left a comment

Choose a reason for hiding this comment

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

Sorry for late response. LGTM modulo one comment, thanks!

source/extensions/filters/http/jwt_authn/authenticator.cc Outdated
@@ -119,7 +119,7 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback
const auto unix_timestamp = std::chrono::duration_cast<std::chrono::seconds>(
std::chrono::system_clock::now().time_since_epoch())
.count();
if (jwt_.exp_ < unix_timestamp) {
if (jwt_.exp_ > 0 && jwt_.exp_ < unix_timestamp) {
Copy link
Member

Choose a reason for hiding this comment

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

OK thanks, can you add a comment to clarify the 0 case?

@vvinod1310 vvinod1310 requested a review from zuercher as a code owner August 12, 2018 14:15
@vvinod1310 vvinod1310 force-pushed the master branch 2 times, most recently from 533ea3d to f9a336c Compare August 12, 2018 14:27
@vvinod1310
Handle validation of non expiring tokens in jwt_authn filter
47226bf 
Signed-off-by: Vinod <vvinod1310@hotmail.com>
@vvinod1310
Copy link
Contributor Author

@lizan Sure added comment please merge thanks!

lizan
lizan reviewed Aug 13, 2018
View reviewed changes
include/envoy/json/json_object.h Outdated
@@ -85,6 +85,18 @@ class Object {
*/
virtual bool isNull() const PURE;

/**
Copy link
Member

Choose a reason for hiding this comment

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

these shouldn't appear in diff, can you merge master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lizan done thanks!

zuercher
zuercher approved these changes Aug 14, 2018
View reviewed changes
Copy link
Member

@zuercher zuercher left a comment

Choose a reason for hiding this comment

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

LGTM. I restarted the failed ASAN build -- I believe it's unrelated to your change.

@vvinod1310
Copy link
Contributor Author

vvinod1310 commented Aug 15, 2018 •
edited
Loading

@lizan & @zuercher - thank you for approving! May I know when the changes will get merged?

@zuercher
Copy link
Member

@vvinod1310 please update the description to include all the required fields from the pull request template.

@vvinod1310
Copy link
Contributor Author

@zuercher done thanks!

@lizan lizan merged commit 706e262 into envoyproxy:master Aug 16, 2018
rshriram pushed a commit to rshriram/envoy that referenced this pull request Oct 30, 2018
@PiotrSikora @istio-testing
Update Envoy SHA to latest with MetricImpl optimizations. (envoyproxy…
c9d2230 
…#1938)

This is far from finished, but it reduces memory usage by ~10%.

Pulling the following changes from github.com/envoyproxy/envoy:

c1cc68d stats: refactoring MetricImpl without strings (envoyproxy#4190)
36809d8 fuzz: coverage profile generation instructions. (envoyproxy#4193)
ba40cc9 upstream: rebuild cluster when health check config is changed (envoyproxy#4075)
05c0d52 build: use clang-6.0. (envoyproxy#4168)
01f403e thrift_proxy: introduce header transport (envoyproxy#4082)
564d256 tcp: allow connection pool callers to store protocol state (envoyproxy#4131)
3e1d643 configs: match latest API changes (envoyproxy#4185)
bc6a10c Fix wrong mock function name. (envoyproxy#4187)
e994c1c Bump yaml-cpp so it builds with Visual Studio 15.8 (envoyproxy#4182)
3d1325e Converting envoy configs to V2 (envoyproxy#2957)
8d0680f Add timestamp to HealthCheckEvent definition (envoyproxy#4119)
497efb9 server: handle non-EnvoyExceptions safely if thrown in constructor. (envoyproxy#4173)
6d6fafd config: strengthen validation for gRPC config sources. (envoyproxy#4171)
132302c fuzz: reduce log level when running under fuzz engine. (envoyproxy#4161)
7c04ac2 test: fix V6EmptyOptions in coverage with IPv6 enable (envoyproxy#4155)
1b2219b ci: remove deprecated bazel --batch option (envoyproxy#4166)
2db6a4c ci: update clang to version 6.0 in the Ubuntu build image. (envoyproxy#4157)
71152b7 ratelimit: Add ratelimit custom response headers (envoyproxy#4015)
3062874 ssl: make Ssl::Connection const everywhere (envoyproxy#4179)
706e262 Handle validation of non expiring tokens in jwt_authn filter (envoyproxy#4007)
f06e958 fuzz: tag trivial fuzzers with no_fuzz. (envoyproxy#4156)
27fb1d3 thrift_proxy: add service name matching to router implementation (envoyproxy#4130)
8c189a5 Make over provisioning factor configurable (envoyproxy#4003)
6c08fb4 Making test less flaky (envoyproxy#4149)
592775b fuzz: bare bones HCM fuzzer. (envoyproxy#4118)

For istio/istio#7912.

Signed-off-by: Piotr Sikora <piotrsikora@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zuercher zuercher zuercher approved these changes

@lizan lizan lizan approved these changes

@qiwzhang qiwzhang qiwzhang approved these changes

Assignees

@lizan lizan

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vvinod1310 @ccaraman @mattklein123 @zuercher @lizan @qiwzhang