Skip to content

JwtAuthn: complete padding on forward jwt payload header#14779

Merged
lizan merged 5 commits intoenvoyproxy:mainfrom
TAOXUY:jwtPadding
Feb 17, 2021
Merged

JwtAuthn: complete padding on forward jwt payload header#14779
lizan merged 5 commits intoenvoyproxy:mainfrom
TAOXUY:jwtPadding

Conversation

@TAOXUY
Copy link
Copy Markdown
Contributor

@TAOXUY TAOXUY commented Jan 21, 2021

Signed-off-by: Xuyang Tao taoxuy@google.com

Commit Message: add completePadding util in base64 to complete padding if it is missing for base64 encoded binary. Use this util method to add padding for the forward_payload_header in jwtAuthn filter.

Context: although padding is not required by base64 encoding spec, some decode libraries are not robust enough to handle the one without padding

Risk Level: low. This is an enhancement without backward compatible issue.

Testing:done

@TAOXUY
add completePadding
839b85c 
Signed-off-by: Xuyang Tao <taoxuy@google.com>
@TAOXUY TAOXUY requested a review from lizan as a code owner January 21, 2021 06:58
@TAOXUY TAOXUY changed the title Based64: add completePadding JwtAuthn: add completePadding Jan 21, 2021
@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Jan 21, 2021

@qiwzhang
@nareddyt

@TAOXUY TAOXUY changed the title JwtAuthn: add completePadding JwtAuthn: complete padding on forward jwt payload header Jan 21, 2021
nareddyt
nareddyt previously approved these changes Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@nareddyt nareddyt left a comment

Choose a reason for hiding this comment

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

Nit: This is for base64url encoding, not base64. Please fix the PR description to make this distinction. Padding is always required in base64 but optional in base64url

https://tools.ietf.org/html/rfc4648#section-5
https://tools.ietf.org/html/rfc4648#section-3.2

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Jan 21, 2021 •
edited
Loading

Nit: This is for base64url encoding, not base64. Please fix the PR description to make this distinction. Padding is always required in base64 but optional in base64url

https://tools.ietf.org/html/rfc4648#section-5
https://tools.ietf.org/html/rfc4648#section-3.2

Hi Teju, I believe padding is optional all the base-N encoding also under the situation when length is known. You can also see envoy has encode with add_padding and the decodingWithoutPadding.

@TAOXUY
NIT format
7c40e20 
Signed-off-by: Xuyang Tao <taoxuy@google.com>
qiwzhang
qiwzhang reviewed Jan 21, 2021
View reviewed changes
qiwzhang
qiwzhang reviewed Jan 21, 2021
View reviewed changes
@TAOXUY
NIT
e0e616f 
Signed-off-by: Xuyang Tao <taoxuy@google.com>
qiwzhang
qiwzhang reviewed Jan 21, 2021
View reviewed changes
@TAOXUY
NIT
46f3ed9 
Signed-off-by: Xuyang Tao <taoxuy@google.com>
qiwzhang
qiwzhang previously approved these changes Jan 22, 2021
View reviewed changes
@TAOXUY
NIT
b5c6278 
Signed-off-by: Xuyang Tao <taoxuy@google.com>
@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Jan 24, 2021

GoogleCloudPlatform/esp-v2#178

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Jan 30, 2021

@lizan Please take a look, thanks.

@lizan
Copy link
Copy Markdown
Member

lizan commented Feb 2, 2021

/retest

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Feb 2, 2021

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #14779 (comment) was created by @lizan.

see: more, trace.

@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Feb 7, 2021

@lizan could you please submit it?

@TAOXUY TAOXUY mentioned this pull request Feb 10, 2021
Closed
@TAOXUY
Copy link
Copy Markdown
Contributor Author

TAOXUY commented Feb 16, 2021

@lizan @mattklein123
could you please merge this PR or anything I need to further resolve?

@lizan lizan merged commit 8b0aacc into envoyproxy:main Feb 17, 2021
@wozz wozz mentioned this pull request May 17, 2021
Closed
@TAOXUY TAOXUY mentioned this pull request May 19, 2021
Merged
TAOXUY added a commit to TAOXUY/envoy that referenced this pull request May 20, 2021
@TAOXUY
Revert "JwtAuthn: complete padding on forward jwt payload header (env…
b1435e7 
…oyproxy#14779)"

This reverts commit 8b0aacc.

Signed-off-by: Xuyang Tao <taoxuy@google.com>
lizan pushed a commit that referenced this pull request May 25, 2021
@TAOXUY
Revert "JwtAuthn: complete padding on forward jwt payload header (#14779
2cf511a 
)" (#16576)

This reverts commit 8b0aacc.

Signed-off-by: Xuyang Tao <taoxuy@google.com>
lizan pushed a commit that referenced this pull request May 25, 2021
Revert "JwtAuthn: complete padding on forward jwt payload header (#14779
e07665a 
)" (#16620)

This reverts commit 8b0aacc.

Signed-off-by: Dmitri Dolguikh <ddolguik@redhat.com>
leyao-daily pushed a commit to leyao-daily/envoy that referenced this pull request Sep 30, 2021
@TAOXUY
Revert "JwtAuthn: complete padding on forward jwt payload header (env…
b33af13 
…oyproxy#14779)" (envoyproxy#16576)

This reverts commit 8b0aacc.

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

Reviewers

3 more reviewers

@lizan lizan lizan approved these changes

@nareddyt nareddyt nareddyt left review comments

@qiwzhang qiwzhang qiwzhang left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@lizan lizan

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@TAOXUY @lizan @nareddyt @qiwzhang