Skip to content

jwt_authn: make "issuer" field in JwtProvider as optional#14414

Merged
mattklein123 merged 11 commits intoenvoyproxy:masterfrom
qiwzhang:jwt-iusser-optional
Jan 5, 2021
Merged

jwt_authn: make "issuer" field in JwtProvider as optional#14414
mattklein123 merged 11 commits intoenvoyproxy:masterfrom
qiwzhang:jwt-iusser-optional

Conversation

@qiwzhang
Copy link
Copy Markdown
Contributor

@qiwzhang qiwzhang commented Dec 15, 2020 •
edited
Loading

Signed-off-by: Wayne Zhang qiwzhang@google.com

To fix #12377

According to rfc, "iss" field should be optional. But "issuer" in JwtProvider filter_config is marked as required.

This PR changes it to be optional. Its usage implications are following:

  • If a Jwt has "iss" field and the "issuer" field in JwtProvider config is specified, they have to match. In all other cases, the JWT "iss" field is not checked.
  • If allow_missing or allow_missing_or_fail is used, the Jwt "iss" field is used to find JwtProvider to verify the signature. If the Jwt doesn't have "iss" field, the first JwtProvider without "issuer" is used.

Commit Message:
Additional Description:
Risk Level: None as new features are added to allow empty issuer in the JwtProvider
Testing: Added unit-tests
Docs Changes: None
Release Notes: Yes

@qiwzhang qiwzhang requested a review from lizan as a code owner December 15, 2020 07:10
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Dec 15, 2020

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
API shepherd assignee is @htuch
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #14414 was opened by qiwzhang.

see: more, trace.

@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Dec 15, 2020

@yangminzhu @nareddyt could you help to review this code? Thanks

yangminzhu
yangminzhu reviewed Dec 15, 2020
View reviewed changes
nareddyt
nareddyt reviewed Dec 15, 2020
View reviewed changes
nareddyt
nareddyt previously approved these changes Dec 16, 2020
View reviewed changes
@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Dec 17, 2020

@lizan @htuch and envoyproxy/api-shepherds ping

htuch
htuch reviewed Dec 17, 2020
View reviewed changes
htuch
htuch reviewed Dec 17, 2020
View reviewed changes
htuch
htuch reviewed Dec 17, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

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

/lgtm api

@mattklein123 mattklein123 assigned lizan and unassigned htuch Dec 18, 2020
@qiwzhang
jwt_authn: make issuer as optional
6cd25e8 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
polish the code
e153d6e 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
fix proto doc format
c90ecea 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Update per comment
cc74c5c 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
fix spelling
c859cca 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Update proto comment
21bfbc0 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang qiwzhang force-pushed the jwt-iusser-optional branch from ff74deb to 21bfbc0 Compare December 22, 2020 18:32
@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Dec 22, 2020

Sorry, did a force-push. Hope to fix a test failure unrelated to this change.

@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Dec 23, 2020

It had passed all tests, but after added a few comment in the proto, some tests failed.

@qiwzhang
update release note
fc68ae9 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
Copy link
Copy Markdown
Contributor Author

qiwzhang commented Dec 30, 2020

Hi @htuch I just did a force-push to cause a retest. there is not new changes. Could you help to review it again? Thanks

@mattklein123 mattklein123 self-assigned this Jan 1, 2021
mattklein123
mattklein123 requested changes Jan 2, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Some small API/doc comments. Thank you!

/wait

@qiwzhang
Update release note and fix a typo in config proto file
22728e4 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@qiwzhang
update release note link
9c85a87 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
mattklein123
mattklein123 reviewed Jan 5, 2021
View reviewed changes
@qiwzhang
use RST link in the release note
cc4570d 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
mattklein123
mattklein123 reviewed Jan 5, 2021
View reviewed changes
docs/root/version_history/current.rst Outdated
* http: clusters now support selecting HTTP/1 or HTTP/2 based on ALPN, configurable via :ref:`alpn_config <envoy_v3_api_field_extensions.upstreams.http.v3.HttpProtocolOptions.auto_config>` in the :ref:`http_protocol_options <envoy_v3_api_msg_extensions.upstreams.http.v3.HttpProtocolOptions>` message.
* jwt_authn: added support for :ref:`per-route config <envoy_v3_api_msg_extensions.filters.http.jwt_authn.v3.PerRouteConfig>`.
* jwt_authn: changed config field :ref:`issuer <envoy_v3_api_field_extensions.filters.http.jwt_authn.v3.JwtProvider.issuer>` to be optional to comply with JWT [RFC](https://tools.ietf.org/html/rfc7519#section-4.1.1) requirements.
* jwt_authn: changed config field :ref:`issuer <envoy_v3_api_field_extensions.filters.http.jwt_authn.v3.JwtProvider.issuer>` to be optional to comply with JWT `RFC <https://tools.ietf.org/html/rfc7519#section-4.1.1>_` requirements.
Copy link
Copy Markdown
Member

@mattklein123 mattklein123 Jan 5, 2021

Choose a reason for hiding this comment

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

This is still not correct. Please look at the rendered docs. (undescore at the end)

/wait

Copy link
Copy Markdown
Contributor Author

@qiwzhang qiwzhang Jan 5, 2021

Choose a reason for hiding this comment

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

Done. Thanks.

@qiwzhang
fixed the link
38e04de 
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
@repokitteh-read-only repokitteh-read-only bot removed the api label Jan 5, 2021
@mattklein123 mattklein123 merged commit f0117e5 into envoyproxy:master Jan 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

@lizan lizan Awaiting requested review from lizan

+3 more reviewers

@yangminzhu yangminzhu yangminzhu left review comments

@htuch htuch htuch left review comments

@nareddyt nareddyt nareddyt left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lizan lizan

@mattklein123 mattklein123

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

JWT authentication issuer not configured error

6 participants

@qiwzhang @yangminzhu @mattklein123 @htuch @nareddyt @lizan