From e87c2c9f27f7f9756a8b664d118d357b166bbd14 Mon Sep 17 00:00:00 2001 From: Cao Shufeng Date: Mon, 22 Jan 2018 15:19:15 +0800 Subject: [PATCH 1/2] Log rbac info into advanced audit event --- plugin/pkg/auth/authorizer/rbac/rbac.go | 4 +- .../apiserver/pkg/endpoints/filters/BUILD | 1 + .../pkg/endpoints/filters/audit_test.go | 6 +- .../pkg/endpoints/filters/authorization.go | 18 +++++ .../endpoints/filters/authorization_test.go | 66 +++++++++++++++++++ 5 files changed, 92 insertions(+), 3 deletions(-) diff --git a/plugin/pkg/auth/authorizer/rbac/rbac.go b/plugin/pkg/auth/authorizer/rbac/rbac.go index bb714a01a08..122a10b2f3a 100644 --- a/plugin/pkg/auth/authorizer/rbac/rbac.go +++ b/plugin/pkg/auth/authorizer/rbac/rbac.go @@ -62,7 +62,7 @@ type authorizingVisitor struct { func (v *authorizingVisitor) visit(source fmt.Stringer, rule *rbac.PolicyRule, err error) bool { if rule != nil && RuleAllows(v.requestAttributes, rule) { v.allowed = true - v.reason = fmt.Sprintf("allowed by %s", source.String()) + v.reason = fmt.Sprintf("RBAC: allowed by %s", source.String()) return false } if err != nil { @@ -120,7 +120,7 @@ func (r *RBACAuthorizer) Authorize(requestAttributes authorizer.Attributes) (aut reason := "" if len(ruleCheckingVisitor.errors) > 0 { - reason = fmt.Sprintf("%v", utilerrors.NewAggregate(ruleCheckingVisitor.errors)) + reason = fmt.Sprintf("RBAC: %v", utilerrors.NewAggregate(ruleCheckingVisitor.errors)) } return authorizer.DecisionNoOpinion, reason, nil } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD index 98dbb32b745..179522ebc52 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/BUILD @@ -20,6 +20,7 @@ go_test( embed = [":go_default_library"], deps = [ "//vendor/github.com/pborman/uuid:go_default_library", + "//vendor/github.com/stretchr/testify/assert:go_default_library", "//vendor/k8s.io/api/authentication/v1:go_default_library", "//vendor/k8s.io/api/batch/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go index a7c1486a700..f8086ba7ca0 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go @@ -736,7 +736,8 @@ func TestAudit(t *testing.T) { } type fakeRequestContextMapper struct { - user *user.DefaultInfo + user *user.DefaultInfo + audit *auditinternal.Event } func (m *fakeRequestContextMapper) Get(req *http.Request) (request.Context, bool) { @@ -744,6 +745,9 @@ func (m *fakeRequestContextMapper) Get(req *http.Request) (request.Context, bool if m.user != nil { ctx = request.WithUser(ctx, m.user) } + if m.audit != nil { + ctx = request.WithAuditEvent(ctx, m.audit) + } resolver := newTestRequestInfoResolver() info, err := resolver.NewRequestInfo(req) diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go index b4610dfcea6..6de62bde5e9 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization.go @@ -23,11 +23,23 @@ import ( "github.com/golang/glog" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apiserver/pkg/audit" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" "k8s.io/apiserver/pkg/endpoints/request" ) +const ( + // Annotation key names set in advanced audit + decisionAnnotationKey = "authorization.k8s.io/decision" + reasonAnnotationKey = "authorization.k8s.io/reason" + + // Annotation values set in advanced audit + decisionAllow = "allow" + decisionForbid = "forbid" + reasonError = "internal error" +) + // WithAuthorizationCheck passes all authorized requests on to handler, and returns a forbidden error otherwise. func WithAuthorization(handler http.Handler, requestContextMapper request.RequestContextMapper, a authorizer.Authorizer, s runtime.NegotiatedSerializer) http.Handler { if a == nil { @@ -40,6 +52,7 @@ func WithAuthorization(handler http.Handler, requestContextMapper request.Reques responsewriters.InternalError(w, req, errors.New("no context found for request")) return } + ae := request.AuditEventFrom(ctx) attributes, err := GetAuthorizerAttributes(ctx) if err != nil { @@ -49,15 +62,20 @@ func WithAuthorization(handler http.Handler, requestContextMapper request.Reques authorized, reason, err := a.Authorize(attributes) // an authorizer like RBAC could encounter evaluation errors and still allow the request, so authorizer decision is checked before error here. if authorized == authorizer.DecisionAllow { + audit.LogAnnotation(ae, decisionAnnotationKey, decisionAllow) + audit.LogAnnotation(ae, reasonAnnotationKey, reason) handler.ServeHTTP(w, req) return } if err != nil { + audit.LogAnnotation(ae, reasonAnnotationKey, reasonError) responsewriters.InternalError(w, req, err) return } glog.V(4).Infof("Forbidden: %#v, Reason: %q", req.RequestURI, reason) + audit.LogAnnotation(ae, decisionAnnotationKey, decisionForbid) + audit.LogAnnotation(ae, reasonAnnotationKey, reason) responsewriters.Forbidden(ctx, attributes, w, req, reason, s) }) } diff --git a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization_test.go b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization_test.go index 477e5d4e3ed..9eba3a5a905 100644 --- a/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization_test.go +++ b/staging/src/k8s.io/apiserver/pkg/endpoints/filters/authorization_test.go @@ -23,7 +23,11 @@ import ( "reflect" "testing" + "github.com/stretchr/testify/assert" batch "k8s.io/api/batch/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/serializer" + auditinternal "k8s.io/apiserver/pkg/apis/audit" "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/endpoints/handlers/responsewriters" "k8s.io/apiserver/pkg/endpoints/request" @@ -127,3 +131,65 @@ func TestGetAuthorizerAttributes(t *testing.T) { } } } + +type fakeAuthorizer struct { + decision authorizer.Decision + reason string + err error +} + +func (f fakeAuthorizer) Authorize(a authorizer.Attributes) (authorizer.Decision, string, error) { + return f.decision, f.reason, f.err +} + +func TestAuditAnnotation(t *testing.T) { + testcases := map[string]struct { + authorizer fakeAuthorizer + decisionAnnotation string + reasonAnnotation string + }{ + "decision allow": { + fakeAuthorizer{ + authorizer.DecisionAllow, + "RBAC: allowed to patch pod", + nil, + }, + "allow", + "RBAC: allowed to patch pod", + }, + "decision forbid": { + fakeAuthorizer{ + authorizer.DecisionDeny, + "RBAC: not allowed to patch pod", + nil, + }, + "forbid", + "RBAC: not allowed to patch pod", + }, + "error": { + fakeAuthorizer{ + authorizer.DecisionNoOpinion, + "", + errors.New("can't parse user info"), + }, + "", + reasonError, + }, + } + + scheme := runtime.NewScheme() + negotiatedSerializer := serializer.DirectCodecFactory{CodecFactory: serializer.NewCodecFactory(scheme)} + for k, tc := range testcases { + audit := &auditinternal.Event{Level: auditinternal.LevelMetadata} + handler := WithAuthorization(&fakeHTTPHandler{}, &fakeRequestContextMapper{ + audit: audit, + }, tc.authorizer, negotiatedSerializer) + + req, _ := http.NewRequest("GET", "/api/v1/namespaces/default/pods", nil) + req.RemoteAddr = "127.0.0.1" + handler.ServeHTTP(httptest.NewRecorder(), req) + assert.Equal(t, tc.decisionAnnotation, audit.Annotations[decisionAnnotationKey], k+": unexpected decision annotation") + assert.Equal(t, tc.reasonAnnotation, audit.Annotations[reasonAnnotationKey], k+": unexpected reason annotation") + } + +} From 440aab5b8661b63aa41cbdeb8f969f7725ea9a74 Mon Sep 17 00:00:00 2001 From: Cao Shufeng Date: Wed, 14 Feb 2018 15:10:20 +0800 Subject: [PATCH 2/2] add e2e test --- test/e2e/auth/audit.go | 179 ++++++++++++++++++++++++++++++----------- 1 file changed, 131 insertions(+), 48 deletions(-) diff --git a/test/e2e/auth/audit.go b/test/e2e/auth/audit.go index 2305bfe1ee1..10d9e715bd3 100644 --- a/test/e2e/auth/audit.go +++ b/test/e2e/auth/audit.go @@ -26,12 +26,14 @@ import ( apiv1 "k8s.io/api/core/v1" extensions "k8s.io/api/extensions/v1beta1" apiextensionsv1beta1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1beta1" - "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" + apiextensionclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset" "k8s.io/apiextensions-apiserver/test/integration/testserver" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/wait" "k8s.io/apiserver/pkg/apis/audit/v1beta1" + clientset "k8s.io/client-go/kubernetes" + restclient "k8s.io/client-go/rest" "k8s.io/kubernetes/test/e2e/framework" imageutils "k8s.io/kubernetes/test/utils/image" @@ -63,9 +65,19 @@ var _ = SIGDescribe("Advanced Audit", func() { config, err := framework.LoadConfig() framework.ExpectNoError(err, "failed to load config") - apiExtensionClient, err := clientset.NewForConfig(config) + apiExtensionClient, err := apiextensionclientset.NewForConfig(config) framework.ExpectNoError(err, "failed to initialize apiExtensionClient") + By("Creating a kubernetes client that impersonates an unauthorized anonymous user") + config, err = framework.LoadConfig() + framework.ExpectNoError(err) + config.Impersonate = restclient.ImpersonationConfig{ + UserName: "system:anonymous", + Groups: []string{"system:unauthenticated"}, + } + anonymousClient, err := clientset.NewForConfig(config) + framework.ExpectNoError(err) + testCases := []struct { action func() events []auditEvent @@ -118,6 +130,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseComplete, @@ -129,6 +142,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseComplete, @@ -140,6 +154,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseStarted, @@ -151,6 +166,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseComplete, @@ -162,6 +178,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequestResponse, v1beta1.StageResponseComplete, @@ -173,6 +190,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, { v1beta1.LevelRequestResponse, v1beta1.StageResponseComplete, @@ -184,6 +202,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, { v1beta1.LevelRequestResponse, v1beta1.StageResponseComplete, @@ -195,6 +214,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, }, }, @@ -239,6 +259,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseComplete, @@ -250,6 +271,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseComplete, @@ -261,6 +283,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseStarted, @@ -272,6 +295,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequest, v1beta1.StageResponseComplete, @@ -283,6 +307,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelRequestResponse, v1beta1.StageResponseComplete, @@ -294,6 +319,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, { v1beta1.LevelRequestResponse, v1beta1.StageResponseComplete, @@ -305,6 +331,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, { v1beta1.LevelRequestResponse, v1beta1.StageResponseComplete, @@ -316,6 +343,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, true, true, + "allow", }, }, }, @@ -366,6 +394,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -377,6 +406,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -388,6 +418,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseStarted, @@ -399,6 +430,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -410,6 +442,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -421,6 +454,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -432,6 +466,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -443,6 +478,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, }, }, @@ -492,6 +528,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -503,6 +540,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -514,6 +552,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseStarted, @@ -525,6 +564,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -536,6 +576,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -547,6 +588,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -558,6 +600,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, { v1beta1.LevelMetadata, v1beta1.StageResponseComplete, @@ -569,6 +612,7 @@ var _ = SIGDescribe("Advanced Audit", func() { namespace, false, false, + "allow", }, }, }, @@ -581,50 +625,87 @@ var _ = SIGDescribe("Advanced Audit", func() { }, []auditEvent{ { - level: v1beta1.LevelRequestResponse, - stage: v1beta1.StageResponseComplete, - requestURI: "/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions", - verb: "create", - code: 201, - user: auditTestUser, - resource: "customresourcedefinitions", - requestObject: true, - responseObject: true, + level: v1beta1.LevelRequestResponse, + stage: v1beta1.StageResponseComplete, + requestURI: "/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions", + verb: "create", + code: 201, + user: auditTestUser, + resource: "customresourcedefinitions", + requestObject: true, + responseObject: true, + authorizeDecision: "allow", }, { - level: v1beta1.LevelMetadata, - stage: v1beta1.StageResponseComplete, - requestURI: fmt.Sprintf("/apis/%s/v1beta1/%s", crdNamespace, crdName), - verb: "create", - code: 201, - user: auditTestUser, - resource: crdName, - requestObject: false, - responseObject: false, + level: v1beta1.LevelMetadata, + stage: v1beta1.StageResponseComplete, + requestURI: fmt.Sprintf("/apis/%s/v1beta1/%s", crdNamespace, crdName), + verb: "create", + code: 201, + user: auditTestUser, + resource: crdName, + requestObject: false, + responseObject: false, + authorizeDecision: "allow", }, { - level: v1beta1.LevelRequestResponse, - stage: v1beta1.StageResponseComplete, - requestURI: fmt.Sprintf("/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions/%s", crd.Name), - verb: "delete", - code: 200, - user: auditTestUser, - resource: "customresourcedefinitions", - requestObject: false, - responseObject: true, + level: v1beta1.LevelRequestResponse, + stage: v1beta1.StageResponseComplete, + requestURI: fmt.Sprintf("/apis/apiextensions.k8s.io/v1beta1/customresourcedefinitions/%s", crd.Name), + verb: "delete", + code: 200, + user: auditTestUser, + resource: "customresourcedefinitions", + requestObject: false, + responseObject: true, + authorizeDecision: "allow", }, { - level: v1beta1.LevelMetadata, - stage: v1beta1.StageResponseComplete, - requestURI: fmt.Sprintf("/apis/%s/v1beta1/%s/setup-instance", crdNamespace, crdName), - verb: "delete", - code: 200, - user: auditTestUser, - resource: crdName, - requestObject: false, - responseObject: false, + level: v1beta1.LevelMetadata, + stage: v1beta1.StageResponseComplete, + requestURI: fmt.Sprintf("/apis/%s/v1beta1/%s/setup-instance", crdNamespace, crdName), + verb: "delete", + code: 200, + user: auditTestUser, + resource: crdName, + requestObject: false, + responseObject: false, + authorizeDecision: "allow", }, }, }, } + // test authorizer annotations, RBAC is required. + annotationTestCases := []struct { + action func() + events []auditEvent + }{ + + // get a pod with unauthorized user + { + func() { + _, err := anonymousClient.CoreV1().Pods(namespace).Get("another-audit-pod", metav1.GetOptions{}) + expectForbidden(err) + }, + []auditEvent{ + { + level: v1beta1.LevelRequest, + stage: v1beta1.StageResponseComplete, + requestURI: fmt.Sprintf("/api/v1/namespaces/%s/pods/another-audit-pod", namespace), + verb: "get", + code: 403, + user: auditTestUser, + resource: "pods", + namespace: namespace, + requestObject: false, + responseObject: false, + authorizeDecision: "forbid", + }, + }, + }, + } + + if framework.IsRBACEnabled(f) { + testCases = append(testCases, annotationTestCases...) + } expectedEvents := []auditEvent{} for _, t := range testCases { t.action() @@ -647,16 +728,17 @@ var _ = SIGDescribe("Advanced Audit", func() { }) type auditEvent struct { - level v1beta1.Level - stage v1beta1.Stage - requestURI string - verb string - code int32 - user string - resource string - namespace string - requestObject bool - responseObject bool + level v1beta1.Level + stage v1beta1.Stage + requestURI string + verb string + code int32 + user string + resource string + namespace string + requestObject bool + responseObject bool + authorizeDecision string } // Search the audit log for the expected audit lines. @@ -725,5 +807,6 @@ func parseAuditLine(line string) (auditEvent, error) { if e.RequestObject != nil { event.requestObject = true } + event.authorizeDecision = e.Annotations["authorization.k8s.io/decision"] return event, nil }