Allow audit to log authorization failures #51119

soltysh · 2017-08-22T14:56:03Z

What this PR does / why we need it:
This PR extends our current audit mechanism allowing to audit failed login attempts.

Release note:

Advanced audit allows logging failed login attempts

soltysh · 2017-08-22T14:57:12Z

The only remaining part is tests for the new bits.

sttts · 2017-08-22T15:00:17Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+		}
+
+		// otherwise, we need to create the event by ourselves and log the auth error
+		// the majority of this code is copied from upstream WithAudit filter


s/upstream//

sttts · 2017-08-22T15:01:23Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+		ev.ResponseStatus = &metav1.Status{}
+		ev.ResponseStatus.Code = int32(code)
+		ev.Stage = auditinternal.StageResponseStarted
+		processEvent(a.sink, ev)


as we are in another file now, maybe rename this to processAuditEvent

soltysh · 2017-08-23T12:41:04Z

@sttts I've added tests and run bazel, ptal

crassirostris · 2017-08-24T10:13:06Z

@destijl Could you please take a look?

/cc @tallclair

sttts · 2017-08-24T14:04:38Z

lgtm. But two more eyes on it wouldn't harm.

destijl · 2017-08-29T00:41:32Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+		if err != nil {
+			return basic
+		}
+		cred := strings.SplitN(string(str), ":", 2)


This seems to give an unauthenticated attacker complete control over the username field? Potential XSS vector if downstream consumers are displaying the contents?

This piece of code is taken directly from basic auth, so the same problem applies there. If I'm able to address other comment of yours, this should not be a problem anymore.

/cc @liggitt

Any user can fake a user-name and this name is extracted here to be added to the audit event on authn failure. This by itself is expected and intended. But I agree that allowing any kind of binary data might open up an attack vector on buggy consumers. What is best practice here?

same vector applies to resource name parsed from the url, right?

For resource names at least the charset is restricted.

For resource names at least the charset is restricted.

not by request parsing, the restrictions are per resource

