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

Introduce changes from master to sig-auth-acceptance branch #300

Open
wants to merge 9 commits into
base: sig-auth-acceptance
Choose a base branch
from
+645 −873
Open

Introduce changes from master to sig-auth-acceptance branch #300

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
b438fe7
go.*: bump deps
ibihim Jun 28, 2024
ad75c81
c/k/app,p/authn,test/e2e/oidc: fix oidc after bump
ibihim Jun 28, 2024
c598642
c/k/a/options: add oidc-username-prefix
ibihim Jul 2, 2024
7382c08
c/k/a/opts: add format to info log
ibihim Jun 25, 2024
7aaa847
examples,scripts,test: removed deprecated flags
ibihim Jun 26, 2024
142ff30
c/k/app: add the ability to disable http/2
ibihim Jun 27, 2024
a817834
Makefile: make e2e work for arm64
ibihim Jun 26, 2024
c7e1aff
Makefile: Introdocue Makefile from master
ibihim Jun 28, 2024
0b4a724
c/k/app,pkg/{authn,server}: use upstream oidc opts
ibihim Jul 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/kube-rbac-proxy/app/kube-rbac-proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ func Run(cfg *server.KubeRBACProxyConfig) error {
var authenticator authenticator.Request
// If OIDC configuration provided, use oidc authenticator
if cfg.KubeRBACProxyInfo.OIDC.IssuerURL != "" {
oidcAuthenticator, err := authn.NewOIDCAuthenticator(cfg.KubeRBACProxyInfo.OIDC)
oidcAuthenticator, err := authn.NewOIDCAuthenticator(ctx, cfg.KubeRBACProxyInfo.OIDC)
if err != nil {
return fmt.Errorf("failed to instantiate OIDC authenticator: %w", err)
}
Expand Down Expand Up @@ -232,7 +232,7 @@ func Run(cfg *server.KubeRBACProxyConfig) error {

handler := identityheaders.WithAuthHeaders(proxy, cfg.KubeRBACProxyInfo.UpstreamHeaders)
handler = kubefilters.WithAuthorization(handler, authz, scheme.Codecs)
handler = kubefilters.WithAuthentication(handler, authenticator, http.HandlerFunc(filters.UnauthorizedHandler), cfg.DelegatingAuthentication.APIAudiences)
handler = kubefilters.WithAuthentication(handler, authenticator, http.HandlerFunc(filters.UnauthorizedHandler), cfg.DelegatingAuthentication.APIAudiences, nil)
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens when you supply the request header nil here? It won't get autodetected from the cluster, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

TODO add explanation of what nil does.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It enables you to cleanse custom headers that you use for your proxy to contain additional information. It is necessary to drop those.

We write custom headers in case of request headers and we read custom headers based on the rewrite logic: link.
The rewrite logic for Query parameters relies on upstream to interpret them, which is usually a hostile behavior that you want to avoid, but in our case, we want Prometheus to be able to interpret it. Therefore I am hesitant to introduce it for custom headers as well. We could add a boolean flag that defaults to true, which enables / disables cleansing of header values? WDYT @stlaz?

Ref: k8s.io/apiserver/pkg/endpoints/filters/authentication.go

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should avoid forwarding the default headers at least, which is what WithAuthentication()does already.

Generally I think we should drop any headers we're processing, more so if we're using them for authentication. I don't think we wired any authn via request headers, correct?

Outside of that we should also make sure that none of the headers we produce can be just passed upstream from the original request.

// passing an empty RequestInfoFactory results in attaching a non-resource RequestInfo to the context
handler = kubefilters.WithRequestInfo(handler, &request.RequestInfoFactory{})
handler = rewrite.WithKubeRBACProxyParamsHandler(handler, cfg.KubeRBACProxyInfo.Authorization.RewriteAttributesConfig)
Expand Down
2 changes: 1 addition & 1 deletion pkg/authn/identityheaders/identityheaders_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ func TestProxyWithOIDCSupport(t *testing.T) {
handler := http.Handler(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}))
handler = identityheaders.WithAuthHeaders(handler, cfg)
handler = kubefilters.WithAuthorization(handler, v.authorizer, scheme.Codecs)
handler = kubefilters.WithAuthentication(handler, authenticator, http.HandlerFunc(filters.UnauthorizedHandler), []string{})
handler = kubefilters.WithAuthentication(handler, authenticator, http.HandlerFunc(filters.UnauthorizedHandler), []string{}, nil)
handler = kubefilters.WithRequestInfo(handler, &request.RequestInfoFactory{})

handler.ServeHTTP(w, v.req)
Expand Down
27 changes: 19 additions & 8 deletions pkg/authn/oidc.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"context"
"net/http"

"k8s.io/apiserver/pkg/apis/apiserver"
"k8s.io/apiserver/pkg/authentication/authenticator"
"k8s.io/apiserver/pkg/authentication/request/bearertoken"
"k8s.io/apiserver/pkg/server/dynamiccertificates"
Expand All @@ -36,20 +37,30 @@ var (
)

// NewOIDCAuthenticator returns OIDC authenticator
func NewOIDCAuthenticator(config *OIDCConfig) (*OIDCAuthenticator, error) {
func NewOIDCAuthenticator(ctx context.Context, config *OIDCConfig) (*OIDCAuthenticator, error) {
dyCA, err := dynamiccertificates.NewDynamicCAContentFromFile("oidc-ca", config.CAFile)
if err != nil {
return nil, err
}

tokenAuthenticator, err := oidc.New(oidc.Options{
IssuerURL: config.IssuerURL,
ClientID: config.ClientID,
tokenAuthenticator, err := oidc.New(ctx, oidc.Options{
JWTAuthenticator: apiserver.JWTAuthenticator{
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we remove the old options and just wire the new config, since we have the chance?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@stlaz, WDYT?

a610979

Copy link
Collaborator

Choose a reason for hiding this comment

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

Not quite what I had in mind. Keeping the legacy struct for legacy options is fine, and I think preferable.

Perhaps this is not for this PR (I'd rather we removed a610979) but why don't we try to move https://github.com/kubernetes/kubernetes/blob/04434b7bf4f8afe258fed430f163102abe1a24d5/pkg/kubeapiserver/options/authentication.go#L719-L787 somewhere where we could import it - it's how most of the other functionality in that file is implemented and this TODO mentions that it might be preferable here, too.

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,
},
},
},
CAContentProvider: dyCA,
UsernameClaim: config.UsernameClaim,
UsernamePrefix: config.UsernamePrefix,
GroupsClaim: config.GroupsClaim,
GroupsPrefix: config.GroupsPrefix,
SupportedSigningAlgs: config.SupportedSigningAlgs,
})
if err != nil {
Expand Down