pgwire: clarify the special cases as pre-defined HBA rules#43726
pgwire: clarify the special cases as pre-defined HBA rules#43726craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
ddbbf85 to
1da6f97
Compare
0e6c797 to
ef1612d
Compare
4099668 to
8ca2bb0
Compare
pkg/sql/pgwire/hba/conf.rl
Outdated
| s = String{Value: string(data[mark:p])} | ||
| s = tree.Name(string(data[mark:p])) | ||
| all = false | ||
| if s == "all" { |
There was a problem hiding this comment.
I'm opposed to hard-coding special keywords in the parser because, although we only support all now, we could easily support others in the future. https://www.postgresql.org/docs/current/auth-pg-hba-conf.html lists a bunch they support that we could easily want to use like sameuser and samerole, or come up with some on our own.
| table.SetTrimWhiteSpaceAtEOL(true) | ||
| table.SetTablePadding(" ") | ||
|
|
||
| row := []string{"# TYPE", "DATABASE", "USER", "ADDRESS", "METHOD", "OPTIONS"} |
There was a problem hiding this comment.
While this happens to be the format of the host lines, the postgres hba.conf allows different connection types that can have other field types. For example if you want to add a socket type, the address column may not be needed. Can we avoid hard coding the column types for the entire table, since the spec allows for them to differ?
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mjibson)
pkg/sql/pgwire/hba/conf.rl, line 64 at r2 (raw file):
Previously, mjibson (Matt Jibson) wrote…
I'm opposed to hard-coding special keywords in the parser because, although we only support
allnow, we could easily support others in the future. https://www.postgresql.org/docs/current/auth-pg-hba-conf.html lists a bunch they support that we could easily want to use likesameuserandsamerole, or come up with some on our own.
I tend to agree. Based on your comment on the other PR I will attempt to produce a simple parser.
pkg/sql/pgwire/hba/hba.go, line 50 at r2 (raw file):
Previously, mjibson (Matt Jibson) wrote…
While this happens to be the format of the
hostlines, the postgres hba.conf allows different connection types that can have other field types. For example if you want to add a socket type, the address column may not be needed. Can we avoid hard coding the column types for the entire table, since the spec allows for them to differ?
I think it's fine to have all the columns, and just let some become empty when the value is not relevant.
For example:
TYPE DATABASE USER ADDRESS METHOD OPTIONS
local all all password
I will tweak the code accordingly.
8ca2bb0 to
41df98f
Compare
knz
left a comment
There was a problem hiding this comment.
RFAL.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mjibson)
pkg/sql/pgwire/hba/conf.rl, line 64 at r2 (raw file):
Previously, knz (kena) wrote…
I tend to agree. Based on your comment on the other PR I will attempt to produce a simple parser.
Done.
pkg/sql/pgwire/hba/hba.go, line 50 at r2 (raw file):
Previously, knz (kena) wrote…
I think it's fine to have all the columns, and just let some become empty when the value is not relevant.
For example:TYPE DATABASE USER ADDRESS METHOD OPTIONS local all all passwordI will tweak the code accordingly.
Done.
f07323b to
978c06c
Compare
pkg/sql/pgwire/hba/hba.go
Outdated
|
|
||
| func (h Entry) String() string { | ||
| // UserMatches returns true iff the provided username matches the | ||
| // first entry in the User list or if the entry matches all. |
There was a problem hiding this comment.
I don't think this comment is accurate. This function looks like it can match any entry, not just the first.
76a74db to
6094b98
Compare
knz
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @mjibson)
pkg/sql/pgwire/hba/hba.go, line 117 at r4 (raw file):
Previously, mjibson (Matt Jibson) wrote…
I don't think this comment is accurate. This function looks like it can match any entry, not just the first.
Good catch. That comment was for the previous implementation, which I reverted. Thanks. Fixed.
Prior to this patch, the authentication code was relatively complex to
understand:
- the code path to use the HBA configuration would only be activated
if the cluster setting was set. There was a special case if it
wasn't. The special case was in the per-conn auth logic.
- the special case for the root login escape was also present in the
per-conn auth logic.
- every time the cluster setting would be updated, the result of
parsing the setting was cached as-is, and the textual strings
re-interpreted over and over upon every connection.
This complexity can be interpreted as a defect, for the main reason
that **the per-conn auth code should be as simple as possible** to
facilitate security audits and future maintainance. Additionally, an
argument could be made that the per-conn re-interpretation of the
result was a performance mishap.
This patch aims to simplify the per-conn auth logic by folding all the
special casing as pre-defined rules in the HBA configuration:
- the default logic (when the cluster setting is empty or invalid)
becomes a predefined HBA configuration with just two rules:
host all root all cert
host all all all cert-password
- each time the config is loaded from a cluster setting, the
root escape is implemented by force-inserting `host all root all
cert` at the start of the configuration.
With this in place, the auth logic can be simplified to always
and exclusively use the HBA rules.
This special casing can also be inspected in the output of
`/debug/hba_conf`.
Additionally, this patch optimizes the code by pre-normalizing upfront
when the setting is updated. Normalizing includes:
- unicode-normalizing and case-folding usernames, since the username
upon new connection is also normalized and case-folded.
- expanding lists of multiple usernames into multiple rules, so
that the checking code can be simplified to only check one username
per rule.
- name resolution of the authentication method into its function
pointer.
Release note (security): The authentication code for new SQL
connections has been simplified to always use the HBA configuration
defined per `server.host_based_authentication.configuration`.
The format of this file generally follows that of `pg_hba.conf` as
defined here:
https://www.postgresql.org/docs/current/auth-pg-hba-conf.html.
Upon each configuration change, CockroachDB auto-magically inserts the
entry `host all root all cert` as a first rule, to ensure the root
user can always log in with a valid client certificate.
If the configuration is set to empty, or found to be invalid in the
cluster setting, the following default configuration is automatically
used:
host all root all cert
host all all all cert-password
At any moment the current configuration on each node can be inspected
using the `/debug/hba_conf` URL on the HTTP endpoint.
The list of valid authentication methods is currently:
- `cert`, for certificate-based authentication over a SSL connection
exclusively;
- `cert-password`, which allows either cert-based or password-based
authentication over a SSL connection;
- `password` for password-based authentication over a SSL connection;
- `gss` for Kerberos-based authentication over a SSL connection,
enabled when running a CCL binary and an Enterprise license.
In effect CockroachDB treats all the `host` rules as `hostssl`, and
behaves as per a default of `hostnossl all all all reject`.
It is not currently possible to define authentication rules over
non-SSL connections: as of this writing, non-SSL connections are only
possible when running with `--insecure`, and on insecure nodes all the
authentication logic is entirely disabled.
This behavior remains equivalent to previous CockroachDB versions,
and this change is only discussed here for clarity.
6094b98 to
2200d26
Compare
|
TFYR! bors r+ |
43726: pgwire: clarify the special cases as pre-defined HBA rules r=knz a=knz First commit from #43779. This PR is needed ahead of fixing #31113, which will require supporting `local` HBA rules. Prior to this patch, the authentication code was relatively complex to understand: - the code path to use the HBA configuration would only be activated if the cluster setting was set. There was a special case if it wasn't. The special case was in the per-conn auth logic. - the special case for the root login escape was also present in the per-conn auth logic. - every time the cluster setting would be updated, the result of parsing the setting was cached as-is, and the textual strings re-interpreted over and over upon every connection. This complexity can be interpreted as a defect, for the main reason that **the per-conn auth code should be as simple as possible** to facilitate security audits and future maintainance. Additionally, an argument could be made that the per-conn re-interpretation of the result was a performance mishap. This patch aims to simplify the per-conn auth logic by folding all the special casing as pre-defined rules in the HBA configuration: - the default logic (when the cluster setting is empty or invalid) becomes a predefined HBA configuration with just two rules: host all root all cert host all all all cert-password - each time the config is loaded from a cluster setting, the root escape is implemented by force-inserting `host all root all cert` at the start of the configuration. With this in place, the auth logic can be simplified to always and exclusively use the HBA rules. This special casing can also be inspected in the output of `/debug/hba_conf`. Additionally, this patch optimizes the code by pre-normalizing upfront when the setting is updated. Normalizing includes: - unicode-normalizing and case-folding usernames, since the username upon new connection is also normalized and case-folded. - expanding lists of multiple usernames into multiple rules, so that the checking code can be simplified to only check one username per rule. Release note (security): The authentication code for new SQL connections has been simplified to always use the HBA configuration defined per `server.host_based_authentication.configuration`. The format of this file generally follows that of `pg_hba.conf` as defined here: https://www.postgresql.org/docs/current/auth-pg-hba-conf.html. Upon each configuration change, CockroachDB auto-magically inserts the entry `host all root all cert` as a first rule, to ensure the root user can always log in with a valid client certificate. If the configuration is set to empty, or found to be invalid in the cluster setting, the following default configuration is automatically used: host all root all cert host all all all cert-password At any moment the current configuration on each node can be inspected using the `/debug/hba_conf` URL on the HTTP endpoint. The list of valid authentication methods is currently: - `cert`, for certificate-based authentication over a SSL connection exclusively; - `cert-password`, which allows either cert-based or password-based authentication over a SSL connection; - `password` for password-based authentication over a SSL connection; - `gss` for Kerberos-based authentication over a SSL connection, enabled when running a CCL binary and an Enterprise license. In effect CockroachDB treats all the `host` rules as `hostssl`, and behaves as per a default of `hostnossl all all all reject`. It is not currently possible to define authentication rules over non-SSL connections: as of this writing, non-SSL connections are only possible when running with `--insecure`, and on insecure nodes all the authentication logic is entirely disabled. This behavior remains equivalent to previous CockroachDB versions, and this change is only discussed here for clarity. Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
Build succeeded |
43731: pgwire,hba: introduce the 'trust' and 'reject' auth methods r=knz a=knz First commits from #43734 and #43726. The 'trust' and 'reject' methods, as their name implies, unconditionally allow and deny authentication of matching connections. This patch introduces them as a prerequisite to later work on Unix socket authentication, but also to introduce a bit of infrastructure that binds new auth methods to a minimum required cluster version. This new infrastructure ensures that future new auth methods do not risk hosting a mixed-version cluster. (See discussion on #43717.) The patch also introduces support for the 'local' rule prefix, which is still unused. Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
First commit from #43779.
This PR is needed ahead of fixing #31113, which will require supporting
localHBA rules.Prior to this patch, the authentication code was relatively complex to
understand:
if the cluster setting was set. There was a special case if it
wasn't. The special case was in the per-conn auth logic.
per-conn auth logic.
parsing the setting was cached as-is, and the textual strings
re-interpreted over and over upon every connection.
This complexity can be interpreted as a defect, for the main reason
that the per-conn auth code should be as simple as possible to
facilitate security audits and future maintainance. Additionally, an
argument could be made that the per-conn re-interpretation of the
result was a performance mishap.
This patch aims to simplify the per-conn auth logic by folding all the
special casing as pre-defined rules in the HBA configuration:
the default logic (when the cluster setting is empty or invalid)
becomes a predefined HBA configuration with just two rules:
each time the config is loaded from a cluster setting, the
root escape is implemented by force-inserting
host all root all certat the start of the configuration.With this in place, the auth logic can be simplified to always
and exclusively use the HBA rules.
This special casing can also be inspected in the output of
/debug/hba_conf.Additionally, this patch optimizes the code by pre-normalizing upfront
when the setting is updated. Normalizing includes:
upon new connection is also normalized and case-folded.
that the checking code can be simplified to only check one username
per rule.
Release note (security): The authentication code for new SQL
connections has been simplified to always use the HBA configuration
defined per
server.host_based_authentication.configuration.The format of this file generally follows that of
pg_hba.confasdefined here:
https://www.postgresql.org/docs/current/auth-pg-hba-conf.html.
Upon each configuration change, CockroachDB auto-magically inserts the
entry
host all root all certas a first rule, to ensure the rootuser can always log in with a valid client certificate.
If the configuration is set to empty, or found to be invalid in the
cluster setting, the following default configuration is automatically
used:
At any moment the current configuration on each node can be inspected
using the
/debug/hba_confURL on the HTTP endpoint.The list of valid authentication methods is currently:
cert, for certificate-based authentication over a SSL connectionexclusively;
cert-password, which allows either cert-based or password-basedauthentication over a SSL connection;
passwordfor password-based authentication over a SSL connection;gssfor Kerberos-based authentication over a SSL connection,enabled when running a CCL binary and an Enterprise license.
In effect CockroachDB treats all the
hostrules ashostssl, andbehaves as per a default of
hostnossl all all all reject.It is not currently possible to define authentication rules over
non-SSL connections: as of this writing, non-SSL connections are only
possible when running with
--insecure, and on insecure nodes all theauthentication logic is entirely disabled.
This behavior remains equivalent to previous CockroachDB versions,
and this change is only discussed here for clarity.