-
Notifications
You must be signed in to change notification settings - Fork 72
[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
Conversation
bc9ded2 to
fcdfb59
Compare
fcdfb59 to
693c92c
Compare
315ccaa to
0209639
Compare
701bb68 to
c19a0ef
Compare
c19a0ef to
32e6f81
Compare
There was a problem hiding this 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
Secretabstraction and implementations (secret,Secrets,secretSet,emptySecret). - Update token creation and validation flows, adding trimming/filling of secrets and a unified
Claimsbuilder with generic mods. - Update codebase and tests to use the new
Claims.SignandSecret.Validatemethods, 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
GetTokenSecretStringandGetTokenSecretare 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
Validatefunction only usesjwt.WithIssuedAt()and does not enforce expiration checks, so expired tokens will be accepted. Consider addingjwt.WithExpiry()or a similar option tojwt.Parseto validate theexpclaim.
func Validate(t string, secret []byte) (Token, error) {
pkg/util/token/secrets.go:52
- [nitpick] The
Secrets.Signmethod 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
emptyModRis 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 {
No description provided.