feat(logging): RHICOMPL-1217 introduce audit logging #734
Conversation
Codecov Report
@@ Coverage Diff @@
## master #734 +/- ##
==========================================
- Coverage 97.30% 95.70% -1.61%
==========================================
Files 129 132 +3
Lines 2379 2494 +115
==========================================
+ Hits 2315 2387 +72
- Misses 64 107 +43
Continue to review full report at Codecov.
|
vkrizan
force-pushed
the
feat-audit-logging-1217
branch
from
c00d49f to
671af0e
Compare
There was a problem hiding this comment.
Looking good! I've already reviewed this on RedHatInsights/insights-api-common-rails#217. Thanks, @vkrizan!
Introduction of a middleware and utility to add entries to an audit log. By default it logs them to STDOUT, but eventually it can be tied into CloudWatch. Log entries are formatted into JSON, split by a new-line.
vkrizan
force-pushed
the
feat-audit-logging-1217
branch
from
671af0e to
ba1e683
Compare
There was a problem hiding this comment.
Looking great, @vkrizan! Just a couple small comments about naming, paths, etc., but I was able to enable the middleware without much fuss locally. Here's what a log look like:
{"@timestamp":"2021-02-09T23:05:15.780582Z","hostname":"dfa3fe94f424","pid":1,"thread_id":"2b0cf4a3925c","level":"warning","transaction_id":"57ba8762f3b44f8b08a3","message":"GET /api/compliance/v1/systems/435c3df2-4e81-4986-8462-8a9ba930c512 -> 404 Not Found","controller":"V1::SystemsController#show","remote_ip":"127.0.0.1"}
Are you intending to enable the logger in this PR? It'd certainly help to review if it was enabled. I'd love if we could get this going to log/audit.log in Rails.env == 'development' for debugging purposes (instead of mixed into STDOUT and log/development.log with all the rest).
| begin | ||
| yield | ||
| ensure | ||
| Thread.current[:audit_account_number] = original |
There was a problem hiding this comment.
👍
Should we wait until the other PR is ACK'd to see if there are any other recommendations?
There was a problem hiding this comment.
We can always go back and change it. 😉
| module Insights | ||
| module API | ||
| module Common | ||
| class AuditLog | ||
| # Request events listener, capturing for evidence | ||
| class Middleware |
There was a problem hiding this comment.
This represents the top level object, the middleware itself. If I understand correctly, it's intended to use this like:
config.middleware.use Insights::API::Common::AuditLog::MiddlewareIf that's the case, we need to ensure all the requires start from here so things like AuditLog.logger are already defined properly.
There was a problem hiding this comment.
Yes, the usage is correct.
The logger is setup with Insights::API::Common::AuditLog.setup(Logger.new). Require has no effect on it, only the first use of the logger. The default one can be overridden by the mentioned setup.
| @@ -0,0 +1,130 @@ | |||
| # frozen_string_literal: true | |||
|
|
|||
| require 'audit_log/audit_log' | |||
There was a problem hiding this comment.
I suppose this is the require intended to be used within application.rb?
No. Please treat it as a library.
Sure, you can use setup: |
Introduction of a middleware and utility to add entries to an audit log. By default it logs them to STDOUT, but eventually it can be tied into CloudWatch. Log entries are formatted into JSON, split by a new-line.
Originally posted at RedHatInsights/insights-api-common-rails#217
TODO:
Secure Coding Practices Checklist GitHub Link
Secure Coding Checklist