platformmanagement_public_704: Added basic auditing capabilities #8815

soltysh · 2016-05-10T09:56:36Z

This implements basic audit capabilities as described in this trello.
The only thing is there's no log from the response, I'd have to do it completely differently to get it.
Sample log looks like this:

I0510 09:55:56.405651   17618 audit.go:21] GET - test-admin@openshift - /oapi/v1/namespaces/openshift/imagestreams/python

and is enabled with following config option:

auditConfig:
  enabled: true

@openshift/api-review for api changes
@pweil- @danmcp ptal
@mpbarrett fyi

danmcp · 2016-05-10T11:14:22Z

pkg/cmd/server/origin/audit.go

+		if len(namespace) != 0 {
+			usernamespace = fmt.Sprintf("%s@%s", user.GetName(), namespace)
+		}
+		glog.Infof("%s - %s - %v", req.Method, usernamespace, req.URL)


Can we print the user as well?

We do, it's in the form user@namespace, or just user when no namespace is there. See the sample code which shows test-admin@openshift.

I'd like to suggest a standard prefix for all audit statements so they're easy to parse out of the log with a grep rather than finding all info messages. Maybe "AUDIT: <field>:%s,<field>:%s, ...">. Adding the field name in there will probably also save us a bit of explanation for those trying to parse it. @soltysh WDYT?

I had that, but then I figured out I'd go with audit.go file name which will do just that 😄 Field names sgtm.

Again, see the sample line ;)

I think we want something machine parsable. %q does escaping, right?

How good an audit trail is this supposed to be? Do we need query params?

I had that, but then I figured out I'd go with audit.go file name which will do just that Field names sgtm.

That is dependent on a particular glog formatting choice, which is theoretically changable. I'd pay for the extra characters in this method.

How good an audit trail is this supposed to be? Do we need query params?

it couldn't hurt to have URL.RawQuery

You'll want to track if this is an impersonation request.

soltysh · 2016-05-10T15:28:07Z

Comments addressed, now the log consists of two lines, (request, response) (@pweil & @danmcp for you two especially 😉):

AUDIT: uniqueid: ip:<value>, method:<value>, username:<value>, namespace:<value>, uri:<value>
AUDIT: uniqueid: response:<value>

Samples from my logs (without glog prefixes):

AUDIT: 70234f32-b3c6-458b-be1f-bdceae2ce6cc: ip:127.0.0.1:40468, method:GET, username:test-admin, namespace: openshift, uri:/oapi/v1/namespaces/openshift/imagestreams/python
AUDIT: 70234f32-b3c6-458b-be1f-bdceae2ce6cc: response:200

AUDIT: be569c80-116a-4937-87ec-59d36ecb5f91: ip:192.168.121.118:57320, method:GET, username:system:openshift-master, namespace: <none>, uri:/apis/extensions/v1beta1/replicasets?resourceVersion=0
AUDIT: be569c80-116a-4937-87ec-59d36ecb5f91: response:200

@deads2k ptal as well

pweil- · 2016-05-10T16:39:30Z

this LGTM for the basics. @smarterclayton anything you'd like to see added?

deads2k · 2016-05-10T16:41:43Z

pkg/cmd/server/origin/audit.go

