-
Notifications
You must be signed in to change notification settings - Fork 195
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
Bump krp to k8s 1.30 #287
Bump krp to k8s 1.30 #287
Conversation
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.
Just one comment about the golang.org/x/net
version
go.mod
Outdated
@@ -8,13 +8,13 @@ require ( | |||
github.com/oklog/run v1.1.0 | |||
github.com/spf13/cobra v1.8.0 | |||
github.com/spf13/pflag v1.0.5 | |||
golang.org/x/net v0.21.0 | |||
golang.org/x/net v0.22.0 |
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.
< v0.23.0 are affected by https://pkg.go.dev/vuln/GO-2024-2687
JWTAuthenticator: apiserver.JWTAuthenticator{ | ||
Issuer: apiserver.Issuer{ | ||
URL: config.IssuerURL, | ||
Audiences: []string{config.ClientID}, | ||
}, | ||
ClaimMappings: apiserver.ClaimMappings{ | ||
Username: apiserver.PrefixedClaimOrExpression{ | ||
Prefix: &config.UsernamePrefix, | ||
Claim: config.UsernameClaim, | ||
}, | ||
Groups: apiserver.PrefixedClaimOrExpression{ | ||
Prefix: &config.GroupsPrefix, | ||
Claim: config.GroupsClaim, | ||
}, | ||
}, | ||
}, |
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.
This change looks good!
@@ -120,6 +120,7 @@ func (o *ProxyRunOptions) Flags() k8sapiflag.NamedFlagSets { | |||
flagset.StringVar(&o.Auth.Authentication.OIDC.ClientID, "oidc-clientID", "", "The client ID for the OpenID Connect client, must be set if oidc-issuer-url is set.") | |||
flagset.StringVar(&o.Auth.Authentication.OIDC.GroupsClaim, "oidc-groups-claim", "groups", "Identifier of groups in JWT claim, by default set to 'groups'") | |||
flagset.StringVar(&o.Auth.Authentication.OIDC.UsernameClaim, "oidc-username-claim", "email", "Identifier of the user in JWT claim, by default set to 'email'") | |||
flagset.StringVar(&o.Auth.Authentication.OIDC.UsernamePrefix, "oidc-username-prefix", "", "If provided, the username will be prefixed with this value to prevent conflicts with other authentication strategies.") | |||
flagset.StringVar(&o.Auth.Authentication.OIDC.GroupsPrefix, "oidc-groups-prefix", "", "If provided, all groups will be prefixed with this value to prevent conflicts with other authentication strategies.") | |||
flagset.StringArrayVar(&o.Auth.Authentication.OIDC.SupportedSigningAlgs, "oidc-sign-alg", []string{"RS256"}, "Supported signing algorithms, default RS256") |
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.
should we mention deprecation of this flag as in upstream?
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.
Yes, we should. Good point.
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.
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.
Interesting. Looks like the only place where this is mentioned is this (recent) enhancement: https://github.com/kubernetes/enhancements/tree/master/keps/sig-auth/3331-structured-authentication-configuration#goals
Feel free to ignore my original comment, then.
IssuerURL: config.IssuerURL, | ||
ClientID: config.ClientID, | ||
tokenAuthenticator, err := oidc.New(ctx, oidc.Options{ | ||
JWTAuthenticator: apiserver.JWTAuthenticator{ |
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.
do we get the dynamic CA for free when we move it here instead of using it as below?
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.
Which of the properties enables a dynamic CA from within JWTAuthenticator?
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.
The dynamic CA content provider is being used to create the transport for the client.
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.
never mind, I seem to have misremembered how the CA file was wired
/lgtm |
@ibihim when are you planning for the next release with these cves fixes? |
What
Bump dependencies to higher versions. Most prominent: k8s v1.29.3
Why
It makes sense to stay up to date with deps.
In particular it should fix the CVE reports about telemetry.io