sql/sem/tree, backupccl, *: sanitize URLs during Format

[michae2]

Prior to this change, all SQL statements containing URLs would be formatted with the full URL in cleartext, including any secrets such as keys or passwords. These secrets would sometimes show up in the slow query log or sql audit log. This PR adds new functions tree.(*FmtCtx).FormatURIand tree.(*FmtCtx).FormatURIs which are designed to sanitize URLs during formatting.

See individual commits for details.

Fixes: CRDB-39710, TREQ-284

Epic: None

Release note: None

[blathers-crl]

It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR?

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

[cockroach-teamcity]

This change is 

[blathers-crl]

Your pull request contains more than 1000 changes. It is strongly encouraged to split big PRs into smaller chunks.

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

[rytaft]

Very nice! A few questions / nits.

Reviewed 35 of 35 files at r14, 15 of 15 files at r15, 3 of 3 files at r16, 4 of 4 files at r17, 7 of 7 files at r18, 8 of 8 files at r19, 3 of 3 files at r20, 4 of 4 files at r21, 3 of 3 files at r22, 35 of 35 files at r23, 14 of 14 files at r24, 2 of 2 files at r25, 3 of 3 files at r26, 6 of 6 files at r27, 7 of 7 files at r28, 2 of 2 files at r29, 3 of 3 files at r30, 2 of 2 files at r31, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @dt, @michae2, @msbutler, and @xinhaoz)

---

pkg/sql/sem/tree/alter_changefeed.go line 94 at r18 (raw file):

func (node *AlterChangefeedSetOptions) Format(ctx *FmtCtx) {
	ctx.WriteString(" SET ")
	// TODO(michae2): sanitize SET sink

Does this change need to happen as part of this PR?

---

pkg/sql/sem/tree/format.go line 486 at r31 (raw file):

func (ctx *FmtCtx) FormatURIs(uris []Expr) {
	if len(uris) > 1 {
		ctx.WriteString("(")

I noticed that this caused import logic to change with parentheses. Is this going to cause issues anywhere else? Any reason we need this special logic with parentheses now? And is this well tested?

---

pkg/sql/parser/testdata/import_export line 4 at r31 (raw file):

IMPORT TABLE foo FROM PGDUMPCREATE 'nodelocal://0/foo/bar' WITH temp = 'path/to/temp'
----
IMPORT TABLE foo FROM PGDUMPCREATE '*****' WITH OPTIONS (temp = 'path/to/temp') -- normalized!

should we be concerned about 'path/to/temp'?

[dt]

temp = ...

temp hasn't been a valid option to IMPORT for 5+ years. The parser testdata is just showing that you can pass any pair of strings to these untyped string option = string value maps, and we don't do any validation of which keys you pass or what you pass for them until execution of the stmt (at which point you'd get ERROR: invalid option "temp". Fortunately we don't put many URIs in these unstructured option maps.

[michae2]

TFTRs, @dt and @rytaft! Any objection if I add a cluster setting to bring back the old behavior, default off?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @dt, @msbutler, @rytaft, and @xinhaoz)

---

pkg/sql/sem/tree/alter_changefeed.go line 94 at r18 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

Does this change need to happen as part of this PR?

Ah, good catch. Yes. Done.

---

pkg/sql/sem/tree/format.go line 486 at r31 (raw file):

Previously, rytaft (Rebecca Taft) wrote…

I noticed that this caused import logic to change with parentheses. Is this going to cause issues anywhere else? Any reason we need this special logic with parentheses now? And is this well tested?

Great question. Every field using FormatURIs except Import.Files comes from string_or_placeholder_opt_list in the parser, which makes the parentheses optional for a single value, so I made that the default behavior. This matches the behavior of StringOrPlaceholderOptList.Format.

Import.Files is a special case: it comes from string_or_placeholder_list surrounded by parentheses.

This is tested by the cases in pkg/sql/parser/testdata/import_export which shouldn't have changed w.r.t. parentheses.

(The other weird one is the INCREMENTAL FROM clause of BACKUP which comes from string_or_placeholder_list without any parentheses, so I also had to add a special case for that.)

[michae2]

Any objection if I add a cluster setting to bring back the old behavior, default off?

Actually, this is turning out to be very painful to plumb into all calls to AsString*. Forget I said this.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @andyyang890, @dt, @msbutler, @rytaft, and @xinhaoz)

[rytaft]

Reviewed 36 of 36 files at r37, 14 of 14 files at r38, 2 of 2 files at r39, 3 of 3 files at r40, 8 of 8 files at r41, 7 of 7 files at r42, 2 of 2 files at r43, 3 of 3 files at r44, 2 of 2 files at r45, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @andyyang890, @dt, @msbutler, and @xinhaoz)

---

pkg/sql/sem/tree/format.go line 486 at r31 (raw file):

Previously, michae2 (Michael Erickson) wrote…

Great question. Every field using FormatURIs except Import.Files comes from string_or_placeholder_opt_list in the parser, which makes the parentheses optional for a single value, so I made that the default behavior. This matches the behavior of StringOrPlaceholderOptList.Format.

Import.Files is a special case: it comes from string_or_placeholder_list surrounded by parentheses.

This is tested by the cases in pkg/sql/parser/testdata/import_export which shouldn't have changed w.r.t. parentheses.

(The other weird one is the INCREMENTAL FROM clause of BACKUP which comes from string_or_placeholder_list without any parentheses, so I also had to add a special case for that.)

Thanks for the explanation!

[michae2]

Thanks for the reviews!

bors r=dt,rytaft

[craig]

Build succeeded:

• check_generated_code
• linux_arm64_build
• macos_arm64_build
• docker_image_amd64
• linux_amd64_fips_build
• acceptance
• local_roachtest
• unit_tests
• lint
• linux_amd64_build
• local_roachtest_fips
• macos_amd64_build
• windows_build
• examples_orms

[blathers-crl]

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

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

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

---

error creating merge commit from 5bfbead to blathers/backport-release-23.1-126970: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

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

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

---

error creating merge commit from 5bfbead to blathers/backport-release-23.2-126970: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

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

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

---

error creating merge commit from 5bfbead to blathers/backport-release-24.1-126970: POST https://api.github.com/repos/cockroachdb/cockroach/merges: 409 Merge conflict []

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

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

---

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