Skip to content

sql: slight refactor to audit logging code#86791

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
RichardJCai:audit_logging_slight_refactor
Aug 25, 2022
Merged

sql: slight refactor to audit logging code#86791
craig[bot] merged 1 commit intocockroachdb:masterfrom
RichardJCai:audit_logging_slight_refactor

Conversation

@RichardJCai
Copy link
Copy Markdown
Contributor

Release justification: Small refactor, no functionality change

Release note: None

Fixes #84760

@RichardJCai RichardJCai requested review from a team and rafiss August 24, 2022 18:07
@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

@RichardJCai RichardJCai requested a review from a team August 24, 2022 20:05
Release justification: Small refactor, no functionality change

Release note: None
@RichardJCai RichardJCai force-pushed the audit_logging_slight_refactor branch from cbdaccb to badf22d Compare August 24, 2022 20:22
Copy link
Copy Markdown
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

refactor lgtm, but i'm a bit surprised that it was only logging INSERT, DELETE , and UPDATE on tables. do you know why not the other privs?

@RichardJCai
Copy link
Copy Markdown
Contributor Author

refactor lgtm, but i'm a bit surprised that it was only logging INSERT, DELETE , and UPDATE on tables. do you know why not the other privs?

It does log it, from what I understand, we either log "READWRITE" for those 3 and "READ" for everything else. I suppose it could have been more granular but I'm not familiar with the historical context.

			mode := "r"
			if ev.writing {
				mode = "rw"
			}
			tableName := ""
			// We only have a valid *table* name if the object being
			// audited is table-like (includes view, sequence etc). For
			// now, this is sufficient because the auditing feature can
			// only audit tables. If/when the mechanisms are extended to
			// audit databases and schema, we need more logic here to
			// extract a name to include in the logging events.
			tn, err := p.getQualifiedTableName(ctx, ev.desc)
			if err != nil {
				log.Warningf(ctx, "name for audited table ID %d not found: %v", ev.desc.GetID(), err)
			} else {
				tableName = tn.FQString()
			}
			entries[i] = &eventpb.SensitiveTableAccess{
				CommonSQLEventDetails: eventpb.CommonSQLEventDetails{
					DescriptorID: uint32(ev.desc.GetID()),
				},
				CommonSQLExecDetails: execDetails,
				TableName:            tableName,
				AccessMode:           mode,
			}

@RichardJCai
Copy link
Copy Markdown
Contributor Author

Thanks!

bors r=rafiss

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Aug 25, 2022

Build succeeded:

@craig craig bot merged commit 2787ff1 into cockroachdb:master Aug 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

sql: audit logging for synthetic privileges

3 participants