From 31cac6f72026db8ed8265ea4e6e09e470c545514 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Thu, 30 Mar 2023 10:55:05 -0400 Subject: [PATCH 1/3] Mo review Signed-off-by: Monis Khan --- cmd/kube-rbac-proxy/app/kube-rbac-proxy.go | 35 ++++++++++++++-------- pkg/authorization/rewrite/rewrite.go | 3 +- pkg/authorization/static/static.go | 7 ++--- pkg/server/config.go | 2 +- 4 files changed, 29 insertions(+), 18 deletions(-) diff --git a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go index f752f0cbb..45c1dd952 100644 --- a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go +++ b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go @@ -77,6 +77,7 @@ that can perform RBAC authorization against the Kubernetes API using SubjectAcce verflag.PrintAndExitIfRequested() // convert previous version of options + // TODO(enj): rename to ApplyTo so that this is consistent with other methods that mutate inputs if err := o.LegacyOptions.ConvertToNewOptions( o.SecureServing, o.DelegatingAuthentication, @@ -168,11 +169,12 @@ func Complete(o *options.ProxyRunOptions) (*completedProxyRunOptions, error) { } func Run(opts *completedProxyRunOptions) error { + // TODO(enj): Run should not mutate the input config, Complete should handle that cfg, err := createKubeRBACProxyConfig(opts) if err != nil { return err } - ctx, cancel := context.WithCancel(context.Background()) + ctx, cancel := context.WithCancel(context.Background()) // TODO(enj): signal context instead of background context? defer cancel() var authenticator authenticator.Request @@ -194,9 +196,12 @@ func Run(opts *completedProxyRunOptions) error { return fmt.Errorf("failed to setup an authorizer: %v", err) } + // TODO(enj): this uses the deprecated Director func, we should instead use the Rewrite func + // also confirm that custom Connection headers cannot be used to drop headers proxy := httputil.NewSingleHostReverseProxy(cfg.KubeRBACProxyInfo.UpstreamURL) proxy.Transport = cfg.KubeRBACProxyInfo.UpstreamTransport + // TODO(enj): so plaintext to the backend is fine but only if the backend is on 127.0.0.1 or ::1 or unix domain socket if cfg.KubeRBACProxyInfo.UpstreamForceH2C { // Force http/2 for connections to the upstream i.e. do not start with HTTP1.1 UPGRADE req to // initialize http/2 session. @@ -221,22 +226,27 @@ func Run(opts *completedProxyRunOptions) error { mux := http.NewServeMux() mux.Handle("/", handler) - gr := &run.Group{} - { - if len(opts.LegacyOptions.SecureListenAddress) > 0 { - gr.Add(secureServerRunner(ctx, cfg.SecureServing, mux)) + gr := &run.Group{} // TODO(enj): can we use the safe wait group from the k/k code? + func() { + if len(opts.LegacyOptions.SecureListenAddress) == 0 { + return + } - if cfg.KubeRBACProxyInfo.ProxyEndpointsSecureServing != nil { - proxyEndpointsMux := http.NewServeMux() - proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("ok")) }) + gr.Add(secureServerRunner(ctx, cfg.SecureServing, mux)) - gr.Add(secureServerRunner(ctx, cfg.KubeRBACProxyInfo.ProxyEndpointsSecureServing, proxyEndpointsMux)) - } + if cfg.KubeRBACProxyInfo.ProxyEndpointsSecureServing != nil { + // TODO(enj): describe the need for two listeners and consider if any checks need to be made to assert healthyness + proxyEndpointsMux := http.NewServeMux() + proxyEndpointsMux.HandleFunc("/healthz", func(w http.ResponseWriter, r *http.Request) { _, _ = w.Write([]byte("ok")) }) + + gr.Add(secureServerRunner(ctx, cfg.KubeRBACProxyInfo.ProxyEndpointsSecureServing, proxyEndpointsMux)) } - } + }() { sig := make(chan os.Signal, 1) gr.Add(func() error { + // TODO(enj): I would expect this to be part of the context + _ = serverconfig.SetupSignalContext signal.Notify(sig, os.Interrupt, syscall.SIGTERM) <-sig klog.Info("received interrupt, shutting down") @@ -319,7 +329,7 @@ func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverco return nil, fmt.Errorf("failed to create static authorizer: %w", err) } - var authz authorizer.Authorizer = rewrite.NewRewritingAuthorizer( + authz := rewrite.NewRewritingAuthorizer( union.New( staticAuthorizer, delegatedAuthz.Authorizer, @@ -331,6 +341,7 @@ func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverco authz = union.New(path.NewAllowPathAuthorizer(allowPaths), authz) } + // TODO(enj): add comment as to why you cannot use the regular path authorizer for the k/k code if ignorePaths := krbInfo.IgnorePaths; len(ignorePaths) > 0 { authz = union.New(path.NewAlwaysAllowPathAuthorizer(ignorePaths), authz) } diff --git a/pkg/authorization/rewrite/rewrite.go b/pkg/authorization/rewrite/rewrite.go index d3344e26e..73450f021 100644 --- a/pkg/authorization/rewrite/rewrite.go +++ b/pkg/authorization/rewrite/rewrite.go @@ -66,7 +66,7 @@ type ResourceAttributes struct { Name string `json:"name,omitempty"` } -func NewRewritingAuthorizer(delegate authorizer.Authorizer, config *RewriteAttributesConfig) *rewritingAuthorizer { +func NewRewritingAuthorizer(delegate authorizer.Authorizer, config *RewriteAttributesConfig) authorizer.Authorizer { rewriteConfig := config if rewriteConfig == nil { rewriteConfig = &RewriteAttributesConfig{} @@ -189,6 +189,7 @@ func templateWithValue(templateString, value string) string { func WithKubeRBACProxyParamsHandler(handler http.Handler, config *RewriteAttributesConfig) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + // TODO(enj): this needs to describe why we are taking untrusted input and doing magic re-writes with it r = r.WithContext(WithKubeRBACProxyParams(r.Context(), requestToParams(config, r))) handler.ServeHTTP(w, r) }) diff --git a/pkg/authorization/static/static.go b/pkg/authorization/static/static.go index 755d2c245..527ef0148 100644 --- a/pkg/authorization/static/static.go +++ b/pkg/authorization/static/static.go @@ -39,7 +39,7 @@ type StaticAuthorizationConfig struct { type UserConfig struct { Name string `json:"name,omitempty"` - Groups []string `json:"groups,omitempty"` + Groups []string `json:"groups,omitempty"` // TODO(enj): this field is unused, is that on purpose? } type staticAuthorizer struct { @@ -47,7 +47,7 @@ type staticAuthorizer struct { } // NewStaticAuthorizer creates an authorizer for static SubjectAccessReviews -func NewStaticAuthorizer(config []StaticAuthorizationConfig) (*staticAuthorizer, error) { +func NewStaticAuthorizer(config []StaticAuthorizationConfig) (authorizer.Authorizer, error) { for _, c := range config { if c.ResourceRequest != (c.Path == "") { return nil, fmt.Errorf("invalid configuration: resource requests must not include a path: %v", config) @@ -60,9 +60,8 @@ func (saConfig StaticAuthorizationConfig) Matches(a authorizer.Attributes) bool isAllowed := func(staticConf string, requestVal string) bool { if staticConf == "" { return true - } else { - return staticConf == requestVal } + return staticConf == requestVal } userName := "" diff --git a/pkg/server/config.go b/pkg/server/config.go index 00cf742f2..572879a9d 100644 --- a/pkg/server/config.go +++ b/pkg/server/config.go @@ -92,7 +92,7 @@ func (i *KubeRBACProxyInfo) SetUpstreamTransport(upstreamCAPath, upstreamClientC return errors.New("error parsing upstream CA certificate") } - transport.ForceAttemptHTTP2 = false + transport.ForceAttemptHTTP2 = false // TODO(enj): is this meant to be left true when both if statements are skipped? transport.TLSClientConfig = &tls.Config{RootCAs: upstreamCACertPool} } From 5822c37b4cce6fd3d27a352b2cd803bbb3badc70 Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 4 Apr 2023 13:02:59 -0400 Subject: [PATCH 2/3] more comments Signed-off-by: Monis Khan --- cmd/kube-rbac-proxy/app/kube-rbac-proxy.go | 27 ++++++----- pkg/authorization/path/path.go | 4 ++ pkg/authorization/rewrite/rewrite.go | 56 +++++++++++++--------- 3 files changed, 54 insertions(+), 33 deletions(-) diff --git a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go index 45c1dd952..3c2d37706 100644 --- a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go +++ b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go @@ -220,7 +220,7 @@ func Run(opts *completedProxyRunOptions) 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.WithRequestInfo(handler, &request.RequestInfoFactory{}) + handler = kubefilters.WithRequestInfo(handler, &request.RequestInfoFactory{}) // TODO(enj): describe what empty request info factory means handler = rewrite.WithKubeRBACProxyParamsHandler(handler, cfg.KubeRBACProxyInfo.Authorization.RewriteAttributesConfig) mux := http.NewServeMux() @@ -324,18 +324,21 @@ func secureServerRunner( } func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverconfig.AuthorizationInfo) (authorizer.Authorizer, error) { - staticAuthorizer, err := static.NewStaticAuthorizer(krbInfo.Authorization.Static) - if err != nil { - return nil, fmt.Errorf("failed to create static authorizer: %w", err) + authz := delegatedAuthz.Authorizer + + if len(krbInfo.Authorization.Static) > 0 { + staticAuthorizer, err := static.NewStaticAuthorizer(krbInfo.Authorization.Static) + if err != nil { + return nil, fmt.Errorf("failed to create static authorizer: %w", err) + } + authz = union.New(staticAuthorizer, authz) } - authz := rewrite.NewRewritingAuthorizer( - union.New( - staticAuthorizer, - delegatedAuthz.Authorizer, - ), - krbInfo.Authorization.RewriteAttributesConfig, - ) + // TODO(enj): test that this indeed makes it so that the re-writing authz does not get invoked unless configured + if krbInfo.Authorization.RewriteAttributesConfig != nil { + // TODO(enj): does this actually belong in this project? + authz = rewrite.NewRewritingAuthorizer(authz, krbInfo.Authorization.RewriteAttributesConfig) + } if allowPaths := krbInfo.AllowPaths; len(allowPaths) > 0 { authz = union.New(path.NewAllowPathAuthorizer(allowPaths), authz) @@ -346,5 +349,7 @@ func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverco authz = union.New(path.NewAlwaysAllowPathAuthorizer(ignorePaths), authz) } + // TODO(enj): what should the union logic do if an authorizer returns an error? currently it keeps going + return authz, nil } diff --git a/pkg/authorization/path/path.go b/pkg/authorization/path/path.go index 05d90aacc..094c47fa4 100644 --- a/pkg/authorization/path/path.go +++ b/pkg/authorization/path/path.go @@ -23,6 +23,7 @@ import ( "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/authorization/authorizer" + authorizationpath "k8s.io/apiserver/pkg/authorization/path" ) type pathAuthorizer struct { @@ -74,6 +75,7 @@ func (a *pathAuthorizer) Authorize(ctx context.Context, attr authorizer.Attribut return a.noMatchDecision, "", nil } +// NewDenyPathAuthorizerUnlessAllowed ?? func NewAllowPathAuthorizer(allowPaths []string) authorizer.Authorizer { if len(allowPaths) == 0 { return authorizer.AuthorizerFunc(func(context.Context, authorizer.Attributes) (authorizer.Decision, string, error) { @@ -84,5 +86,7 @@ func NewAllowPathAuthorizer(allowPaths []string) authorizer.Authorizer { } func NewAlwaysAllowPathAuthorizer(alwaysAllowPaths []string) authorizer.Authorizer { + _ = authorizationpath.NewAuthorizer // <-- just use this one ?? + return newPathAuthorizer(authorizer.DecisionAllow, authorizer.DecisionNoOpinion, alwaysAllowPaths) } diff --git a/pkg/authorization/rewrite/rewrite.go b/pkg/authorization/rewrite/rewrite.go index 73450f021..9c1d31496 100644 --- a/pkg/authorization/rewrite/rewrite.go +++ b/pkg/authorization/rewrite/rewrite.go @@ -30,6 +30,7 @@ import ( var _ authorizer.Authorizer = &rewritingAuthorizer{} +// TODO(enj): use a dedicated type const rewriterParams = iota type RewriteAttributesConfig struct { @@ -67,13 +68,8 @@ type ResourceAttributes struct { } func NewRewritingAuthorizer(delegate authorizer.Authorizer, config *RewriteAttributesConfig) authorizer.Authorizer { - rewriteConfig := config - if rewriteConfig == nil { - rewriteConfig = &RewriteAttributesConfig{} - } - return &rewritingAuthorizer{ - config: rewriteConfig, + config: config, delegate: delegate, } } @@ -98,8 +94,10 @@ func (n *rewritingAuthorizer) Authorize(ctx context.Context, attrs authorizer.At reason string err error ) + // TODO(enj): describe that this is saying that all proxy attrs must be allowed for this overall statement to be allowed for _, at := range proxyAttrs { authorized, reason, err = n.delegate.Authorize(ctx, at) + // TODO(enj): this fails on error unlike the regular union, so maybe we always want it to fail on error? if err != nil { return authorizer.DecisionDeny, "AuthorizationError", @@ -113,7 +111,7 @@ func (n *rewritingAuthorizer) Authorize(ctx context.Context, attrs authorizer.At } if authorized == authorizer.DecisionAllow { - return authorized, "", nil + return authorizer.DecisionAllow, "", nil } return authorizer.DecisionDeny, @@ -124,20 +122,24 @@ func (n *rewritingAuthorizer) Authorize(ctx context.Context, attrs authorizer.At func (n *rewritingAuthorizer) getKubeRBACProxyAuthzAttributes(ctx context.Context, origAttrs authorizer.Attributes) []authorizer.Attributes { u := origAttrs.GetUser() apiVerb := origAttrs.GetVerb() - path := origAttrs.GetPath() + // path := origAttrs.GetPath() attrs := []authorizer.Attributes{} if n.config.ResourceAttributes == nil { // Default attributes mirror the API attributes that would allow this access to kube-rbac-proxy - return append(attrs, - authorizer.AttributesRecord{ - User: u, - Verb: apiVerb, - ResourceRequest: false, - Path: path, - }) + // TODO(enj): this seems to drop a bunch of data from origAttrs, is this on purpose? + // /apis/namespace/name would get changed from a resource request to a non-resource request? + // return append(attrs, + // authorizer.AttributesRecord{ + // User: u, + // Verb: apiVerb, + // ResourceRequest: false, + // Path: path, + // }) + return []authorizer.Attributes{origAttrs} } + // TODO(enj): describe that this turns a dynamic path based authz check into a static resource authz check if n.config.Rewrites == nil { return append(attrs, authorizer.AttributesRecord{ @@ -151,7 +153,6 @@ func (n *rewritingAuthorizer) getKubeRBACProxyAuthzAttributes(ctx context.Contex Name: n.config.ResourceAttributes.Name, ResourceRequest: true, }) - } params := GetKubeRBACProxyParams(ctx) @@ -178,8 +179,10 @@ func (n *rewritingAuthorizer) getKubeRBACProxyAuthzAttributes(ctx context.Contex } func templateWithValue(templateString, value string) string { + // TODO(enj): make sure on process startup when rewrites are specified that these all valid templates + // and also pre-parse and have them ready for re-use instead of doing it on every request tmpl, _ := template.New("valueTemplate").Parse(templateString) - out := bytes.NewBuffer(nil) + out := bytes.NewBuffer(nil) // TODO(enj): consider using a sync.Pool with buffers that are reset err := tmpl.Execute(out, struct{ Value string }{Value: value}) if err != nil { return "" @@ -188,6 +191,10 @@ func templateWithValue(templateString, value string) string { } func WithKubeRBACProxyParamsHandler(handler http.Handler, config *RewriteAttributesConfig) http.Handler { + if config == nil { + return handler + } + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { // TODO(enj): this needs to describe why we are taking untrusted input and doing magic re-writes with it r = r.WithContext(WithKubeRBACProxyParams(r.Context(), requestToParams(config, r))) @@ -197,15 +204,20 @@ func WithKubeRBACProxyParamsHandler(handler http.Handler, config *RewriteAttribu func requestToParams(config *RewriteAttributesConfig, req *http.Request) []string { params := []string{} - if config == nil || config.Rewrites == nil { + if config.Rewrites == nil { return nil } + // TODO(enj): if this can be collapsed into either by query OR by header, this entire logic can be + // moved into a custom request info parser. that would imply that there can only be a single + // query param as well. + if config.Rewrites.ByQueryParameter != nil && config.Rewrites.ByQueryParameter.Name != "" { if ps, ok := req.URL.Query()[config.Rewrites.ByQueryParameter.Name]; ok { params = append(params, ps...) } } + // TODO(enj): if you only support using a single header, make sure to fail if there is more than one if config.Rewrites.ByHTTPHeader != nil && config.Rewrites.ByHTTPHeader.Name != "" { mimeHeader := textproto.MIMEHeader(req.Header) mimeKey := textproto.CanonicalMIMEHeaderKey(config.Rewrites.ByHTTPHeader.Name) @@ -217,13 +229,13 @@ func requestToParams(config *RewriteAttributesConfig, req *http.Request) []strin } func WithKubeRBACProxyParams(ctx context.Context, params []string) context.Context { + if params == nil { + return ctx + } return request.WithValue(ctx, rewriterParams, params) } func GetKubeRBACProxyParams(ctx context.Context) []string { - params, ok := ctx.Value(rewriterParams).([]string) - if !ok { - return nil - } + params, _ := ctx.Value(rewriterParams).([]string) return params } From e7a5ea763e4b59b250a41f59d630b5fea69b618f Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Tue, 11 Apr 2023 13:02:10 -0400 Subject: [PATCH 3/3] more comments Signed-off-by: Monis Khan --- cmd/kube-rbac-proxy/app/kube-rbac-proxy.go | 13 ++++--------- .../app/options/legacyoptions.go | 7 ++++--- .../app/options/proxyoptions.go | 16 +++++++++------- pkg/authn/config.go | 2 +- pkg/authn/identityheaders/identityheaders.go | 18 +++++++++++++----- pkg/authorization/rewrite/rewrite.go | 3 +++ 6 files changed, 34 insertions(+), 25 deletions(-) diff --git a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go index 3c2d37706..24d2da5b3 100644 --- a/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go +++ b/cmd/kube-rbac-proxy/app/kube-rbac-proxy.go @@ -70,6 +70,7 @@ that can perform RBAC authorization against the Kubernetes API using SubjectAcce PersistentPreRunE: func(*cobra.Command, []string) error { // silence client-go warnings. // kube-apiserver loopback clients should not log self-issued warnings. + // TODO(enj): we should log warnings if we try to use deprecated kube APIs rest.SetDefaultWarningHandler(rest.NoWarnings{}) return nil }, @@ -102,14 +103,7 @@ that can perform RBAC authorization against the Kubernetes API using SubjectAcce return Run(completedOptions) }, - Args: func(cmd *cobra.Command, args []string) error { - for _, arg := range args { - if len(arg) > 0 { - return fmt.Errorf("%q does not take any arguments, got %q", cmd.CommandPath(), args) - } - } - return nil - }, + Args: cobra.NoArgs, } fs := cmd.Flags() @@ -186,7 +180,7 @@ func Run(opts *completedProxyRunOptions) error { } go oidcAuthenticator.Run(ctx) - authenticator = oidcAuthenticator + authenticator = oidcAuthenticator // TODO(enj): wrap as audience agnostic } else { authenticator = cfg.DelegatingAuthentication.Authenticator } @@ -317,6 +311,7 @@ func secureServerRunner( } interrupter := func(err error) { + // TODO(enj): log the error? serverCtxCancel() } diff --git a/cmd/kube-rbac-proxy/app/options/legacyoptions.go b/cmd/kube-rbac-proxy/app/options/legacyoptions.go index 68f86761a..d06ddef77 100644 --- a/cmd/kube-rbac-proxy/app/options/legacyoptions.go +++ b/cmd/kube-rbac-proxy/app/options/legacyoptions.go @@ -43,15 +43,16 @@ func (o *LegacyOptions) AddFlags(flagset *pflag.FlagSet) { // kube-rbac-proxy flags flagset.StringVar(&o.SecureListenAddress, "secure-listen-address", "", "The address the kube-rbac-proxy HTTPs server should listen on.") - //Kubeconfig flag + // Kubeconfig flag flagset.StringVar(&o.KubeconfigLocation, "kubeconfig", "", "Path to a kubeconfig file, specifying how to connect to the API server. If unset, in-cluster configuration will be used") } func (o *LegacyOptions) Validate() []error { - var errs []error + var errs []error // TODO(enj): validate?? return errs } +// TODO(enj): rename to ApplyTo func (o *LegacyOptions) ConvertToNewOptions( so *genericoptions.SecureServingOptions, authn *genericoptions.DelegatingAuthenticationOptions, @@ -82,5 +83,5 @@ func (o *LegacyOptions) ConvertToNewOptions( } func (o *LegacyOptions) ApplyTo(c *server.KubeRBACProxyInfo) error { - return nil + return nil // TODO(enj): why an empty function? } diff --git a/cmd/kube-rbac-proxy/app/options/proxyoptions.go b/cmd/kube-rbac-proxy/app/options/proxyoptions.go index 48d46d4bd..373addb6a 100644 --- a/cmd/kube-rbac-proxy/app/options/proxyoptions.go +++ b/cmd/kube-rbac-proxy/app/options/proxyoptions.go @@ -45,9 +45,9 @@ type ProxyOptions struct { UpstreamHeader *identityheaders.AuthnHeaderConfig - ConfigFileName string - AllowPaths []string - IgnorePaths []string + AuthzConfigFileName string + AllowPaths []string + IgnorePaths []string ProxyEndpointsPort int @@ -64,7 +64,7 @@ func (o *ProxyOptions) AddFlags(flagset *pflag.FlagSet) { flagset.StringVar(&o.UpstreamClientCertFile, "upstream-client-cert-file", "", "If set, the client will be used to authenticate the proxy to upstream. Requires --upstream-client-key-file to be set, too.") flagset.StringVar(&o.UpstreamClientKeyFile, "upstream-client-key-file", "", "The key matching the certificate from --upstream-client-cert-file. If set, requires --upstream-client-cert-file to be set, too.") - flagset.StringVar(&o.ConfigFileName, "config-file", "", "Configuration file to configure static and rewrites authorization of the kube-rbac-proxy.") + flagset.StringVar(&o.AuthzConfigFileName, "config-file", "", "Configuration file to configure static and rewrites authorization of the kube-rbac-proxy.") flagset.StringSliceVar(&o.AllowPaths, "allow-paths", nil, "Comma-separated list of paths against which kube-rbac-proxy pattern-matches the incoming request. If the request doesn't match, kube-rbac-proxy responds with a 404 status code. If omitted, the incoming request path isn't checked. Cannot be used with --ignore-paths.") flagset.StringSliceVar(&o.IgnorePaths, "ignore-paths", nil, "Comma-separated list of paths against which kube-rbac-proxy pattern-matches the incoming request. If the requst matches, it will proxy the request without performing an authentication or authorization check. Cannot be used with --allow-paths.") @@ -73,7 +73,7 @@ func (o *ProxyOptions) AddFlags(flagset *pflag.FlagSet) { flagset.StringVar(&o.UpstreamHeader.GroupsFieldName, "auth-header-groups-field-name", "x-remote-groups", "The name of the field inside a http(2) request header to tell the upstream server about the user's groups") flagset.StringVar(&o.UpstreamHeader.GroupSeparator, "auth-header-groups-field-separator", "|", "The separator string used for concatenating multiple group names in a groups header field's value") - //Authn OIDC flags + // Authn OIDC flags flagset.StringVar(&o.OIDC.IssuerURL, "oidc-issuer", "", "The URL of the OpenID issuer, only HTTPS scheme will be accepted. If set, it will be used to verify the OIDC JSON Web Token (JWT).") flagset.StringVar(&o.OIDC.ClientID, "oidc-clientID", "", "The client ID for the OpenID Connect client, must be set if oidc-issuer-url is set.") flagset.StringVar(&o.OIDC.GroupsClaim, "oidc-groups-claim", "groups", "Identifier of groups in JWT claim, by default set to 'groups'") @@ -128,7 +128,7 @@ func (o *ProxyOptions) ApplyTo(c *server.KubeRBACProxyInfo, a *serverconfig.Auth return fmt.Errorf("failed to setup transport for upstream: %w", err) } - if configFileName := o.ConfigFileName; len(configFileName) > 0 { + if configFileName := o.AuthzConfigFileName; len(configFileName) > 0 { c.Authorization, err = parseAuthorizationConfigFile(configFileName) if err != nil { return fmt.Errorf("failed to read the config file: %w", err) @@ -138,7 +138,9 @@ func (o *ProxyOptions) ApplyTo(c *server.KubeRBACProxyInfo, a *serverconfig.Auth c.OIDC = o.OIDC c.IgnorePaths = o.IgnorePaths c.AllowPaths = o.AllowPaths - a.APIAudiences = o.TokenAudiences + // TODO(enj): at a min, we should require SA tokens created by the token request API to have an aud that is not the API server's + // maybe require opt-in to support legacy SA tokens which lack aud protection? + a.APIAudiences = o.TokenAudiences // TODO(enj): this cannot be set when OIDC is in use (because it doesn't make sense) return nil } diff --git a/pkg/authn/config.go b/pkg/authn/config.go index 216bacabd..50bf8e262 100644 --- a/pkg/authn/config.go +++ b/pkg/authn/config.go @@ -37,5 +37,5 @@ type OIDCConfig struct { UsernamePrefix string GroupsClaim string GroupsPrefix string - SupportedSigningAlgs []string + SupportedSigningAlgs []string // TODO(enj): what does this actually mean } diff --git a/pkg/authn/identityheaders/identityheaders.go b/pkg/authn/identityheaders/identityheaders.go index 98a063269..8bde85d0f 100644 --- a/pkg/authn/identityheaders/identityheaders.go +++ b/pkg/authn/identityheaders/identityheaders.go @@ -37,6 +37,7 @@ type AuthnHeaderConfig struct { // WithAuthHeaders adds identity information to the headers. // Must not be used, if connection is not encrypted with TLS. +// TODO(enj): this must only be allowed to be used on 127.0.0.1 / ::1 OR a mTLS connection func WithAuthHeaders(handler http.Handler, cfg *AuthnHeaderConfig) http.Handler { upstreamHeadersEnabled := len(cfg.GroupsFieldName) > 0 || len(cfg.UserFieldName) > 0 if !upstreamHeadersEnabled { @@ -45,13 +46,20 @@ func WithAuthHeaders(handler http.Handler, cfg *AuthnHeaderConfig) http.Handler return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { u, ok := request.UserFrom(req.Context()) - if ok { - // Seemingly well-known headers to tell the upstream about user's identity - // so that the upstream can achieve the original goal of delegating RBAC authn/authz to kube-rbac-proxy - req.Header.Set(cfg.UserFieldName, u.GetName()) - req.Header.Set(cfg.GroupsFieldName, strings.Join(u.GetGroups(), cfg.GroupSeparator)) + if !ok { + http.Error(w, "no user info on request", http.StatusInternalServerError) + return } + // Seemingly well-known headers to tell the upstream about user's identity + // so that the upstream can achieve the original goal of delegating RBAC authn/authz to kube-rbac-proxy + req.Header.Set(cfg.UserFieldName, u.GetName()) + // TODO(enj): this is cleaner with multiple header values so that group names can contain the group separator + // this code does not do anything to escape groups that contain the separator + // at a minimum, groups that contain the separator should be dropped and logged + // also, you need to drop the username and group headers from the incoming request + req.Header.Set(cfg.GroupsFieldName, strings.Join(u.GetGroups(), cfg.GroupSeparator)) + handler.ServeHTTP(w, req) }) } diff --git a/pkg/authorization/rewrite/rewrite.go b/pkg/authorization/rewrite/rewrite.go index 9c1d31496..ab0355e4d 100644 --- a/pkg/authorization/rewrite/rewrite.go +++ b/pkg/authorization/rewrite/rewrite.go @@ -41,6 +41,9 @@ type RewriteAttributesConfig struct { // SubjectAccessReviewRewrites describes how SubjectAccessReview may be // rewritten on a given request. type SubjectAccessReviewRewrites struct { + // TODO(enj): ideally, we can drop this config, and have something very specific to the prometheus use case + // if we cant do that for some reason, I would like to support only header based config + // and if we cant do that, this needs to be documented as a rather scary config ByQueryParameter *QueryParameterRewriteConfig `json:"byQueryParameter,omitempty"` ByHTTPHeader *HTTPHeaderRewriteConfig `json:"byHttpHeader,omitempty"` }