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
audit-log: Only capture API method args when asked #8222
Conversation
apiserver/observer/recorder.go
Outdated
@@ -16,20 +16,26 @@ import ( | |||
// recorders that that will update the observer and the auditlog | |||
// recorder when it records a request or reply. The auditlog recorder | |||
// can be nil. | |||
func NewRecorderFactory(observerFactory rpc.ObserverFactory, recorder *auditlog.Recorder) rpc.RecorderFactory { | |||
func NewRecorderFactory( | |||
captureArgs bool, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personal preference, but this arg IMO should go to the end of the param list.
So (factory, record, capture)
It looks icky to see callers go NewRecorderFactory(true, factory, recorder)
Also, we should define consts for RecordArgs and NoRecordArgs and use those instead of true/false
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks!
apiserver/observer/recorder.go
Outdated
} | ||
} | ||
} | ||
|
||
// combinedRecorder wraps an observer (which might be a multiplexer) | ||
// up with an auditlog recorder into an rpc.Recorder. | ||
type combinedRecorder struct { | ||
observer rpc.Observer | ||
recorder *auditlog.Recorder | ||
captureArgs bool |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before - make it go to the end
bca4774
to
823729c
Compare
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
Looks like AWS killed it? |
Build failed: Tests failed |
|
Status: merge request accepted. Url: http://ci.jujucharms.com/job/github-merge-juju |
Description of change
In general we don't think it will be necessary to save the API method arguments in the audit log (because people using it will be more interested in the client commands), and there's some risk of there being secrets in the log. If
audit-log-capture-args=true
is specified in the controller config then we still capture them, but by default we don't.QA steps
Bootstrap with
--config="auditing-enabled=true"
. Therequest
messages in the audit log don't include arguments for the API calls.Bootstrap with
--config="auditing-enabled=true" --config="audit-log-capture-args=true"
.request
messages include serialised JSON method parameters.