+		}
+		id := uuid.NewRandom().String()
+
+		glog.Infof("AUDIT: %s: ip:%s, method:%s, username:%s, namespace: %s, uri:%v",


You either need to add an AUDIT message in the impersonation filter using the same UID (obviously you'd have to attach it to the context) or this needs to understand the impersonation annotation.

soltysh · 2016-05-11T08:20:42Z

The current state is as follows:
Configuration:

auditConfig:
  enabled: true

Format:

AUDIT: uniqueid: ip:<value>, method:<value>, user:<value>, as:<value>, namespace:<value>, uri:<value>
AUDIT: uniqueid: response:<value>

Sample logs:
as myself:

AUDIT: d34c3759-f290-49c8-96cd-e005faf498b5: ip:127.0.0.1:51022, method:GET, user:test-admin, as:<himself>, namespace: openshift, uri:/oapi/v1/namespaces/openshift/imagestreams/python
AUDIT: d34c3759-f290-49c8-96cd-e005faf498b5: response:200

system:admin as test-admin:

AUDIT: c913de1a-58e6-433d-81e1-0bc94d580485: ip:192.168.121.118:46732, method:GET, user:system:admin, as:test-admin, namespace: openshift, uri:/oapi/v1/namespaces/openshift/imagestreams/python
AUDIT: c913de1a-58e6-433d-81e1-0bc94d580485: response:200

@deads2k @smarterclayton need your final singoff

soltysh · 2016-05-11T08:22:13Z

pkg/cmd/server/origin/master.go

@@ -164,6 +164,8 @@ func (c *MasterConfig) Run(protected []APIInstaller, unprotected []APIInstaller)
 	handler := c.versionSkewFilter(safe)
 	handler = c.authorizationFilter(handler)
 	handler = c.impersonationFilter(handler)
+	// audit handler must comes before the impersonationFilter to read the original user
+	handler = c.auditHandler(handler)


@deads2k that surprises me, I thought the handler order is FIFO, but from what I've tested it's LIFO.

@deads2k that surprises me, I thought the handler order is FIFO, but from what I've tested it's LIFO.

One handler wraps another. I don't think you can build it the other way around.

deads2k · 2016-05-11T20:18:10Z

minor comments. I don't need a second look.

soltysh · 2016-05-11T21:10:55Z

Comments addressed. One bug found on the way (see upstream pr). @smarterclayton merge at will.

smarterclayton · 2016-05-11T21:37:18Z

pkg/cmd/server/origin/audit.go

+		user, _ := kapi.UserFrom(ctx)
+		asuser := req.Header.Get(authenticationapi.ImpersonateUserHeader)
+		if len(asuser) == 0 {
+			asuser = "<himself>"


This is a little weird - I would say <self>

… the host:port format

smarterclayton · 2016-05-11T21:39:49Z

pkg/cmd/server/origin/audit.go

+		}
+		id := uuid.NewRandom().String()
+
+		glog.Infof("AUDIT: %s: ip:%v, method:%s, user:%q, as:%q, namespace:%s, uri:%v",


This needs to be greppable, I would recommend the following:

Infof("AUDIT: id=%q ip=%q method=%q user=%q as=%q namespace=%q uri=%q")

That ensures that each segment is wellformed and can be safely deconstructed later. I do not like adding unescaped values, since this is going to be assumed to be machine readable.

%q will allow you to safely quote escaped strings.

soltysh · 2016-05-11T21:55:43Z

<self> and %q it is.

Sample logs:

as <self>:

AUDIT: id="75114bb5-970a-47d5-a5f1-1e99cea0574c" ip="127.0.0.1" method="GET" user="test-admin" as="<self>" namespace="openshift" uri="/oapi/v1/namespaces/openshift/imagestreams/python"
AUDIT: id="75114bb5-970a-47d5-a5f1-1e99cea0574c" response=200

as user:

AUDIT: id="b0a443ae-f7d8-408c-a355-eb9501fd5c59" ip="192.168.121.118" method="GET" user="system:admin" as="test-admin" namespace="openshift" uri="/oapi/v1/namespaces/openshift/imagestreams/python"
AUDIT: id="b0a443ae-f7d8-408c-a355-eb9501fd5c59" response=200

@smarterclayton ptal

smarterclayton · 2016-05-12T20:34:59Z

To be consistent response should be in quotes as well.

soltysh · 2016-05-12T20:49:15Z

@smarterclayton quoted response, as requested.

smarterclayton · 2016-05-12T23:41:58Z

Lgtm [merge] thanks

On May 12, 2016, at 4:49 PM, Maciej Szulik notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton quoted response, as
requested.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8815 (comment)

openshift-bot · 2016-05-13T00:20:12Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5904/) (Image: devenv-rhel7_4185)

smarterclayton · 2016-05-13T00:47:29Z

Tests are broken

On May 12, 2016, at 8:20 PM, OpenShift Bot notifications@github.com wrote:

continuous-integration/openshift-jenkins/merge FAILURE (
https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/5902/
)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#8815 (comment)

openshift-bot · 2016-05-13T08:35:15Z

Evaluated for origin merge up to 9d872ad

soltysh · 2016-05-13T08:36:07Z

bummer... 😊
Updated, now jenkins, do your job 👍

danmcp reviewed May 10, 2016
View reviewed changes

soltysh force-pushed the basic_audit branch from 3fdaeac to fb62ab8 Compare May 10, 2016 15:22

deads2k reviewed May 10, 2016
View reviewed changes

soltysh force-pushed the basic_audit branch from fb62ab8 to d5c43dd Compare May 11, 2016 08:15

soltysh reviewed May 11, 2016
View reviewed changes

soltysh force-pushed the basic_audit branch from d5c43dd to 329f4f1 Compare May 11, 2016 21:10

smarterclayton reviewed May 11, 2016
View reviewed changes

UPSTREAM: 25501: SplitHostPort is needed since Request.RemoteAddr has…

cb5820f

… the host:port format

smarterclayton reviewed May 11, 2016
View reviewed changes

soltysh force-pushed the basic_audit branch 2 times, most recently from fe1dd7e to 193e57c Compare May 11, 2016 21:53

soltysh force-pushed the basic_audit branch from 193e57c to 076ef74 Compare May 12, 2016 20:48

platformmanagement_public_704: Added basic auditing capabilities

9d872ad

soltysh force-pushed the basic_audit branch from 076ef74 to 9d872ad Compare May 13, 2016 08:34

openshift-bot merged commit fc8af39 into openshift:master May 13, 2016

soltysh deleted the basic_audit branch May 14, 2016 07:40

detiber mentioned this pull request Jul 13, 2016

[master] add support for setting auditConfig openshift/openshift-ansible#2149

Merged

bergerx mentioned this pull request Sep 5, 2016

Audit log #3976

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

platformmanagement_public_704: Added basic auditing capabilities #8815

platformmanagement_public_704: Added basic auditing capabilities #8815

soltysh commented May 10, 2016 •

edited by liggitt

Loading

danmcp May 10, 2016

soltysh May 10, 2016

pweil- May 10, 2016

soltysh May 10, 2016

soltysh May 10, 2016

deads2k May 10, 2016

deads2k May 10, 2016

deads2k May 10, 2016

pweil- May 10, 2016

deads2k May 10, 2016

soltysh commented May 10, 2016

pweil- commented May 10, 2016

deads2k May 10, 2016

soltysh commented May 11, 2016

soltysh May 11, 2016

deads2k May 11, 2016

deads2k commented May 11, 2016

soltysh commented May 11, 2016

smarterclayton May 11, 2016

0xmichalis May 11, 2016

smarterclayton May 11, 2016

smarterclayton May 11, 2016

soltysh commented May 11, 2016

smarterclayton commented May 12, 2016

soltysh commented May 12, 2016

smarterclayton commented May 12, 2016

openshift-bot commented May 13, 2016 •

edited

Loading

smarterclayton commented May 13, 2016

openshift-bot commented May 13, 2016

soltysh commented May 13, 2016

platformmanagement_public_704: Added basic auditing capabilities #8815

platformmanagement_public_704: Added basic auditing capabilities #8815

Conversation

soltysh commented May 10, 2016 • edited by liggitt 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

soltysh commented May 10, 2016

pweil- commented May 10, 2016

Choose a reason for hiding this comment

soltysh commented May 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deads2k commented May 11, 2016

soltysh commented May 11, 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

soltysh commented May 11, 2016

smarterclayton commented May 12, 2016

soltysh commented May 12, 2016

smarterclayton commented May 12, 2016

openshift-bot commented May 13, 2016 • edited Loading

smarterclayton commented May 13, 2016

openshift-bot commented May 13, 2016

soltysh commented May 13, 2016

soltysh commented May 10, 2016 •

edited by liggitt

Loading

openshift-bot commented May 13, 2016 •

edited

Loading