Add auditing and system admin realm joining #705

sethvargo · 2020-09-29T01:45:00Z

Release Note

Add basic auditing and allow system admins to join realms for recovery purposes

google-oss-robot · 2020-09-29T01:45:04Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sethvargo

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sethvargo]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

sethvargo · 2020-09-29T01:45:06Z

/assign @mikehelmick @whaught

cmd/server/assets/users/index.html

pkg/controller/admin/realms.go

cmd/server/assets/realmadmin/events.html

cmd/server/assets/users/index.html

pkg/controller/realmadmin/events.go

sethvargo · 2020-09-30T02:50:05Z

pkg/database/realm.go

+				audits = append(audits, audit)
+			}
+
+			// TODO(sethvargo): diff allowed CIDRs


Gonna do this later.

sethvargo · 2020-09-30T02:50:12Z

@whaught @mikehelmick okay - this is ready for review now after a few iterations and virtual rubber-ducking.

High-level overview:

There's a new interface in the database package, Auditable, that determines if a resource can be audited. These resources can act as the "actor" (entity that did the action) or "target" (entity upon which the action was taken).
Audits are optionally tied to a realm, although some audits are not. There's currently no UI to display non-realm-specific audits, but they are captured and we can always add that later.
Audits are automatically cleaned up after 30d (default).
Audits intentionally don't use foreign keys so that they remain intact even if the resourcing record is deleted.
Audited resources require an auditable actor when calling their Save function. There's a database.System user for tests/when the actor is the system (e.g. seeding, model calculation).
Audited resources are currently manually diffed. We already have enough runtime reflection with gorm, and we want to have tight control over which fields are diffed and which order they are diffed. This means the Save functions are verbose, but I think it's worth the tradeoff.
Audit entries cannot be deleted (unless you do it directly from the database).
With this, system admins can now join and leave realms.

pkg/controller/login/select.go

pkg/controller/realmadmin/events.go

mikehelmick · 2020-09-30T04:14:52Z

pkg/database/audit_entry.go

+type AuditEntry struct {
+	Errorable
+
+	// ID is the entry's ID.


any reason why not the gorm.Model embed?

gorm.Model changes the behavior and semantics for delete. If the resource has DeletedAt, it soft deletes. Audit entries are also immutable and gorm.Model includes an unnecessary UpdatedAt field. Given I expect audit_entries to be our largest table (even with purging), I wanted to keep the number of columns as small as possible.

pkg/database/audit_entry.go

pkg/database/migrations.go

icco

I'd like two metrics, they can be saved for future work if you'd like.

icco · 2020-09-30T12:51:10Z

pkg/database/audit_entry.go

+		Unscoped().
+		Where("created_at < ?", createdBefore).
+		Delete(&AuditEntry{})
+	return result.RowsAffected, result.Error


Could we add a metric, audit_rows_cleaned or something here?

Totally, but it'll be a bit of work because the cleanup job isn't actually configured with metrics right now. I created #716 to add metrics to all the things we cleanup.

icco · 2020-09-30T12:52:04Z

pkg/database/auditable.go

+package database
+
+// Auditable represents a resource that can be audited as an actor or actee.
+type Auditable interface {


My knowledge of gorm is limited. Is there anyway to do a callback here so that we can increment a counter on every audit entry inserted? If there is, can we do it?

We have access to the tx in the callback, so we can increment a SQL counter (that's how we do the in-app stats actually).

We can create a global callback that has access to metrics and then basically guard it with a if table == "audit_entries". What do you hope to glean from counting the number of audits?

I would love a graph in our dashboards to see how many things are happening on a server from an admin point of view. Basically a stackdriver stat that says x number of actions lets us see the rate of actions happening on an instance. Good for debugging or tracking admin abuse.

icco · 2020-09-30T14:56:56Z

/lgtm

* Allow for v1.5+ early key release. Multiple keys can be provided that all have the same start interval * Add configuration params for this * Add tests for new pieces. 100% coverage on exposure model transform. * Add documentation. * add same day release as an optional feature to the generate server Fixes #705 Part of #663

google-oss-robot requested review from jeremyfaller and mikehelmick September 29, 2020 01:45

google-oss-robot added the approved label Sep 29, 2020

google-oss-robot assigned mikehelmick and whaught Sep 29, 2020

googlebot added the cla: yes Auto: added by CLA bot when all committers have signed a CLA. label Sep 29, 2020

google-oss-robot added the size/XL label Sep 29, 2020

sethvargo commented Sep 29, 2020

View reviewed changes

cmd/server/assets/users/index.html Show resolved Hide resolved

pkg/controller/admin/realms.go Outdated Show resolved Hide resolved

sethvargo added the do-not-merge/hold label Sep 29, 2020

whaught reviewed Sep 30, 2020

View reviewed changes

cmd/server/assets/realmadmin/events.html Outdated Show resolved Hide resolved

cmd/server/assets/users/index.html Show resolved Hide resolved

sethvargo force-pushed the sethvargo/audit branch from 2a72ca2 to 6adc5dc Compare September 30, 2020 02:38

google-oss-robot added size/XXL and removed size/XL labels Sep 30, 2020

sethvargo removed the do-not-merge/hold label Sep 30, 2020

sethvargo force-pushed the sethvargo/audit branch 2 times, most recently from ca974c1 to 2d70c7d Compare September 30, 2020 02:48

sethvargo commented Sep 30, 2020

View reviewed changes

Add auditing and expand system admin perms

4c9c025

sethvargo force-pushed the sethvargo/audit branch from 2d70c7d to 4c9c025 Compare September 30, 2020 02:50

mikehelmick suggested changes Sep 30, 2020

View reviewed changes

icco reviewed Sep 30, 2020

View reviewed changes

sethvargo mentioned this pull request Sep 30, 2020

Add metrics to cleanup jobs #716

Closed

Review feedback

8e7f326

sethvargo force-pushed the sethvargo/audit branch from c5161de to 8e7f326 Compare September 30, 2020 13:41

sethvargo mentioned this pull request Sep 30, 2020

Add metrics for cleanup and audits #717

Merged

google-oss-robot assigned icco Sep 30, 2020

google-oss-robot added the lgtm label Sep 30, 2020

google-oss-robot merged commit 66aaf0b into main Sep 30, 2020

google-oss-robot deleted the sethvargo/audit branch September 30, 2020 14:57

github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add auditing and system admin realm joining #705

Add auditing and system admin realm joining #705

sethvargo commented Sep 29, 2020

google-oss-robot commented Sep 29, 2020

sethvargo commented Sep 29, 2020

sethvargo Sep 30, 2020

sethvargo commented Sep 30, 2020

mikehelmick Sep 30, 2020

sethvargo Sep 30, 2020

icco left a comment

icco Sep 30, 2020

sethvargo Sep 30, 2020

icco Sep 30, 2020

icco Sep 30, 2020

sethvargo Sep 30, 2020

icco Sep 30, 2020

icco commented Sep 30, 2020

Add auditing and system admin realm joining #705

Add auditing and system admin realm joining #705

Conversation

sethvargo commented Sep 29, 2020

google-oss-robot commented Sep 29, 2020

sethvargo commented Sep 29, 2020

Choose a reason for hiding this comment

sethvargo commented Sep 30, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

icco commented Sep 30, 2020