-
Notifications
You must be signed in to change notification settings - Fork 109
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 Refactor #345
Merged
Merged
Audit Log Refactor #345
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…of a generic response
… on the presence of specific context This commit mostly contains the logic to make this method for audit logging work. The only audit log event that works in this commit is CreateUser
irshadaj
force-pushed
the
populate-audit-log-fields
branch
from
January 25, 2024 23:18
9b8a441
to
9cb8a3d
Compare
* chore: clean up old approach to reduce noise * chore: cleanup audit logging interface for new usages * chore: fix existing tests (audit integration test needs new target) * chore: improve integration config defaults * feat: add audit logging to UpdateUser This adds a new `AuditableContext` method to the Database, which wraps the Gorm Transaction method with some auditing context and ensures the two phase commit of the audit log is run * feat: add audit logging to DeleteSAMLProvider fix: use actual AuditData instead of the direct model * feat: CreateAssetGroup and DeleteAssetGroup are now back to auditable fix: broken tests * feat: plumb commitID for real this time fix: incorrect formatting of audit log action names fix: additional test fixes * fix: additional test fixes * review comments --------- Co-authored-by: Irshad Ahmed <iahmed@specterops.io>
* BED-3858 - Fix not ingesting tenant count during azure analysis (#328) Co-authored-by: Irshad Ahmed <iahmed@specterops.io> * ESC9a Edge Composition (#354) * feat: esc9a post * test: add esc9 test * chore: add harness files * fix: regen schema after merge * chore: fix small nits * chore: cleanup cert template new function * chore: add missing props * wip: 9a composition * fix: treat failure to grab properties as true * wip: esc9a composition * wip: esc9a composition * feat+chore: add depth controls to dawgs patterns * wip: esc9a composition * fix: do not drop the current segment if the next pattern is optional * wip: esc9a composition * fix: update other continuations to respect depth correctly * wip: edge comp * fix: swap * chore: remove unnecessary logs * feat: esc9a post * test: add esc9 test * chore: fix small nits * wip: 9a composition * wip: esc9a composition * wip: esc9a composition * feat+chore: add depth controls to dawgs patterns * wip: esc9a composition * fix: do not drop the current segment if the next pattern is optional * wip: esc9a composition * fix: update other continuations to respect depth correctly * wip: edge comp * fix: swap * chore: remove unnecessary logs * test: add test covering esc9a edge comp * chore: revert random re-ordering * chore: handle negative min/max depth on continuations --------- Co-authored-by: John Hopper <jhopper@specterops.io> * docs: Add to ESC3 abuse info (#350) * docs: add note in ESC6 abuse info (#356) * feat: Add ADCS pre-built queries (#342) Co-authored-by: Rohan Vazarkar <rvazarkar@users.noreply.github.com> * feat: esc6a edge composition (#359) * feat: esc6a edge composition * chore: allow composition accordion to show for 6a * fix: add trustedby rel to path4 pattern, use outboundwithdepth for optional memberof traversal * chore: update dcfor pattern to use outboundwithdepth for optional group membership * ESC10a Post Processing (#360) * wip: initial ESC10a post * test: all the tests for esc10a * chore: add edges to post processed * chore: add harnessgen script * test: remove edges from harness * chore: don't exit loop if we hit an error, continue instead * chore: log and continue * feat: filter out ESC3 false positives (#351) * feat: filter out ESC3 false positives * fix: handle esc3 filtering without retraversal * fix: handle esc3 filtering without retraversal * fix: handle esc3 filtering without retraversal * chore: rename function for re-use * chore: log and continue --------- Co-authored-by: rvazarkar <rvazarkar@specterops.io> Co-authored-by: Rohan Vazarkar <rvazarkar@users.noreply.github.com> * chore: patch EULAAcceptance bypass to only run if the current user is set to false * fix: incorrect usage of RemoteAddr fix: unnecessary AuditData() calls fix: use pointers for AuditEntry.Model assignments so successful actions can record updated fields like ID --------- Co-authored-by: mistahj67 <26472282+mistahj67@users.noreply.github.com> Co-authored-by: Irshad Ahmed <iahmed@specterops.io> Co-authored-by: Rohan Vazarkar <rvazarkar@users.noreply.github.com> Co-authored-by: John Hopper <jhopper@specterops.io> Co-authored-by: Jonas Bülow Knudsen <12843299+JonasBK@users.noreply.github.com> Co-authored-by: Ulises Rangel <urangel@specterops.io> Co-authored-by: rvazarkar <rvazarkar@specterops.io>
* audit logs for auth tokens and secrets * fix bad copy
…ector, CreateSAMLIdentityProvider, and UpdateSAMLIdentityProvider have audit log support (#374) chore: remove unused RemoveAssetGroupSelector method chore: update tests and mocks to account for interface changes
* more auth handlers for audit log * minor optimization
* feat: create an audit log record when unauthorized access is attempted * chore: Only log unauthorized write access * chore: add audit logging to remaining auth middleware * test: Fix middleware/auth_test to support new audit log changes
It was previously sized to only account for max 1 IPv6 address, but our design changes mean it could now be any length
mistahj67
reviewed
Feb 1, 2024
This reverts commit 2a2c839.
* Refactored unauthorized access audit logging: - moved audit logging responsibility to Authorizer interface - cleaned up injections of *db through several layers of code (no longer necessary) - minor cleanup and support work
zinic
requested changes
Feb 2, 2024
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.
Some changes requested. Rest of the changeset looks good.
zinic
approved these changes
Feb 5, 2024
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.
Changes look good from my end.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR refactors the generation of audit log records to use a combination of new middleware logic and context tracking to store the data needed for the log. The main goal of this change was to easily be able to track failure scenarios on endpoints that generate audit logs as we previously only tracked successful auditable actions. It also let's us capture the error messages to provide information as to why the failure occurred.
With this refactor, audit log creation will now mostly occur at the database layer. A new wrapper function called
AuditableTransaction
was introduced that can be used to ensure audit log entries are created around sensitive changes to the DB. This changes audit log functionality a bit.AuditableTransaction
first kicks off an initial audit log entry will be created with astatus
ofintent
. This indicates that a user has attempted to make a sensitive change to the data that should be audited. The audit log entry will be given acommit_id
UUID that will help associate it with additional audit logs that may be created for the same transaction.status
of this entry will be eithersuccess
orfailure
depending on if the transaction was successful. If an error occurs, that will be captured in thefields
section of the audit log entry.Motivation and Context
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: