Skip to content
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

feat(logger): introduce audit logging #217

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 30 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,36 @@ Or install it yourself as:

## Usage

#### Insights::Api::Common::Filter
### Insights::Api::Common::AuditLog

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.

Rails setup:
```
class Application < Rails::Application
...
require 'insights/api/common'
config.middleware.use Insights::API::Common::AuditLog::Middleware
end
```

Optionally the logger can be set up with:
```
Insights::API::Common::AuditLog.setup(Logger.new)
```

Setting context of an account (e.g. an authenticated account) can be done with these options
```
Insights::API::Common::AuditLog.with_account('12345')

Insights::API::Common::AuditLog.with_account('12345') do
# limited context
end
```

### Insights::Api::Common::Filter
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like this should still be an H4. Same with Insights::Api::Common::AuditLog above. See #### Sorting Results below.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one I was not sure about. I'd let owners to look into it. The "Usage" is H2, so the next one should be H3. I'm not sure whether the "Sorting Results" are part of the Filter or not.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing says you have to use headers in order H1 -> H2 -> H3 etc. H1 -> H3 -> H5 works just as well, probably chosen so the markdown renders more nicely in the GitHub UI without overly large headers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are document composition guidelines, hence the structure (and order).


| Supported Comparators | Comparator |
| --------------------- | ---------- |
Expand Down
1 change: 1 addition & 0 deletions lib/insights/api/common.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
require "insights/api/common/audit_log"
require "insights/api/common/custom_exceptions"
require "insights/api/common/engine"
require "insights/api/common/entitlement"
Expand Down
41 changes: 41 additions & 0 deletions lib/insights/api/common/audit_log.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
# frozen_string_literal: true

require_relative 'audit_log/formatter'
require_relative 'audit_log/middleware'

module Insights::API::Common
# Audit Logger into selected logger, but primarily to CloudWatch
class AuditLog
class << self
def logger
@logger ||= init_logger
end

def logger=(logger)
@logger = init_logger(logger)
end

def setup(logger = nil)
self.logger = logger
end

def with_account(account_number)
original = Thread.current[:audit_account_number]
Thread.current[:audit_account_number] = account_number
return unless block_given?

yield
Thread.current[:audit_account_number] = original
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess, another option would be to set this to nil. I'm fine with the current implementation, so long as logs don't show the incorrect account_number from a thread servicing multiple requests.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use hooks for the auth, which means that the account_number would be only setting. Ensuring that the account num is reset is one thing I'm working on. It would not be a 100%, but at least it should cover controllers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest setting the original account number in ensure block in case of exception

end

private

def init_logger(logger = nil)
logger ||= Logger.new($stdout)
logger.level = Logger::INFO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in topological/sources we are using ENV['LOG_LEVEL'] for ruby and ENV['RAILS_LOG_LEVEL'] for rails.
what about logger.level= ENV[...] || Logger::INFO

Copy link
Author

@vkrizan vkrizan Feb 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The level is hardcoded deliberately, as this is not going to be configurable. The idea is to use info/warn/error for audit logs. The end result might not even use and/or format the level. The difference would be only whether it is a failure/success.

logger.formatter = Formatter.new
logger
end
end
end
end
74 changes: 74 additions & 0 deletions lib/insights/api/common/audit_log/formatter.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# frozen_string_literal: true

require "manageiq/loggers"

module Insights::API::Common
class AuditLog
class Formatter < ManageIQ::Loggers::Container::Formatter
ALLOWED_PAYLOAD_KEYS = %i[message account_number controller remote_ip
transaction_id].freeze

def call(severity, time, progname, msg)
payload = {
:'@timestamp' => format_datetime(time),
:hostname => hostname,
:pid => $PROCESS_ID,
:thread_id => thread_id,
:service => progname,
:level => translate_error(severity),
:account_number => account_number
}
payload.merge!(framework_evidence)
JSON.generate(merge_message(payload, msg).compact) << "\n"
end

def framework_evidence
sidekiq_ctx = sidekiq_current_ctx
if sidekiq_ctx
{
:controller => sidekiq_ctx[:class],
:transaction_id => sidekiq_ctx[:jid]
}
else
{
:transaction_id => rails_transation_id
}
end
end

def merge_message(payload, msg)
if msg.kind_of?(Hash)
payload.merge!(msg.slice(*ALLOWED_PAYLOAD_KEYS))
else
payload[:message] = msg2str(msg)
end
payload
end

private

