IAM doc updates #7061
IAM doc updates #7061
Conversation
|
This change is |
|
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/5307d389d1ecdf35b6bad901b2cb6b147eed35b8/ Edited pages: |
|
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/f3ee937f8d3026922f91fb726013d67ea8a650c0/ Edited pages: |
|
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/d87b81f4afae64f20a35a86ab5ced7e6a5dc29dd/ Edited pages: |
|
Online preview: http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/72e10e5572202d737ef72326a85e4db8e94fab15/ Edited pages: |
|
The broken link is due to a SQL diagram error I think. Might need help looking into it. |
|
Some general comments.
|
Good catch. Fixed.
Me too. I don't know why it's lowercase in some cases in legacy content. I followed the conventions in each doc for now but will follow up in another PR to make capitalization the default.
Yep. I need to get to the authentication updates first, but this docs reorg is on my to-do list. I opened an issue to track this: #7063 |
TFTR!! |
There was a problem hiding this comment.
Left some more comments - mostly things that changed from 19.2 to 20.1.
Also for most of my comments, I only left a comment for the user version but they also apply to the role version.
As a side note I think merging the role / user versions into one version should be a priority and really beneficial.
I didn't review the gssapi_authentication.md doc - don't have enough context.
| @@ -28,6 +28,8 @@ By default, a new user belongs to the `public` role and has no privileges other | |||
| ### `root` user | |||
There was a problem hiding this comment.
I'm just including it here since this line doesn't show in the diff, but theres a block stating
Use the CREATE USER and DROP USER statements to create and remove users, the ALTER USER statement to add or change a user's password, the GRANT <privileges> and REVOKE <privileges> statements to manage the user’s privileges, and the SHOW USERS statement to list users.
Can you update this to say the ALTER USER statement to add or change a users password and role options
v20.1/create-user.md
Outdated
| @@ -25,6 +25,8 @@ The `CREATE USER` [statement](sql-statements.html) creates SQL users, which let | |||
|
|
|||
| The user must have the `INSERT` and `UPDATE` [privileges](authorization.html#assign-privileges) on the `system.users` table. | |||
There was a problem hiding this comment.
This is no longer true - having CREATEROLE privilege is sufficient - this is true to all CREATE/ALTER/DROP role/user commands
| {% include copy-clipboard.html %} | ||
| ~~~ sql | ||
| > CREATE USER carl VALID UNTIL '2021-01-01'; | ||
| ~~~ | ||
|
|
||
| ### Manage users |
There was a problem hiding this comment.
After creating a user, you can use the ALTER USER statement to add or change the user's password and the DROP USER statement to the remove users.
In manage users, you can mention ALTER USER can update role options.
| @@ -19,6 +19,8 @@ The `DROP ROLE` [statement](sql-statements.html) removes one or more SQL roles. | |||
|
|
|||
| Roles can only be dropped by super users, i.e., members of the `admin` role. | |||
There was a problem hiding this comment.
This is no longer true - having CREATEROLE is sufficient.
There was a problem hiding this comment.
Ditto for drop user - DELETE privilege is not required on system.users.
In general permissions on system.users are no longer required for these RBAC commands.
v20.1/show-roles.md
Outdated
| @@ -18,7 +18,7 @@ The `SHOW ROLES` [statement](sql-statements.html) lists the roles for all databa | |||
|
|
|||
| ## Required privileges | |||
|
|
|||
| The user must have the [`SELECT`](select-clause.html) [privilege](authorization.html#assign-privileges) on the system table. | |||
| The role must have the [`SELECT`](select-clause.html) [privilege](authorization.html#assign-privileges) on the system table. | |||
There was a problem hiding this comment.
This one you actually need select on system.users table
There was a problem hiding this comment.
Ditto for system.roles - I believe system should be replaced with system.users
There was a problem hiding this comment.
You've covered most of the ground here. It's very good.
I think the only main open topic at this point is what should be done about NOLOGIN. I'm pretty sure that what you described in this PR is good, and is also what PostgreSQL does. If our code is different (what Richard seems to think, and this comes as a surprise to me), we need to take action on that one way or another.
Reviewed 4 of 7 files at r1, 1 of 3 files at r2, 2 of 6 files at r3, 1 of 6 files at r4, 1 of 1 files at r5, 4 of 5 files at r6, 2 of 2 files at r7.
Reviewable status:complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade and @RichardJCai)
v20.1/alter-role.md, line 38 at r7 (raw file):
----------|------------- `name` | The name of the role whose password you want to create or add. `password` | Let the role [authenticate their access to a secure cluster](authentication.html#client-authentication) using this new password. Passwords should be entered as [string literal](sql-constants.html#string-literals). For compatibility with PostgreSQL, a password can also be entered as an [identifier](#change-password-using-an-identifier), although this is discouraged. <br><br>To prevent a role from using [password authentication](authentication.html#client-authentication) and to mandate [certificate-based client authentication](authentication.html#client-authentication), [set the password as `NULL`](#prevent-a-role-from-using-password-authentication).
see our comments on the alter-user page. All of that applies here equally.
v20.1/alter-user.md, line 39 at r7 (raw file):
`name` | The name of the user whose password you want to create or add. `password` | Let the user [authenticate their access to a secure cluster](authentication.html#client-authentication) using this new password. Passwords should be entered as [string literal](sql-constants.html#string-literals). For compatibility with PostgreSQL, a password can also be entered as an [identifier](#change-password-using-an-identifier), although this is discouraged. <br><br>To prevent a user from using [password authentication](authentication.html#client-authentication) and to mandate [certificate-based client authentication](authentication.html#client-authentication), [set the password as `NULL`](#prevent-a-user-from-using-password-authentication). `valid until` | The date and time (in the [`timestamp`](timestamp.html) format) after which the password is not valid.
I recommend capitalizing the keywords in the first column.
v20.1/alter-user.md, line 40 at r7 (raw file):
I think this comment is only true of VALID UNTIL.
Also the code contradicts what you say: pgwire rejects auth for any user with NOLOGIN prior to determining which auth method to use. So it's not specific to passwords.
Amruta, the postgres doc are clear about LOGIN/NOLOGIN:
These clauses determine whether a role is allowed to log in; that is, whether the role can be given as the initial session authorization name during client connection.
https://www.postgresql.org/docs/11/sql-createrole.html
There is no mention of a restriction per auth method.
v20.1/create-role.md, line 45 at r7 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Same comment as before - only disables password logins.
same comment as before
v20.1/drop-role.md, line 22 at r7 (raw file):
Roles can only be dropped by super users, i.e., members of the `admin` role. To drop other non-admin roles, the role must have the [`createrole`](create-role.html#allow-the-role-to-create-other-roles) parameter set.
nit: capitalize CREATEROLE.
v20.1/drop-user.md, line 17 at r7 (raw file):
The user must have the `DELETE` [privilege](authorization.html#assign-privileges) on the `system.users` table. To drop other non-admin users, the user must have the [`createrole`](create-user.html#allow-the-user-to-create-other-users) parameter set.
ditto
v20.1/gssapi_authentication.md, line 146 at r7 (raw file):
Setting the `server.host_based_authentication.configuration` [cluster setting](cluster-settings.html) cluster setting to this particular value makes it mandatory for all non-`root` users to authenticate using GSSAPI. The `root` user is always an exception and remains able to authenticate using a valid client cert or a user password.
nit: line with spaces
v20.1/show-roles.md, line 21 at r7 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Ditto for system.roles - I believe
systemshould be replaced withsystem.users
Richard is right but it's odd it works this way. Filed cockroachdb/cockroach#47191 .
@knz Oh oops I had a brain fart - I mixed up NOLOGIN with VALID UNTIL. VALID UNTIL only applies to passwords, NOLOGIN applies to every auth method. My bad please disregard what I wrote about that, I'll edit it out. |
There was a problem hiding this comment.
Reviewable status:
v20.1/alter-role.md, line 38 at r7 (raw file):
Previously, knz (kena) wrote…
see our comments on the alter-user page. All of that applies here equally.
Done.
v20.1/alter-user.md, line 39 at r7 (raw file):
Previously, knz (kena) wrote…
I recommend capitalizing the keywords in the first column.
Done.
v20.1/alter-user.md, line 40 at r7 (raw file):
Previously, knz (kena) wrote…
I think this comment is only true of VALID UNTIL.
Also the code contradicts what you say: pgwire rejects auth for any user with NOLOGIN prior to determining which auth method to use. So it's not specific to passwords.Amruta, the postgres doc are clear about LOGIN/NOLOGIN:
These clauses determine whether a role is allowed to log in; that is, whether the role can be given as the initial session authorization name during client connection.
https://www.postgresql.org/docs/11/sql-createrole.htmlThere is no mention of a restriction per auth method.
ack
v20.1/authorization.md, line 28 at r7 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
I'm just including it here since this line doesn't show in the diff, but theres a block stating
Use the CREATE USER and DROP USER statements to create and remove users, the ALTER USER statement to add or change a user's password, the GRANT <privileges> and REVOKE <privileges> statements to manage the user’s privileges, and the SHOW USERS statement to list users.Can you update this to say
the ALTER USER statement to add or change a users password and role options
Done.
v20.1/create-role.md, line 45 at r7 (raw file):
Previously, knz (kena) wrote…
same comment as before
Done.
v20.1/create-user.md, line 26 at r7 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
This is no longer true - having CREATEROLE privilege is sufficient - this is true to all CREATE/ALTER/DROP role/user commands
Done.
v20.1/create-user.md, line 103 at r7 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
After creating a user, you can use the ALTER USER statement to add or change the user's password and the DROP USER statement to the remove users.
In manage users, you can mention ALTER USER can update role options.
Done.
v20.1/drop-role.md, line 20 at r7 (raw file):
Previously, RichardJCai (Richard Cai) wrote…
Ditto for drop user - DELETE privilege is not required on system.users.
In general permissions on system.users are no longer required for these RBAC commands.
Done.
v20.1/drop-user.md, line 17 at r7 (raw file):
Previously, knz (kena) wrote…
ditto
Done.
v20.1/show-roles.md, line 21 at r7 (raw file):
system.users
Do you also need createrole though? Or only having select on system.users is enough?
There was a problem hiding this comment.
Reviewed 10 of 10 files at r8.
Reviewable status:
v20.1/show-roles.md, line 21 at r7 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
system.users
Do you also need createrole though? Or only having select on system.users is enough?
CREATEROLE is not needed (nor is it useful).
Actually it's not just system.users, they must also have SELECT on system.role_members.
v20.1/show-users.md, line 21 at r8 (raw file):
## Required privileges The user must have the [`SELECT`](select-clause.html) [privilege](authorization.html#assign-privileges) on the `system.users` table.
ditto
There was a problem hiding this comment.
Reviewable status:
v20.1/show-roles.md, line 21 at r7 (raw file):
Previously, knz (kena) wrote…
CREATEROLE is not needed (nor is it useful).
Actually it's not just
system.users, they must also have SELECT onsystem.role_members.
Done.
There was a problem hiding this comment.
A few additional changes needed:
- Add
ALTER ROLEtosql-statements.md, with a new in v20.1 callout. - Add
ALTER ROLEto the 20.1 sidenav.
Reviewable status:
v20.1/alter-role.md, line 19 at r9 (raw file):
## Required privileges <span class="version-tag">New in v20.1:</span> To alter other roles, the role must have the [`CREATEROLE`](create-role.html#allow-the-role-to-create-other-roles) parameter set.
Do we need a new in 20.1 callout here? This entire statement is new...
v20.1/alter-role.md, line 36 at r9 (raw file):
----------|------------- `name` | The name of the role whose password you want to create or add. `password` | Let the role [authenticate their access to a secure cluster](authentication.html#client-authentication) using this new password. Passwords should be entered as [string literal](sql-constants.html#string-literals). For compatibility with PostgreSQL, a password can also be entered as an [identifier](#change-password-using-an-identifier), although this is discouraged. <br><br>To prevent a role from using [password authentication](authentication.html#client-authentication) and to mandate [certificate-based client authentication](authentication.html#client-authentication), [set the password as `NULL`](#prevent-a-role-from-using-password-authentication).
nit: a string literal or string literals.
"although this is discouraged." If we are going to say this, we should probably say why it's discouraged, or just not say it.
v20.1/alter-role.md, line 37 at r9 (raw file):
`name` | The name of the role whose password you want to create or add. `password` | Let the role [authenticate their access to a secure cluster](authentication.html#client-authentication) using this new password. Passwords should be entered as [string literal](sql-constants.html#string-literals). For compatibility with PostgreSQL, a password can also be entered as an [identifier](#change-password-using-an-identifier), although this is discouraged. <br><br>To prevent a role from using [password authentication](authentication.html#client-authentication) and to mandate [certificate-based client authentication](authentication.html#client-authentication), [set the password as `NULL`](#prevent-a-role-from-using-password-authentication). `VALID UNTIL` | <span class="version-tag">New in v20.1:</span> The date and time (in the [`timestamp`](timestamp.html) format) after which the password is not valid.
Again, not sure this needs to be called out as new since the entire statement is new. I think this and the following parameter do warrant callout as new in the docs for creating roles, though.
v20.1/alter-role.md, line 38 at r9 (raw file):
`password` | Let the role [authenticate their access to a secure cluster](authentication.html#client-authentication) using this new password. Passwords should be entered as [string literal](sql-constants.html#string-literals). For compatibility with PostgreSQL, a password can also be entered as an [identifier](#change-password-using-an-identifier), although this is discouraged. <br><br>To prevent a role from using [password authentication](authentication.html#client-authentication) and to mandate [certificate-based client authentication](authentication.html#client-authentication), [set the password as `NULL`](#prevent-a-role-from-using-password-authentication). `VALID UNTIL` | <span class="version-tag">New in v20.1:</span> The date and time (in the [`timestamp`](timestamp.html) format) after which the password is not valid. `LOGIN`/`NOLOGIN` | <span class="version-tag">New in v20.1:</span> The `LOGIN` parameter allows a role to login with one of the [client authentication methods](authentication.html#client-authentication). [Setting the parameter to `nologin`](#change-login-privileges-for-a-role) prevents the role from logging in using any authentication method.
Change nologin to NOLOGIN.
v20.1/alter-role.md, line 39 at r9 (raw file):
`VALID UNTIL` | <span class="version-tag">New in v20.1:</span> The date and time (in the [`timestamp`](timestamp.html) format) after which the password is not valid. `LOGIN`/`NOLOGIN` | <span class="version-tag">New in v20.1:</span> The `LOGIN` parameter allows a role to login with one of the [client authentication methods](authentication.html#client-authentication). [Setting the parameter to `nologin`](#change-login-privileges-for-a-role) prevents the role from logging in using any authentication method. `CREATEROLE`/`NOCREATEROLE` | <span class="version-tag">New in v20.1:</span> Allow or disallow the role to create, alter, and drop other roles. <br><br>By default, the parameter is set to `NOCREATEROLE` for all non-admin and non-root roles.
Same comment about the new in 20.1 callout.
v20.1/alter-user.md, line 36 at r9 (raw file):
----------|------------- `name` | The name of the user whose password you want to create or add. `password` | Let the user [authenticate their access to a secure cluster](authentication.html#client-authentication) using this new password. Passwords should be entered as [string literal](sql-constants.html#string-literals). For compatibility with PostgreSQL, a password can also be entered as an [identifier](#change-password-using-an-identifier), although this is discouraged. <br><br>To prevent a user from using [password authentication](authentication.html#client-authentication) and to mandate [certificate-based client authentication](authentication.html#client-authentication), [set the password as `NULL`](#prevent-a-user-from-using-password-authentication).
nit: a string literal or string literals.
"although this is discouraged." If we are going to say this, we should probably say why it's discouraged, or just not say it.
v20.1/alter-user.md, line 38 at r9 (raw file):
`password` | Let the user [authenticate their access to a secure cluster](authentication.html#client-authentication) using this new password. Passwords should be entered as [string literal](sql-constants.html#string-literals). For compatibility with PostgreSQL, a password can also be entered as an [identifier](#change-password-using-an-identifier), although this is discouraged. <br><br>To prevent a user from using [password authentication](authentication.html#client-authentication) and to mandate [certificate-based client authentication](authentication.html#client-authentication), [set the password as `NULL`](#prevent-a-user-from-using-password-authentication). `VALID UNTIL` | <span class="version-tag">New in v20.1:</span> The date and time (in the [`timestamp`](timestamp.html) format) after which the password is not valid. `LOGIN`/`NOLOGIN` | <span class="version-tag">New in v20.1:</span> The `LOGIN` parameter allows a user to login with one of the [client authentication methods](authentication.html#client-authentication). [Setting the parameter to `nologin`](#change-login-privileges-for-a-user) prevents the user from logging in using any authentication method.
Change nologin to NOLOGIN.
v20.1/create-role.md, line 41 at r9 (raw file):
------------|-------------- `name` | The name of the role you want to create. Role names are case-insensitive; must start with either a letter or underscore; must contain only letters, numbers, periods, or underscores; and must be between 1 and 63 characters.<br><br>Note that roles and [users](create-user.html) share the same namespace and must be unique. `password` | Let the role [authenticate their access to a secure cluster](authentication.html#client-authentication) using this password. Passwords must be entered as [string](string.html) values surrounded by single quotes (`'`).<br><br>Password creation is supported only in secure clusters.
Same comments as before.
v20.1/create-role.md, line 43 at r9 (raw file):
`password` | Let the role [authenticate their access to a secure cluster](authentication.html#client-authentication) using this password. Passwords must be entered as [string](string.html) values surrounded by single quotes (`'`).<br><br>Password creation is supported only in secure clusters. `VALID UNTIL` | <span class="version-tag">New in v20.1:</span> The date and time (in the [`timestamp`](timestamp.html) format) after which the password is not valid. `LOGIN`/`NOLOGIN` | <span class="version-tag">New in v20.1:</span> Allow or disallow a role to login with one of the [client authentication methods](authentication.html#client-authentication). <br><br>By default, the parameter is set to `nologin` for the `CREATE ROLE` statement.
Change nologin to NOLOGIN.
v20.1/create-user.md, line 43 at r9 (raw file):
-----------|------------- `user_name` | The name of the user you want to create.<br><br>Usernames are case-insensitive; must start with a letter, number, or underscore; must contain only letters, numbers, or underscores; and must be between 1 and 63 characters. `password` | Let the user [authenticate their access to a secure cluster](#user-authentication) using this password. Passwords must be entered as [string](string.html) values surrounded by single quotes (`'`).<br><br>Password creation is supported only in secure clusters.
Do you need to update the password description to match other statements, or is this different intentionally?
v20.1/create-user.md, line 60 at r9 (raw file):
Password creation is supported only in secure clusters. - [**GSSAPI authentication**](gssapi_authentication.html), which is available to [Enterprise users](enterprise-licensing.html).
nit: We don't bold the other options in the list, so we should either take the bold away here or bold the others.
v20.1/grant.md, line 19 at r9 (raw file):
The user granting privileges must have the `GRANT` privilege on the target databases or tables. <span class="version-tag">New in v20.1</span> In addition to the `GRANT` privilege, the user granting privileges must have the privilege being granted on the target database or tables. For example, a user granting the `SELECT` privilege on a table to another user must have the `GRANT` and `SELECT` privileges on that table.
I'd condense this a bit:
<span class="version-tag">New in v20.1:</span> The user granting privileges must also have the privilege being granted on the target database or tables. For example, a user granting the `SELECT` privilege on a table to another user must have the `GRANT` and `SELECT` privileges on that table.
v20.1/grant.md, line 45 at r9 (raw file):
`DELETE` | Table `UPDATE` | Table <span class="version-tag">New in v20.1</span> `ZONECONFIG` | Database, Table
We need to regenerate the sql diagram to show this option. Feel free to handle this as a follow-up to this PR, though.
v20.1/grant.md, line 152 at r9 (raw file):
This heading doesn't really say what's happening here. Based on the sentence at the end of this example, I suggest:
Grant the privilege to manage the replication zones for a database or table
v20.1/gssapi_authentication.md, line 145 at r9 (raw file):
~~~ Setting the `server.host_based_authentication.configuration` [cluster setting](cluster-settings.html) cluster setting to this particular value makes it mandatory for all non-`root` users to authenticate using GSSAPI. The `root` user is always an exception and remains able to authenticate using a valid client cert or a user password.
Remove second cluster setting.
There was a problem hiding this comment.
Reviewable status:
v20.1/alter-role.md, line 19 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Do we need a new in 20.1 callout here? This entire statement is new...
Done.
v20.1/alter-role.md, line 37 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Again, not sure this needs to be called out as new since the entire statement is new. I think this and the following parameter do warrant callout as new in the docs for creating roles, though.
Done.
v20.1/alter-role.md, line 38 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Change
nologintoNOLOGIN.
Done.
v20.1/alter-role.md, line 39 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Same comment about the new in 20.1 callout.
Done.
v20.1/alter-user.md, line 38 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Change
nologintoNOLOGIN.
Done.
v20.1/create-role.md, line 41 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Same comments as before.
Done.
v20.1/create-user.md, line 43 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Do you need to update the
passworddescription to match other statements, or is this different intentionally?
Needed to update the description. Done.
v20.1/grant.md, line 19 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
I'd condense this a bit:
<span class="version-tag">New in v20.1:</span> The user granting privileges must also have the privilege being granted on the target database or tables. For example, a user granting the `SELECT` privilege on a table to another user must have the `GRANT` and `SELECT` privileges on that table.
Done.
v20.1/grant.md, line 45 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
We need to regenerate the sql diagram to show this option. Feel free to handle this as a follow-up to this PR, though.
Yep.
v20.1/grant.md, line 152 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
This heading doesn't really say what's happening here. Based on the sentence at the end of this example, I suggest:
Grant the privilege to manage the replication zones for a database or table
Done.
v20.1/gssapi_authentication.md, line 145 at r9 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Remove second
cluster setting.
Done.
v20.1/show-users.md, line 21 at r8 (raw file):
Previously, knz (kena) wrote…
ditto
Done.
ericharmeling
force-pushed
the
granular_roles
branch
from
72c845e
to
6fd3917
Compare
Closes #7034 #6586 #6605 #6882 #6890 #6876 #5960 #6622 #6804 #6878
Summary of changes:
ALTER ROLEdocrootuserCREATEROLEzoneconfigcc @aaron-crl for visibility