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

Generate audit logs on login #382

Merged
merged 54 commits into from
Feb 6, 2024
Merged

Generate audit logs on login #382

merged 54 commits into from
Feb 6, 2024

Conversation

juggernot325
Copy link
Contributor

@juggernot325 juggernot325 commented Feb 2, 2024

Description

This PR updates the authenticator to generate audit logs on login attempts. It follows a similar system as database-level audit logs where an intent record is created when the login attempt starts and a second record is generated after completion to record the actual status. These records were added to the authenticator to ensure they would be logged anywhere a password is checked.

One item to note is that the username will be logged in the fields section for the intent record. This is because it is created prior to verification that the user is valid. If the user is found in the database by the principal name then it will be populated in the actor_ fields of the audit log.

Lastly, these logs are only created for login attempts using a password. Access via an API token will not create audit log entries and will need to be investigated in the application logs.

Motivation and Context

  • Create a reliable record of each user that accesses or attempts to access BloodHound

How Has This Been Tested?

  • Tested various successful and failure login cases

Screenshots (if appropriate):

Types of changes

  • Chore (a change that does not modify the application functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • Documentation updates are needed, and have been made accordingly.
  • I have added and/or updated tests to cover my changes.
  • All new and existing tests passed.
  • My changes include a database migration.

juggernot325 and others added 30 commits January 25, 2024 16:40
… 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
* updated IP address parsing

* added tests for IP parsing, changed IP delimiter to comma
* 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 <[email protected]>
* BED-3858 - Fix not ingesting tenant count during azure analysis (#328)

Co-authored-by: Irshad Ahmed <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>

* 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 <[email protected]>
Co-authored-by: Rohan Vazarkar <[email protected]>

* 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 <[email protected]>
Co-authored-by: Irshad Ahmed <[email protected]>
Co-authored-by: Rohan Vazarkar <[email protected]>
Co-authored-by: John Hopper <[email protected]>
Co-authored-by: Jonas Bülow Knudsen <[email protected]>
Co-authored-by: Ulises Rangel <[email protected]>
Co-authored-by: rvazarkar <[email protected]>
* 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
@juggernot325 juggernot325 marked this pull request as draft February 2, 2024 16:47
Base automatically changed from populate-audit-log-fields to main February 5, 2024 18:31
@juggernot325 juggernot325 marked this pull request as ready for review February 5, 2024 20:23
Copy link
Contributor

@superlinkx superlinkx left a comment

Choose a reason for hiding this comment

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

Just some unexpected shifts caused by the upstream branch getting last minute changes and then merging before we could pull them in (each of these were in my PR too). Once fixed, this looks good to merge

cmd/api/src/api/middleware/middleware.go Outdated Show resolved Hide resolved
cmd/api/src/api/middleware/middleware_internal_test.go Outdated Show resolved Hide resolved
cmd/api/src/api/router/router.go Outdated Show resolved Hide resolved
@juggernot325 juggernot325 merged commit c34819a into main Feb 6, 2024
3 checks passed
@juggernot325 juggernot325 deleted the audit-log-new-actions branch February 6, 2024 17:44
@github-actions github-actions bot locked and limited conversation to collaborators Feb 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants