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

Add RBAC information to audit logs #58807

Merged
merged 2 commits into from Apr 7, 2018

Conversation

@CaoShuFeng
Member

CaoShuFeng commented Jan 25, 2018

Depends on: #58806
Release note:

RBAC information is included in audit logs via audit.Event annotations:
authorization.k8s.io/decision = {allow, forbid}
authorization.k8s.io/reason = human-readable reason for the decision
@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Jan 25, 2018

/assign @tallclair

@@ -48,6 +49,10 @@ func WithAuthorization(handler http.Handler, requestContextMapper request.Reques
}
authorized, reason, err := a.Authorize(attributes)
if authorized == authorizer.DecisionAllow {
if len(reason) != 0 {

This comment has been minimized.

@liggitt

liggitt Jan 26, 2018

Member

would it make sense to annotate in all cases? right now, it's unclear whether a 403 came from authorization or a later admission rejection. Possibly something like:

audit.LogAnnotation(ae, "authorization.k8s.io/decision", "allow" or "forbid")
if len(reason) > 0 {
  audit.LogAnnotation(ae, "authorization.k8s.io/reason", reason)
}
if err != nil {
  audit.LogAnnotation(ae, "authorization.k8s.io/error", err.Error())
}

I don't know how detailed we want error/forbidden info to be, volume-wise

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Jan 27, 2018

Member

authorization.k8s.io/decision is added.

err or forbidden reason will be recorded in ResponseStatus at RequestResponse level:

// The response status, populated even when the ResponseObject is not a Status type.

Will it be redundancy of information?

This comment has been minimized.

@liggitt

liggitt Jan 27, 2018

Member

Will it be redundancy of information?

Not sure... I would anticipate most audit settings not including response content because it is very verbose

This comment has been minimized.

@CaoShuFeng

@k8s-ci-robot k8s-ci-robot added size/S and removed size/M labels Feb 9, 2018

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Feb 9, 2018

rebased

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Feb 9, 2018

/test pull-kubernetes-e2e-kops-aws

@liggitt

This comment has been minimized.

Member

liggitt commented Feb 10, 2018

this seems like a reasonable starting point to me. @tallclair, would you rather start with all three pieces of info and filter/remove later if it is too verbose, or start with less and add more later if needed? I'm not sure how sensitive the audit stuff will be, performance-wise?

@crassirostris

Thanks a lot for the PR!

What about tests?

attributes, err := GetAuthorizerAttributes(ctx)
if err != nil {
responsewriters.InternalError(w, req, err)
return
}
authorized, reason, err := a.Authorize(attributes)
if len(reason) != 0 {
audit.LogAnnotation(ae, "authorization.k8s.io/reason", reason)

This comment has been minimized.

@crassirostris

crassirostris Feb 13, 2018

Member

Can we move these strings to constants, or even to the API?

// 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, "authorization.k8s.io/decision", "allow")

This comment has been minimized.

@crassirostris

crassirostris Feb 13, 2018

Member

Move values to constants?

@crassirostris crassirostris self-assigned this Feb 13, 2018

@crassirostris crassirostris referenced this pull request Feb 13, 2018

Closed

Advanced Auditing 1.10 umbrella bug #58083

11 of 11 tasks complete

@k8s-ci-robot k8s-ci-robot added size/L and removed size/S labels Feb 14, 2018

@CaoShuFeng CaoShuFeng changed the title from Add RBAC information to audit logs to [WIP]Add RBAC information to audit logs Feb 14, 2018

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Feb 14, 2018

What about tests?

@crassirostris Now this is WIP, I am adding e2e test.

@crassirostris

This comment has been minimized.

Member

crassirostris commented Feb 14, 2018

@CaoShuFeng OK, thanks. What I meant though is to add some unit tests for the behavior you're implementing

Please ping the PR when it's ready for review again

// Annotation key names set in advanced audit
decisionAnnotationKey = "authorization.k8s.io/decision"
reasonAnnotationKey = "authorization.k8s.io/reason"
errorAnnotationKey = "authorization.k8s.io/error"

This comment has been minimized.

@tallclair

tallclair Feb 16, 2018

Member

I don't think we should include errors here. Those should get printed in the debug logs. The reason could just be "error" in this case.

This comment has been minimized.

@liggitt

liggitt Feb 16, 2018

Member

makes sense (if reason is empty and error is non-nil, reason becomes "error")

This comment has been minimized.

@CaoShuFeng
@@ -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())

This comment has been minimized.

@tallclair

tallclair Feb 16, 2018

Member

I'd like to be able to tell which authorizer allowed a request. This approach is not reliable, and would require updating every reason message.

Instead, I suggest:

  1. Add a Name() function to the Authorizer interface
  2. Update the union authorizer to annotate the audit log with the decider's name

This comment has been minimized.

@liggitt

liggitt Feb 16, 2018

Member

not sure about this approach

Add a Name() function to the Authorizer interface

It's unclear what Name() would be for the union authorizer

Update the union authorizer to annotate the audit log with the decider's name

I didn't think we wanted to plumb audit events down into all the authorizers

that also means you lose visibility to what authorizer made the decision if it was on the other side of a webhook boundary

This comment has been minimized.

@tallclair

tallclair Feb 16, 2018

Member

It's unclear what Name() would be for the union authorizer

union? Or just name the authorizers when they're passed to the union authorizer.

I didn't think we wanted to plumb audit events down into all the authorizers

good point. maybe include it in the reason then? Or is that potentially leaking too much information to the requester?

that also means you lose visibility to what authorizer made the decision if it was on the other side of a webhook boundary

The name doesn't need to be a constant. The webhook could derive its name from the config (although that means you can't have multiplexed authorizers on the other side).

"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

This comment has been minimized.

@tallclair

tallclair Feb 16, 2018

Member

I'd like a decider too (see previous comment)

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Feb 23, 2018

/test pull-kubernetes-e2e-gce

@CaoShuFeng CaoShuFeng changed the title from [WIP]Add RBAC information to audit logs to Add RBAC information to audit logs Feb 23, 2018

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Feb 23, 2018

/test pull-kubernetes-e2e-kops-aws
/test pull-kubernetes-unit

@crassirostris

This comment has been minimized.

Member

crassirostris commented Feb 23, 2018

LGTM, deferring applying the label to @tallclair

@crassirostris crassirostris referenced this pull request Feb 28, 2018

Closed

Advanced Auditing 1.11 umbrella bug #60392

6 of 14 tasks complete

@crassirostris crassirostris added this to the v1.11 milestone Feb 28, 2018

attributes, err := GetAuthorizerAttributes(ctx)
if err != nil {
responsewriters.InternalError(w, req, err)
return
}
authorized, reason, err := a.Authorize(attributes)
if len(reason) != 0 {
audit.LogAnnotation(ae, reasonAnnotationKey, reason)
} else if err != nil {

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

Are reason and err mutually exclusive?

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Mar 19, 2018

Member

No, they are not.
Done.

@@ -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())

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

Reason is also set on line 123.

I'd prefer to just tack on the RBAC: label in the authorize method.

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Mar 19, 2018

Member

Reason is also set on line 123.

Done.

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Mar 19, 2018

Member

I'd prefer to just tack on the RBAC: label in the authorize method.

This requires checking "len(reason) == 0"?
It makes the code a little bit more complex...

// an error is returned for other users.
type fakeAuthorizer struct{}
func (fakeAuthorizer) Authorize(a authorizer.Attributes) (authorizer.Decision, string, error) {

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

nit: I would simplify this to:

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
}

...

And then make the test set the expected values.

(The less logic in the test construct the better)

This comment has been minimized.

@CaoShuFeng
req.RemoteAddr = "127.0.0.1"
handler.ServeHTTP(httptest.NewRecorder(), req)
fmt.Printf(k)
fmt.Print(audit.Annotations)

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

clean up

This comment has been minimized.

@CaoShuFeng
if len(reason) != 0 {
audit.LogAnnotation(ae, reasonAnnotationKey, reason)
} else if err != nil {
audit.LogAnnotation(ae, reasonAnnotationKey, "error")

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

"internal error"?

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

Make this a constant reasonError = "internal error", and then you can use it in the test.

This comment has been minimized.

@CaoShuFeng
@@ -30,6 +30,8 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apiserver/pkg/apis/audit/v1beta1"
coreclientset "k8s.io/client-go/kubernetes"

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

s/coreclientset/clientset/

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Mar 19, 2018

Member

Done.
renamed another clientset.

apiextensionclientset "k8s.io/apiextensions-apiserver/pkg/client/clientset/clientset"
@@ -65,6 +67,20 @@ var _ = SIGDescribe("Advanced Audit", func() {
apiExtensionClient, err := clientset.NewForConfig(config)
framework.ExpectNoError(err, "failed to initialize apiExtensionClient")
if !framework.IsRBACEnabled(f) {
framework.Skipf("RBAC not enabled")
}

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

The other tests can still run w/o RBAC. Just skip the validation of the decision if RBAC is disabled.

This comment has been minimized.

@CaoShuFeng
namespace: namespace,
requestObject: false,
responseObject: false,
authorizeDecision: "forbid",

This comment has been minimized.

@tallclair

tallclair Mar 14, 2018

Member

nit: almost all the tests use allow. Just make this default to allow in the tests, and then you only need to set it here.

This comment has been minimized.

@CaoShuFeng

CaoShuFeng Mar 19, 2018

Member

I think this needs a new function like newAuditEvent(), and this requires more modifications.

@CaoShuFeng

This comment has been minimized.

Member

CaoShuFeng commented Mar 19, 2018

/test pull-kubernetes-e2e-gce

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Mar 23, 2018

[MILESTONENOTIFIER] Milestone Removed From Pull Request

@CaoShuFeng @crassirostris @tallclair

Important: This pull request was missing labels required for the v1.11 milestone for more than 3 days:

kind: Must specify exactly one of kind/bug, kind/cleanup or kind/feature.
priority: Must specify exactly one of priority/critical-urgent, priority/important-longterm or priority/important-soon.
sig owner: Must specify at least one label prefixed with sig/.

Help

@k8s-merge-robot k8s-merge-robot removed this from the v1.11 milestone Mar 23, 2018

@tallclair

This comment has been minimized.

Member

tallclair commented Apr 7, 2018

/lgtm

I updated your release note.

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 7, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 7, 2018

/test all

Tests are more than 96 hours old. Re-running tests.

@liggitt

This comment has been minimized.

Member

liggitt commented Apr 7, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Apr 7, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, liggitt, tallclair

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Apr 7, 2018

Automatic merge from submit-queue (batch tested with PRs 61183, 58807). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-merge-robot k8s-merge-robot merged commit 58c0748 into kubernetes:master Apr 7, 2018

15 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation CaoShuFeng authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment