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

config: allow multiple configurations #164

Closed
wants to merge 3 commits into from
Closed

config: allow multiple configurations #164

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e6e5cc9
config: allow multiple configurations
squat Apr 1, 2022
f57d216
pkg/proxy: make authorization concurrent
squat Apr 1, 2022
98af9c6
pkg/proxy: ensure we only send HTTP response once
squat Apr 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Usage of _output/kube-rbac-proxy:
--auth-header-user-field-name string The name of the field inside a http(2) request header to tell the upstream server about the user's name (default "x-remote-user")
--auth-token-audiences strings Comma-separated list of token audiences to accept. By default a token does not have to have any specific audience. It is recommended to set a specific audience.
--client-ca-file string If set, any request presenting a client certificate signed by one of the authorities in the client-ca-file is authenticated with an identity corresponding to the CommonName of the client certificate.
--config-file string Configuration file to configure kube-rbac-proxy.
--config-file strings Configuration file to configure kube-rbac-proxy. Can be specified multiple times to configure multiple resource authorizations.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would string be more appropriate as it is not a comma-separated list, but it can be specified multiple times? In comparison to --ignore-paths below.

--ignore-paths strings Comma-separated list of paths against which kube-rbac-proxy will proxy without performing an authentication or authorization check. Cannot be used with --allow-paths.
--insecure-listen-address string The address the kube-rbac-proxy HTTP server should listen on.
--kubeconfig string Path to a kubeconfig file, specifying how to connect to the API server. If unset, in-cluster configuration will be used
Expand Down
37 changes: 21 additions & 16 deletions main.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ func main() {
OIDC: &authn.OIDCConfig{},
Token: &authn.TokenConfig{},
},
Authorization: &authz.Config{},
Authorizations: []*authz.Config{},
},
}
configFileName := ""
var configFileNames []string

