Switch to use upstream audit handler #11192

soltysh · 2016-10-03T13:45:57Z

@sttts for the switch itself

@openshift/api-review for config changes

liggitt · 2016-10-03T13:49:38Z

vendor/k8s.io/kubernetes/pkg/apiserver/audit/audit.go

+		asgroups := "<lookup>"
+		requestedGroups := req.Header[authenticationapi.ImpersonateGroupHeader]
+		if len(requestedGroups) > 0 {
+			asgroups = strings.Join(requestedGroups, ", ")


this makes it impossible to determine whether a group name had a comma in it

Can it? If so, how about just space?

Updated to space.

Updated to something like this: "'group 1', 'group 2'", according to @deads2k upstream comment.

smarterclayton · 2016-10-03T18:33:13Z

pkg/cmd/server/api/types.go

@@ -325,6 +325,14 @@ type AuditConfig struct {
 	// If this flag is set, audit log will be printed in the logs.
 	// The logs contains, method, user and a requested URL.
 	Enabled bool
+	// All requests coming to the apiserver will be logged to this file.
+	Path string


If this is a path to a file i would recommend making this AuditFilePath

smarterclayton · 2016-10-03T18:33:53Z

pkg/cmd/server/api/types.go

+	// All requests coming to the apiserver will be logged to this file.
+	Path string
+	// Maximum number of days to retain old audit log files based on the timestamp encoded in their filename.
+	MaxAge int


MaximumAuditRetention* where * is the unit. I don't think Days is appropriate, pick seconds or minutes.

I have to stick to days for now, since the luberjack library supports just days.

smarterclayton · 2016-10-03T18:34:10Z

pkg/cmd/server/api/types.go

+	// Maximum number of days to retain old audit log files based on the timestamp encoded in their filename.
+	MaxAge int
+	// Maximum number of old audit log files to retain.
+	MaxBackups int


MaximumRetainedAuditFiles

smarterclayton · 2016-10-03T18:34:39Z

pkg/cmd/server/api/types.go

+	// Maximum number of old audit log files to retain.
+	MaxBackups int
+	// Maximum size in megabytes of the audit log file before it gets rotated. Defaults to 100MB.
+	MaxSize int


MaximumAuditFileSizeMegabytes (there's an example of this somewhere else, make it consistent with ordering)

smarterclayton

See comments

soltysh · 2016-10-04T08:55:24Z

@smarterclayton updated with one exception, I stick to days for MaximumAuditRetention* since that's the only granularity I have when working with lumberjack log library. ptal

soltysh · 2016-10-06T12:29:38Z

@smarterclayton are we good to go?

soltysh · 2016-10-06T19:39:14Z

@smarterclayton updated

smarterclayton · 2016-10-07T01:35:39Z

API approved

soltysh · 2016-10-07T10:28:43Z

@sttts any objections for merging this in?

deads2k · 2016-10-07T12:49:33Z

@sttts any objections for merging this in?

He's out next week too. I merged the upstream pull, right?

deads2k · 2016-10-07T12:50:46Z

pkg/cmd/server/api/types.go

@@ -325,6 +325,14 @@ type AuditConfig struct {
 	// If this flag is set, audit log will be printed in the logs.
 	// The logs contains, method, user and a requested URL.
 	Enabled bool
+	// All requests coming to the apiserver will be logged to this file.
+	AuditFilePath string


I don't see validation on these fields and I don't see this path listed in the list of paths for resolution.

@deads2k what kind of validation you're thinking about? that the path field is non-empty and ints are >= 0? I don't see any value in that.

deads2k · 2016-10-07T14:41:12Z

Need to handle backwards compatibility. Can you have it continue to come out in the old log if the filename isn't specified?

soltysh · 2016-10-07T16:12:16Z

@deads2k comments addressed in the last commit, ptal

deads2k · 2016-10-07T16:28:26Z

pkg/cmd/server/api/helpers.go

@@ -256,6 +256,10 @@ func GetMasterFileReferences(config *MasterConfig) []*string {
 		refs = append(refs, &config.ControllerConfig.ServiceServingCert.Signer.KeyFile)
 	}

+	if len(config.AuditConfig.AuditFilePath) > 0 {


don't think you need the len check.

Not sure I want to have empty file name here.

OK, I see all the places that use this are ignoring the empty file paths.

deads2k · 2016-10-07T16:28:52Z

pkg/cmd/server/api/validation/master.go

+
+	if len(config.AuditFilePath) == 0 {
+		// for backwards compatibility reasons we can't error this out
+		validationResults.AddWarnings(field.Required(fldPath.Child("auditFilePath"), ""))


use the reason field to describe the change

deads2k · 2016-10-07T16:29:31Z

pkg/cmd/server/origin/master.go

@@ -158,6 +159,17 @@ func (fn APIInstallFunc) InstallAPI(container *restful.Container) ([]string, err
 	return fn(container)
 }

+// auditGlogWriter is for backwards compatibility, where audit was logged
+// to regular log file, instead of separate one like it's currently available.
+type auditGlogWriter struct{}


@stevekuznetsov didn't you build this before?

I remember wrapping the glog writer somewhere ... can't remember where

here:

origin/pkg/cmd/util/log.go

Line 21 in 85eb37b

func NewGLogWriterV(level int) io.Writer {

deads2k · 2016-10-07T16:30:16Z

Minor comments and the API change looks good to me.

soltysh · 2016-10-07T22:16:07Z

Comments addressed. @deads2k ptal

deads2k · 2016-10-10T18:09:30Z

lgtm [merge]

openshift-bot · 2016-10-10T18:12:24Z

[Test]ing while waiting on the merge queue

soltysh · 2016-10-17T09:01:38Z

Flake #8571

[merge]

sttts · 2016-10-18T07:05:16Z

pkg/cmd/server/origin/master.go

@@ -179,7 +182,22 @@ func (c *MasterConfig) Run(protected []APIInstaller, unprotected []APIInstaller)
 	handler = c.authorizationFilter(handler)
 	handler = c.impersonationFilter(handler)
 	// audit handler must comes before the impersonationFilter to read the original user
-	handler = c.auditHandler(handler)
+	if c.Options.AuditConfig.Enabled {


Kind of ugly to have this code block here. Hopefully, we are able to get rid of this in a future rebase.

We'll be able to move it where necessary, but the option will stay forever with us ;)

sttts · 2016-10-18T07:25:54Z

pkg/cmd/server/origin/master.go

@@ -875,6 +893,23 @@ func (c *MasterConfig) getRequestContextMapper() kapi.RequestContextMapper {
 	return c.RequestContextMapper
 }

+// getRequestInfoResolver returns a request resolver.
+func (c *MasterConfig) getRequestInfoResolver() *apiserver.RequestInfoResolver {


This will bite us in the next rebase. Upstream genericapiserver only knows about one APIPrefix and one APIGroupPrefix. Looks like we have to turn those into lists in order to support origin's setup. /cc @deads2k

I do hope the next rebase (I'm the lucky looser/winner) will straight this out.

WIP PR for multiple api group prefixes in genericapiserver: kubernetes/kubernetes#35021

Let me know when you start the master.go rebase. We have moved around so much stuff, hopefully to the better.

I remember, we'll be in touch, definitely.

This will bite us in the next rebase. Upstream genericapiserver only knows about one APIPrefix and one APIGroupPrefix. Looks like we have to turn those into lists in order to support origin's setup. /cc @deads2k

Those are all legacy prefixes which I just made have a list upstream: https://github.com/kubernetes/kubernetes/blob/master/pkg/genericapiserver/config.go#L175

soltysh · 2016-10-18T07:55:10Z

Fixed the test and rebased against latest head, hopefully to have a clean merge.

…ePath is provided

soltysh · 2016-10-20T09:44:28Z

Fixed the remaining unit test.

openshift-bot · 2016-10-20T09:45:28Z

Evaluated for origin test up to 59e59e3

openshift-bot · 2016-10-20T09:45:39Z

Evaluated for origin merge up to 59e59e3

openshift-bot · 2016-10-20T11:05:25Z

continuous-integration/openshift-jenkins/test FAILURE (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10294/) (Base Commit: 7405f17)

openshift-bot · 2016-10-22T04:13:26Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/10459/) (Base Commit: 30bbfa2) (Image: devenv-rhel7_5221)

soltysh added area/security needs-api-review kind/post-rebase labels Oct 3, 2016

liggitt reviewed Oct 3, 2016

View reviewed changes

soltysh force-pushed the issue11037 branch from e58f9c5 to b50ba7b Compare October 3, 2016 15:19

smarterclayton reviewed Oct 3, 2016

View reviewed changes

smarterclayton suggested changes Oct 3, 2016

View reviewed changes

soltysh force-pushed the issue11037 branch 2 times, most recently from 03e7784 to be3768c Compare October 4, 2016 08:54

soltysh force-pushed the issue11037 branch from be3768c to ca93c14 Compare October 6, 2016 19:38

smarterclayton added api-approved and removed needs-api-review labels Oct 7, 2016

soltysh force-pushed the issue11037 branch from ca93c14 to 3e7d6eb Compare October 7, 2016 10:28

deads2k reviewed Oct 7, 2016

View reviewed changes

soltysh force-pushed the issue11037 branch from 14c601c to 3b06327 Compare October 7, 2016 22:15

sttts reviewed Oct 18, 2016

View reviewed changes

soltysh added 3 commits October 18, 2016 09:54

UPSTREAM: 33934: Add asgroups to audit log

a371261

Switch to use upstream audit handler

d069dca

Switch to use upstream audit handler - generated changes

fa8395e

soltysh force-pushed the issue11037 branch from 3b06327 to 8eb72f0 Compare October 18, 2016 07:54

Add AuditConfig validation and backwards compatibility if no AuditFil…

59e59e3

…ePath is provided

soltysh force-pushed the issue11037 branch from 8eb72f0 to 59e59e3 Compare October 20, 2016 09:43

openshift-bot merged commit 8a79d34 into openshift:master Oct 22, 2016

soltysh deleted the issue11037 branch October 24, 2016 08:19

soltysh mentioned this pull request Jan 19, 2017

OpenShift 1.4/3.4 Release Notes Tracker openshift/openshift-docs#2933

Closed

Switch to use upstream audit handler #11192

Switch to use upstream audit handler #11192

Conversation

soltysh commented Oct 3, 2016

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

smarterclayton left a comment

Choose a reason for hiding this comment

soltysh commented Oct 4, 2016

soltysh commented Oct 6, 2016

soltysh commented Oct 6, 2016

smarterclayton commented Oct 7, 2016

soltysh commented Oct 7, 2016

deads2k commented Oct 7, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented Oct 7, 2016

soltysh commented Oct 7, 2016

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

deads2k commented Oct 7, 2016

soltysh commented Oct 7, 2016

deads2k commented Oct 10, 2016

openshift-bot commented Oct 10, 2016

soltysh commented Oct 17, 2016

Choose a reason for hiding this comment

soltysh Oct 18, 2016 • 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

soltysh commented Oct 18, 2016

soltysh commented Oct 20, 2016

openshift-bot commented Oct 20, 2016

openshift-bot commented Oct 20, 2016

openshift-bot commented Oct 20, 2016

openshift-bot commented Oct 22, 2016 • edited Loading

soltysh Oct 18, 2016 •

edited

Loading

openshift-bot commented Oct 22, 2016 •

edited

Loading