telemetry,sql: remove redaction from operational sql data #76676

dhartunian · 2022-02-16T14:46:52Z

Previously, when redaction was introduced into CRDB, all unidentifiable
strings were marked as redacted since that was the safer approach. We
expected to later return with a closer look and differentiate more
carefully between what should be redacted and what shouldn't.

SQL names have been identified as operational-sensitive data that should
not be redacted since it provides very useful debugging information and,
while user-controlled, do not typically contain user-data since those
would be stored in a Datum. This commit marks names as safe from
redaction for telemetry logging in cases where the
sql.telemetry.query_sampling.enabled cluster setting is enabled.

Additionally, some log tags such as client IP addresses are not to be
considered sensitive and are critical to debugging operational issues.
They have also been marked as safe.

In order to help with documenting these cases, a helper
SafeOperational() has been added to the log package. This helps us
mark strings as safe while documenting why we're doing so.

Resolves #76595

Release note (security update, ops change): When the
sql.telemetry.query_sampling.enabled cluster setting is enabled, SQL
names and client IPs are no longer redacted in telemetry logs.

cockroach-teamcity · 2022-02-16T14:47:01Z

This change is

abarganier

The introduction of SafeOperational() - just a couple nits.

I think a couple lingering tests are failing because they expect redaction markers to surround things like client addresses (e.g. TestClientAddrOverride/check-server-log-uses-override).

Changes related to logOperationalEventsOnlyExternally() I'd like to get some more experienced eyes on 🙂

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian and @knz)

pkg/sql/conn_executor_exec.go, line 1221 at r1 (raw file):

	}
	if shouldLog {
		commonSQLEventDetails := ex.planner.getCommonSQLEventDetails(redactionOptions{})

nit: I'm not sure how common it is to get event details like this without redaction options, but would it make sense to maintain a getCommonSQLEventDetails() implementation with no args that delegates to getCommonSQLEventDetails(redactionOptions{})?

Code quote:

ex.planner.getCommonSQLEventDetails(redactionOptions{})

pkg/sql/event_log.go, line 183 at r1 (raw file):

		return tree.FmtOmitNameRedaction
	}
	return 0

nit: should we explicitly return the FmtSimple constant here instead of 0?

Code quote:

return 0

pkg/sql/schema_change_plan_node.go, line 44 at r1 (raw file):

	return formatStmtKeyAsRedactableString(p.getVirtualTabler(),
		statement,
		annotations, tree.FmtFlags(0))

nit: ditto here re: using the FmtSimple constant.

Code quote:

tree.FmtFlags(0)

knz

The overall architecture looks fine to me. Thanks for tackling this.
I'll let your team finalize the review.

Reviewed 12 of 12 files at r1, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian)

Previously, when redaction was introduced into CRDB, all unidentifiable strings were marked as redacted since that was the safer approach. We expected to later return with a closer look and differentiate more carefully between what should be redacted and what shouldn't. SQL names have been identified as operational-sensitive data that should not be redacted since it provides very useful debugging information and, while user-controlled, do not typically contain user-data since those would be stored in a Datum. This commit marks names as safe from redaction for telemetry logging in cases where the `sql.telemetry.query_sampling.enabled` cluster setting is enabled. Additionally, some log tags such as client IP addresses are not to be considered sensitive and are critical to debugging operational issues. They have also been marked as safe. In order to help with documenting these cases, a helper `SafeOperational()` has been added to the `log` package. This helps us mark strings as safe while documenting *why* we're doing so. Resolves cockroachdb#76595 Release note (security update, ops change): When the `sql.telemetry.query_sampling.enabled` cluster setting is enabled, SQL names and client IPs are no longer redacted in telemetry logs.

dhartunian

@abarganier the implementation of logOperationalEventsOnlyExternally() is copied from the method above logEventsOnlyExternally and only differs in the addition of redaction options. Might be worth both of them calling a single helper 🤔

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @abarganier and @knz)

pkg/sql/conn_executor_exec.go, line 1221 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: I'm not sure how common it is to get event details like this without redaction options, but would it make sense to maintain a getCommonSQLEventDetails() implementation with no args that delegates to getCommonSQLEventDetails(redactionOptions{})?

go doesn't allow method overloading with different args so I'd be stuck having to come up with a different name.

What do you think of creating a var: defaultRedactionOptions that can be used?

I think given the low number of usages (2 that I can count) it's okay to require the argument.

