Make "security events" part of the audit log#42653

vrto · 2022-10-06T15:35:51Z

Description

Security events will be included in the audit log by default.

Further details in RFC 734.

Implementation details

I made calls to audit.Log from the SecurityEventLogsStore. I wasn’t 100% sure if this is pure enough (e.g., if we needed to consider some kind of service layer delegating to the store), but we do regularly call sourcegraph/log from the stores, which is the basis for the audit log anyway.

⚠️ Major change

Security events will be enabled by default on all environments. We originally wanted them on .com only (because we expected huge volumes of data from the multi-tenant cloud). Since single-tenancy is the new default, having them everywhere is beneficial. This was discussed in RFC 734 and RFC 732

Enabling the audit log for security events

They’re enabled by default, but can be tweaked in the site config:

  “log”: {
    “auditLog”: {
      “securityEvents”: true,
      “graphQL”: true,
      “gitserverAccess”: false
    }
  }

Sample audit logs

{
  “SeverityText”: “INFO”,
  “Timestamp”: 1665061820174215000,
  “InstrumentationScope”: “server.SecurityEvents”,
  “Caller”: “audit/audit.go:33",
  “Function”: “github.com/sourcegraph/sourcegraph/internal/audit.Log”,
  “Body”: “PasswordChanged (sampling immunity token: 17edbf05-9992-46c7-be44-206b3aa2cc6a)“,
  “Resource”: {
    “service.name”: “frontend”,
    “service.version”: “0.0.0+dev”,
    “service.instance.id”: “Michals-MacBook-Pro-2.local”
  },
  “Attributes”: {
    “audit”: {
      “auditId”: “17edbf05-9992-46c7-be44-206b3aa2cc6a”,
      “entity”: “security events”,
      “actor”: {
        “actorUID”: “714b75b1-8abd-4379-9278-1a6ad50d12ce”,
        “ip”: “127.0.0.1",
        “X-Forwarded-For”: “127.0.0.1, 127.0.0.1"
      }
    },
    “event”: {
      “URL”: “/-/reset-password-code”,
      “source”: “BACKEND”,
      “argument”: “{\“requester\“:0}“,
      “version”: “0.0.0+dev”,
      “timestamp”: “2022-10-06 13:10:20.171337 +0000 UTC”
    }
  }
}

Test plan

Automated tests
Manual exploratory testing

sourcegraph-bot · 2022-10-06T15:37:24Z

Codenotify: Notifying subscribers in CODENOTIFY files for diff 6d8c129...d5f7b16.

Notify	File(s)
@eseliger	enterprise/internal/batches/store/changeset_specs_test.go
@indradhanush	internal/repos/syncer_test.go
@ryanslade	internal/repos/syncer_test.go
@sashaostrikov	internal/repos/syncer_test.go

vrto · 2022-10-06T15:42:00Z

enterprise/internal/batches/store/changeset_specs_test.go

 	t.Run("GetRewirerMappings", func(t *testing.T) {
 		// Create some test data
 		user := bt.CreateTestUser(t, s.DatabaseDB(), true)
+		ctx = actor.WithActor(ctx, &actor.Actor{UID: user.ID})


@eseliger @LawnGnome @adeola-ak I needed to get the actor into the context (as it would be in the prod) for a few of the integration tests here. Can you have a look to see if it makes sense to you from a batch changes perspective (tagging you as y'all authored most of the change code).

Context: security events will be enabled by default now, and in order to store a record in security_event_logs we need to comply with the security_event_logs_check_has_user constraint, so we can't have a missing actor.Actor in the context.Context.

What would happen if the actor is missing? I think this store is also used in background jobs

@eseliger Crash on the database INSERT, as the missing actor defaults to 0.

err.Message -> new row for relation "security_event_logs" violates check constraint "security_event_logs_check_has_user" err.Details -> Failing row contains (3, AccessGranted, , 0, , BACKEND, {"service": "___1TestIntegration_in_github_com_sourcegraph_sourc..., 0.0.0+dev, 2022-10-10 09:22:30.009738+00).

I think that's a problem when we use internal actors, isn't it? 🤔

That would be my hunch as well.

@eseliger @kopancek there's a condition that tackles the internal case. Do you think that instead of smashing the actual user into context (in the test code), it's better to setup an internal one?

for this test in particular, yes I think so :) Also sounds good that we handle the internal case!

@eseliger changed in https://github.com/sourcegraph/sourcegraph/pull/42653/commits/2a6ac249e11e3b5a4156a698ea89afedd857308e, all tests green.

nice, LGTM!

vrto · 2022-10-06T15:44:10Z