Implement audit policy logic #46009

timstclair · 2017-05-18T02:24:35Z

Includes #45315 (comment) (ignore the first commit)

Remaining work:

Load the policy into the server.Config
Rebase on Advanced audit logging API #45315
Establish shared code for audit api scheme (with apiserver: add a webhook implementation of the audit backend #45919)
Once Audit: fill audit.Event in handler chain #45766 is merged, call the policy checker in the audit path

/cc @sttts @soltysh @ericchiang @ihmccreery @pweil- @deads2k

sttts · 2017-05-18T08:15:35Z

staging/src/k8s.io/apiserver/pkg/apis/audit/validation/validation.go

+	case audit.LevelNone, audit.LevelMetadata, audit.LevelRequest, audit.LevelRequestResponse:
+		return nil
+	default:
+		return field.ErrorList{field.Required(fldPath, "")}


add a "" case and a proper error message for non-empty, but invalid level.

sttts · 2017-05-18T08:19:56Z

staging/src/k8s.io/apiserver/pkg/audit/policy/checker.go

+		}
+	}
+
+	if len(r.Namespaces) > 0 || len(r.Resources) > 0 {


why the first clause? We have cluster-scope resources.

ruleMatchesResource checks whether the request matches any Namespaces or Resources from the rule. If neither are set, there's nothing to check :)

misread the operator, it's || not &&

sttts · 2017-05-18T08:23:38Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader.go

+		panic(fmt.Sprintf("failed to enable version %v", groupVersions))
+	}
+
+	install.Install(groupFactoryRegistry, registry, scheme)


don't like to depend on the registry here. Using the scheme with AddToScheme is enough. The registry should be for CRUD api groups.

Ack. This is section is throwaway code.

sttts · 2017-05-18T08:24:25Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader.go

