pgwire: make it possible for a SQL proxy to override the client addr

[knz]

Requested by @logston and @andy-kimball

Intended for backport to v20.2.

Fixes #58346

Release note (security update): When using a SQL proxy, in the default
configuration CockroachDB only knows about the network address of the
proxy. That peer address is then used for logging, authentication
rules, etc. This is undesirable, as security logging and authentication
rules need to operate on the actual (final) client address instead.

CockroachDB can now be configured to solve this problem (conf
mechanism detailed below).

When so configured, a SQL proxy can inform the CockroachDB server of
the real address of the client via a server status parameter called
crdb:remote_addr. The value must be the IP address of the client,
followed by a colon, followed by the port number, using the standard
Go syntax (e.g. 11.22.33.44:5566 for IPv4, [11:22::33]:4455 for
IPv6). When provided, this value overrides the SQL proxy's address
for logging and authentication purposes.

In any case, the original peer address is also logged alongside
the client address (overridden or not), via the new logging tag peer.

Security considerations:

• enabling this feature allows the peer to spoof its address wrt
  authentication and thus bypass authentication rules that would
  otherwise apply to its address, which can introduce a serious security
  vulnerability if the peer is not trusted. This is why this feature is
  not enabled by default, and must only be enabled when using a trusted
  SQL proxy.

• this feature should only be used with SQL proxies which actively
  scrub a crdb:remote_addr parameter received by a remote client,
  and replaces it by its own. If the proxy mistakenly forwards
  the parameter as provided by the client, it opens the door
  to the aforementioned security vulnerability.

• care must be taken in HBA rules: TLS client cert validation, if
  requested by a rule, is still performed using the certificate
  presented by the proxy, not that presented by the client.
  This means that this new feature is not sufficient to forward
  TLS client cert authn through a proxy. (If TLS client cert authn
  is required, it must be performed by the proxy directly.)

• care must be taken in HBA rules: the 'protocol' field (first column)
  continues to apply to the connection type between CockroachDB and the
  proxy, not between the proxy and the client. Only the 4th column
  (the CIDR pattern) is matched against the proxy-provided remote
  address override.

  Therefore, it is not possible to apply different rules to different
  client address when proxying TCP connections via a unix socket,
  because HBA rules for unix connections don't use the address column.

  Also when proxying client SSL connections via a non-SSL proxy
  connection, or proxying client non-SSL connections via a SSL proxy
  connection, care must be taken to configure address-based rule
  matching using the proper connection type. A reliable way
  to bypass this complexity is to only use the host connection
  type which applies equally to SSL and non-SSL connections.

As of this implementation, the feature is enabled using the
non-documented environment variable
COCKROACH_TRUST_CLIENT_PROVIDED_SQL_REMOTE_ADDR. The use of an env
var is a stop-gap so that this feature can be used in CC SQL pods
which do not have access to cluster settings. The env var will be
eventually removed and replaced by another mechanism.

[cockroach-teamcity]

This change is 

[andreimatei]

Check out the PROXY protocol to see if it's suitable
https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @bdarnell)

[knz]

Here's an explanatory blog post on it, with a list of other products than haproxy which supports it as well: https://www.haproxy.com/blog/haproxy/proxy-protocol/

@andy-kimball can you have a look at this? I did not know about this somewhat-standard PROXY protocol before Andrei mentioned it earlier last week. I know that we discussed a SQL-level status parameter, but this PROXY protocol is strictly better: it would even allow us (if we wanted) to forward the TLS client cert authentication cleanly. I recall you and @bdarnell were interested in that.

[andy-kimball]

I'm not against a proxy protocol in general, but I'm against doing this right now. We need to minimize changes to CRDB.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @bdarnell)

[bdarnell]

I agree with Andy. The proxy protocol is nice, but it's a pretty significant amount of work and I'd prefer the simpler approach for now.

Reviewed 7 of 7 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @knz)

---

