apiserver: add a webhook implementation of the audit backend #45919

ericchiang · 2017-05-16T22:46:40Z

This builds off of #45315 and is intended to implement an interfaced defined in #45766.

TODO:

Rebase on top of API types PR.
Rebase on top of API types updates (Update audit API with missing pieces #46065)
Rebase on top of feature flag (Implement audit policy logic #46009)
Rebase on top of audit instrumentation.
Hook up API server flag or register plugin (depending on Audit: fill audit.Event in handler chain #45766)

Features issue kubernetes/enhancements#22

Design proposal https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auditing.md

Webhook added to the API server which omits structured audit log events.

/cc @soltysh @timstclair @soltysh @deads2k

timstclair · 2017-05-17T00:03:02Z

/cc @ihmccreery

timothysc · 2017-05-17T00:06:21Z

/cc @jbeda @kensimon

k8s-ci-robot · 2017-05-17T00:06:23Z

@timothysc: GitHub didn't allow me to request PR reviews from the following users: kensimon.

Note that only people with write access to kubernetes/kubernetes can review this PR.
.

In response to this:

/cc @jbeda @kensimon

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.

timstclair

Awesome! Were you able to test it in an actual cluster?

timstclair · 2017-05-17T00:07:30Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

+// New returns an audit backend at sends events over HTTP to the.
+func New(kubeConfigFile string) (*AuditBackend, error) {
+	// TODO(ericchiang): Figure out some way to allow protobuf encoding? Is there
+	// a kubeconfig extension to request this?


FYI - we were having trouble with the protobuf code generation, so that needs to happen first.

timstclair · 2017-05-17T00:13:41Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

+// LogAuditEvent attempts to POST a serialized audit event to an external service.
+func (a *AuditBackend) LogAuditEvent(ev *audit.Event) error {
+	// TODO(ericchiang): With retry and backoff?
+	return a.w.RestClient.Post().Body(ev).Do().Error()


Can we make this send an EventList instead? Even if we're sending single events now, it means we can add batching later without changing the endpoint api at all.

ericchiang · 2017-05-17T04:46:35Z

Awesome! Were you able to test it in an actual cluster?

Nope. Haven't pulled in sttts' PR yet. Unit tests produce a nice, empty audit object though.

EDIT: everything's broken. going to wait for the other PRs to merge before moving forward.

timothysc · 2017-05-17T22:40:55Z

This needs both a release note and full doc behind it. Is there an entry in the features repo? Almost every operator is going to need to hook into this.

ericchiang · 2017-05-17T22:49:00Z

@timstclair added release notes and linked the feature and design proposal.

timstclair · 2017-05-18T00:41:33Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

+// payload contains an audit.EventList with a single element. The audit webhook may batch
+// multiple events in the future.
+func (a *AuditBackend) LogAuditEvent(ev *audit.Event) error {
+	list := &audit.EventList{Items: []audit.Event{*ev}}


Does this need to be converted to the versioned API, or is that done automatically by the RestClient?

done by the rest client, compare the internal client, e.g. pkg/client/clientset_generated/internalclientset/typed/batch/internalversion/job.go

That's correct. Let me update the test to show that.

timstclair · 2017-05-18T00:42:56Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

+	// Since the audit API isn't registered with the client or apimachinery API,
+	// create a unique scheme to register this API group with here.
+	//
+	// TODO(ericchiang): Refactor this once the audit API group is moved to k8s.io/api.


I think the current plan is that it will move into a new repository, e.g. k8s.io/apiserverapi (needs a better name...)

Repository inflation :-/

timstclair · 2017-05-18T00:44:00Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

+	}
+
+	install.Install(groupFactoryRegistry, registry, scheme)
+}


Can we move this to a common location? I'm not sure what the "right" way to do it is, but I need something similar to parse the Policy out of a file.

This first bit, at least, is just copied from other webhooks.[1] The install call was because "k8s.io/apiserver/pkg/apis/audit/install" doesn't call this like other api groups.[2] Maybe we could have the audit/install package actually register the API group?

[1]

kubernetes/staging/src/k8s.io/apiserver/plugin/pkg/authorizer/webhook/webhook.go

Lines 210 to 221 in e9b02c2

// NOTE: client-go doesn't provide a registry. client-go does registers the

// authorization/v1beta1. We construct a registry that acknowledges

// authorization/v1beta1 as an enabled version to pass a check enforced in

// NewGenericWebhook.

var registry = registered.NewOrDie("")

func init() {

registry.RegisterVersions(groupVersions)

if err := registry.EnableVersions(groupVersions...); err != nil {

panic(fmt.Sprintf("failed to enable version %v", groupVersions))

}

}

[2]

kubernetes/pkg/apis/imagepolicy/install/install.go

Lines 31 to 33 in e9b02c2

func init() {

Install(api.GroupFactoryRegistry, api.Registry, api.Scheme)

}

It looks like most API groups are installed on k8s.io/kubernetes/pkg/api (see the second link above). If I'm not mistaken, we can't import that package here. The other k8s.io/apiserver/pkg/apis group doesn't do this import.

We don't have a global scheme and registry in k8s.io/apiserver. All the new api servers (aggregation, apiextensions, ...) have their private one for the crud served resources. Having a private scheme is the way to go.

ericchiang · 2017-05-31T15:49:02Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

+
+	c := conversion.NewCloner()
+	for _, f := range metav1.GetGeneratedDeepCopyFuncs() {
+		if err := c.RegisterGeneratedDeepCopyFunc(f); err != nil {


also the audit deepcopy funcs. Feel free to re-apply lgtm after that.

sttts · 2017-05-31T15:49:54Z

/lgtm

sttts · 2017-05-31T15:50:49Z

@deads2k can you approve?

ericchiang · 2017-05-31T16:48:18Z

staging/src/k8s.io/apiserver/plugin/pkg/audit/webhook/webhook.go

+
+// Copied from generated code in k8s.io/apiserver/pkg/apis/audit.
+//
+// TODO(ericchiang): Have the generated code expose these methods like metav1.GetGeneratedDeepCopyFuncs().


Going to follow up with a change to not have this copied from the audit package. It's currently in a generated file that I don't want to mess with:

https://github.com/kubernetes/apiserver/blob/c809cf8581e1e44c6174bf5ab4415e6ee39965ca/pkg/apis/audit/zz_generated.deepcopy.go#L37-L45

@sttts

ericchiang · 2017-05-31T16:53:38Z

Re-applying lgtm per #45919 (comment)

deads2k · 2017-05-31T17:00:44Z

/approve

k8s-github-robot · 2017-05-31T17:01:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, ericchiang, sttts, timstclair

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

~~cmd/kube-apiserver/OWNERS~~ [deads2k]
~~federation/cmd/federation-apiserver/OWNERS~~ [deads2k]
~~hack/OWNERS~~ [deads2k]
~~staging/src/k8s.io/apiserver/OWNERS~~ [deads2k,sttts]
~~staging/src/k8s.io/kube-apiextensions-server/OWNERS~~ [deads2k,sttts]

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

ericchiang · 2017-05-31T17:39:31Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

timstclair · 2017-05-31T18:57:16Z

Test failing due to #46713

timstclair · 2017-05-31T18:57:46Z

@k8s-bot gce etcd3 e2e test this

timstclair · 2017-05-31T21:34:30Z

@k8s-bot gce etcd3 e2e test this

ericchiang · 2017-05-31T22:53:49Z

:(

would rebasing be a good idea?

timstclair · 2017-05-31T23:12:30Z

I think we just need to wait for #46713 to be resolved :(

timstclair · 2017-06-01T00:31:21Z

@k8s-bot pull-kubernetes-e2e-gce-etcd3 test this

sttts · 2017-06-01T06:13:56Z

@k8s-bot pull-kubernetes-node-e2e test this
@k8s-bot gce etcd3 e2e test this

sttts · 2017-06-01T07:15:16Z

@k8s-bot kops aws e2e test this
@k8s-bot gce etcd3 e2e test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-ci-robot · 2017-06-01T08:02:40Z

@ericchiang: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins kops AWS e2e	08e8d564fd455206567abe0b1e209d9a13ad4991	link	`@k8s-bot kops aws e2e test this`
Jenkins GCE etcd3 e2e	08e8d564fd455206567abe0b1e209d9a13ad4991	link	`@k8s-bot gce etcd3 e2e test this`

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.

timstclair · 2017-06-01T16:32:36Z

@k8s-bot pull-kubernetes-federation-e2e-gce test this

k8s-github-robot · 2017-06-01T20:41:58Z

Automatic merge from submit-queue

@sttts

Automatic merge from submit-queue (batch tested with PRs 46620, 46732, 46773, 46772, 46725) Instrument advanced auditing Add prometheus metrics for audit logging, including: - A total count of audit events generated and sent to the output backend - A count of audit events that failed to be audited due to an error (per backend) - A count of request audit levels (1 per request) For kubernetes/enhancements#22 - [x] TODO: Call `HandlePluginError` from the webhook backend, once #45919 merges (in this or a separate PR, depending on timing of the merge) /cc @ihmccreery @sttts @soltysh @ericchiang

k8s-ci-robot requested review from soltysh, timstclair and deads2k May 16, 2017 22:46

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

k8s-github-robot assigned timothysc and ncdc May 16, 2017

k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 16, 2017

k8s-ci-robot requested a review from ikehz May 17, 2017 00:03

k8s-ci-robot requested a review from jbeda May 17, 2017 00:06

timstclair reviewed May 17, 2017

View reviewed changes

ericchiang force-pushed the audit-webhook-backend branch 2 times, most recently from e397f24 to ca6f107 Compare May 17, 2017 04:42

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 17, 2017

ericchiang force-pushed the audit-webhook-backend branch from ca6f107 to 1d7f225 Compare May 17, 2017 04:45

k8s-github-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 17, 2017

k8s-github-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels May 17, 2017

timstclair reviewed May 18, 2017

View reviewed changes

ericchiang force-pushed the audit-webhook-backend branch from 11fad6e to e7235bf Compare May 31, 2017 15:48

ericchiang commented May 31, 2017

View reviewed changes

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

apiserver: add a webhook implementation of the audit backend

a88e018

ericchiang force-pushed the audit-webhook-backend branch from e7235bf to a88e018 Compare May 31, 2017 16:45

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

ericchiang commented May 31, 2017

View reviewed changes

ericchiang added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 31, 2017

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

timstclair mentioned this pull request May 31, 2017

Instrument advanced auditing #46732

Merged

1 task

k8s-github-robot merged commit f7a1f10 into kubernetes:master Jun 1, 2017

ericchiang deleted the audit-webhook-backend branch June 1, 2017 21:23

	// NOTE: client-go doesn't provide a registry. client-go does registers the
	// authorization/v1beta1. We construct a registry that acknowledges
	// authorization/v1beta1 as an enabled version to pass a check enforced in
	// NewGenericWebhook.
	var registry = registered.NewOrDie("")

	func init() {
	registry.RegisterVersions(groupVersions)
	if err := registry.EnableVersions(groupVersions...); err != nil {
	panic(fmt.Sprintf("failed to enable version %v", groupVersions))
	}
	}

	func init() {
	Install(api.GroupFactoryRegistry, api.Registry, api.Scheme)
	}

apiserver: add a webhook implementation of the audit backend #45919

apiserver: add a webhook implementation of the audit backend #45919

Conversation

ericchiang commented May 16, 2017 • edited

timstclair commented May 17, 2017

timothysc commented May 17, 2017

k8s-ci-robot commented May 17, 2017

timstclair left a 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

ericchiang commented May 17, 2017 • edited

timothysc commented May 17, 2017

ericchiang commented May 17, 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

sttts commented May 31, 2017

sttts commented May 31, 2017

Choose a reason for hiding this comment

ericchiang commented May 31, 2017

deads2k commented May 31, 2017

k8s-github-robot commented May 31, 2017

ericchiang commented May 31, 2017

timstclair commented May 31, 2017

timstclair commented May 31, 2017

timstclair commented May 31, 2017

ericchiang commented May 31, 2017

timstclair commented May 31, 2017

timstclair commented Jun 1, 2017

sttts commented Jun 1, 2017

sttts commented Jun 1, 2017

k8s-ci-robot commented Jun 1, 2017 • edited

timstclair commented Jun 1, 2017

k8s-github-robot commented Jun 1, 2017

ericchiang commented May 16, 2017 •

edited

ericchiang commented May 17, 2017 •

edited

k8s-ci-robot commented Jun 1, 2017 •

edited