+		return nil, fmt.Errorf("failed to read file path %s: %+v", filePath, err)
+	}
+	if len(policyDef) == 0 {
+		return nil, fmt.Errorf("file was empty: %s", filePath)


won't that fail the decoding?

Actually no, it will just create an empty policy.

sttts · 2017-05-18T08:25:37Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader.go

+
+	decoder := codecs.UniversalDecoder(v1alpha1.SchemeGroupVersion)
+	if err := runtime.DecodeInto(decoder, policyDef, policyVersioned); err != nil {
+		return nil, fmt.Errorf("failed decoding file: %v", err)


doesn't matter where, but we need the file name in the error messages somewhere.

timstclair · 2017-05-18T21:28:06Z

staging/src/k8s.io/apiserver/pkg/audit/policy/reader.go

+
+// FIXME: Figure out a shared location for this vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
+// Duplicated from @ericchiang's PR:
+//   https://github.com/kubernetes/kubernetes/pull/45919


@ericchiang FYI

sttts · 2017-05-19T09:40:13Z

staging/src/k8s.io/apiserver/pkg/features/kube_features.go

+	// AdvancedAuditing enables a much more general API auditing pipeline, which includes support for
+	// pluggable output backends and an audit policy specifying how different requests should be
+	// audited.
+	AdvancedAuditing utilfeature.Feature = "AdvancedAuditing"


What do we want to switch here? By default auditing is off. If you use the --audit-log-* flags, you will get the log plugin. What about the default level? It used to be meta data only, but not "none".

We could re-add the old WithAudit filter as WithBasicAudit and use that if the feature is disabled. Wdyt?

We could re-add the old WithAudit filter as WithBasicAudit and use that if the feature is disabled. Wdyt?

That's what I was suggesting in #45766 (comment). Since the pipeline is growing in complexity, I think we should do that.

Reasons for adding a feature flag:

This code is coming in hot and it mitigates risk. Additionally, it would make a code freeze exception easier if we end up needing it...

Default audit level (off = metadata, on = none)

If users specify flags for the new features (e.g. set a policy, different backend) they should be aware that they are using an alpha feature. Requiring the feature gate to be set makes that more explicit

Makes the support & completeness level of advanced auditing explicit

sttts · 2017-05-19T09:40:57Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

@@ -45,9 +50,30 @@ func (o *AuditLogOptions) AddFlags(fs *pflag.FlagSet) {
 		"The maximum number of old audit log files to retain.")
 	fs.IntVar(&o.MaxSize, "audit-log-maxsize", o.MaxSize,
 		"The maximum size in megabytes of the audit log file before it gets rotated.")
+
+	fs.StringVar(&o.PolicyPath, "audit-policy", o.PolicyPath,
+		"Path to the file that defines the audit policy configuration.")


add something about the feature flag.

And about the default of "none".

sttts · 2017-05-19T09:41:54Z

staging/src/k8s.io/apiserver/pkg/server/options/audit.go

 }

 func (o *AuditLogOptions) ApplyTo(c *server.Config) error {
+	if utilfeature.DefaultFeatureGate.Enabled(features.AdvancedAuditing) {


this answer my question from above.

@timstclair

Automatic merge from submit-queue (batch tested with PRs 45766, 46223) Audit: fill audit.Event in handler chain Related: - external API types #45315 - policy checker #46009 Decisions: - ~~[ ] decide whether we want to send an event before `WriteHeader` #45766 (review) Follow-up described in https://github.com/kubernetes/kubernetes/pull/46065/files#r117438531 - [ ] decide how to handle `AuditID`s and the IP chain #45766 (review). Is the variant in the proposal (kubernetes/community#625) final? Then we need the API type update. - ~~[ ] decide how to mark intermediate/incomplete events? set a special reason in `ResponseStatus.Reason` vs. having extra fields for that `Event.NonFinal` #45766 (comment) Follow-up of #46065 - [ ] decide whether and how to protect the `Audit-Level` header #45766 (review) TODOs: - ~~[ ] move `AuditIDHeader`, `AuditLevelHeader` to types #45766 (comment), @timstclair for the type PR~~ Follow-up of #46065 - [x] add SourceIP/ForwardedFor support #45766 (comment) - [x] adapt ObjectReference.Resource to API PR #45766 (review)

timstclair · 2017-05-23T22:57:34Z

staging/src/k8s.io/apiserver/pkg/apis/audit/scheme/register.go

+	"k8s.io/apiserver/pkg/apis/audit/v1alpha1"
+)
+
+var Scheme = runtime.NewScheme()


@ericchiang FYI

Can we move this scheme into pkg/audit? This has nothing todo with the actual API, but the plumbing around it. In general we are trying to make schemes more local.

timstclair · 2017-05-23T22:58:45Z

Rebased on #45766, and finished remaining work items.

This PR is now complete. PTAL

sttts · 2017-05-24T07:19:13Z

I would prefer to have the scheme locally in pkg/audit. Otherwise, lgtm.

timstclair · 2017-05-24T15:14:04Z

Moved scheme to pkg/audit, rebased & squashed. PTAL.

sttts · 2017-05-24T20:22:19Z

/lgtm
/approve

thockin · 2017-05-24T21:04:23Z

/approve

k8s-github-robot · 2017-05-24T21:04:38Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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

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

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

timstclair · 2017-05-25T14:44:43Z

Trivial rebase, reapplying LGTM.

k8s-ci-robot · 2017-05-25T15:31:03Z

@timstclair: The following test(s) failed:

Test name	Commit	Details	Rerun command
Jenkins Bazel Build	d5a391d11a30852924d1df1795b90485ea056087	link	`@k8s-bot bazel test this`
Jenkins Kubemark GCE e2e	d5a391d11a30852924d1df1795b90485ea056087	link	`@k8s-bot kubemark e2e test this`
Jenkins GCE Node e2e	d5a391d11a30852924d1df1795b90485ea056087	link	`@k8s-bot node e2e test this`
Jenkins GCE etcd3 e2e	d5a391d11a30852924d1df1795b90485ea056087	link	`@k8s-bot gce etcd3 e2e test this`
Jenkins kops AWS e2e	d5a391d11a30852924d1df1795b90485ea056087	link	`@k8s-bot kops aws e2e test this`
Jenkins unit/integration	d5a391d11a30852924d1df1795b90485ea056087	link	`@k8s-bot unit test this`
Jenkins verification	d5a391d11a30852924d1df1795b90485ea056087	link	`@k8s-bot verify 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-05-25T15:53:45Z

@k8s-bot pull-kubernetes-unit test this

k8s-github-robot · 2017-05-26T02:40:58Z

Automatic merge from submit-queue (batch tested with PRs 45949, 46009, 46320, 46423, 46437)

@soltysh

Automatic merge from submit-queue apiserver: add a webhook implementation of the audit backend This builds off of #45315 and is intended to implement an interfaced defined in #45766. TODO: - [x] Rebase on top of API types PR. - [x] Rebase on top of API types updates (#46065) - [x] Rebase on top of feature flag (#46009) - [x] Rebase on top of audit instrumentation. - [x] Hook up API server flag or register plugin (depending on #45766) Features issue kubernetes/enhancements#22 Design proposal https://github.com/kubernetes/community/blob/master/contributors/design-proposals/auditing.md ```release-notes Webhook added to the API server which omits structured audit log events. ``` /cc @soltysh @timstclair @soltysh @deads2k

@timstclair

Automatic merge from submit-queue (batch tested with PRs 45766, 46223) Audit: fill audit.Event in handler chain Related: - external API types kubernetes/kubernetes#45315 - policy checker kubernetes/kubernetes#46009 Decisions: - ~~[ ] decide whether we want to send an event before `WriteHeader` kubernetes/kubernetes#45766 (review) Follow-up described in https://github.com/kubernetes/kubernetes/pull/46065/files#r117438531 - [ ] decide how to handle `AuditID`s and the IP chain kubernetes/kubernetes#45766 (review). Is the variant in the proposal (kubernetes/community#625) final? Then we need the API type update. - ~~[ ] decide how to mark intermediate/incomplete events? set a special reason in `ResponseStatus.Reason` vs. having extra fields for that `Event.NonFinal` kubernetes/kubernetes#45766 (comment) Follow-up of #46065 - [ ] decide whether and how to protect the `Audit-Level` header kubernetes/kubernetes#45766 (review) TODOs: - ~~[ ] move `AuditIDHeader`, `AuditLevelHeader` to types kubernetes/kubernetes#45766 (comment), @timstclair for the type PR~~ Follow-up of kubernetes/kubernetes#46065 - [x] add SourceIP/ForwardedFor support kubernetes/kubernetes#45766 (comment) - [x] adapt ObjectReference.Resource to API PR kubernetes/kubernetes#45766 (review) Kubernetes-commit: 1f45c4846bafaa8f2f17deb53f4284cc78e83210

timstclair added this to the v1.7 milestone May 18, 2017

timstclair self-assigned this May 18, 2017

k8s-ci-robot requested review from ikehz, soltysh, sttts, ericchiang, pweil- and deads2k May 18, 2017 02:24

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

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

sttts reviewed May 18, 2017

View reviewed changes

sttts mentioned this pull request May 18, 2017

Audit: fill audit.Event in handler chain #45766

Merged

4 tasks

timstclair force-pushed the audit-policy branch from a092f9a to 935d615 Compare May 18, 2017 21:26

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 18, 2017

timstclair commented May 18, 2017

View reviewed changes

sttts reviewed May 19, 2017

View reviewed changes

timstclair force-pushed the audit-policy branch from 2fd326c to 2837d38 Compare May 23, 2017 22:31

timstclair commented May 23, 2017

View reviewed changes

timstclair changed the title ~~WIP: Implement audit policy logic~~ Implement audit policy logic May 23, 2017

ericchiang mentioned this pull request May 23, 2017

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

Merged

5 tasks

timstclair force-pushed the audit-policy branch from 36982d3 to 73eacd1 Compare May 24, 2017 15:13

timstclair force-pushed the audit-policy branch from 73eacd1 to fdb13d3 Compare May 24, 2017 15:23

timstclair added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels May 24, 2017

timstclair assigned sttts and unassigned timstclair May 24, 2017

timstclair force-pushed the audit-policy branch from fdb13d3 to 8cc8d21 Compare May 24, 2017 17:23

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

k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels May 24, 2017

Implement audit policy logic

a5de309

timstclair force-pushed the audit-policy branch from 8cc8d21 to a5de309 Compare May 25, 2017 14:44

timstclair added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 25, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 25, 2017

k8s-github-robot merged commit ae03f22 into kubernetes:master May 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement audit policy logic #46009

Implement audit policy logic #46009

timstclair commented May 18, 2017 •

edited

sttts May 18, 2017

timstclair May 18, 2017

sttts May 18, 2017

timstclair May 18, 2017

sttts May 18, 2017

sttts May 18, 2017

timstclair May 18, 2017

sttts May 18, 2017

timstclair May 18, 2017

sttts May 18, 2017

sttts May 18, 2017 •

edited

timstclair May 18, 2017

timstclair May 18, 2017

sttts May 19, 2017

timstclair May 19, 2017

sttts May 19, 2017

sttts May 19, 2017

timstclair May 19, 2017

sttts May 19, 2017

timstclair May 23, 2017

sttts May 24, 2017

timstclair commented May 23, 2017

sttts commented May 24, 2017

timstclair commented May 24, 2017

sttts commented May 24, 2017

thockin commented May 24, 2017

k8s-github-robot commented May 24, 2017

timstclair commented May 25, 2017

k8s-ci-robot commented May 25, 2017 •

edited

timstclair commented May 25, 2017

k8s-github-robot commented May 26, 2017

Implement audit policy logic #46009

Implement audit policy logic #46009

Conversation

timstclair commented May 18, 2017 • edited

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 May 18, 2017 • edited

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

timstclair commented May 23, 2017

sttts commented May 24, 2017

timstclair commented May 24, 2017

sttts commented May 24, 2017

thockin commented May 24, 2017

k8s-github-robot commented May 24, 2017

timstclair commented May 25, 2017

k8s-ci-robot commented May 25, 2017 • edited

timstclair commented May 25, 2017

k8s-github-robot commented May 26, 2017

timstclair commented May 18, 2017 •

edited

sttts May 18, 2017 •

edited

k8s-ci-robot commented May 25, 2017 •

edited