sql: introduce the new role option CREATELOGIN

[knz]

This commit introduces a new SQL-level role option (alongside LOGIN
etc) called CREATELOGIN.

This is used to control access to:

• the WITH PASSWORD clause for CREATE/ALTER USER/ROLE:
  initializing, changing or removing password on users.

• the VALID UNTIL clause for CREATE/ALTER USER/ROLE:
  changing the expiration date for a password.

• ALTER USER/ROLE CREATELOGIN/NOCREATELOGIN:
  granting or removing the option for a user or role.

• ALTER USER/ROLE LOGIN/NOLOGIN:
  enabling a user/role to log in using either passwords, certs or
  another valid auth method

This feature enables a site operator to separate the responsibility of
administrating users and roles, which requires the CREATEROLE
privilege, from that of administrating authentication principals,
which now requires CREATELOGIN.

For example, it can be used to ensure that no SQL client can choose
their own passwords for users/roles, even if they are privileged
enough to create new users or roles.

Release note (security update): Defining or changing authentication
principals or their credentials now requires the new CREATELOGIN
option to be set for the requesting user or one of its roles. This
includes setting/removing the LOGIN option (wether the principal can
log in); initializing or changing the password of a SQL user, as well
as setting the expiration date for a password. Previously, only the
CREATEROLE option was sufficient to perform these changes. The
pseudo-option NOCREATELOGIN can be used to revoke CREATELOGIN.

The two predefined root and admin roles have the option
CREATELOGIN set by default.

Release note (security update): Only a user which already has
option CREATELOGIN (either itself or one of its roles) can grant
this option or use NOCREATELOGIN.

Release note (security update): Roles that had the CREATEROLE
privilege prior to upgrading to this version are also automatically
granted CREATELOGIN. After the upgrade, CREATELOGIN is not granted
automatically anymore.

cc @joshimhoff @aaron-crl @thtruo

[cockroach-teamcity]

This change is 

[rohany]

Looks good, but @RichardJCai should take a look.

[rohany]

Should there be validation here that option is one of the allowed strings? Or does the role_options table have a check constraint to ensure this?

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @RichardJCai, @rohany, and @solongordon)

---

pkg/sql/alter_role.go, line 98 at r1 (raw file):

				clusterversion.VersionByKey(clusterversion.VersionSetPasswordPrivilege))
		}
	} else {

I don't think you need this else here

---

pkg/sql/create_role.go, line 85 at r1 (raw file):

	// Check that the requested combination of
	// PASSWORD/SETPASSWORD/NOSETPASSWORD is compatible with the user's

Missing VALID UNTIL

Nit: Maybe rewrite this comment as:
ensure the user has SETPASSWORD privilege if setting any PASSWORD related privileges (PASSWORD/SETPASSWORD/NOSETPASSWORD/VALIDUNTIL)

---

pkg/sql/create_role.go, line 122 at r1 (raw file):

	if n.roleOptions.Contains(roleoption.PASSWORD) {
		if err := params.p.HasRoleOption(params.ctx, roleoption.SETPASSWORD); err != nil {
			return err

I think this check might not be necessary since we already check once before this.

Also I find it a little strange that you can't create a role with a password even if you have create role. (just a thought, not asking for a change)

---

pkg/sql/logictest/testdata/logic_test/role, line 1220 at r1 (raw file):

statement ok
CREATE ROLE pseudo_admin;
  ALTER ROLE pseudo_admin CREATEROLE;

Looks like theres random indents here

---

pkg/sql/logictest/testdata/logic_test/role, line 1230 at r1 (raw file):

statement ok
CREATE USER testuser2

Nit: I would create this user right before its used, (move it below CREATE USER testuser3 to make it a little easier to follow)

---

pkg/sqlmigrations/migrations.go, line 1599 at r1 (raw file):

Previously, rohany (Rohan Yadav) wrote…

Should there be validation here that option is one of the allowed strings? Or does the role_options table have a check constraint to ensure this?

There's no check constraint, maybe that would be a good idea to add? Could maybe catch a typo when inserting options

[RichardJCai]

Looks mostly good to me with some nits. Just wondering, is there some documentation on the motivation for this?

[knz]

I thought the motivation was in the commit message, but apparently not enough :)

The idea is that for CockroachCloud we want to prevent any customer from setting their passwords themselves, and instead ensure that only the CC console can do it. Meanwhile, customers still want to create other SQL users.

[knz]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @RichardJCai, @rohany, and @solongordon)

---

pkg/sql/alter_role.go, line 98 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

I don't think you need this else here

Indeed. Fixed.

---

pkg/sql/create_role.go, line 85 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Missing VALID UNTIL

Nit: Maybe rewrite this comment as:
ensure the user has SETPASSWORD privilege if setting any PASSWORD related privileges (PASSWORD/SETPASSWORD/NOSETPASSWORD/VALIDUNTIL)

Done.

---

pkg/sql/create_role.go, line 122 at r1 (raw file):

I think this check might not be necessary since we already check once before this.
woops, oversight.

Also I find it a little strange that you can't create a role with a password even if you have create role. (just a thought, not asking for a change)

See the motivation - we want the definition of roles and the definition of authentication credentials to be different authorities.

---

pkg/sql/logictest/testdata/logic_test/role, line 1220 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Looks like theres random indents here

How so random? I always indent every line after the first in a multi-line statement logic test.

---

pkg/sql/logictest/testdata/logic_test/role, line 1230 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Nit: I would create this user right before its used, (move it below CREATE USER testuser3 to make it a little easier to follow)

Done.

---

pkg/sqlmigrations/migrations.go, line 1599 at r1 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

There's no check constraint, maybe that would be a good idea to add? Could maybe catch a typo when inserting options

I'm not sold on the check constraint. How would you deal with cluster version migrations? We can't simply drop the constraint and re-create it?

[rohany]

I'm not sold on the check constraint. How would you deal with cluster version migrations? We can't simply drop the constraint and re-create it?

I think just a check in the code before doing the insert makes sense.

[RichardJCai]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @RichardJCai, @rohany, and @solongordon)

