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

Feature Gate - Kubeadm Audit Logging #59067

Merged
merged 1 commit into from Feb 12, 2018

Conversation

@chuckha
Member

chuckha commented Jan 30, 2018

Fixes kubernetes/kubeadm#623

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.

@@ -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 {

This comment has been minimized.

@timothysc

timothysc Jan 30, 2018

Member

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

This comment has been minimized.

@liggitt

liggitt Jan 31, 2018

Member

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

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

This comment has been minimized.

@dixudx

dixudx Jan 31, 2018

Member

Users cannot customize the rules here.

This comment has been minimized.

@timothysc

timothysc Jan 31, 2018

Member

How do you propose we expose?

This comment has been minimized.

@dixudx

dixudx Feb 1, 2018

Member

@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.

This comment has been minimized.

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

This comment has been minimized.

@dixudx

dixudx Jan 31, 2018

Member

s/pod/policy

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?

This comment has been minimized.

@timothysc

timothysc Feb 1, 2018

Member

Need to rationalize this comment.

This comment has been minimized.

@chuckha

chuckha Feb 2, 2018

Member

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.

@timothysc timothysc self-assigned this Feb 1, 2018

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

This comment has been minimized.

@chuckha

chuckha Feb 2, 2018

Member

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.

This comment has been minimized.

@chuckha

chuckha Feb 2, 2018

Member

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

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

This comment has been minimized.

@liggitt

liggitt Feb 4, 2018

Member

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?

This comment has been minimized.

@chuckha

chuckha Feb 5, 2018

Member

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?

This comment has been minimized.

@liggitt

liggitt Feb 5, 2018

Member

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

This comment has been minimized.

@chuckha

chuckha Feb 5, 2018

Member

that seems sensible! 👍 thanks for the input

@ncdc

This comment has been minimized.

Member

ncdc commented Feb 6, 2018

/ok-to-test

@ncdc

This comment has been minimized.

Member

ncdc commented Feb 6, 2018

/test all

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

This comment has been minimized.

@chuckha

chuckha Feb 6, 2018

Member

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

@@ -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,

This comment has been minimized.

@kargakis

kargakis Feb 7, 2018

Member

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

This comment has been minimized.

@timothysc

timothysc Feb 7, 2018

Member

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?
@@ -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,

This comment has been minimized.

@timothysc

timothysc Feb 7, 2018

Member

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.

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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

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

This comment has been minimized.

@chuckha

chuckha Feb 8, 2018

Member

sounds good!

// 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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

How about changing the type to int32?

@@ -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"

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

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

This comment has been minimized.

@timothysc

timothysc Feb 8, 2018

Member

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

This comment has been minimized.

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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

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

} 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)

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

Also print the default file path here?

This comment has been minimized.

@chuckha

chuckha Feb 8, 2018

Member

sure, seems reasonable

@@ -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}},

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

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

This comment has been minimized.

@chuckha

chuckha Feb 8, 2018

Member

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

This comment has been minimized.

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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

s/2017/2018

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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

extra line

This comment has been minimized.

@chuckha

chuckha Feb 8, 2018

Member

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

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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

same as above

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

This comment has been minimized.

@xiangpengzhao

xiangpengzhao Feb 8, 2018

Member

s/2017/2018

@timothysc

address other comment then lgtm

/approve

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.

This comment has been minimized.

@timothysc

timothysc Feb 8, 2018

Member

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

This comment has been minimized.

@chuckha

chuckha Feb 8, 2018

Member

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

@@ -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"

This comment has been minimized.

@timothysc

timothysc Feb 8, 2018

Member

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

@timothysc

This comment has been minimized.

Member

timothysc commented Feb 9, 2018

/test pull-kubernetes-kubemark-e2e-gce

Enable Audit Logs Behind a Feature Gate
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

This comment has been minimized.

Member

chuckha commented Feb 9, 2018

/test pull-kubernetes-unit

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

@timothysc

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Feb 12, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 12, 2018

[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:

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 Feb 12, 2018

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

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 12, 2018

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

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

12 of 13 checks passed

Submit Queue Required Github CI test is not green: pull-kubernetes-e2e-gce
Details
cla/linuxfoundation chuckha 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-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

@chuckha chuckha deleted the chuckha:audit branch Feb 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment