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.

Sign up for GitHub

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

Jump to bottom

security: allow users to enable password auth for 'root' #43847

Closed
knz opened this issue Jan 9, 2020 · 11 comments · Fixed by #43893
Closed

security: allow users to enable password auth for 'root' #43847

knz opened this issue Jan 9, 2020 · 11 comments · Fixed by #43893
Labels
A-security C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)

Comments

@knz
Copy link
Contributor

knz commented Jan 9, 2020 •
edited
Loading

tldr; this issue proposes to enable:

  • setting/changing the password of the root user;
  • allowing root to log in using their password on the UI;
  • allowing root to log in using their password on SQL, subject to regular rules in the HBA configuration.

It also fixes the regression introduced by #42563, by enabling root authentication on the admin UI to obtain a login cookie.

Background

CockroachDB currently offers 5 separate guardrails to ensure that root is always able to connect even when the authentication configuration is botched:

  1. when running --insecure, anyone can log in without auth.
  2. the crdb HBA configuration enforces that host all root all cert is always the first rule, meaning that clients can log in as root if and only if they present a valid TLS client cert for root (and because the method is cert and not cert-password, password auth is rejected for root in any case).
  3. ALTER USER WITH PASSWORD is disallowed for root, so root cannot have a password
  4. the internal "check password" mechanism, shared by both SQL and HTTP connections, to compare a client-provided password with a user record fails with an error if the user is root.
  5. the UI UserLogin HTTP API reports an error if the user is root.
  6. the cockroach CLI commands using SQL connections report an error if the user is root and a password is supplied.

Meanwhile, there is a separate, unrelated (but relevant) rule:

  1. if a user's password is unset/empty, then this user is not able to use password authentication.

Proposal

This issue proposes to remove rules 3-6 specifically, without changing the others.

This proposal would not change the security rules for a cluster using the default configuration: by default, root would not be able to log in using password anywhere (by default, the root account has no password so rule 6 applies) and is required to present a cert on SQL (due to rule 2).

Non-Pitfalls

A possible counter-argument to this proposal is that CockroachdB uses root internally to establish SQL client connections towards itself.

This is not an obstacle: in insecure clusters, the password would be ignored anyway; in secure clusters, CockroachDB's internal connections use a valid client cert.
@knz
Copy link
Contributor Author

knz commented Jan 9, 2020

@piyush-singh I am putting this on the docket because addressing this would simplify the product and make it easier to document / reason about it.

@aaron-crl can you check the proposal?

@knz knz added A-security C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception) labels Jan 9, 2020
@kenliu
Copy link

kenliu commented Jan 9, 2020

@knz please make sure that @rolandcrosby is aware of any security related feature proposals as he is responsible for this area.

@knz
Copy link
Contributor Author

knz commented Jan 9, 2020

oh sorry I got the PMs mixed up. thanks for the reminder.

@aaron-crl
Copy link

Three quick questions:

(1) Was there a reason we did this as certificate only access?
(2) Would it be possible to separate “root” from internal connections and instead use a reserved exclusive account for those?
(3) Why do we have a “root” account? I know this sounds pedantic but what use case(s) does this account currently support (possibly related to question 1).

@knz
Copy link
Contributor Author

knz commented Jan 9, 2020

(1) the reason is historical: this predefined rule existed before we started to support password auth

(2) we already use another special account node for most internal connections. The only few that use SQL could be migrated to use a separate special account I guess.

(3) the root account is the one account with the admin role available in a newly created cluster. The admin role is necessary to create other users, grant privileges etc.

Pursuant to (3) I guess one could ask "what makes root special and why could it not become a regular user account" -- the reason is that root cannot be deleted from the cluster; we have this as ground rule to prevent users from shooting themselves in the foot and becoming unable to administrate their cluster.

@aaron-crl
Copy link

Thanks!

Sounds good to me with the caveat that it would be nice to see all cluster internal functions use their own security principals to make security logging and eventing easier in the future re: (2).

@knz
Copy link
Contributor Author

knz commented Jan 9, 2020

all cluster internal functions use their own security principals

That's quite an interesting idea.

FYI for all internal uses of SQL we have this already, because SQL connections have a special token called application_name which is used in logging and our internal uses already populate this with a different value each time.

For RPC connections I guess we could consider this as an addition. Seems orthogonal though.

@mattcrdb
Copy link

mattcrdb commented Jan 9, 2020 •
edited by knz
Loading

Our recent security fix #42563 is preventing access to the HTTP endpoints in non-enterprise clusters. Specifically the #/node/x/logs endpoint requires root user access or the admin-role enabled on the sql user. Without an enterprise licence you can only view that endpoint as the root user (as granting roles is an enterprise feature), however because the root user can't have a password - that page and any other pages that quire this role are blocked.

