Feature Gate - Kubeadm Audit Logging #59067

chuckha · 2018-01-30T21:21:00Z

Signed-off-by: Chuck Ha ha.chuck@gmail.com

What this PR does / why we need it:
This PR enables Auditing behind a featureGate. A user can supply their own audit policy with configuration option as well as a place for the audit logs to live. If no policy is supplied a default policy will be provided. The default policy will log all Metadata level policy logs. It is the example provided in the documentation.
Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#623

Special notes for your reviewer:

Release note:

kubeadm: Enable auditing behind a feature gate.

timothysc

Need some feedback on knob & default before we continue.

timothysc · 2018-01-30T21:27:31Z

cmd/kubeadm/app/phases/controlplane/manifests.go

@@ -128,6 +129,12 @@ func createStaticPodFiles(manifestDir string, cfg *kubeadmapi.MasterConfiguratio
 		return err
 	}

+	// create audit log file for apiserver
+	if err := audit.CreateDefaultAuditLogPolicy(cfg.AuditPolicyDir); err != nil {


People are going to want to knob paths here for policy on logging. Given it's beta I'm debating whether we should feature gate it or just provide a 1st class override.

/cc @roberthbailey @kubernetes/sig-auth-pr-reviews

seems like a natural point to drop in a custom policy, especially since nothing else in kubeadm is really functionally dependent on this particular default audit policy

dixudx · 2018-01-31T06:38:05Z

cmd/kubeadm/app/util/audit/utils.go

+		},
+		Rules: []auditv1beta1.PolicyRule{
+			{
+				Level: auditv1beta1.LevelMetadata,


Users cannot customize the rules here.

How do you propose we expose?

@chuckha @timothysc Let the users use their own policy files optionally.
Just make a slight change to current AuditPolicyDir behavior.
If AuditPolicyDir is not set, create and use this default policy file. If set, find that file named audit.yaml and use it.

dixudx · 2018-01-31T06:41:26Z

cmd/kubeadm/app/util/audit/utils.go

+	// Registers auditv1beta1 with the runtime Scheme
+	auditv1beta1.AddToScheme(scheme.Scheme)
+
+	// writes the pod to disk


s/pod/policy

timothysc · 2018-02-01T15:20:21Z

cmd/kubeadm/app/apis/kubeadm/v1alpha1/defaults.go

+		obj.AuditPolicyLogDir = DefaultAuditPolicyLogDir
+	}
+
+	// TODO(chuckha) is 0 a meaningful value here? Does it mean 0 days or forever or can it be used as the empty value?


Need to rationalize this comment.

https://github.com/natefinch/lumberjack/blob/v2.0/lumberjack.go#L78

0 is a valid number and, in addition to MaxBackups == 0, means nothing will get deleted. I'll need to update this to accept 0 as valid and meaningful.

chuckha · 2018-02-02T15:24:22Z

cmd/kubeadm/app/constants/constants.go

+	// AuditPolicyFileName defines audit policy resource file for apiserver
+	AuditPolicyFileName = "audit.yaml"
+	// AuditPolicyLogFileName defines the audit log filename
+	AuditPolicyLogFileName = "audit.log"


Audit log uses lumberjack under the hood which uses timestamps embedded in the filename to determine age of logs. I don't think defining an AuditPolicyLogFileName is the right thing to do here, unless I'm missing something.

nope, my misundertsanding. lumberjack will take care of this automatically after the max-size.

timothysc

/approve - minor style nits, but I don't feel strongly about them.

@dixudx - poke for once over and lgtm

liggitt · 2018-02-04T07:08:24Z

cmd/kubeadm/app/phases/controlplane/manifests.go

@@ -128,6 +129,15 @@ func createStaticPodFiles(manifestDir string, cfg *kubeadmapi.MasterConfiguratio
 		return err
 	}

+	// use the default audit policy if no audit policy exists
+	auditPolicyFilePath := filepath.Join(cfg.AuditPolicyDir, kubeadmconstants.AuditPolicyFileName)


Pinned file names inside user specified directories makes for a really opaque interface. Is there a reason not to simply have the user specify the policy file directly?

I could add an --audit-policy-file flag to combine with the --audit-policy-dir flag to make this customizable but default to audit.yaml. Does that sound reasonable?

why not just --audit-policy-file as the full filepath and drop --audit-policy-dir?

that seems sensible! 👍 thanks for the input

ncdc · 2018-02-06T21:38:58Z

/ok-to-test

ncdc · 2018-02-06T21:39:22Z

/test all

chuckha · 2018-02-06T21:53:55Z

cmd/kubeadm/app/apis/kubeadm/v1alpha1/defaults.go

+
+// SetDefaults_AuditPolicyConfiguration sets default values for the AuditPolicyConfiguration
+func SetDefaults_AuditPolicyConfiguration(obj *MasterConfiguration) {
+	if obj.AuditPolicyConfiguration.Path == "" {


"" is actually the default I want here (as in, this is wrong)

0xmichalis · 2018-02-07T09:46:51Z

cmd/kubeadm/app/cmd/init.go

@@ -191,6 +192,18 @@ func AddInitConfigFlags(flagSet *flag.FlagSet, cfg *kubeadmapiext.MasterConfigur
 		&cfg.TokenTTL.Duration, "token-ttl", cfg.TokenTTL.Duration,
 		"The duration before the bootstrap token is automatically deleted. If set to '0', the token will never expire.",
 	)
+	flagSet.StringVar(
+		&cfg.AuditPolicyConfiguration.Path, "audit-policy", cfg.AuditPolicyConfiguration.Path,


I think there was agreement in yesterday's SIG meeting to not proceed with adding flags until we sort out whether we want to be supporting these in the future.

@kubernetes/sig-cluster-lifecycle-misc

I think we may have to default off with a opt-in via config option for the time being. Due to the logistical constraints on both args, and self-hosting HA concerns.

This will enable folks to turn on b/default with a separate PR, and unblock this one.

fabriziopandini

@chuckha :+1 for this PR!

Two high level comments about the implementation:

if we are extending how static pod manifest are created in kubeadm init, it is necessary to ensure a consistent behaviour also in kubeadm alpha phase controlplane (e.g. same flags, same logic for the creation of the policy file if it doesn't exists).
the dependency from the kubea-apiserver to the audit policy file introduce a new element that should be taken into account for self-hosting and HA. I think it is important to inform as many people as possible about this. @timothysc, what about discussing this in the next SIG-meeting?

timothysc · 2018-02-07T19:23:23Z

cmd/kubeadm/app/cmd/init.go

@@ -191,6 +192,18 @@ func AddInitConfigFlags(flagSet *flag.FlagSet, cfg *kubeadmapiext.MasterConfigur
 		&cfg.TokenTTL.Duration, "token-ttl", cfg.TokenTTL.Duration,
 		"The duration before the bootstrap token is automatically deleted. If set to '0', the token will never expire.",
 	)
+	flagSet.StringVar(
+		&cfg.AuditPolicyConfiguration.Path, "audit-policy", cfg.AuditPolicyConfiguration.Path,


I think we may have to default off with a opt-in via config option for the time being. Due to the logistical constraints on both args, and self-hosting HA concerns.

This will enable folks to turn on b/default with a separate PR, and unblock this one.

xiangpengzhao

just some nits.

xiangpengzhao · 2018-02-08T01:38:53Z

cmd/kubeadm/app/apis/kubeadm/fuzzer/fuzzer.go

+			obj.AuditPolicyConfiguration = kubeadm.AuditPolicyConfiguration{
+				Path:      "foo",
+				LogDir:    "/foo",
+				LogMaxAge: &zero,


If the type of LogMaxAge is going to be changed to int32, then you can use utilpointer.Int32Ptr(0) instead :)

sounds good!

xiangpengzhao · 2018-02-08T01:40:57Z

cmd/kubeadm/app/apis/kubeadm/types.go

+	// LogDir is the local pth to the directory where logs should be stored.
+	LogDir string
+	// LogMaxAge is the number of days logs will be stored for. 0 indicates forever.
+	LogMaxAge *int


How about changing the type to int32?

xiangpengzhao · 2018-02-08T01:44:27Z

cmd/kubeadm/app/apis/kubeadm/v1alpha1/defaults.go

@@ -67,6 +67,15 @@ const (
 	DefaultProxyBindAddressv6 = "::"
 	// KubeproxyKubeConfigFileName defines the file name for the kube-proxy's KubeConfig file
 	KubeproxyKubeConfigFileName = "/var/lib/kube-proxy/kubeconfig.conf"
+
+	// DefaultAuditPolicyLogDir is the default directory where the audit system will write
+	DefaultAuditPolicyLogDir = "/var/log/kubernetes"


How about creating a sub-dir? This is OK anyway.

@xiangpengzhao - Do you mean /var/log/kubernetes/audit ?

@timothysc exactly :)

xiangpengzhao · 2018-02-08T01:46:01Z

cmd/kubeadm/app/apis/kubeadm/types.go

+	LogDir string
+	// LogMaxAge is the number of days logs will be stored for. 0 indicates forever.
+	LogMaxAge *int
+}


Add the same TODO here? i.e., //TODO(chuckha) add other options for audit policy

xiangpengzhao · 2018-02-08T01:48:16Z

cmd/kubeadm/app/cmd/init.go

+		} else {
+			i.cfg.AuditPolicyConfiguration.Path = filepath.Join(kubeConfigDir, kubeadmconstants.AuditPolicyDir, kubeadmconstants.AuditPolicyFile)
+			if err := auditutil.CreateDefaultAuditLogPolicy(i.cfg.AuditPolicyConfiguration.Path); err != nil {
+				return fmt.Errorf("error creating default audit policy [%v]", err)


Also print the default file path here?

sure, seems reasonable

xiangpengzhao · 2018-02-08T01:55:06Z

cmd/kubeadm/app/features/features.go

@@ -53,6 +56,7 @@ var InitFeatureGates = FeatureList{
 	HighAvailability:     {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}, MinimumVersion: v190, HiddenInHelpText: true},
 	CoreDNS:              {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}, MinimumVersion: v190},
 	DynamicKubeletConfig: {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}, MinimumVersion: v190},
+	Auditing:             {FeatureSpec: utilfeature.FeatureSpec{Default: false, PreRelease: utilfeature.Alpha}},


s/Alpha/Beta as per the comment in L45 // Auditing is beta in 1.8 ?

Auditing in k8s is in beta as of 1.8, but the feature to enable it with kubeadm, I assumed, would be considered Alpha

xiangpengzhao · 2018-02-08T01:57:25Z

cmd/kubeadm/app/util/audit/utils.go

@@ -0,0 +1,69 @@
+/*
+Copyright 2017 The Kubernetes Authors.


s/2017/2018

xiangpengzhao · 2018-02-08T01:58:10Z

cmd/kubeadm/app/util/audit/utils.go

+
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	auditv1beta1 "k8s.io/apiserver/pkg/apis/audit/v1beta1"
+


weird, maybe my goimports settings are wonky, i'll need to look into this so they are in line with k8s expectations.

xiangpengzhao · 2018-02-08T01:58:20Z

cmd/kubeadm/app/util/audit/utils.go

+	auditv1beta1 "k8s.io/apiserver/pkg/apis/audit/v1beta1"
+
+	"k8s.io/kubernetes/cmd/kubeadm/app/util"
+


same as above

xiangpengzhao · 2018-02-08T01:59:12Z

cmd/kubeadm/app/util/audit/utils_test.go

@@ -0,0 +1,61 @@
+/*
+Copyright 2017 The Kubernetes Authors.


s/2017/2018

timothysc

address other comment then lgtm

/approve

timothysc · 2018-02-08T00:20:55Z

cmd/kubeadm/app/cmd/init.go

+	if features.Enabled(i.cfg.FeatureGates, features.Auditing) {
+		// Setup the AuditPolicy (either it was passed in and exists or it wasn't passed in and generate a default policy)
+		if i.cfg.AuditPolicyConfiguration.Path != "" {
+			// TODO(chuckha) ensure passed in audit policy is valid so users don't have to find the error in the api server log.


@kubernetes/sig-auth-pr-reviews - is there a mechanism to verify that the audit policy is sane?

I was thinking try to serialize it into a Policy type and see if it was valid, that would probably catch a lot of the cases

timothysc · 2018-02-08T20:28:37Z

cmd/kubeadm/app/apis/kubeadm/v1alpha1/defaults.go

@@ -67,6 +67,15 @@ const (
 	DefaultProxyBindAddressv6 = "::"
 	// KubeproxyKubeConfigFileName defines the file name for the kube-proxy's KubeConfig file
 	KubeproxyKubeConfigFileName = "/var/lib/kube-proxy/kubeconfig.conf"
+
+	// DefaultAuditPolicyLogDir is the default directory where the audit system will write
+	DefaultAuditPolicyLogDir = "/var/log/kubernetes"


@xiangpengzhao - Do you mean /var/log/kubernetes/audit ?

timothysc · 2018-02-09T00:57:17Z

/test pull-kubernetes-kubemark-e2e-gce

Audit logs are configurable via the MasterConfiguration file. All options are ignored unless the FeatureGate is enabled. Fixes kubernetes/kubeadm#623 Signed-off-by: Chuck Ha <ha.chuck@gmail.com>

chuckha · 2018-02-09T22:19:54Z

/test pull-kubernetes-unit

timothysc

/lgtm

k8s-ci-robot · 2018-02-12T16:53:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chuckha, timothysc

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 OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [timothysc]

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

k8s-github-robot · 2018-02-12T16:56:04Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-12T17:35:41Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot requested review from dmmcquay and 0xmichalis January 30, 2018 21:21

timothysc added sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 30, 2018

chuckha mentioned this pull request Jan 30, 2018

Collect Audit Logs vmware-tanzu/sonobuoy#163

Closed

timothysc suggested changes Jan 30, 2018

View reviewed changes

dixudx reviewed Jan 31, 2018

View reviewed changes

timothysc reviewed Feb 1, 2018

View reviewed changes

timothysc self-assigned this Feb 1, 2018

chuckha commented Feb 2, 2018

View reviewed changes

chuckha force-pushed the audit branch 2 times, most recently from 6da599a to b5ae3fb Compare February 2, 2018 19:04

timothysc approved these changes Feb 2, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 2, 2018

liggitt reviewed Feb 4, 2018

View reviewed changes

chuckha force-pushed the audit branch from b5ae3fb to 8d56507 Compare February 6, 2018 21:23

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2018

chuckha force-pushed the audit branch from 8d56507 to 5f08ca9 Compare February 6, 2018 21:29

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

chuckha force-pushed the audit branch from 5f08ca9 to 891aeff Compare February 6, 2018 21:38

chuckha commented Feb 6, 2018

View reviewed changes

chuckha force-pushed the audit branch 2 times, most recently from 682afde to 73b4fd6 Compare February 6, 2018 23:30

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 7, 2018

0xmichalis reviewed Feb 7, 2018

View reviewed changes

fabriziopandini reviewed Feb 7, 2018

View reviewed changes

chuckha force-pushed the audit branch from 73b4fd6 to 00ba42a Compare February 7, 2018 15:17

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

chuckha force-pushed the audit branch from 00ba42a to 7399be3 Compare February 7, 2018 15:59

timothysc suggested changes Feb 7, 2018

View reviewed changes

chuckha force-pushed the audit branch from 7399be3 to 4eaed69 Compare February 7, 2018 20:50

xiangpengzhao suggested changes Feb 8, 2018

View reviewed changes

timothysc suggested changes Feb 8, 2018

View reviewed changes

k8s-ci-robot added the sig/auth Categorizes an issue or PR as relevant to SIG Auth. label Feb 8, 2018

chuckha force-pushed the audit branch 2 times, most recently from 02371b6 to 386f853 Compare February 8, 2018 23:09

chuckha force-pushed the audit branch from 386f853 to 07dac15 Compare February 9, 2018 20:52

Enable Audit Logs Behind a Feature Gate

ed76917

Audit logs are configurable via the MasterConfiguration file. All options are ignored unless the FeatureGate is enabled. Fixes kubernetes/kubeadm#623 Signed-off-by: Chuck Ha <ha.chuck@gmail.com>

chuckha force-pushed the audit branch from 07dac15 to ed76917 Compare February 9, 2018 21:44

timothysc changed the title ~~Enable Audit Logs~~ Feature Gate - Kubeadm Audit Logging Feb 12, 2018

timothysc approved these changes Feb 12, 2018

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 12, 2018

k8s-github-robot merged commit fdeaa8c into kubernetes:master Feb 12, 2018

chuckha deleted the audit branch February 12, 2018 17:40

hh mentioned this pull request Apr 18, 2018

Enable Advanced Auditing Webhook Backend Configuration kubernetes/kubeadm#759

Closed


		metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
		auditv1beta1 "k8s.io/apiserver/pkg/apis/audit/v1beta1"

		auditv1beta1 "k8s.io/apiserver/pkg/apis/audit/v1beta1"

		"k8s.io/kubernetes/cmd/kubeadm/app/util"

Feature Gate - Kubeadm Audit Logging #59067

Feature Gate - Kubeadm Audit Logging #59067

Conversation

chuckha commented Jan 30, 2018 • edited Loading

timothysc left a comment

Choose a reason for hiding this comment

timothysc Jan 30, 2018 • 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

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

timothysc 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

Choose a reason for hiding this comment

ncdc commented Feb 6, 2018

ncdc commented Feb 6, 2018

chuckha Feb 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiangpengzhao 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

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

timothysc 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

timothysc commented Feb 9, 2018

chuckha commented Feb 9, 2018

timothysc left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 12, 2018

k8s-github-robot commented Feb 12, 2018

k8s-github-robot commented Feb 12, 2018

chuckha commented Jan 30, 2018 •

edited

Loading

timothysc Jan 30, 2018 •

edited

Loading

chuckha Feb 6, 2018 •

edited

Loading