pkg/sql/event_log.go, line 183 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: should we explicitly return the FmtSimple constant here instead of 0?

Done.

pkg/sql/schema_change_plan_node.go, line 44 at r1 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: ditto here re: using the FmtSimple constant.

Done. I didn't like the raw 0 in there either, thx for the solution.

abarganier

I don't feel strongly about that bit, so I'll defer to you on that one. Everything else !

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @abarganier, @dhartunian, and @knz)

pkg/sql/conn_executor_exec.go, line 1221 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

go doesn't allow method overloading with different args so I'd be stuck having to come up with a different name.

What do you think of creating a var: defaultRedactionOptions that can be used?

I think given the low number of usages (2 that I can count) it's okay to require the argument.

Ah! My java roots are showing 🙈

If the number of usages is low, then yeah I think it's fine to keep 🙂

pkg/sql/schema_change_plan_node.go, line 44 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

Done. I didn't like the raw 0 in there either, thx for the solution.

No prob!

dhartunian · 2022-02-25T16:00:13Z

TFTR

bors r=abarganier

craig · 2022-02-25T17:31:34Z

Build failed (retrying...):

GitHub CI (Cockroach)

blathers-crl · 2022-02-25T22:02:50Z

Encountered an error creating backports. Some common things that can go wrong:

The backport branch might have already existed.
There was a merge conflict.
The backport branch contained merge commits.

You might need to create your backport manually using the backport tool.

error creating merge commit from 10add46 to blathers/backport-release-21.2-76676: POST https://api.github.com/repos/cockroachlabs/cockroach/merges: 403 Resource not accessible by integration []

you may need to manually resolve merge conflicts with the backport tool.

Backport to branch 21.2.x failed. See errors above.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is otan.}

craig · 2022-02-25T22:02:53Z

Build succeeded:

GitHub CI (Cockroach)

dhartunian requested review from knz and a team February 16, 2022 14:46

dhartunian requested a review from a team as a code owner February 16, 2022 14:46

dhartunian requested a review from a team February 16, 2022 14:46

abarganier reviewed Feb 16, 2022

View reviewed changes

knz reviewed Feb 17, 2022

View reviewed changes

dhartunian force-pushed the telemetry-expose-operational-sensitive-data branch from 284c497 to 10add46 Compare February 18, 2022 00:40

dhartunian commented Feb 18, 2022

View reviewed changes

dhartunian requested a review from abarganier February 22, 2022 20:34

dhartunian added the backport-21.2.x label Feb 22, 2022

abarganier approved these changes Feb 22, 2022

View reviewed changes

craig bot merged commit 2826900 into cockroachdb:master Feb 25, 2022

cockroach-teamcity mentioned this pull request Feb 25, 2022

telemetry,sql: remove redaction from operational sql data cockroachdb/docs#13119

Open

dhartunian mentioned this pull request Feb 26, 2022

release-21.2: telemetry,sql: remove redaction from operational sql data #77072

Merged

pransudash mentioned this pull request Mar 23, 2022

telemetry: do not redact error text names in telemetry logs #78353

Closed

tbg mentioned this pull request Mar 24, 2022

perf: kv0/kv95 regression around Feb 25 [loosely coupled log truncation] #78412

Open

24 tasks

abarganier mentioned this pull request Aug 29, 2022

pkg/util/log: configuration for log redaction "sensitivity levels" #87038

Open

dhartunian deleted the telemetry-expose-operational-sensitive-data branch November 14, 2022 16:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

telemetry,sql: remove redaction from operational sql data #76676

telemetry,sql: remove redaction from operational sql data #76676

dhartunian commented Feb 16, 2022

cockroach-teamcity commented Feb 16, 2022

abarganier left a comment

knz left a comment

dhartunian left a comment

abarganier left a comment

dhartunian commented Feb 25, 2022

craig bot commented Feb 25, 2022

blathers-crl bot commented Feb 25, 2022

craig bot commented Feb 25, 2022

telemetry,sql: remove redaction from operational sql data #76676

telemetry,sql: remove redaction from operational sql data #76676

Conversation

dhartunian commented Feb 16, 2022

cockroach-teamcity commented Feb 16, 2022

abarganier left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

dhartunian left a comment

Choose a reason for hiding this comment

abarganier left a comment

Choose a reason for hiding this comment

dhartunian commented Feb 25, 2022

craig bot commented Feb 25, 2022

blathers-crl bot commented Feb 25, 2022

craig bot commented Feb 25, 2022