Skip to content

[Bugfix] Fix JWT Secret Tail characters #1867

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.

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
merged 4 commits into from
Jul 16, 2025
Merged

Conversation

ajanikow
Copy link
Collaborator

@ajanikow ajanikow commented Apr 9, 2025

No description provided.

@cla-bot cla-bot bot added the cla-signed label Apr 9, 2025
@ajanikow ajanikow force-pushed the bugfix/fix_secret_discovery branch 2 times, most recently from bc9ded2 to fcdfb59 Compare April 29, 2025 14:55
@ajanikow ajanikow force-pushed the bugfix/fix_secret_discovery branch from fcdfb59 to 693c92c Compare May 14, 2025 11:28
@ajanikow ajanikow force-pushed the bugfix/fix_secret_discovery branch 5 times, most recently from 315ccaa to 0209639 Compare June 3, 2025 08:01
@ajanikow ajanikow force-pushed the bugfix/fix_secret_discovery branch from 701bb68 to c19a0ef Compare July 14, 2025 15:50
@ajanikow ajanikow requested a review from Copilot July 14, 2025 16:02
Copilot

This comment was marked as outdated.

@ajanikow ajanikow force-pushed the bugfix/fix_secret_discovery branch from c19a0ef to 32e6f81 Compare July 15, 2025 17:20
@ajanikow ajanikow requested a review from Copilot July 15, 2025 17:20
Copilot

This comment was marked as outdated.

@ajanikow ajanikow requested a review from Copilot July 16, 2025 07:08
Copilot

This comment was marked as outdated.

@ajanikow ajanikow requested a review from Copilot July 16, 2025 07:34
Copilot

This comment was marked as outdated.

@ajanikow ajanikow requested a review from Copilot July 16, 2025 08:17
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors JWT handling by introducing a flexible Secret interface and related types, standardizing token creation and validation, and updating all callers to use the new API.

  • Replace raw byte-based token parsing with a Secret abstraction and implementations (secret, Secrets, secretSet, emptySecret).
  • Update token creation and validation flows, adding trimming/filling of secrets and a unified Claims builder with generic mods.
  • Update codebase and tests to use the new Claims.Sign and Secret.Validate methods, including trimming and filling behaviors.

Reviewed Changes

Copilot reviewed 29 out of 29 changed files in this pull request and generated no comments.

Show a summary per file
File Description
pkg/util/token/token.go Introduce Validate and newToken, remove old Parse.
pkg/util/token/secrets.go Add Secrets slice type and NewSecrets.
pkg/util/token/secret_set.go Implement secretSet wrapping primary and secondary.
pkg/util/token/secret_empty.go Add emptySecret for no-secret scenarios.
pkg/util/token/secret.go Core secret implementation with trimming/filling.
pkg/util/token/mods.go Migrate to generic ModR, remove older Mod type.
pkg/util/token/*.go (interface, consts, claims, method) Define interfaces, constants, and claims builder.
pkg/util/k8sutil/secrets.go Overload GetTokenSecret/GetTokenSecretString.
Multiple callers/tests Updated callers across codebase and tests to use new API.

Comments suppressed due to low confidence (5)

pkg/util/token/token_test.go:207

  • [nitpick] This test name is duplicated earlier in the file, which can be confusing. Rename one of the tests to clearly describe its distinct purpose (e.g., "Ensure token gets trimmed on both ends").
	t.Run("Ensure token gets trimmed", func(t *testing.T) {

pkg/util/k8sutil/secrets.go:265

  • [nitpick] The comments for GetTokenSecretString and GetTokenSecret are nearly identical and do not distinguish their return types (string vs. token.Secret). Please clarify each comment to reflect the correct return value and behaviour.
// GetTokenSecretString loads the token secret from a Secret with given name.

pkg/util/token/token.go:29

  • The Validate function only uses jwt.WithIssuedAt() and does not enforce expiration checks, so expired tokens will be accepted. Consider adding jwt.WithExpiry() or a similar option to jwt.Parse to validate the exp claim.
func Validate(t string, secret []byte) (Token, error) {

pkg/util/token/secrets.go:52

  • [nitpick] The Secrets.Sign method is unimplemented and always returns an unsupported error. If consumers will never call it, consider removing it; otherwise provide a real implementation or document its stub behavior.
func (s Secrets) Sign(method jwt.SigningMethod, claims Claims) (string, error) {

pkg/util/mod.go:105

  • [nitpick] The helper emptyModR is defined after it's used and not referenced here. To improve readability, move its declaration above or inline as needed.
func ApplyModsR[T any](in T, mods ...ModR[T]) T {

@ajanikow ajanikow merged commit aa56b20 into master Jul 16, 2025
3 checks passed
@ajanikow ajanikow deleted the bugfix/fix_secret_discovery branch July 16, 2025 08:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants