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

pgwire: clarify the special cases as pre-defined HBA rules #43726

Merged
merged 1 commit into from
Jan 7, 2020

Conversation

knz
Copy link
Contributor

@knz knz commented Jan 4, 2020

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.

@knz knz requested a review from maddyblue January 4, 2020 15:10
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz force-pushed the 20200103-normalize-hba branch 4 times, most recently from ddbbf85 to 1da6f97 Compare January 5, 2020 13:28
@knz knz requested a review from a team as a code owner January 5, 2020 13:28
@knz knz force-pushed the 20200103-normalize-hba branch 5 times, most recently from 0e6c797 to ef1612d Compare January 6, 2020 06:55
@knz knz force-pushed the 20200103-normalize-hba branch 6 times, most recently from 4099668 to 8ca2bb0 Compare January 6, 2020 10:19
s = String{Value: string(data[mark:p])}
s = tree.Name(string(data[mark:p]))
all = false
if s == "all" {
Copy link
Contributor

Choose a reason for hiding this comment

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

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"}
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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 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.

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 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?

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.

@knz knz force-pushed the 20200103-normalize-hba branch from 8ca2bb0 to 41df98f Compare January 7, 2020 16:12
Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

RFAL.

Reviewable status: :shipit: 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          password

I will tweak the code accordingly.

Done.

@knz knz force-pushed the 20200103-normalize-hba branch 3 times, most recently from f07323b to 978c06c Compare January 7, 2020 18:31
@@ -74,43 +113,103 @@ func (h Entry) GetOptions(name string) []string {
return val
}

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.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this comment is accurate. This function looks like it can match any entry, not just the first.

@knz knz force-pushed the 20200103-normalize-hba branch 2 times, most recently from 76a74db to 6094b98 Compare January 7, 2020 22:23
Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: 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.
@knz knz force-pushed the 20200103-normalize-hba branch from 6094b98 to 2200d26 Compare January 7, 2020 22:24
@knz
Copy link
Contributor Author

knz commented Jan 7, 2020

TFYR!

bors r+

craig bot pushed a commit that referenced this pull request Jan 7, 2020
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 <[email protected]>
@craig
Copy link
Contributor

craig bot commented Jan 7, 2020

Build succeeded

@craig craig bot merged commit 2200d26 into cockroachdb:master Jan 7, 2020
@knz knz deleted the 20200103-normalize-hba branch January 7, 2020 23:38
craig bot pushed a commit that referenced this pull request Jan 8, 2020
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 <[email protected]>
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