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

CORE-548 base64 url decode #18195

Merged
merged 2 commits into from
May 7, 2024
Merged

CORE-548 base64 url decode #18195

merged 2 commits into from
May 7, 2024

Conversation

michael-redpanda
Copy link
Contributor

Creates a base64 URL decoder in utils to replace the one in CryptoPP.

Backports Required

  • none - not a bug fix
  • none - this is a backport
  • none - issue does not exist in previous branches
  • none - papercut/not impactful enough to backport
  • v24.1.x
  • v23.3.x
  • v23.2.x

Release Notes

  • none

@michael-redpanda michael-redpanda requested review from a team, pgellert and BenPope and removed request for a team May 1, 2024 15:57
@vbotbuildovich
Copy link
Collaborator

vbotbuildovich commented May 1, 2024 •
edited
Loading

new failures in https://buildkite.com/redpanda/redpanda/builds/48567#018f3537-f378-44f1-8fc4-3f66439fe57c:

"rptest.tests.read_replica_e2e_test.ReadReplicasUpgradeTest.test_upgrades.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48567#018f3537-f37a-4326-9f14-5b80e5aee3db:

"rptest.tests.upgrade_test.UpgradeFromPriorFeatureVersionCloudStorageTest.test_rolling_upgrade.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48567#018f3540-8920-401f-8dcc-915b82246de6:

"rptest.tests.read_replica_e2e_test.ReadReplicasUpgradeTest.test_upgrades.cloud_storage_type=CloudStorageType.S3"
"rptest.tests.partition_movement_test.SIPartitionMovementTest.test_cross_shard.num_to_upgrade=2.cloud_storage_type=CloudStorageType.S3"
"rptest.tests.partition_movement_test.SIPartitionMovementTest.test_shadow_indexing.num_to_upgrade=2.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48567#018f3540-8928-49e6-a8b7-c1ab6e360055:

"rptest.tests.upgrade_test.UpgradeFromPriorFeatureVersionCloudStorageTest.test_rolling_upgrade.cloud_storage_type=CloudStorageType.S3"

new failures in https://buildkite.com/redpanda/redpanda/builds/48686#018f3f03-dddf-4a8c-84f5-3a08692f9495:

"rptest.tests.cloud_storage_scrubber_test.CloudStorageScrubberTest.test_scrubber.cloud_storage_type=CloudStorageType.ABS"

dotnwat
dotnwat reviewed May 1, 2024
View reviewed changes
src/v/utils/base64.h Outdated Show resolved Hide resolved
src/v/security/jwt.h Outdated Show resolved Hide resolved
src/v/security/jwt.h Outdated Show resolved Hide resolved
pgellert
pgellert reviewed May 2, 2024
View reviewed changes
src/v/security/jwt.h Outdated Show resolved Hide resolved
src/v/utils/base64.cc Show resolved Hide resolved
src/v/utils/base64.cc Show resolved Hide resolved
BenPope
BenPope reviewed May 2, 2024
View reviewed changes
src/v/utils/base64.cc Show resolved Hide resolved
src/v/utils/tests/base64_test.cc Outdated Show resolved Hide resolved
src/v/security/jwt.h Show resolved Hide resolved
@michael-redpanda michael-redpanda force-pushed the CORE-548-base64-url-decode branch from eec7e5c to 1a65a02 Compare May 3, 2024 12:39
@michael-redpanda michael-redpanda force-pushed the CORE-548-base64-url-decode branch from 1a65a02 to c497b4b Compare May 3, 2024 14:08
pgellert
pgellert previously approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@pgellert pgellert left a comment

Choose a reason for hiding this comment

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

nice, lgtm

BenPope
BenPope reviewed May 3, 2024
View reviewed changes
src/v/utils/tests/base64_test.cc Show resolved Hide resolved
src/v/utils/base64.cc Show resolved Hide resolved
BenPope
BenPope previously approved these changes May 3, 2024
View reviewed changes
@michael-redpanda
utils: Added base64 URL decoding
710dc18 
Added a utility function to perform Base64 URL decoding.  This
approach was selected because the base64 library we currently use
does not support encoding/decoding Base64URL messages.  The
only use case we have for this is in OIDC and happens infrequently
enough that performance is not a large concern.

Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda
s/oidc: Replaced cryptopp with utility function
f0dd8f4 
Signed-off-by: Michael Boquard <michael@redpanda.com>
@michael-redpanda michael-redpanda dismissed stale reviews from BenPope and pgellert via f0dd8f4 May 6, 2024 12:37
@michael-redpanda michael-redpanda force-pushed the CORE-548-base64-url-decode branch from c497b4b to f0dd8f4 Compare May 6, 2024 12:37
@michael-redpanda
Copy link
Contributor Author

Force push f0dd8f4:

  • Added failure unit test

@dotnwat dotnwat merged commit 403b139 into redpanda-data:dev May 7, 2024
17 checks passed
@rockwotj
Copy link
Contributor

rockwotj commented May 7, 2024

FYI absl has these: https://github.com/abseil/abseil-cpp/blob/e0df4a721bba5f5bd10d68fc52568ca3ea6b0123/absl/strings/escaping.h#L137

@michael-redpanda
Copy link
Contributor Author

FYI absl has these: https://github.com/abseil/abseil-cpp/blob/e0df4a721bba5f5bd10d68fc52568ca3ea6b0123/absl/strings/escaping.h#L137

who's this guy ;)

@BenPope
Copy link
Member

BenPope commented May 7, 2024

FYI absl has these: https://github.com/abseil/abseil-cpp/blob/e0df4a721bba5f5bd10d68fc52568ca3ea6b0123/absl/strings/escaping.h#L137

Even our version has them! Thanks @rockwotj !

@michael-redpanda michael-redpanda mentioned this pull request May 29, 2024
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dotnwat dotnwat dotnwat approved these changes

@pgellert pgellert Awaiting requested review from pgellert

@BenPope BenPope Awaiting requested review from BenPope

Assignees
No one assigned
Labels
area/redpanda
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@michael-redpanda @vbotbuildovich @rockwotj @BenPope @dotnwat @pgellert