// Add klog flags
klogFlags := flag.NewFlagSet(os.Args[0], flag.ExitOnError)
Expand All @@ -104,7 +104,7 @@ func main() {
flagset.StringVar(&cfg.upstream, "upstream", "", "The upstream URL to proxy to once requests have successfully been authenticated and authorized.")
flagset.BoolVar(&cfg.upstreamForceH2C, "upstream-force-h2c", false, "Force h2c to communiate with the upstream. This is required when the upstream speaks h2c(http/2 cleartext - insecure variant of http/2) only. For example, go-grpc server in the insecure mode, such as helm's tiller w/o TLS, speaks h2c only")
flagset.StringVar(&cfg.upstreamCAFile, "upstream-ca-file", "", "The CA the upstream uses for TLS connection. This is required when the upstream uses TLS and its own CA certificate")
flagset.StringVar(&configFileName, "config-file", "", "Configuration file to configure kube-rbac-proxy.")
flagset.StringSliceVar(&configFileNames, "config-file", []string{}, "Configuration file to configure kube-rbac-proxy. Can be specified multiple times to configure multiple resource authorizations.")
flagset.StringSliceVar(&cfg.allowPaths, "allow-paths", nil, "Comma-separated list of paths against which kube-rbac-proxy 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(&cfg.ignorePaths, "ignore-paths", nil, "Comma-separated list of paths against which kube-rbac-proxy will proxy without performing an authentication or authorization check. Cannot be used with --allow-paths.")

Expand Down Expand Up @@ -146,21 +146,22 @@ func main() {
klog.Fatalf("Failed to parse upstream URL: %v", err)
}

if configFileName != "" {
klog.Infof("Reading config file: %s", configFileName)
b, err := ioutil.ReadFile(configFileName)
if err != nil {
klog.Fatalf("Failed to read resource-attribute file: %v", err)
}
if len(configFileNames) != 0 {
for _, cfn := range configFileNames {
klog.Infof("Reading config file: %s", cfn)
b, err := ioutil.ReadFile(cfn)
if err != nil {
klog.Fatalf("Failed to read resource-attribute file: %v", err)
}

configfile := configfile{}
configfile := configfile{}

err = yaml.Unmarshal(b, &configfile)
if err != nil {
klog.Fatalf("Failed to parse config file content: %v", err)
}
if err := yaml.Unmarshal(b, &configfile); err != nil {
klog.Fatalf("Failed to parse config file content: %v", err)
}

cfg.auth.Authorization = configfile.AuthorizationConfig
cfg.auth.Authorizations = append(cfg.auth.Authorizations, configfile.AuthorizationConfig)
}
}

kubeClient, err := kubernetes.NewForConfig(kcfg)
Expand Down Expand Up @@ -202,7 +203,11 @@ func main() {
klog.Fatalf("Failed to create sar authorizer: %v", err)
}

staticAuthorizer, err := authz.NewStaticAuthorizer(cfg.auth.Authorization.Static)
var sacs []authz.StaticAuthorizationConfig
for _, ac := range cfg.auth.Authorizations {
sacs = append(sacs, ac.Static...)
}
staticAuthorizer, err := authz.NewStaticAuthorizer(sacs)
if err != nil {
klog.Fatalf("Failed to create static authorizer: %v", err)
}
Expand Down
193 changes: 112 additions & 81 deletions pkg/proxy/proxy.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,9 +18,12 @@ package proxy

import (
"bytes"
"context"
"fmt"
"net/http"
"strings"
"sync"
"sync/atomic"
"text/template"

"github.com/brancz/kube-rbac-proxy/pkg/authn"
Expand All @@ -35,7 +38,7 @@ import (
// Config holds proxy authorization and authentication settings
type Config struct {
Authentication *authn.AuthnConfig
Authorization *authz.Config
Authorizations []*authz.Config
}

type kubeRBACProxy struct {
Expand All @@ -50,7 +53,7 @@ type kubeRBACProxy struct {
}

func new(authenticator authenticator.Request, authorizer authorizer.Authorizer, config Config) *kubeRBACProxy {
return &kubeRBACProxy{authenticator, authorizer, newKubeRBACProxyAuthorizerAttributesGetter(config.Authorization), config}
return &kubeRBACProxy{authenticator, authorizer, newKubeRBACProxyAuthorizerAttributesGetter(config.Authorizations), config}
}

// New creates an authenticator, an authorizer, and a matching authorizer attributes getter compatible with the kube-rbac-proxy
Expand Down Expand Up @@ -88,21 +91,38 @@ func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool {
return false
}

var wg sync.WaitGroup
var authzDone uint32
ctx, cancel := context.WithCancel(ctx)
defer cancel()
for _, attrs := range allAttrs {
// Authorize
authorized, reason, err := h.Authorize(ctx, attrs)
if err != nil {
msg := fmt.Sprintf("Authorization error (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource())
klog.Errorf("%s: %s", msg, err)
http.Error(w, msg, http.StatusInternalServerError)
return false
}
if authorized != authorizer.DecisionAllow {
msg := fmt.Sprintf("Forbidden (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource())
klog.V(2).Infof("%s. Reason: %q.", msg, reason)
http.Error(w, msg, http.StatusForbidden)
return false
}
// Authorize concurrently, as there can be many outbound requests to the kube API.
wg.Add(1)
go func(attrs authorizer.Attributes) {
defer wg.Done()
authorized, reason, err := h.Authorize(ctx, attrs)
if err != nil {
msg := fmt.Sprintf("Authorization error (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource())
klog.Errorf("%s: %s", msg, err)
if atomic.CompareAndSwapUint32(&authzDone, 0, 1) {
http.Error(w, msg, http.StatusInternalServerError)
}
cancel()
return
}
if authorized != authorizer.DecisionAllow {
msg := fmt.Sprintf("Forbidden (user=%s, verb=%s, resource=%s, subresource=%s)", u.User.GetName(), attrs.GetVerb(), attrs.GetResource(), attrs.GetSubresource())
klog.V(2).Infof("%s. Reason: %q.", msg, reason)
if atomic.CompareAndSwapUint32(&authzDone, 0, 1) {
http.Error(w, msg, http.StatusForbidden)
}
cancel()
}
}(attrs)
}
wg.Wait()
if authzDone == 1 {
return false
Copy link
Collaborator

@ibihim ibihim Jun 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The solution is pretty effective. And this part is the reason, why it took me ages to make a PR review. I have huge respect for concurrency and the problems it brings.

I would've tried to use something well known like a fan out pattern or tried to separate the threading logic from the rest as much as possible.

There is a very low level of abstraction in the code base and the hierarchy is pretty flat with lengthy functions, so I won't object as it fits existing code.

}

if h.Config.Authentication.Header.Enabled {
Expand All @@ -116,12 +136,12 @@ func (h *kubeRBACProxy) Handle(w http.ResponseWriter, req *http.Request) bool {
return true
}

func newKubeRBACProxyAuthorizerAttributesGetter(authzConfig *authz.Config) *krpAuthorizerAttributesGetter {
return &krpAuthorizerAttributesGetter{authzConfig}
func newKubeRBACProxyAuthorizerAttributesGetter(authzConfigs []*authz.Config) *krpAuthorizerAttributesGetter {
return &krpAuthorizerAttributesGetter{authzConfigs}
}

type krpAuthorizerAttributesGetter struct {
authzConfig *authz.Config
authzConfigs []*authz.Config
}

// GetRequestAttributes populates authorizer attributes for the requests to kube-rbac-proxy.
Expand All @@ -148,67 +168,76 @@ func (n krpAuthorizerAttributesGetter) GetRequestAttributes(u user.Info, r *http
}
}()

if n.authzConfig.ResourceAttributes == nil {
// Default attributes mirror the API attributes that would allow this access to kube-rbac-proxy
allAttrs := append(allAttrs, authorizer.AttributesRecord{
User: u,
Verb: apiVerb,
Namespace: "",
APIGroup: "",
APIVersion: "",
Resource: "",
Subresource: "",
Name: "",
ResourceRequest: false,
Path: r.URL.Path,
})
return allAttrs
// Default attributes mirror the API attributes that would allow this access to kube-rbac-proxy
Copy link
Collaborator

@ibihim ibihim Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe I am lazy, but I would've map'ed the old GetRequestAttributes to the new n.authzConfigs list.

func (n krpAuthorizerAttributesGetter) GetRequestAttributes(u user.Info, r *http.Request) []authorizer.Attributes {
    allAttrs := []authorizer.Attributes{}
    
    for _, ac := range n.authzConfigs{
            allAttrs = append(allAttrs, getRequestAttributes(u, r, ac)...)
    }

    return allAttrs
}

defaultAttributesRecord := authorizer.AttributesRecord{
User: u,
Verb: apiVerb,
Namespace: "",
APIGroup: "",
APIVersion: "",
Resource: "",
Subresource: "",
Name: "",
ResourceRequest: false,
Path: r.URL.Path,
}

if n.authzConfig.Rewrites == nil {
allAttrs := append(allAttrs, authorizer.AttributesRecord{
User: u,
Verb: apiVerb,
Namespace: n.authzConfig.ResourceAttributes.Namespace,
APIGroup: n.authzConfig.ResourceAttributes.APIGroup,
APIVersion: n.authzConfig.ResourceAttributes.APIVersion,
Resource: n.authzConfig.ResourceAttributes.Resource,
Subresource: n.authzConfig.ResourceAttributes.Subresource,
Name: n.authzConfig.ResourceAttributes.Name,
ResourceRequest: true,
})
if len(n.authzConfigs) == 0 {
allAttrs = append(allAttrs, defaultAttributesRecord)
return allAttrs
}

params := []string{}
if n.authzConfig.Rewrites.ByQueryParameter != nil && n.authzConfig.Rewrites.ByQueryParameter.Name != "" {
if ps, ok := r.URL.Query()[n.authzConfig.Rewrites.ByQueryParameter.Name]; ok {
params = append(params, ps...)
for _, ac := range n.authzConfigs {
if ac.ResourceAttributes == nil {
allAttrs = append(allAttrs, defaultAttributesRecord)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a benefit to having the defaultAttributesRecord several times in the allAttrs?

continue
}
}
if n.authzConfig.Rewrites.ByHTTPHeader != nil && n.authzConfig.Rewrites.ByHTTPHeader.Name != "" {
if p := r.Header.Get(n.authzConfig.Rewrites.ByHTTPHeader.Name); p != "" {
params = append(params, p)

if ac.Rewrites == nil {
allAttrs = append(allAttrs, authorizer.AttributesRecord{
User: u,
Verb: apiVerb,
Namespace: ac.ResourceAttributes.Namespace,
APIGroup: ac.ResourceAttributes.APIGroup,
APIVersion: ac.ResourceAttributes.APIVersion,
Resource: ac.ResourceAttributes.Resource,
Subresource: ac.ResourceAttributes.Subresource,
Name: ac.ResourceAttributes.Name,
ResourceRequest: true,
})
continue
}
}

if len(params) == 0 {
return allAttrs
}
params := []string{}
if ac.Rewrites.ByQueryParameter != nil && ac.Rewrites.ByQueryParameter.Name != "" {
if ps, ok := r.URL.Query()[ac.Rewrites.ByQueryParameter.Name]; ok {
params = append(params, ps...)
}
}
if ac.Rewrites.ByHTTPHeader != nil && ac.Rewrites.ByHTTPHeader.Name != "" {
if p := r.Header.Get(ac.Rewrites.ByHTTPHeader.Name); p != "" {
params = append(params, p)
}
}

if len(params) == 0 {
continue
}

for _, param := range params {
attrs := authorizer.AttributesRecord{
User: u,
Verb: apiVerb,
Namespace: templateWithValue(n.authzConfig.ResourceAttributes.Namespace, param),
APIGroup: templateWithValue(n.authzConfig.ResourceAttributes.APIGroup, param),
APIVersion: templateWithValue(n.authzConfig.ResourceAttributes.APIVersion, param),
Resource: templateWithValue(n.authzConfig.ResourceAttributes.Resource, param),
Subresource: templateWithValue(n.authzConfig.ResourceAttributes.Subresource, param),
Name: templateWithValue(n.authzConfig.ResourceAttributes.Name, param),
ResourceRequest: true,
for _, param := range params {
attrs := authorizer.AttributesRecord{
User: u,
Verb: apiVerb,
Namespace: templateWithValue(ac.ResourceAttributes.Namespace, param),
APIGroup: templateWithValue(ac.ResourceAttributes.APIGroup, param),
APIVersion: templateWithValue(ac.ResourceAttributes.APIVersion, param),
Resource: templateWithValue(ac.ResourceAttributes.Resource, param),
Subresource: templateWithValue(ac.ResourceAttributes.Subresource, param),
Name: templateWithValue(ac.ResourceAttributes.Name, param),
ResourceRequest: true,
}
allAttrs = append(allAttrs, attrs)
}
allAttrs = append(allAttrs, attrs)
}
return allAttrs
}
Expand Down Expand Up @@ -238,17 +267,19 @@ func (c *Config) DeepCopy() *Config {
}
}

if c.Authorization != nil {
if c.Authorization.ResourceAttributes != nil {
res.Authorization = &authz.Config{
ResourceAttributes: &authz.ResourceAttributes{
Namespace: c.Authorization.ResourceAttributes.Namespace,
APIGroup: c.Authorization.ResourceAttributes.APIGroup,
APIVersion: c.Authorization.ResourceAttributes.APIVersion,
Resource: c.Authorization.ResourceAttributes.Resource,
Subresource: c.Authorization.ResourceAttributes.Subresource,
Name: c.Authorization.ResourceAttributes.Name,
},
if len(c.Authorizations) != 0 {
Copy link
Collaborator

@ibihim ibihim Jun 2, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

We don't need this any more. DeepCopy was unused and removed.

for _, a := range c.Authorizations {
if a != nil && a.ResourceAttributes != nil {
res.Authorizations = append(res.Authorizations, &authz.Config{
ResourceAttributes: &authz.ResourceAttributes{
Namespace: a.ResourceAttributes.Namespace,
APIGroup: a.ResourceAttributes.APIGroup,
APIVersion: a.ResourceAttributes.APIVersion,
Resource: a.ResourceAttributes.Resource,
Subresource: a.ResourceAttributes.Subresource,
Name: a.ResourceAttributes.Name,
},
})
}
}
}
Expand Down
Loading