Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
audit-logging: Filter uninteresting conversations out of audit log #8242
Conversation
babbageclunk
added some commits
Dec 18, 2017
anastasiamac
reviewed
Dec 19, 2017
Looks pretty neat!
(I love the fact that you went with observer pattern :D my favourite!)
My only concern at this stage is that Login call itself is not audited.
| @@ -160,8 +160,11 @@ func (a *admin) getAuditRecorder(req params.LoginRequest, authResult *authResult | ||
| if !authResult.userLogin || a.srv.auditLogger == nil { | ||
| return nil, nil | ||
| } | ||
| + // Wrap the audit logger in a filter that prevents us from logging | ||
| + // lots of readonly conversations (like juju status requests). |
| @@ -967,9 +969,23 @@ func (s *loginSuite) TestLoginAddsAuditConversation(c *gc.C) { | ||
| err := conn.APICall("Admin", 3, "", "Login", request, &result) | ||
| c.Assert(err, jc.ErrorIsNil) | ||
| c.Assert(result.UserInfo, gc.NotNil) | ||
| + // Nothing's logged at this point because there haven't been any |
anastasiamac
Dec 19, 2017
Member
I am a bit concerned that "Login" request itself is not considered "interesting"... Most of auditing is actually VERY interested in "login" and "login" attempts :D
babbageclunk
Dec 19, 2017
Member
Hmm. Since every client API interaction requires a login, including all logins would kind of defeat the purpose of filtering here - my watch juju status will still fill up the log with noise. I think you're right about logging failed login attempts though. I'll add a card for it and do it as a separate PR, if that's ok - it's pretty separate from this change.
| @@ -1003,10 +1032,23 @@ func (s *loginSuite) TestAuditLoggingFailurePreventsLogin(c *gc.C) { | ||
| request := ¶ms.LoginRequest{ | ||
| AuthTag: user.Tag().String(), | ||
| Credentials: password, | ||
| - CLIArgs: "hey you guys", | ||
| + |
| + "Client.GetModelConstraints", | ||
| + "Client.StatusHistory", | ||
| + "Controller.AllModels", | ||
| + "Controller.ControllerConfig", |
anastasiamac
Dec 19, 2017
Member
Is this a read-only call? I seem to recall that it was also a mutator based on whether the data was passed-in or not...
| + "Controller.AllModels", | ||
| + "Controller.ControllerConfig", | ||
| + "Controller.GetControllerAccess", | ||
| + "Controller.ModelConfig", |
| + "Action.ApplicationsCharmsActions", | ||
| + "Action.FindActionsByNames", | ||
| + "Action.FindActionTagsByPrefix", | ||
| + "Application.Get", |
anastasiamac
Dec 19, 2017
Member
We will not be recording it :D
This is a black list of Facade.Method - meaning "do not audit these calls". This will also ignore all arguments, parameters and return values to/from these methods.
wgrant
Dec 19, 2017
Right, but if you're going to audit things, surely you want to audit things that will be the easiest way to exfiltrate secrets.
babbageclunk
Dec 19, 2017
Member
Yeah, that's a good point - and I don't think users will do it so often in the normal course of events that it'll obscure other information in the logs.
babbageclunk
Dec 19, 2017
•
Edited 1 time
-
babbageclunk
Dec 19, 2017
Member
-
Dec 19, 2017
Oops, I meant to say, so I'll remove it from the filter list.
anastasiamac
approved these changes
Dec 19, 2017
|
I would be much happier if this filtering was being done on read rather than on write. Because you can't ever recover the information if it was never written in the first place. |
|
Thanks John - yeah, looking at it now I think I went overboard on the filtering. Maybe a better way would be minimal filtering by default (just |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
babbageclunk commentedDec 19, 2017
Description of change
Read-only API requests (like status) aren't interesting from the perspective of audit logging. To avoid burying important information in the log we buffer the conversation and any uninteresting requests/responses until we see an interesting request, at which point we flush the buffer and write out any later messages immediately.
This means that any logged conversation will have the full set of requests even if some of them are only read-only.
QA steps
Bootstrap a controller with auditing enabled. Run various read-only commands, check that none of them generate audit log entries. Run commands that change state (setting config, deploying apps, adding and removing models, units and machines). Check that all of those show up in the audit log.