[WIP] Mo review

cmd/kube-rbac-proxy/app/kube-rbac-proxy.go

@@ -70,13 +70,15 @@ 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
 		},
 		RunE: func(cmd *cobra.Command, args []string) error {
 			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,
@@ -101,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()
@@ -168,11 +163,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
@@ -184,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
 	}
@@ -194,9 +190,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.
@@ -215,28 +214,33 @@ 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()
 	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")
@@ -307,33 +311,40 @@ func secureServerRunner(
 	}
 
 	interrupter := func(err error) {
+		// TODO(enj): log the error?
 		serverCtxCancel()
 	}
 
 	return runner, interrupter
 }
 
 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)
 	}
 
-	var authz authorizer.Authorizer = 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)
 	}
 
+	// 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)
 	}
 
+	// TODO(enj): what should the union logic do if an authorizer returns an error?  currently it keeps going
+
 	return authz, nil
 }

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?
 }

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
 }

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
 }

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)
 	})
 }

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)
 }