---

pkg/sql/logictest/testdata/logic_test/role, line 1220 at r1 (raw file):

Previously, knz (kena) wrote…

How so random? I always indent every line after the first in a multi-line statement logic test.

ah I see, I haven't actually seen that but before but it's fine

[knz]

I'm going to hold off on merging this until I also get a green light from both @aaron-crl and @bdarnell

[knz]

Comment from @aaron-crl :

Will this allow any user with this permissions to change the password for any other user or will this respect something similar to the pg ownership model where creators of objects have permissions on those objects only unless explicitly granted otherwise?

As implemented, any user with SETPASSWORD option will be able to change the password for any other user.

This is the case because we do not yet implement "ownership" for user accounts. If we should introduce this, it sounds like a different (and orthogonal) project.

@aaron-crl can you provide some background for your question, and color it with an example scenario where this matters?

[bdarnell]

can you provide some background for your question, and color it with an example scenario where this matters?

I'll let Aaron answer for himself, but I'm concerned that this is a confusing option to document and message. The commit message says that NOSETPASSWORD is a way to prevent users from setting their own passwords, but the converse SETPASSWORD is actually a superuser option to let you set passwords for (and therefore impersonate) any user.

This is the case because we do not yet implement "ownership" for user accounts. If we should introduce this, it sounds like a different (and orthogonal) project.

When we have that, this would become a three-way option: set any user's password, set own password, set no passwords. At a minimum our naming here should prepare for that eventuality.

The idea is that for CockroachCloud we want to prevent any customer from setting their passwords themselves, and instead ensure that only the CC console can do it.

Why do we want to prevent users from setting their own passwords? If it's about enforcing password complexity, I think we should build some direct enforcement for this instead of restrictions that force password changes to go through an external process for complexity enforcement.

Meanwhile, customers still want to create other SQL users.

But CC doesn't currently support client certificates, so if you also restrict use of passwords, you can't create users who can log in (you can still create roles, though).

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @RichardJCai, @rohany, and @solongordon)

[aaron-crl]

Thanks Ben. I concur on the messaging angle especially since when I first read the command name SETPASSWORD, I presumed it was a non-admin function.

@knz :

@aaron-crl can you provide some background for your question, and color it with an example scenario where this matters?

This is how most other password auth systems behave (including pg). Users tend to be permitted to update their own password without admin intervention.

