Skip to content

fix: Do not add padding in Client-Side CAB tokens. #1728

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

Merged
lqiu96 merged 4 commits into from
Apr 22, 2025
Merged

fix: Do not add padding in Client-Side CAB tokens. #1728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0be74eb
fix: Do not add padding in Client-Side CAB tokens.
huangjiahua Apr 1, 2025
0dd70a0
Add unit test assertions to verify the generated token has no padding.
huangjiahua Apr 11, 2025
03154a1
Add comment to explain why padding is removed.
huangjiahua Apr 11, 2025
b16cfde
Run mvn format.
huangjiahua Apr 15, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion ...a/com/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,8 +206,13 @@ public AccessToken generateToken(CredentialAccessBoundary accessBoundary)

byte[] encryptedRestrictions = this.encryptRestrictions(rawRestrictions, sessionKey);

// withoutPadding() is used to stay consistent with server-side CAB
// withoutPadding() avoids additional URL encoded token issues (i.e. extra equal signs `=` in
// the path)
String tokenValue =
intermediateToken + "." + Base64.getUrlEncoder().encodeToString(encryptedRestrictions);
intermediateToken
+ "."
+ Base64.getUrlEncoder().withoutPadding().encodeToString(encryptedRestrictions);

return new AccessToken(tokenValue, intermediateTokenExpirationTime);
}
Expand Down
10 changes: 10 additions & 0 deletions ...m/google/auth/credentialaccessboundary/ClientSideCredentialAccessBoundaryFactoryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -745,6 +745,11 @@ public void generateToken_withAvailablityCondition_success() throws Exception {
CabToken cabToken = parseCabToken(token);
assertEquals("accessToken", cabToken.intermediateToken);

// Base64 encoding output by default has `=` padding at the end if the input length
// is not a multiple of 3. Here we verify the use of `withoutPadding` that removes
// this padding.
assertFalse(cabToken.encryptedRestriction.contains(String.valueOf("=")));
Copy link
Member

@lqiu96 lqiu96 Apr 14, 2025

Choose a reason for hiding this comment

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

qq, does the Base64.Encoder keep the = by default? I was assuming that it was keeping something like additional spaces at the end. Would the test case make more sense to ensure that the token value doesn't end with spaces? If so, do you have an example/ mock token that runs into these padding issues?

Copy link
Contributor Author

@huangjiahua huangjiahua Apr 14, 2025

Choose a reason for hiding this comment

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

= is used to pad the base64 encoding output if the input length in bytes is not a multiple of 3. This is the default behavior for Base64.Encoder if withoutPadding is not used. The base64 encoded restriction of this test case includes the padding at the end.

Copy link
Member

@lqiu96 lqiu96 Apr 15, 2025

Choose a reason for hiding this comment

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

Gotcha, thanks! Can you just add a small comment above in the test to explain that padding adds = for values not in multiple of 3 (just for future maintainers who don't know this/ aren't familiar with padding)

Copy link
Contributor Author

@huangjiahua huangjiahua Apr 15, 2025

Choose a reason for hiding this comment

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

Added.


// Checks the encrypted restriction is the correct proto format of the CredentialAccessBoundary.
ClientSideAccessBoundary clientSideAccessBoundary =
decryptRestriction(
Expand Down Expand Up @@ -795,6 +800,11 @@ public void generateToken_withoutAvailabilityCondition_success() throws Exceptio
CabToken cabToken = parseCabToken(token);
assertEquals("accessToken", cabToken.intermediateToken);

// Base64 encoding output by default has `=` padding at the end if the input length
// is not a multiple of 3. Here we verify the use of `withoutPadding` that removes
// this padding.
assertFalse(cabToken.encryptedRestriction.contains(String.valueOf("=")));

// Checks the encrypted restriction is the correct proto format of the CredentialAccessBoundary.
ClientSideAccessBoundary clientSideAccessBoundary =
decryptRestriction(
Expand Down
Loading