Skip to content

Commit

Permalink
*: addressing PR review comments
Browse files Browse the repository at this point in the history
  • Loading branch information
ibihim committed Jul 7, 2023
1 parent 8bd8c70 commit a539b29
Show file tree
Hide file tree
Showing 5 changed files with 30 additions and 32 deletions.
12 changes: 6 additions & 6 deletions cmd/kube-rbac-proxy/app/kube-rbac-proxy.go
Original file line number Diff line number Diff line change
Expand Up @@ -297,18 +297,18 @@ func Run(cfg *server.KubeRBACProxyConfig) error {
func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverconfig.AuthorizationInfo) (authorizer.Authorizer, error) {
// Pre authorizer is running before any other authorizer.
// It works outside of the default authorizers.
var pre authorizer.Authorizer
var pathAuthorizer authorizer.Authorizer

// AllowPaths denies access to paths that are not listed.
// IgnorePaths doesn't auth(n/z) paths that are listed.
switch {
case len(krbInfo.AllowPaths) > 0:
pre = path.NewAllowPathAuthorizer(krbInfo.AllowPaths)
pathAuthorizer = path.NewAllowPathAuthorizer(krbInfo.AllowPaths)
case len(krbInfo.IgnorePaths) > 0:
pre = path.NewAlwaysAllowPathAuthorizer(krbInfo.IgnorePaths)
pathAuthorizer = path.NewAlwaysAllowPathAuthorizer(krbInfo.IgnorePaths)
}

// Delegated authorizers are running after the pre authorizer
// Delegated authorizers are running after the pathAuthorizer
// and after the attributes have been rewritten.
var delegated []authorizer.Authorizer
// Static authorization authorizes against a static file is ran before the SubjectAccessReview.
Expand Down Expand Up @@ -342,8 +342,8 @@ func setupAuthorizer(krbInfo *server.KubeRBACProxyInfo, delegatedAuthz *serverco
attrsGenerator,
)

if pre != nil {
return union.New(pre, rewritingAuthorizer), nil
if pathAuthorizer != nil {
return union.New(pathAuthorizer, rewritingAuthorizer), nil
}

return rewritingAuthorizer, nil
Expand Down
2 changes: 1 addition & 1 deletion pkg/authorization/rewrite/attributes.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ func NewRewritingAttributesGenerator(attributes *ResourceAttributes) *RewritingA
// is VERY DANGEROUS. It should only be used in a narrow use-case, where the
// upstream is interpreting the input data as well.
func (r *RewritingAttributesGenerator) Generate(ctx context.Context, attr authorizer.Attributes) []authorizer.Attributes {
params := GetKubeRBACProxyParams(ctx)
params := getKubeRBACProxyParams(ctx)
if len(params) == 0 {
return nil
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/authorization/rewrite/middleware.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ func WithKubeRBACProxyParams(ctx context.Context, params []string) context.Conte
return request.WithValue(ctx, rewriterParams, params)
}

// GetKubeRBACProxyParams returns the values from the context that should be
// getKubeRBACProxyParams returns the values from the context that should be
// used to rewrite the attributes.
func GetKubeRBACProxyParams(ctx context.Context) []string {
func getKubeRBACProxyParams(ctx context.Context) []string {
params, _ := ctx.Value(rewriterParams).([]string)

return params
Expand Down
42 changes: 20 additions & 22 deletions pkg/authorization/rewrite/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -13,29 +13,27 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package rewrite_test
package rewrite

import (
"net/http"
"net/http/httptest"
"net/url"
"reflect"
"testing"

"github.com/brancz/kube-rbac-proxy/pkg/authorization/rewrite"
)

func TestRewriteParamsMiddleware(t *testing.T) {
testCases := []struct {
name string
rewrite *rewrite.SubjectAccessReviewRewrites
rewrite *SubjectAccessReviewRewrites
request *http.Request
expected []string
}{
{
name: "with query param rewrites config",
rewrite: &rewrite.SubjectAccessReviewRewrites{
ByQueryParameter: &rewrite.QueryParameterRewriteConfig{
rewrite: &SubjectAccessReviewRewrites{
ByQueryParameter: &QueryParameterRewriteConfig{
Name: "namespace",
},
},
Expand All @@ -46,8 +44,8 @@ func TestRewriteParamsMiddleware(t *testing.T) {
},
{
name: "with query param rewrites config but missing URL query",
rewrite: &rewrite.SubjectAccessReviewRewrites{
ByQueryParameter: &rewrite.QueryParameterRewriteConfig{
rewrite: &SubjectAccessReviewRewrites{
ByQueryParameter: &QueryParameterRewriteConfig{
Name: "namespace",
},
},
Expand All @@ -56,8 +54,8 @@ func TestRewriteParamsMiddleware(t *testing.T) {
},
{
name: "with http header rewrites config",
rewrite: &rewrite.SubjectAccessReviewRewrites{
ByHTTPHeader: &rewrite.HTTPHeaderRewriteConfig{Name: "namespace"},
rewrite: &SubjectAccessReviewRewrites{
ByHTTPHeader: &HTTPHeaderRewriteConfig{Name: "namespace"},
},
request: createRequest(withHeaderParameters(map[string][]string{
"namespace": {"default"},
Expand All @@ -66,8 +64,8 @@ func TestRewriteParamsMiddleware(t *testing.T) {
},
{
name: "with http header rewrites config and additional header",
rewrite: &rewrite.SubjectAccessReviewRewrites{
ByHTTPHeader: &rewrite.HTTPHeaderRewriteConfig{Name: "namespace"},
rewrite: &SubjectAccessReviewRewrites{
ByHTTPHeader: &HTTPHeaderRewriteConfig{Name: "namespace"},
},
request: createRequest(withHeaderParameters(map[string][]string{
"namespace": {"default", "other"},
Expand All @@ -76,17 +74,17 @@ func TestRewriteParamsMiddleware(t *testing.T) {
},
{
name: "with http header rewrites config but missing header",
rewrite: &rewrite.SubjectAccessReviewRewrites{
ByQueryParameter: &rewrite.QueryParameterRewriteConfig{Name: "namespace"},
rewrite: &SubjectAccessReviewRewrites{
ByQueryParameter: &QueryParameterRewriteConfig{Name: "namespace"},
},
request: createRequest(),
expected: nil,
},
{
name: "with http header and query param rewrites config",
rewrite: &rewrite.SubjectAccessReviewRewrites{
ByHTTPHeader: &rewrite.HTTPHeaderRewriteConfig{Name: "namespace"},
ByQueryParameter: &rewrite.QueryParameterRewriteConfig{Name: "namespace"},
rewrite: &SubjectAccessReviewRewrites{
ByHTTPHeader: &HTTPHeaderRewriteConfig{Name: "namespace"},
ByQueryParameter: &QueryParameterRewriteConfig{Name: "namespace"},
},
request: createRequest(
withHeaderParameters(map[string][]string{"namespace": {"default"}}),
Expand All @@ -96,8 +94,8 @@ func TestRewriteParamsMiddleware(t *testing.T) {
},
{
name: "with query header rewrites config but header params",
rewrite: &rewrite.SubjectAccessReviewRewrites{
ByQueryParameter: &rewrite.QueryParameterRewriteConfig{Name: "namespace"},
rewrite: &SubjectAccessReviewRewrites{
ByQueryParameter: &QueryParameterRewriteConfig{Name: "namespace"},
},
request: createRequest(withHeaderParameters(map[string][]string{
"namespace": {"default", "other"},
Expand All @@ -108,14 +106,14 @@ func TestRewriteParamsMiddleware(t *testing.T) {

for _, tc := range testCases {
t.Run(tc.name, func(t *testing.T) {
rewrite.WithKubeRBACProxyParamsHandler(
WithKubeRBACProxyParamsHandler(
http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
values := rewrite.GetKubeRBACProxyParams(r.Context())
values := getKubeRBACProxyParams(r.Context())
if !reflect.DeepEqual(values, tc.expected) {
t.Errorf("expected values to be %v, have %v", tc.expected, values)
}
}),
&rewrite.RewriteAttributesConfig{Rewrites: tc.rewrite},
&RewriteAttributesConfig{Rewrites: tc.rewrite},
).ServeHTTP(nil, tc.request)
})
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/authorization/rewrite/rewrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ type rewritingAuthorizer struct {

var _ authorizer.Authorizer = &rewritingAuthorizer{}

// It generates a list of attributes based on the given attributes generator
// Authorize generates a list of attributes based on the given attributes generator
// and context. All attributes must be authorized to allow the request.
// If no attributes are generated, the request is denied.
func (n *rewritingAuthorizer) Authorize(ctx context.Context, attrs authorizer.Attributes) (authorizer.Decision, string, error) {
Expand Down

0 comments on commit a539b29

Please sign in to comment.