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: integrate auditlog.Recorder into API server #8184
Conversation
| @@ -276,6 +282,27 @@ func (c RateLimitConfig) Validate() error { | ||
| return nil | ||
| } | ||
| +// AuditLogConfig holds parameters to control audit logging. | ||
| +type AuditLogConfig struct { |
wallyworld
Dec 7, 2017
Owner
Maybe as a drive by, move these config structs (audit, logsink etc) to a separate config.go file. We have stuffed way too much into this file already.
| + // Whether to capture API method args (command line args will | ||
| + // always be captured). | ||
| + CaptureAPIArgs bool | ||
| + // Max log file size (in Mb). |
| + // always be captured). | ||
| + CaptureAPIArgs bool | ||
| + // Max log file size (in Mb). | ||
| + MaxSize int |
wallyworld
Dec 7, 2017
Owner
MaxSizeMB
(as we do elsewhere to allow the reader to grok the units from the var name)
| apiObserver observer.Observer, | ||
| host string, | ||
| ) error { | ||
| codec := jsoncodec.NewWebsocket(wsConn.Conn) | ||
| - conn := rpc.NewConn(codec, apiObserver) | ||
| + conn := rpc.NewConn(codec, observer.NewRecorderFactory(apiObserver, nil)) |
babbageclunk
Dec 10, 2017
Member
Because we only create an audit recorder when we get a login from a human.
| @@ -937,7 +972,7 @@ func (srv *Server) serveConn( | ||
| for apiVersion, factory := range adminAPIFactories { | ||
| adminAPIs[apiVersion] = factory(srv, h, apiObserver) | ||
| } | ||
| - conn.ServeRoot(newAdminRoot(h, adminAPIs), serverError) | ||
| + conn.ServeRoot(newAdminRoot(h, adminAPIs), nil, serverError) |
wallyworld
Dec 7, 2017
Owner
we don't need the recorder here because we aren't doing anything yet, just setting up?
babbageclunk
Dec 8, 2017
Member
No, that code is wrong now - it needs to be a recorder factory with at least the apiObserver there.
| @@ -53,6 +53,11 @@ type apiHandler struct { | ||
| // user manager and model manager api endpoints from here. | ||
| modelUUID string | ||
| + // this connection ID is shared between the API observer |
| + // RPCObserver will return a new Observer usually constructed | ||
| + // from the state previously built up in the Observer. The | ||
| + // returned instance will be utilized per RPC request. | ||
| + RPCObserver() Observer |
wallyworld
Dec 7, 2017
Owner
maybe just Observer() ?
there's no need to bring RPC into this interface method, we're already in the rpc package
babbageclunk
Dec 8, 2017
Member
This is an existing interface - I've just moved it here. I think it's RPCObserver to make it clearer when it's embedded in the apiserver/observer.Observer interface (rather than it just being Observer.Observer). Agreed that it's a bit awkward though. I can change it, but it'll hit the other observer implementations.
| +// details of a single request/response. | ||
| +type RecorderFactory func() Recorder | ||
| + | ||
| +// Recorder represents something the connection use to record requests |
| +// and replies. Recording a message can fail (for example for audit | ||
| +// logging), and when it does the request should be failed as well. | ||
| +type Recorder interface { | ||
| + ServerRequest(hdr *Header, body interface{}) error |
wallyworld
Dec 7, 2017
Owner
Would prefer these method names start with a verb, eg
HandleServerRequest()
HandleServerReply()
or just
HandleRequest()
HandleReply()
the later nicely matches the name of the funcs in which these would be called
babbageclunk
Dec 8, 2017
Member
I picked these to match the Observer interface methods (the only difference is the error return). But I think you're right it would be better for them to be verbs, and they don't actually need to match the Observer methods - maybe having them be distinct might make it clearer anyway. I'll change them.
| - | ||
| -func (nopObserverFactory) RPCObserver() Observer { | ||
| - return nopObserver{} | ||
| +func ensureFactory(f RecorderFactory) RecorderFactory { |
wallyworld
Dec 7, 2017
Owner
There's a few places where we pass in nil for the factory.
We should always use ensureFactory?
babbageclunk
Dec 8, 2017
Member
The methods that can be passed a nil recorder (Serve and ServeRoot) both call serve which calls ensureFactory before assigning it.
babbageclunk
added some commits
Dec 5, 2017
babbageclunk
changed the base branch from
develop
to
2.3
Dec 11, 2017
|
!!build!! |
babbageclunk
added some commits
Dec 10, 2017
babbageclunk
changed the title from
WIP audit-logging: integrate auditlog.Recorder into API server
to
audit-logging: integrate auditlog.Recorder into API server
Dec 11, 2017
| + }, | ||
| + ) | ||
| + if err != nil { | ||
| + logger.Errorf("couldn't add login to audit log: %T %+v", err, err) |
wallyworld
Dec 11, 2017
Owner
Do we need the %T here? is this just for debugging? Do we need the logged message here at all, or will the err eventually get logged at the call site?
|
$$merge$$ |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
babbageclunk commentedDec 7, 2017
•
Edited 1 time
-
babbageclunk
Dec 11, 2017
Description of change
This hooks up the core/auditlog package to the API server, by creating a recorder in the Admin facade when the user logs in and having the server
rpc.Connnotify it about requests and replies.Replaces the observer factory the Conn was using with a
RecorderFactory- the only real difference is that theRecorder.ServerRequestand.ServerReplymethods allow returning an error (so we don't allow an operation to proceed if we can't write it to the audit log). Then the API server passes in aRecorderthat combines the observers with an audit logger.At the moment the audit logging always captures method arguments, and doesn't extract errors from the responses. Those will come in subsequent PRs.
QA steps
Spin up a controller with
--auditing-enabled=true. Check that messages corresponding to user commands likedeployandadd-unitare logged in/var/log/juju/audit.log, as well as the resulting API calls.Documentation changes
The audit logging will need documentation.