pkg/server/server.go, line 1966 at r1 (raw file):

	stopper.RunWorker(pgCtx, func(pgCtx context.Context) {
		netutil.FatalIfUnexpected(connManager.ServeWith(pgCtx, stopper, pgL, func(conn net.Conn) {
			connCtx := logtags.AddTag(pgCtx, "peer", conn.RemoteAddr().String())

Is there established nomenclature for this pattern in logging? "Peer" doesn't sound quite right to me but I don't have something better in mind.

Thank you for logging both the network-level IP and the passed-in IP. That'll give us some additional information in the event this functionality ever gets abused. But it's unfortunate to redundantly log the IP as both client and peer for the overwhelmingly common case that this is not enabled. I think it would be better to only log this once (as "client") unless this feature is enabled.

---

pkg/sql/pgwire/auth_test.go, line 493 at r1 (raw file):

	}

	const specialAddr = "11.22.33.44"

We should have a test with an ipv6 address too to make sure the parsing works correctly in that case.

---

pkg/sql/pgwire/server.go, line 738 at r1 (raw file):

// client could use this field to pretend being from another address
// than its own and defeat the HBA rules.
var trustClientProvidedRemoteAddr = envutil.EnvOrDefaultBool("COCKROACH_TRUST_CLIENT_PROVIDED_SQL_REMOTE_ADDR", false)

We shouldn't use environment variables just to hide settings. Better to make this a cluster setting (with whatever hidden/non-public flags we have).

I think it's good to have something more specific than a boolean, i.e. to accept the provided address only from certain peer IPs, or with certain certificates. I'm not sure it's worth actually implementing that at this time but something to think about in the interface design. (could it be a special hba.conf directive?)

[aaron-crl]

Nothing further to add to Ben and Andy's feedback and very much in favor of a more robust protocol at a later date.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @aaron-crl and @knz)

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell)

---

pkg/server/server.go, line 1966 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Is there established nomenclature for this pattern in logging? "Peer" doesn't sound quite right to me but I don't have something better in mind.

Thank you for logging both the network-level IP and the passed-in IP. That'll give us some additional information in the event this functionality ever gets abused. But it's unfortunate to redundantly log the IP as both client and peer for the overwhelmingly common case that this is not enabled. I think it would be better to only log this once (as "client") unless this feature is enabled.

Good idea. Done.

---

pkg/sql/pgwire/auth_test.go, line 493 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

We should have a test with an ipv6 address too to make sure the parsing works correctly in that case.

Good idea. Done.

---

pkg/sql/pgwire/server.go, line 738 at r1 (raw file):
The env var is not to hide the setting, but it's because the thing must be backportable to v20.2 and v20.2 does not support cluster settings in SQL pods.
I copy-pasted the following comment from the other PR: #58379

// Usage of an env var here makes it possible to unconditionally
// enable this feature when cluster settings do not work reliably,
// e.g. in multi-tenant setups in v20.2. This override mechanism can
// be removed after all of CC is moved to use v21.1 or a version which
// supports cluster settings.

I think it's good to have something more specific than a boolean [...]
Added as a TODO in the comment. We can consider that for a cluster setting, or as you suggest, a HBA config directive.

[bdarnell]

Reviewed 2 of 9 files at r2, 7 of 7 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell and @knz)

---

pkg/sql/pgwire/server.go, line 738 at r1 (raw file):

Previously, knz (kena) wrote…

The env var is not to hide the setting, but it's because the thing must be backportable to v20.2 and v20.2 does not support cluster settings in SQL pods.
I copy-pasted the following comment from the other PR: #58379

// Usage of an env var here makes it possible to unconditionally
// enable this feature when cluster settings do not work reliably,
// e.g. in multi-tenant setups in v20.2. This override mechanism can
// be removed after all of CC is moved to use v21.1 or a version which
// supports cluster settings.

I think it's good to have something more specific than a boolean [...]
Added as a TODO in the comment. We can consider that for a cluster setting, or as you suggest, a HBA config directive.

Ack. LGTM.

[knz]

TFYRs!

bors r=bdarnell,aaron-crl

[knz]

there's a merge skew here. will wait and rebase

bors r-

[craig]

Canceled.

[knz]

bors r=bdarnell,aaron-crl

[craig]

Build failed:

• GitHub CI (Cockroach)

[knz]

bors r=bdarnell,aaron-crl

[craig]

Build failed:

• GitHub CI (Cockroach)

[knz]

Hm unclear what's going on. Wil look tomorrow.

[knz]

Fixed.

bors r=bdarnell,aaron-crl

[knz]

I made one more mistake here ...
bors r-

[craig]

Canceled.

[knz]

bors r=bdarnell,aaron-crl

[knz]

oh there is a bug after all

bors r-

[craig]

Canceled.

[knz]

bors r=bdarnell,aaron-crl

[craig]

Build succeeded:

• GitHub CI (Cockroach)