disable audit logging to db by default#51686

daxmc99 · 2023-05-10T00:50:08Z

This modifies our audit configuration. After this security_event_logs will be output to stdout in our audit log format by default.

Test plan

Go tests included in this PR

bobheadxi · 2023-05-16T00:56:09Z

schema/site.schema.json

+            "location": {
+              "description": "Where to output the audit log",
+              "type": "string",
+              "enum": ["none", "stdout", "database", "both"],
+              "default": "none"
            }


is the plan to make it possible to also render audit logs in the database? right now the implementation only touches the security event logs

IMO: we should never introduce an option for audit logs to go to database, that sounds disastrous. we should have a completely separate field for securityEventLogs that dictates the destination:

{ "auditLog": { ... } "securityEvents": { "location": "auditlogs" // enum: ["none", "auditlogs", "database", "all"] } }

security event logs !== audit logs, and we should be careful to not conflate the two 😅 the only overlap is that security event logs also can be rendered as audit logs

+1 to create another top level config for security events,

it's really confusing for me to see a mix of config for two totally different things

like can I use severityLevel to tune the verbose level for securityEvents? (you can't, but that's the point)

+1 from me. It's confusing to understand what I am configuring if we put both at the same level.

Changed to be another property at the top-level Log

internal/database/repos.go

kopancek · 2023-05-16T09:43:12Z

schema/site.schema.json

+            "location": {
+              "description": "Where to output the audit log",
+              "type": "string",
+              "enum": ["none", "stdout", "database", "both"],
+              "default": "none"
            }


+1 from me. It's confusing to understand what I am configuring if we put both at the same level.

sourcegraph-bot · 2023-05-16T23:36:56Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff b2e59d0...bee3f1f.

Notify	File(s)
@eseliger	internal/database/repos.go internal/database/repos_perm_test.go
@sourcegraph/delivery	doc/admin/audit_log.md

daxmc99 · 2023-05-16T23:40:32Z

internal/database/security_event_logs.go

 }

 func (s *securityEventLogsStore) InsertList(ctx context.Context, events []*SecurityEvent) error {
+	cfg := conf.SiteConfig()


TODO: consider refactoring this out of hot path

this is a fairly minimal concern I think, there is usually a cache in front of this. The alternative would be to have a conf.Watch that updates securityEventLogsStore but store instances are typically set up on the fly so that might not be feasible

internal/audit/security_events.go

schema/site.schema.json

kopancek

LGTM now 👍

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

bobheadxi

I think the changelog entry needs more rewording, no longer writing to DB by default is a significant change in behaviour

CHANGELOG.md

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

bobheadxi · 2023-05-23T17:13:23Z

CHANGELOG.md

 - Added the ability to block auto-indexing scheduling and inference via the `codeintel_autoindexing_exceptions` Postgres table. [#51578](https://github.com/sourcegraph/sourcegraph/pull/51578)
 - When an admin has configured rollout windows for Batch Changes changesets, the configuration details are now visible to all users on the Batch Changes settings page. [#50479](https://github.com/sourcegraph/sourcegraph/pull/50479)
 - Added support for regular expressions in`exclude` repositories for GitLab code host connections. [#51862](https://github.com/sourcegraph/sourcegraph/pull/51862)
+- Security event logs no longer write to database by default - instead, they will be written in the [audit log format](https://docs.sourcegraph.com/admin/audit_log) to console. There is a new site config setting `log.securityEventLogs` that can be used to configure security event logs to write to database if the old behaviour is desired. This new default will significantly improve performance for large instances. In addition, the old environment variable `SRC_DISABLE_LOG_PRIVATE_REPO_ACCESS` no longer does anything. [#51686](https://github.com/sourcegraph/sourcegraph/pull/51686)


nit - can this just replace the entry in Changed? i.e. just move this whole thing down

internal/database/access_tokens_test.go

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

filiphaftek

Awesome!

…52571) As of https://github.com/sourcegraph/sourcegraph/pull/51686 we will no longer write audit logs to DB by default. I noticed this background prune job is causing high load on a customer instance, and I think this should be disabled by default now as well. While here, I also removed the log15 usages from this file. ## Test plan n/a

Security event logs are no longer logged to the `security_event_log` table by default after this change. They are now configurable through the site_config --------- Co-authored-by: Robert Lin <robert@bobheadxi.dev>

…52571) As of https://github.com/sourcegraph/sourcegraph/pull/51686 we will no longer write audit logs to DB by default. I noticed this background prune job is causing high load on a customer instance, and I think this should be disabled by default now as well. While here, I also removed the log15 usages from this file. ## Test plan n/a

daxmc99 · 2023-06-30T06:57:34Z

CHANGELOG.md

 - Access tokens now begin with the prefix `sgp_` to make them identifiable as secrets. You can also prepend `sgp_` to previously generated access tokens, although they will continue to work as-is without that prefix.
 - The commit message defined in a batch spec will now be quoted when git is invoked, i.e. `git commit -m "commit message"`, to improve how the message is interpreted by the shell in certain edge cases, such as when the commit message begins with a dash. This may mean that previous escaping strategies will behave differently.
 - 429 errors from external services Sourcegraph talks to are only retried automatically if the Retry-After header doesn't indicate that a retry would be useless. The time grace period can be configured using `SRC_HTTP_CLI_EXTERNAL_RETRY_AFTER_MAX_DURATION` and `SRC_HTTP_CLI_INTERNAL_RETRY_AFTER_MAX_DURATION`. [#51743](https://github.com/sourcegraph/sourcegraph/pull/51743)
+- Security Events NO LONGER write to database by default - instead, they will be written in the [audit log format](https://docs.sourcegraph.com/admin/audit_log) to console. There is a new site config setting `log.securityEventLogs` that can be used to configure security event logs to write to database if the old behaviour is desired. This new default will significantly improve performance for large instances. In addition, the old environment variable `SRC_DISABLE_LOG_PRIVATE_REPO_ACCESS` no longer does anything. [#51686](https://github.com/sourcegraph/sourcegraph/pull/51686)


This should be log.securityEventLog not log.securityEventLogs

cla-bot bot added the cla-signed label May 10, 2023

daxmc99 force-pushed the dax/remove_audit_log branch 5 times, most recently from 6f87466 to 716d22f Compare May 16, 2023 00:51

bobheadxi reviewed May 16, 2023

View reviewed changes

kopancek reviewed May 16, 2023

View reviewed changes

daxmc99 force-pushed the dax/remove_audit_log branch from b1fc79f to 2c8457e Compare May 16, 2023 23:08

daxmc99 requested review from bobheadxi, kopancek and michaellzc May 16, 2023 23:08

daxmc99 force-pushed the dax/remove_audit_log branch from 2c8457e to 3069900 Compare May 16, 2023 23:11

daxmc99 marked this pull request as ready for review May 16, 2023 23:35

daxmc99 commented May 16, 2023

View reviewed changes

evict approved these changes May 17, 2023

View reviewed changes

bobheadxi reviewed May 17, 2023

View reviewed changes

internal/audit/security_events.go Outdated Show resolved Hide resolved

bobheadxi reviewed May 17, 2023

View reviewed changes

internal/audit/security_events.go Show resolved Hide resolved

michaellzc reviewed May 17, 2023

View reviewed changes

schema/site.schema.json Show resolved Hide resolved

kopancek approved these changes May 18, 2023

View reviewed changes

daxmc99 requested review from bobheadxi and michaellzc May 18, 2023 21:58

daxmc99 and others added 7 commits May 19, 2023 11:31

Refactor audit logs into security_events

41d679e

Refactor audit logs based on review comments

4e6638d

Remove deprecated field

600e73f

Fixup tests

6937e24

Make bazel happy

6b05fe2

Update internal/audit/security_events.go

d0ffb68

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

Update CHANGELOG

76cb674

bobheadxi suggested changes May 19, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

This comment was marked as duplicate.

Sign in to view

daxmc99 added 2 commits May 19, 2023 16:08

Fixup tests for concurrency

6130ee4

Add more notes to changelog

775e713

bobheadxi reviewed May 19, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

daxmc99 and others added 3 commits May 22, 2023 11:09

Update CHANGELOG.md

6460acf

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

Update CHANGELOG.md

b9aba54

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

Setup tests to allow for clean setup and teardown

1189af2

daxmc99 requested a review from bobheadxi May 22, 2023 21:33

Merge remote-tracking branch 'origin/main' into dax/remove_audit_log

83461bc

bobheadxi reviewed May 23, 2023

View reviewed changes

Condense CHANGELOG entry

ad2e36d

bobheadxi reviewed May 23, 2023

View reviewed changes

daxmc99 and others added 2 commits May 23, 2023 10:18

Update internal/database/access_tokens_test.go

a8d7a0e

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

Update internal/database/access_tokens_test.go

c8d5757

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

bobheadxi approved these changes May 23, 2023

View reviewed changes

daxmc99 and others added 5 commits May 23, 2023 10:19

Apply suggestions from code review

e1f54d1

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

Update internal/database/access_tokens_test.go

efa9fd3

Co-authored-by: Robert Lin <robert@bobheadxi.dev>

Format with prettier

b77d6cb

Remove unused env var

9f29ebd

fmt

bee3f1f

daxmc99 enabled auto-merge (squash) May 23, 2023 18:02

daxmc99 merged commit c0dee2b into main May 23, 2023

daxmc99 deleted the dax/remove_audit_log branch May 23, 2023 18:10

filiphaftek reviewed May 25, 2023

View reviewed changes

bobheadxi mentioned this pull request May 30, 2023

frontend/bg: disable deleteOldSecurityEventLogsInPostgres by default #52571

Merged

daxmc99 commented Jun 30, 2023

View reviewed changes

Conversation

daxmc99 commented May 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test plan

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaellzc May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sourcegraph-bot commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kopancek left a comment

Choose a reason for hiding this comment

Uh oh!

bobheadxi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment was marked as duplicate.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

filiphaftek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

daxmc99 commented May 10, 2023 •

edited

Loading

michaellzc May 16, 2023 •

edited

Loading

sourcegraph-bot commented May 16, 2023 •

edited

Loading