From postgres docs (https://www.postgresql.org/docs/8.0/sql-alteruser.html):

The first variant of this command listed in the synopsis changes certain per-user privileges and authentication settings. (See below for details.) Database superusers can change any of these settings for any user. Ordinary users can only change their own password.

(emphasis mine)

For a use case: If a database user (non-admin) inadvertently spills their username and password. They might reasonably expect to change the password themselves. In the case where the user does not have this ability they would need to find an admin to reissue them a password, or allow them to update theirs. That introduces all kinds of headaches around creating the password, then transferring to the user or admin, etc.

This is less applicable with certificates because we explicitly expect a third party service to handle issuance and revocation but not with password auth.

[knz]

Discussed elsewhere with @aaron-crl and @bdarnell : the functionality in this PR is incomplete.

A more correct description of the goal to attain is to enable the creation of roles, while disallowign the creation of authn principals (user accounts able to log in).

Therefore the new priv, if any, should restrict the attribution of the LOGIN option and probably changing the other password properties (eg VALID UNTIL), not just setting or changing passwords.

The name of the privilege in that case should not be SETPASSWORD but instead CREATELOGIN or similar.

Will iterate once more on this PR when the overall strategy has been refined further.

[knz]

Updated the PR to become "CREATELOGIN" as discussed. RFAL.

[bdarnell]

LGTM, but I wonder whether role options are the right model to use here. As we decompose the admin role, there are going to be a lot of flags like this. Do we want to create option flags for all of them, or is there some other role inheritance model we should be using instead?

One specific concern I have is that if a user had created their own partial-admin role with the CREATEROLE option, they may now have to manually update their role with CREATELOGIN to avoid breaking their existing functionality. Is there something more future-proof we can do?

Reviewed 16 of 16 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @knz, @RichardJCai, @rohany, and @solongordon)

[knz]

Do we want to create option flags for all of them, or is there some other role inheritance model we should be using instead?

My impression is that we will operate in two steps:

1. define the role options
2. pre-define a new role cc_admin or operator or similar with these options, then use that to define pseudo-admins in the clusters from CC console.

(In the context of CC specifically, CREATEROLE would be granted this way, but not CREATELOGIN. Other deployments can use roles similarly.)

One specific concern I have is that if a user had created their own partial-admin role with the CREATEROLE option, they may now have to manually update their role with CREATELOGIN to avoid breaking their existing functionality. Is there something more future-proof we can do?

In my view the two options are unrelated: the first one is about grouping privilege levels together, the second one is about granting access. Can you outline a scenario where we'd be in a poor UX situation?

[bdarnell]

One specific concern I have is that if a user had created their own partial-admin role with the CREATEROLE option, they may now have to manually update their role with CREATELOGIN to avoid breaking their existing functionality. Is there something more future-proof we can do?

In my view the two options are unrelated: the first one is about grouping privilege levels together, the second one is about granting access. Can you outline a scenario where we'd be in a poor UX situation?

Yes, once both permissions exist, the use cases are distinct. But prior to this PR, the CREATEROLE option is used for both use cases. Current users of CREATEROLE may be using it to create roles, logins, or both. If they were using it for login creation, this would be a backwards-incompatible change. We could preserve backwards-compatibility by granting the CREATELOGIN option to all roles with CREATEROLE as a part of this migration. Backwards-incompatibility in the service of security is sometimes acceptable, but it's not always worth the disruption (and I'm not looking to argue whether that's true of the CREATELOGIN split, but to raise the general issue because we'll be breaking out more and more of these permissions over time).

One possible future-proofing I had in mind was to create multiple system-defined roles for a bundle of permissions, which we could keep up to date over time instead of leaving users to assemble their own roles by hand. But naming these roles and defining what they include seems tricky.

[knz]

Modified as discussed above -- there is now a migration that auto-grants CREATELOGIN to the roles that had CREATEROLE prior to this PR.

PTAL

[solongordon]

New migration LGTM

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @bdarnell, @knz, @RichardJCai, @rohany, and @solongordon)

[joshimhoff]

CC @DuskEagle

[knz]

thanks!

bors r=RichardJCai,solongordon,bdarnell

[craig]

Build succeeded:

• GitHub CI (Cockroach)