-
Notifications
You must be signed in to change notification settings - Fork 188
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
Rewrite from header #104
Rewrite from header #104
Conversation
cc @s-urbaniak @simonpasquier I don't have time to think through whether this is safe or not. At first glance it seems fine, but don't have the time to spend on this right now, so if you could have a look that would be amazing! |
pkg/proxy/proxy.go
Outdated
} | ||
|
||
if len(params) == 0 { | ||
return []authorizer.Attributes{} |
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.
nit: i would continue returning nil
here, there is no need to allocate an empty slice, as the zero value (nil
) of a slice also returns 0 for its capacity and length.
pkg/proxy/proxy.go
Outdated
params, ok := r.URL.Query()[n.authzConfig.Rewrites.ByQueryParameter.Name] | ||
if !ok { | ||
return nil | ||
if n.authzConfig.Rewrites != nil { |
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.
suggestion: let's untangle those nested if/else statements, it starts being complex. We can have a more linear code if we change the method signature like so:
// GetRequestAttributes populates authorizer attributes for the requests to kube-rbac-proxy.
func (n krpAuthorizerAttributesGetter) GetRequestAttributes(u user.Info, r *http.Request) (allAttrs []authorizer.Attributes) {
apiVerb := ""
switch r.Method {
case "POST":
apiVerb = "create"
case "GET":
apiVerb = "get"
case "PUT":
apiVerb = "update"
case "PATCH":
apiVerb = "patch"
case "DELETE":
apiVerb = "delete"
}
defer func() {
for attrs := range allAttrs {
klog.V(5).Infof("kube-rbac-proxy request attributes: attrs=%#v", attrs)
}
}()
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
}
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,
})
return
}
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...)
}
}
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 len(params) == 0 {
return
}
for _, param := range params {
allAttrs = append(allAttrs, 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,
})
}
return
}
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.
thank you very much for the contribution! Generally this lgtm, i have just one suggestion to make the code linear.
Also, do you mind to add some unit tests?
Thank you again! 🎉
@s-urbaniak thanks for the review and suggestions! |
for attrs := range allAttrs { | ||
klog.V(5).Infof("kube-rbac-proxy request attributes: attrs=%#+v", attrs) | ||
if len(params) == 0 { | ||
return allAttrs |
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.
nit: we can return nil
here
@marpio yeah agreed, the non-named-return version looks better to me as well. I really have just one nit, else lgtm, thank you so much! 💚 |
@s-urbaniak hmm.. returning nil causes the e2e test failure (see https://github.com/brancz/kube-rbac-proxy/pull/104/checks?check_run_id=1566450831) |
@marpio interesting, let me inspect why that is the case 🤔 |
@s-urbaniak have you been able to find anything? |
Amazing @marpio this is exactly what we were looking for and already wanted to implement ourselves. Our usecase is the same as you describe to also enable the multi-tenancy capability with Grafana Loki. Would this configuration work after your PR is merged?:
EDIT: I'll answer the question myself, yes it works that way. Tested it with this PR 🎉 |
I think the failing e2e test was a flake as it passes for me locally with the
If you want please submit a follow-up PR. However, this being a nit is not a stopper, hence merging. Sorry for the delay and thank you for the contribution! |
@s-urbaniak thanks for taking the time! |
@brancz I think this deservers a new release, however I have not the powers to publish new images. Happy to help out. wdyt? |
Seems like after merging this, our e2e tests started failing 🤔 More in https://github.com/brancz/kube-rbac-proxy/runs/1748074270 Personally, I would like to know why those are failing before creating a release. |
hmm, let's open an issue then, it seems to be related with certs rotation so i can have a look. |
Description
This PR extends the SubjectAccessReviews rewrite functionality by allowing to rewrite the attributes using a value from http header.
Motivation
I would like to put kube-rbac-proxy in front of Loki (https://grafana.com/docs/loki/latest/). Loki, when deployed in multi-tenant mode, requires X-Scope-OrgID header containing the tenantID. In my case, the tenantID is just the namespace and so I could check if a given user/SA has access to a particular resource in a given namespace provided in the X-Scope-OrgID header. This way I could separate the logs on a per namespace basis.