Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

release-20.2: pgwire: enhancements to enable a security log #58380

Merged
merged 4 commits into from
Jan 8, 2021

Conversation

knz
Copy link
Contributor

@knz knz commented Dec 30, 2020

Backport 1/1 commits from #58379.
Backport 1/1 commits from #58491.
Backport 2/2 commits from #58381.

/cc @cockroachdb/release


Requested by @logston. See the individual commits.

@knz knz requested a review from andy-kimball December 30, 2020 18:06
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz requested a review from otan January 6, 2021 10:38
@knz knz changed the title release-20.2: pgwire: new env var COCKROACH_ALWAYS_LOG_AUTHN_EVENTS release-20.2: pgwire: enhancements to enable a security log Jan 6, 2021
@knz knz requested a review from bdarnell January 6, 2021 16:40
@knz knz force-pushed the backport20.2-58379 branch from 2b24415 to c630e32 Compare January 6, 2021 16:49
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

changes look reasonable provided #58491 is approved
(and tests pass)

@knz knz force-pushed the backport20.2-58379 branch from c630e32 to 77ff1e6 Compare January 6, 2021 19:37
knz added 2 commits January 8, 2021 15:41
This commit adds support for a new env var
`COCKROACH_ALWAYS_LOG_AUTHN_EVENTS`. When set to a true-like value, it
overrides the `server.auth_log.sql_sessions.enabled` cluster setting
and unconditionally enables logging of authentication events.

This is a temporary stop-gap so that it becomes possible to log
authentication events in SQL pods for multi-tenant deployments. This
stop-gap can be removed when multi-tenant CockroachDB learns to
support cluster settings.

Release note: None
The "crdb-v1" format is brittle and certainly not designed to be
unambiguous and reliably parseable. Nevertheless, we can do a little
bit more to allow IPv6 addresses in logging tags, to enable parsing
the subset of messages that's needed for an authentication log.

Release note: None
@knz knz force-pushed the backport20.2-58379 branch from 77ff1e6 to 6dc7fe8 Compare January 8, 2021 15:48
knz added 2 commits January 8, 2021 17:48
Prior to this patch, when `FetchEntriesFromFiles` was fetching entries
from multiple file groups (previously known as "secondary loggers"),
it mistakenly stopped after processing entries from just one logger.

This patch fixes it.

Note: this API is deprecated and should be replaced by something that
knows about file groups.

Release note: None
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.
@knz knz force-pushed the backport20.2-58379 branch from 6dc7fe8 to c49afe4 Compare January 8, 2021 16:48
@knz
Copy link
Contributor Author

knz commented Jan 8, 2021

TFYR!

@knz knz merged commit bd0ed16 into cockroachdb:release-20.2 Jan 8, 2021
@knz knz deleted the backport20.2-58379 branch January 8, 2021 18:44
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.

3 participants