Use SRC_LOG_LEVEL for audit logs#52566
Conversation
| func getLoggerFuncWithSeverity(logger log.Logger) func(string, ...log.Field) { | ||
| lvl := log.Level(env.LogLevel).Parse() |
There was a problem hiding this comment.
The PR description states:
We should have our audit logs default to same level
But this to me looks more like we will always use the SRC_LOG_LEVEL, no matter what. So are we trying to default to at least the level that instance uses or are we trying to always set it to SRC_LOG_LEVEL?
There was a problem hiding this comment.
Based on the site configuration and the PR title, I think the intention might be to just use SRC_LOG_LEVEL directly. In a way I think this makes sense, since it ensures that it is always logging output while also keeping it the lowest-priority log level available
However it could be confusing if each service has a different SRC_LOG_LEVEL (not always the case, but can happen). If we're going this direction we should call out that the log level of security event audit logs is not to be depended on - we just do whatever it takes to ensure the logs show up.
There was a problem hiding this comment.
Audit logging will always use the instance's configured log level so that if audit logs are configured they are always emitted.
- I don't think we should have users collecting audit logs based on the SeverityLevel.
SRC_LOG_LEVELis not immediately viewable by site admins. They might think they setup audit logging and then be surprised that they set the value to low. For example, setting the value toINFOwhenSRC_LOG_LEVELis set toWARNresults in no audit logs being emitted.
There was a problem hiding this comment.
I don't think we should have users collecting audit logs based on the SeverityLevel.
That's my point, we should document this intention, since if we ask customers to set up their own sinks for these logs it's possible someone might assume log level is related.
There was a problem hiding this comment.
Added notes to the CHANGELOG and marked this field as deprecated.
There was a problem hiding this comment.
Changelog entries are ephemeral - can you add it to the permanent docs? In audit_logs.md:
### On Premises
There are two easy approaches to filtering the audit logs:
- JSON-based: look for the presence of the `Attributes.audit` node. Do not depend on the log level, as it can change based on `SRC_LOG_LEVEL`.(can't suggest on this portion of the docs, sorry!)
There was a problem hiding this comment.
Nice catch! Updated
SRC_LOG_LEVEL is not immediately viewable to the site admin. We should have our audit logs default to same level to avoid the footgun of setting the log level below the instance level which would result in an unexpected behavior of NO audit logs being captured Co-authored-by: Robert Lin <robert@bobheadxi.dev>
027e4f6 to
7e09993
Compare
|
Codenotify: Notifying subscribers in CODENOTIFY files for diff 94b925e...baa50b6.
|
Co-authored-by: Robert Lin <robert@bobheadxi.dev>
Co-authored-by: Robert Lin <robert@bobheadxi.dev>
…ub.com/sourcegraph/sourcegraph into dax/validate_log_level_with_audit_logs
|
Happy to address any other comments post-merge as well. |
Co-authored-by: Robert Lin <robert@bobheadxi.dev>
SRC_LOG_LEVEL is not immediately viewable to the site admin. We should set our audit logs to the same level to avoid the footgun of setting the log level below the instance level, which would result in unexpected behavior of NO audit logs being captured.
This results in audit logging occurring by default on startup for security event logs which may be unexpected and requires the user to explicitly set no audit logs to be gathered.
Test plan
Go tests and manual testing