Collect Audit Logs From RP Frontend #1243
Conversation
pkg/frontend/middleware/audit.go
Outdated
| return func(h http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| // skip requests to the 'operationsstatus' endpoints | ||
| if strings.Contains(r.URL.Path, "operationsstatus") { |
There was a problem hiding this comment.
If someone names their cluster "operationsstatus"?
This check feels somewhat arbitrary - I wonder whether we would be better off dropping it, or maybe deciding not to audit log all GET (r/o) requests?
We will already be writing 2 loglines already for every operationsstatus call, so the fact that there is a 3rd audit log doesn't bother me massively in terms of log explosion.
There was a problem hiding this comment.
it is quite arbitrary, honestly. if we don't mind the noise, i don't mind removing this check. i think there are at least 6 of these operationstatus calls during az aro create.
as for removing all GET requests, will GET secret requests ever made it to the frontend, or are they blocked somewhere else? do we want to log those?
pkg/frontend/middleware/audit.go
Outdated
| if v := r.Context().Value(ContextKeyCorrelationData); v != nil { | ||
| if correlationData, ok := v.(*api.CorrelationData); ok { |
There was a problem hiding this comment.
The only case in which this code should fail to run is if the Audit middleware is not run after the Log middleware.
If that happens, this code will fail silently and audit logs will be affected.
Write correlationData := r.Context().Value(middleware.ContextKeyCorrelationData).(*api.CorrelationData) here, so that this code will panic if there is a refactor which breaks it.
We have already taken a similar decision elsewhere in the codebase, e.g. in _putOrPatchOpenShiftCluster().
There was a problem hiding this comment.
so that this code will panic
although i don't like the idea of letting my code panic because someone else decided to make some incompatible changes elsewhere, it makes sense in this case because we are making a deliberate design decision to couple the audit middleware to the log middleware, on the basis that they should log the same data. i will include this change.
fwiw, i believe the recover.Panic() in frontend.Run() recovers the panic, before exiting, so it's not as scary as it seems in prod.
|
Please rebase pull request. |
github-actions
bot
added
needs-rebase
|
Please rebase pull request. |
|
Please rebase pull request. |
1. Add a new middleware to audit inbound requests 2. Requests to the Azure 'operationsstatus' endpoints are skipped 3. Remove the embedded 'env' from the audit log hook to decouple the dependency. The 'env' is passed in to the Audit middleware 4. Replace unnecessary custom string types with basic string types 5. Update the testutil 'AssertLoggingOutput' method to skip asserting audit logs to reduce flakiness in tests. Audit logs assertion is done in a new 'audit.AssertAuditingOutput()' testutil method Signed-off-by: Ivan Sim <isim@redhat.com>
Signed-off-by: Ivan Sim <isim@redhat.com>
This filter rewrites the input tag of journald logs that have the field LOGKIND=ifxaudit, to ifxaudit. Using a different tag for ifxaudit logs allows us to separate them from non-audit logs in the mdsd configuration. Signed-off-by: Ivan Sim <isim@redhat.com>
Signed-off-by: Ivan Sim <isim@redhat.com>
1. Merge the 'audit' middleware with the 'log' middleware to avoid type assertions 3. Update security_test.go with audit test 4. Remove pointer reference to audit constructor 5. Add new audit log entry to testinfra struct Signed-off-by: Ivan Sim <isim@redhat.com>
1. Add unit test to test supported URL patterns Signed-off-by: Ivan Sim <isim@redhat.com>
|
Let's merge this and validate in INT the day after that non-audit logs are still flowing (i.e. the addition of an additional fluent bit tag hasn't broken us). Then let's set up a separate VSTS story to handle updating the Geneva configuration and bump |
1. Move adminOp 'if' conditional to log middleware 2. Extract out the 'if' conditional check into a helper function 3. Add start and end symbols to new regex expressions Signed-off-by: Ivan Sim <isim@redhat.com>
Which issue this PR addresses:
Part of 7311453.
What this PR does / why we need it:
There are 2 parts to this PR.
Go middleware
This PR introduces a new
auditmiddleware that intercepts and auditsall inbound HTTP requests to the RP
frontend. It follows thelogmiddleware pattern, and relies on
logto set some of the audit-relevantdata in the request context.
Requests to the
operationstatusesendpoints are skipped.Fluent Bit Configuration
The RP's Fluent Bit configuration has been updated with a
rewrite_taginput filter, where journals logs with the field
LOGKIND=ifauditareretagged with a
ifxaudittag. This rewrite enables us to separate ifxauditlogs from non-audit logs in Geneva Logs.
Test plan for issue:
Updated unit tests in
pkg/util/log/auditandpkg/frontend.For sample logs output generated by the
auditmiddleware, see commentsin the VSTS story.
For testing the Fluent Bit config changes, I ran the td-agent process locally
following the installation steps here with the following config:
Sample log outputs are uploaded to the VSTS story.
Is there any documentation that needs to be updated for this PR?
No.