destijl · 2017-08-29T00:54:58Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+}
+
+// getUsername returns username or information on the authn method being used.
+func getUsername(req *http.Request) string {


AIUI we get an authentication failure, then handover the failed request to this code to try and extract some context from the request. Is there something stopping us logging a more meaningful failure at the actual authentication failure time? Seems like the point where we determine the password/cert/etc is bad is the place where we should create the log.

This hard-codes some assumptions about the auth methods we accept which would seem to better belong in a single library (or libraries, one for each method) so that we can be careful about how we handle this untrusted input and reason about code changes needed to deprecate authentication methods if necessary.

Hrm... I'm not a big fan of it either, but it was the first thing I could think of. I'll dig into the code once more and see how we can do a better job here.

liggitt · 2017-08-29T13:55:14Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+
+	// check basic auth
+	const basicScheme string = "Basic "
+	if strings.HasPrefix(auth, basicScheme) {


https://golang.org/pkg/net/http/#Request.BasicAuth ?

liggitt · 2017-08-29T13:55:58Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+	}
+
+	// other tokens
+	token := strings.TrimSpace(req.URL.Query().Get("access_token"))


remove this, this method is not supported

liggitt · 2017-08-29T13:58:13Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+		if len(cred) < 2 {
+			return basic
+		}
+		return cred[0]


I agree that putting unverified data about users in audit logs is fraught

soltysh · 2017-08-30T15:34:04Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+		}
+
+		// if there already exists an audit event in the context we don't need to do anything
+		if ae := request.AuditEventFrom(ctx); ae != nil {


I left this check here, but it shouldn't be needed, since we hook in failed authn specifically.

soltysh · 2017-08-30T15:35:35Z

@liggitt @sttts @destijl I've addressed your concerns, can I get a quick review on this one?
The only remaining thing I'd like to do here, is to inject auth method name in user.Extras, b/c in majority of cases we're not getting username, but we could at least point to which methods were tried, wdyt?

liggitt · 2017-08-30T15:39:18Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go

 }

 func (m *fakeRequestContextMapper) Get(req *http.Request) (request.Context, bool) {
-	ctx := request.NewContext()
+	if m.ctx == nil {
+		m.ctx = request.NewContext()


this makes fakeRequestContextMapper non-thread safe

But this is just test, for that particular usecase. I doubt fakeRequestContextMapper will be use anywhere else. Same for the next call, as well.

liggitt · 2017-08-30T15:39:48Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit_test.go

 	}

 	resolver := newTestRequestInfoResolver()
 	info, err := resolver.NewRequestInfo(req)
 	if err == nil {
-		ctx = request.WithRequestInfo(ctx, info)
+		m.ctx = request.WithRequestInfo(m.ctx, info)


and this makes it not safe to be called multiple times

liggitt · 2017-08-30T15:41:08Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authentication.go

@@ -63,6 +63,9 @@ func WithAuthentication(handler http.Handler, mapper genericapirequest.RequestCo
 				if err != nil {
 					glog.Errorf("Unable to authenticate the request due to an error: %v", err)
 				}
+				if auditFailedAuthn != nil {
+					w = auditFailedAuthn.Decorate(w, user, req)


why are you passing user? when err != nil || !ok, user is unusable

and if you don't need user, then auditFailedAuthn.Decorate has the same args as http.Handler, and you can simple pass a wrapped failed http.Handler in here that logs, then handles

sttts · 2017-08-31T04:53:48Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go

@@ -48,37 +48,17 @@ func WithAudit(handler http.Handler, requestContextMapper request.RequestContext
 			return
 		}

-		attribs, err := GetAuthorizerAttributes(ctx)
+		ev, err := createAuditEvent(ctx, policy, requestContextMapper, req, w)
 		if err != nil {


shouldn't this still lead to http 500?

Ic, createAuditEvent writes the error. I would prefer to see it here.

or at least a good comment here.

I'll move the error handling up, it'll be more explicit.

sttts · 2017-08-31T05:07:29Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/audit.go

+		return nil, err
+	}
+
+	ctx = request.WithAuditEvent(ctx, ev)


this is unexpected in a create* function as a side-effect.

either move that our or call the func createAuditEventAndAttachToCtx

I moved context extraction also to that method and went with createAuditEventAndAttachToContext.

sttts · 2017-08-31T05:12:30Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+			return
+		}
+
+		ev, err := createAuditEvent(ctx, policy, requestContextMapper, req, w)


so this attaches an event to the context. The WithAudit filter will override that. Correct?

sttts · 2017-08-31T05:17:31Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+		ev.ResponseStatus.Message = getAuthMethods(req)
+		ev.Stage = auditinternal.StageResponseStarted
+
+		rw := decorateResponseWriter(w, ev, sink)


This makes the rw.WriteHeader to process an event. The WithAudit also decorates. So if an endpoint handler writes the header, the WithAudit decorator will send an event to the audit backend, then your new outer decorator will the same. Moreover, you ev here is another one than the one of WithAudit. So yours will be pretty empty.

No double decoration will happen. he reason for that is that WithFailedAuthenticationAudit decorates only the failed handler that is passed to WithAuthentication. At that point no request will ever reach regular WithAudit because the processing is passed through a different handler chain.

got it. Please call the parameter failedHandler here. Then it's clearer.

CaoShuFeng · 2017-08-31T09:26:39Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+
+// WithFailedAuthenticationAudit decorates a http.Handler with a fallback audit,
+// logging information only when the original one did was not triggered.
+// This needs to be used with WithAuditTriggeredMarker, which wraps the original


I can't find "WithAuditTriggeredMarker" in other places.
Maybe this doc string is out of date?

CaoShuFeng · 2017-08-31T09:51:24Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit_test.go

+	auditinternal "k8s.io/apiserver/pkg/apis/audit"
+	"k8s.io/apiserver/pkg/audit/policy"
+	// import to call webhook's init() function to register audit.Event to schema
+	_ "k8s.io/apiserver/plugin/pkg/audit/webhook"


I have tested that it's not necessary to import this in this unit test.

soltysh · 2017-08-31T10:22:03Z

Updated with addressed comments from @CaoShuFeng

sttts · 2017-08-31T10:23:21Z

lgtm after the failedHandler and @CaoShuFeng's comment are fixed.

sttts · 2017-08-31T10:49:51Z

/approve no-issue
/lgtm

k8s-github-robot · 2017-08-31T10:50:48Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: soltysh, sttts

Associated issue requirement bypassed by: sttts

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~staging/src/k8s.io/apiserver/OWNERS~~ [sttts]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

soltysh · 2017-08-31T13:04:01Z

/retest

k8s-ci-robot · 2017-08-31T15:37:53Z

@soltysh: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-bazel	`2c8c5c0`	link	`/test pull-kubernetes-bazel`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

fejta-bot · 2017-08-31T19:33:52Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-09-03T02:26:21Z

Automatic merge from submit-queue (batch tested with PRs 50832, 51119, 51636, 48921, 51712)

CaoShuFeng · 2017-09-04T08:32:34Z

staging/src/k8s.io/apiserver/pkg/endpoints/filters/authn_audit.go

+
+		ev.ResponseStatus = &metav1.Status{}
+		ev.ResponseStatus.Message = getAuthMethods(req)
+		ev.Stage = auditinternal.StageResponseStarted


I am OK with auditinternal.StageResponseStarted here.
But why StageResponseStarted is better than StageRequestReceived?

soltysh added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Aug 22, 2017

soltysh assigned sttts and crassirostris Aug 22, 2017

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 22, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 22, 2017

sttts reviewed Aug 22, 2017

View reviewed changes

soltysh force-pushed the failed_authn_audit branch 2 times, most recently from b20b905 to 2c8c5c0 Compare August 23, 2017 12:40

soltysh mentioned this pull request Aug 23, 2017

Move audit filter before authn to log those failures as well openshift/origin#14535

Closed

k8s-ci-robot requested a review from tallclair August 24, 2017 10:13

destijl reviewed Aug 29, 2017

View reviewed changes

liggitt reviewed Aug 29, 2017

View reviewed changes

soltysh force-pushed the failed_authn_audit branch from 2c8c5c0 to ca8fb73 Compare August 30, 2017 15:33

soltysh commented Aug 30, 2017

View reviewed changes

liggitt reviewed Aug 30, 2017

View reviewed changes

soltysh force-pushed the failed_authn_audit branch 2 times, most recently from 8310c21 to c2443ab Compare August 30, 2017 20:37

sttts reviewed Aug 31, 2017

View reviewed changes

soltysh force-pushed the failed_authn_audit branch from c2443ab to 4e5f11f Compare August 31, 2017 09:15

CaoShuFeng reviewed Aug 31, 2017

View reviewed changes

soltysh force-pushed the failed_authn_audit branch from 4e5f11f to 6e0c511 Compare August 31, 2017 10:21

Allow audit to log authorization failures

9fef244

soltysh force-pushed the failed_authn_audit branch from 6e0c511 to 9fef244 Compare August 31, 2017 10:40

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017

soltysh added this to the v1.8 milestone Aug 31, 2017

CaoShuFeng mentioned this pull request Aug 31, 2017

Provide a way to omit Event stages in audit policy #49280

Merged

ericchiang added the area/audit label Aug 31, 2017

k8s-github-robot merged commit f4c6cbd into kubernetes:master Sep 3, 2017

CaoShuFeng reviewed Sep 4, 2017

View reviewed changes

soltysh deleted the failed_authn_audit branch September 8, 2017 15:10

soltysh mentioned this pull request Sep 28, 2017

Wire advanced audit to asset and oauth api servers openshift/origin#16567

Merged

Allow audit to log authorization failures #51119

Allow audit to log authorization failures #51119

Conversation

soltysh commented Aug 22, 2017

soltysh commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Aug 23, 2017

crassirostris commented Aug 24, 2017

sttts commented Aug 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liggitt Aug 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Aug 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soltysh commented Aug 31, 2017

sttts commented Aug 31, 2017

sttts commented Aug 31, 2017

k8s-github-robot commented Aug 31, 2017

soltysh commented Aug 31, 2017

k8s-ci-robot commented Aug 31, 2017 • edited Loading

fejta-bot commented Aug 31, 2017

k8s-github-robot commented Sep 3, 2017

Choose a reason for hiding this comment

liggitt Aug 29, 2017 •

edited

Loading

k8s-ci-robot commented Aug 31, 2017 •

edited

Loading