def format_datetime(time)
time.utc.strftime('%Y-%m-%dT%H:%M:%S.%6NZ')
end

def account_number
Thread.current[:audit_account_number]
end
Comment on lines +54 to +56
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could just be an attr_accessor on the Formatter and set by the AuditLog in with_account.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is only a one instance of Formatter. Setting it on that instance would not work correctly on multi-threaded env.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There will be one instance of Formatter per thread as written afaik. Thread.current represents only the local thread, so it also won't share information across threads in a multi-threaded env.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thread.current represents only the local thread, so it also won't share information across threads in a multi-threaded env.

^ That's the sole purpose of this.

I'd like to keep it defensive as is. I don't think that there is a separate instance for each thread as it is instantiated once with a logger. I don't want to rely on that assumption either.


def sidekiq_current_ctx
return unless Module.const_defined?(:Sidekiq)

if ::Sidekiq.const_defined?(:Context)
Sidekiq::Context.current
else
# versions up to 6.0.0
Thread.current[:sidekiq_context]
end
end

def rails_transation_id
ActiveSupport::Notifications.instrumenter.id
end
end
end
end
92 changes: 92 additions & 0 deletions lib/insights/api/common/audit_log/middleware.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,92 @@
module Insights::API::Common
class AuditLog
class Middleware
attr_reader :logger, :evidence, :request, :status

def initialize(app)
@app = app
@logger = AuditLog.logger
@subscribers = []
@evidence = {}
end

def call(env)
subscribe
@request = ActionDispatch::Request.new(env)
@app.call(env).tap do |status, _headers, _body|
@status = status
response_finished
end
Comment on lines +16 to +19
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! 💯

ensure
unsubscribe
end

private

def response_finished
payload = {
:controller => evidence[:controller],
:remote_ip => request.remote_ip,
:message => generate_message
}
log(payload)
end

def generate_message
status_label = Rack::Utils::HTTP_STATUS_CODES[status]
msg = "#{request.method} #{request.original_fullpath} -> #{status} #{status_label}"
if evidence[:unpermitted_parameters]
msg += "; unpermitted params #{fmt_params(evidence[:unpermitted_parameters])}"
end
if evidence[:halted_callback].present?
msg += "; filter chain halted by :#{evidence[:halted_callback]}"
end
msg
end
Comment on lines +35 to +45
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are the headers logged anywhere? It may be good to include, but it may also present a security risk to include the X_RH_IDENTITY header.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Inspecting X_RH_IDENTITY is a good idea, and can be done here. That's mainly the reason why this is a middleware (to access the request and headers).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The headers may also include X_RH_REQUEST_ID (or something similar), which can be important to tracing uploads, etc. across services.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! I guess we can incorporate that in the second iteration.


def log(payload)
if status < 400
logger.info(payload)
elsif status < 500
logger.warn(payload)
else
logger.error(payload)
end
end

def subscribe
@subscribers << subscribe_conroller
end

def subscribe_conroller
ActiveSupport::Notifications.subscribe(/\.action_controller$/) do |name, _started, _finished, _unique_id, payload|
# https://guides.rubyonrails.org/active_support_instrumentation.html#action-controller
case name.split('.')[0]
when 'process_action'
@evidence[:controller] = fmt_controller(payload)
when 'halted_callback'
@evidence[:halted_callback] = payload[:filter]
when 'unpermitted_parameters'
@evidence[:unpermitted_parameters] = payload[:keys]
end
end
end

def unsubscribe
@subscribers.each do |sub|
ActiveSupport::Notifications.unsubscribe(sub)
end
end

def fmt_controller(payload)
return if payload[:controller].blank?

[payload[:controller], payload[:action]].compact.join('#')
end

def fmt_params(params)
params.map { |e| ":#{e}" }.join(", ")
end
Comment on lines +87 to +89
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it work with rails parameter filtering for sensitive params? We should verify this/add a test for this case.

https://guides.rubyonrails.org/action_controller_overview.html#log-filtering

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only lists the params, not their value. The code snippet is strait from Rails logger.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! I missed that. Is it possible to log the param values? I think it'd be really useful to be able to debug errors/malfunctions with requests, but we need to ensure they're filtered based on the built in log filtering.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be possible, but the notification does not provide them. https://guides.rubyonrails.org/active_support_instrumentation.html#unpermitted-parameters-action-controller

Please keep in mind that this is a audit log with a purpose of retracing steps one has taken. Mostly access/denial/changes to a resource, etc. If details would be necessary an app log can be of help. Hence the transation_ids. 😉

end
end
end