(edit knz: fixing this issue would solve that regression.)

@knz knz added the regression Regression from a release. label Jan 9, 2020
@aaron-crl
Copy link

aaron-crl commented Jan 9, 2020 •
edited
Loading

Seems orthogonal though.

Agreed, we should move that discussion elsewhere. It's a nice to have from a security hygiene standpoint; no other concerns.

@israellot
Copy link

Allowing to assign the admin role to one additional user would also fix the problem described by @mattcrdb

This was referenced Jan 10, 2020
Closed
Closed
@knz
Copy link
Contributor Author

knz commented Jan 10, 2020

Root passwords are only one of the solutions for the regression. I have generalized the statement of the regression in the following issue: #43870

@knz knz removed the regression Regression from a release. label Jan 10, 2020
This was referenced Jan 10, 2020
Merged
Merged
Merged
craig bot pushed a commit that referenced this issue Jan 14, 2020
@knz
Merge #43892
e72274f 
43892: sql: enable clearing user password r=knz a=knz

Fixes #43891. Needed/useful for #43847.

It is useful to clear a user's password, as this is the only way to
enforce they authenticate using client certificates regardless of the
HBA configuration.

Prior to this patch, it was not possible to clear a user's password.
This patch makes it possible.

Release note (security update): Previously if a user password's had
been set once, thus enabling them to use password authentication, it
was not possible revert that choice and disable password
authentication for them any more other than either dropping the user,
or adding a specific per-user HBA rule. This has been fixed and the
PostgreSQL-compatible `ALTER USER WITH PASSWORD NULL` statement can
now be used to clear the user's password.

Release note (sql change): CREATE USER and ALTER USER now accept the
parameter `[WITH] PASSWORD NULL` to indicate the user's password
should be removed, thus preventing them from using password
authentication. This is compatible with PostgreSQL.

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@knz knz mentioned this issue Jan 14, 2020
Open
craig bot pushed a commit that referenced this issue Jan 17, 2020
@knz
Merge #43872
50728e3 
43872: cli: new command `auth-session {login,logout,list}` r=ajwerner,aaron-crl a=knz

Fixes #43870.

tldr: this adds new CLI commands to log users in and out of the
HTTP interface and produce a HTTP cookie for use in monitoring
scripts. This is suitable for use by the `root` user without an
Enterprise license.

Also the new feature is client-side only, so the client binary with
this feature can be used with a CockroachDB server/cluster running at
an older version.

**Motivation:** users who wish to use certain HTTP monitoring tools,
in particular those that retrieve privileged information like logs,
need a valid HTTP authentication token for an admin user (#42567). This token
can be constructed by accessing the HTTP endpoint `/login`, however:

- manually crafting the token using `/login` is cumbersome;
- it's not possible to use `/login` for the `root` user (#43847);
- it's not possible to create another admin user than `root` without
  a valid Enterprise license (because that requires role management).

**Solution:**

```
cockroach auth-session login <username> [--expire-after=...] [--only-cookie]
cockroach auth-session logout <username>
cockroach auth-session list
```

- all three commands also support the standard SQL command-line
  arguments, e.g. `--url`, `--certs-dir`, `--echo-sql` and
  `--format`.
- the `--expire-after` argument customizes the expiry period. The
  default is one hour.
- the `--only-cookie` arguments limits the output of the command
  to just the HTTP cookie. By default, the session ID and
  the authentication cookie are printed using regular table formatting.

Also see the two release notes below.

Release note (cli change): Three new CLI commands `cockroach
auth-session login`, `cockroach auth-session list` and `cockroach
auth-session logout` are now provided to facilitate the management of
web sessions. The command `auth-session login` also produces a HTTP
cookie which can be used by non-interactive HTTP-based database
management tools. It also can generate such a cookie for the `root`
user, who would not otherwise be able to do so using a web browser.

Release note (security update): The new command `cockroach
auth-session login` (reserved to administrators) is able to create
authentication tokens with an arbitrary expiration date. Operators
should be careful to monitor `system.web_sessions` and enforce
policy-mandated expirations either using SQL queries or the new
command `cockroach auth-session logout`.


Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>
@craig craig bot closed this as completed in 3862ec7 Jan 18, 2020
@dbist dbist mentioned this issue Apr 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-security C-enhancement Solution expected to add code/behavior + preserve backward-compat (pg compat issues are exception)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

server,cli,sql: enable setting a password for the root user knz/cockroach
5 participants
@kenliu @knz @israellot @mattcrdb @aaron-crl