CRDB authorization overview doc overhaul #6332
Conversation
|
This change is |
Amruta-Ranade
force-pushed
the
authorization_docs
branch
from
84e1643
to
c5422ac
Compare
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @Amruta-Ranade, @kannanlakshmi, and @taroface)
v20.1/authorization.md, line 10 at r1 (raw file):
User authorization is the act of defining access policies for authenticated CockroachDB users. CockroachDB allows you to create, manage, and remove your cluster's [users](#sql-users) and assign SQL-level [privileges](#assign-privileges) to the users. Additionally, if you have an [Enterprise license](get-started-with-enterprise-trial.html), you can use [role-based access management (RBAC)](#roles) for simplified user management. ## SQL Users
SQL users
(sentence case, right?)
v20.1/authorization.md, line 12 at r1 (raw file):
## SQL Users A SQL user can interact with a CockroachDB database using the [built-in SQL shell](https://www.cockroachlabs.com/docs/stable/cockroach-sql.html) or through an application.
Should this be a relative link?
v20.1/authorization.md, line 21 at r1 (raw file):
{{site.data.alerts.callout_info}} By default, a new user belongs to the `public` role and does not have any privileges except the privileges assigned to the `public` role. For more information, see [Public role](#public-role).
Suggest "and has no privileges other than those assigned to the �public role."
v20.1/authorization.md, line 24 at r1 (raw file):
{{site.data.alerts.end}} ### `root` user
Just checking that this renders correctly as an H3?
v20.1/authorization.md, line 36 at r1 (raw file):
{{site.data.alerts.callout_info}} PostgresSQL considers users and roles as the same thing. CockroachDB, however, has different concepts of users and roles.
typo: PostgreSQL
It's unclear to me if "users" and "roles" both exist on PostgreSQL but are functionally identical, or if they are one entity that uses the two terms interchangeably.
Assuming it's the latter, I would suggest "PostgreSQL uses the terms "users" and "roles" interchangeably."
If the former, I suggest something like "PostgreSQL considers users and roles as equivalent entities" or "as synonymous".
v20.1/authorization.md, line 56 at r1 (raw file):
#### `admin` role The admin role is created by default and cannot be dropped. Users belonging to the `admin` role have all privileges for all database objects across the cluster. The `root` user belongs to the `admin` role by default.
admin role
v20.1/authorization.md, line 58 at r1 (raw file):
The admin role is created by default and cannot be dropped. Users belonging to the `admin` role have all privileges for all database objects across the cluster. The `root` user belongs to the `admin` role by default. A `superuser` or `admin` user is a member of the admin role. Only superusers can [`CREATE ROLE`](create-role.html) or [`DROP ROLE`](drop-role.html).
admin role
v20.1/authorization.md, line 71 at r1 (raw file):
{{site.data.alerts.callout_info}} The terms “`admin` role” and “role admin” can be confusing. Use the `admin role` if you want the SQL user to have privileges across cluster. Use the `role admin` if you want to limit the SQL user’s privileges to that role, but with an option to grant or revoke role membership to other users.
Suggest "Assign the admin role" for clarity (note formatting change)
Suggest "Assign the role admin" (note formatting change -- unless role admin is a platform-specific term, in which case role admin should also be used in the text above this callout)
With the latter, I'm unclear what "that role" refers to. Maybe "limit the SQL user's privileges to its current role, but with the ability to grant or revoke role membership..."?
v20.1/authorization.md, line 77 at r1 (raw file):
A user or role that is an immediate member of the role. Example: A is a member of B.
Add a line break before this? Also works as one line.
If you want there to be a line break without a space, add two spaces " " after the line above.
v20.1/authorization.md, line 82 at r1 (raw file):
A user or role that is a member of the role by association. Example: A is a member of C ... is a member of B where "..." is an arbitrary number of memberships.
Same as above.
v20.1/authorization.md, line 97 at r1 (raw file):
{{site.data.alerts.callout_info}} The user does not get privileges to existing tables in the database. To grant privileges to a user on all existing tables in a database, see [Grant privileges on all tables in a database](https://www.cockroachlabs.com/docs/stable/grant.html#grant-privileges-on-all-tables-in-a-database)
Relative link here
v20.1/authorization.md, line 150 at r1 (raw file):
~~~ 2. Create the database admin (named `db_admin`) who can perform all database operations for existing tables as well as tables added in the future:
"as well as for tables added..."
v20.1/authorization.md, line 253 at r1 (raw file):
Let's say we want to create the following access control setup for the `movr` database: - Two database admins (named `db_admin_1` and `db_admin_2`) who can perform all database operations for existing tables as well as tables added in the future.
"as well as for tables added..."
v20.1/authorization.md, line 257 at r1 (raw file):
- Five users (named `report_user_1`, `report_user_2`, `report_user_3`, `report_user_4`, `report_user_5`) who can only read the `vehicles` table. 1. Use the [`cockroach demo`](cockroach-demo.html) command to load the `movr` database and dataset into a CockroachDB cluster.:
extra period before the ending colon
v20.1/authorization.md, line 264 at r1 (raw file):
~~~ Each `cockroach demo` instance comes with a temporary enterprise license which enables you to try out enterprise features such as role-based access control. The license expires after an hour.
"role-based access management"?
Also, should this link to authorization.html#create-and-manage-roles or is that excessive?
v20.1/authorization.md, line 266 at r1 (raw file):
Each `cockroach demo` instance comes with a temporary enterprise license which enables you to try out enterprise features such as role-based access control. The license expires after an hour. 2. Create the database admin role (named `db_admin_role`) whose members can perform all database operations for existing tables as well as tables added in the future:
"as well as for tables..."
v20.1/grant.md, line 26 at r1 (raw file):
{{site.data.alerts.callout_info}} The user does not get privileges to existing tables in the database. To grant privileges to a user on all existing tables in a database, see [Grant privileges on all tables in a database](https://www.cockroachlabs.com/docs/stable/grant.html#grant-privileges-on-all-tables-in-a-database)
Relative link?
|
I reviewed this at a high level and it LGTM. I am however not a SQL roles expert and only filling in till we find someone to backfill the previous PM. So I am hesitant to be considered an expert reviewer in this case. Are you still comfortable merging given that? Sorry that's not very helpful... |
Yep! That's perfect, actually, since the users won't be CRDB roles experts :) Did the example help in understanding how to set users/privileges/roles? Did the steps work? |
There was a problem hiding this comment.
Reviewable status:
v20.1/authorization.md, line 10 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
SQL users
(sentence case, right?)
Done.
v20.1/authorization.md, line 12 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Should this be a relative link?
Gah..yes. I always forget that. Done.
v20.1/authorization.md, line 21 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Suggest "and has no privileges other than those assigned to the �
publicrole."
Done.
v20.1/authorization.md, line 36 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
typo: PostgreSQL
It's unclear to me if "users" and "roles" both exist on PostgreSQL but are functionally identical, or if they are one entity that uses the two terms interchangeably.
Assuming it's the latter, I would suggest "PostgreSQL uses the terms "users" and "roles" interchangeably."
If the former, I suggest something like "PostgreSQL considers users and roles as equivalent entities" or "as synonymous".
Rephrased for clarity.
v20.1/authorization.md, line 56 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
adminrole
Done.
v20.1/authorization.md, line 58 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
adminrole
Done.
v20.1/authorization.md, line 71 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Suggest "Assign the
adminrole" for clarity (note formatting change)Suggest "Assign the role admin" (note formatting change -- unless
role adminis a platform-specific term, in which caserole adminshould also be used in the text above this callout)With the latter, I'm unclear what "that role" refers to. Maybe "limit the SQL user's privileges to its current role, but with the ability to grant or revoke role membership..."?
Done.
v20.1/authorization.md, line 77 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Add a line break before this? Also works as one line.
If you want there to be a line break without a space, add two spaces " " after the line above.
Done.
v20.1/authorization.md, line 82 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Same as above.
Done.
v20.1/authorization.md, line 97 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Relative link here
Done.
v20.1/authorization.md, line 150 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
"as well as for tables added..."
Done.
v20.1/authorization.md, line 253 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
"as well as for tables added..."
Done.
v20.1/authorization.md, line 257 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
extra period before the ending colon
Done.
v20.1/authorization.md, line 264 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
"role-based access management"?
Also, should this link to authorization.html#create-and-manage-roles or is that excessive?
Done.
v20.1/authorization.md, line 266 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
"as well as for tables..."
Done.
v20.1/grant.md, line 26 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Relative link?
Done.
There was a problem hiding this comment.
Thanks, Amruta. Some comments/questions for you. Haven't reviewed the example yet. Will get to that soon.
Reviewable status:
v20.1/authorization.md, line 36 at r1 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
Rephrased for clarity.
Why are we adding this note about postgres? Have we seen questions/confusions fromo users? If not, I'd leave it out. If we do include it, though, the expression "database user" should just be "user". Also, in first sentence, a database users should be a database user, I think.
v20.1/authorization.md, line 41 at r2 (raw file):
### Create and manage roles To create and manage your cluster's roles, use the following statements:
It's odd that we're introducing the relevant SQL statements here as a bulleted list, whereas we introduce them in paragraph form for uses. I like providing a summary of each statement's purpose, as you do for users. Let's do that here as well. Instead of paragraph form, perhaps a table would be better, though.
v20.1/authorization.md, line 53 at r2 (raw file):
The
adminrole andpublicroles...
should be:
The
adminandpublicroles...
For clarity, the second sentences, should probably start:
However, only Enterprise customers can...
v20.1/authorization.md, line 56 at r2 (raw file):
#### `admin` role The `admin` role is created by default and cannot be dropped. Users belonging to the `admin` role have all privileges for all database objects across the cluster. The `root` user belongs to the `admin` role by default.
Just above, you said that Enterprise customers can change the privileges for the admin role. If so, should the second sentence here clarify that admin gets all privileges by default?
v20.1/authorization.md, line 58 at r2 (raw file):
Do we need to call out an "admin user" entity? Is this something specific, or is this just our description of a user with the admin role? If we don't want to talk about "admin" users, this text could be something like:
Only users belonging to the
adminrole can create and remove additional roles viaCREATE ROLEandDROP ROLE.
v20.1/authorization.md, line 62 at r2 (raw file):
#### `public` role All new users and roles belong to the `public` role by default and can create and manage database objects in the `public` schema. Enterprise customers can grant and revoke the privileges on the `public` role to manage access to the public schema.
I find this role confusing. Can you check these docs about schemas and see if you can clarify? Based on that doc, the pubic schema is really the only schema that database and tables can be created in. Is it accurate to say that "All new users" can "create and manage database objects in the public schema"? https://www.cockroachlabs.com/docs/v19.2/sql-name-resolution.html#logical-schemas-and-namespaces
v20.1/authorization.md, line 68 at r2 (raw file):
#### Role admin A `role admin` is a member of the role that's allowed to modify role membership. To create a `role admin`, use [`WITH ADMIN OPTION`](https://www.cockroachlabs.com/docs/stable/grant-roles.html#grant-the-admin-option).
Can you clarify what "modify role membership" means? Probably just rephrase that to be more specific.
v20.1/authorization.md, line 70 at r2 (raw file):
A `role admin` is a member of the role that's allowed to modify role membership. To create a `role admin`, use [`WITH ADMIN OPTION`](https://www.cockroachlabs.com/docs/stable/grant-roles.html#grant-the-admin-option). {{site.data.alerts.callout_info}}
I'd make this a tip.
v20.1/authorization.md, line 71 at r2 (raw file):
{{site.data.alerts.callout_info}} The terms “`admin` role” and “`role admin`” can be confusing. Assign the `admin` role to a SQL user if you want the user to have privileges across cluster. Make a SQL user the `role admin` if you want to limit the user’s privileges to its current role, but with an option to grant or revoke role membership to other users.
privileges across cluster > privileges across the cluster
But I don't know exactly what that's trying to say. Perhaps rephrase to be more specific?
v20.1/authorization.md, line 94 at r2 (raw file):
Use the [`GRANT <privileges>`](grant.html) and [`REVOKE <privileges>`](revoke.html) to manage privileges for users and roles (for enterprise customers). The following rules apply when roles and users are granted privileges or inherit privileges:
These aren't all rules, really.
v20.1/authorization.md, line 99 at r2 (raw file):
{{site.data.alerts.callout_info}} The user does not get privileges to existing tables in the database. To grant privileges to a user on all existing tables in a database, see [Grant privileges on all tables in a database](grant.html#grant-privileges-on-all-tables-in-a-database)
I'd just make this a second sentence in the bullet above. Right now, it's a continuation of the first sentence, which is odd for a callout.
v20.1/authorization.md, line 120 at r2 (raw file):
`UPDATE` | Table ## Security best practices
I'd rename this "Authentication best practices". Also, we should probably refactor the security part of the production checklist to cover authentication and authorization best practices like this. http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/d9eeda7a102794c8e649cabb5d35badf8b1ed7a9/v20.1/recommended-production-settings.html#security
v20.1/authorization.md, line 124 at r2 (raw file):
Let's make this active:
Use the
rootuser only for database administration tasks... Do not use therootuser for applications; instead, create users with specific privileges...
v20.1/authorization.md, line 125 at r2 (raw file):
I'd revise this as follows:
Enterprise customers: Create roles with specific privileges, create users, and then add the users to the relevant roles.
v20.1/authorization.md, line 126 at r2 (raw file):
- The `root` user should be used only for database administration tasks such as creating and managing other users, creating and managing roles (for enterprise customers), and creating and managing databases. The `root` user should not be used by the applications. Instead, we recommend that you create users with specific privileges based on your application’s access requirements. - For enterprise customers, we recommend that you create roles with specific privileges, create users, and then assign the roles to the users. - Use the `least privilege model` to grant privileges to users and roles.
What does this mean? Is there a wikipedia article to link to or should we just paragraph what this means? I'd make this the second bullet.
There was a problem hiding this comment.
Reviewable status:
v20.1/authorization.md, line 53 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
The
adminrole andpublicroles...should be:
The
adminandpublicroles...For clarity, the second sentences, should probably start:
However, only Enterprise customers can...
Core users can also assign privileges to the public role. Neither can modify the admin role, at least to my knowledge.
v20.1/authorization.md, line 58 at r2 (raw file):
The `admin` role is created by default and cannot be dropped. Users belonging to the `admin` role have all privileges for all database objects across the cluster. The `root` user belongs to the `admin` role by default. An `admin` user is a member of the `admin` role. Only admin users can use [`CREATE ROLE`](create-role.html) and [`DROP ROLE`](drop-role.html).
This is true, but just as a heads up it will be changing in 20.1. We are introducing a CREATEROLE privilege which gives non-admin users the ability to create and drop roles. Work for this is in progress.
v20.1/authorization.md, line 62 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
I find this role confusing. Can you check these docs about schemas and see if you can clarify? Based on that doc, the
pubicschema is really the only schema that database and tables can be created in. Is it accurate to say that "All new users" can "create and manage database objects in the public schema"? https://www.cockroachlabs.com/docs/v19.2/sql-name-resolution.html#logical-schemas-and-namespaces
Amruta and I discussed this in person but just to document: the public role is unrelated to the public schema. It's just a built-in role which all users automatically belong to. Both Core and Enterprise users can grant and revoke privileges on this role.
v20.1/authorization.md, line 76 at r2 (raw file):
#### Direct member A user or role that is an immediate member of the role.
Roles cannot be members of roles.
v20.1/authorization.md, line 92 at r2 (raw file):
### Assign privileges Use the [`GRANT <privileges>`](grant.html) and [`REVOKE <privileges>`](revoke.html) to manage privileges for users and roles (for enterprise customers).
Non-enterprise users can do this too, only on the public role.
v20.1/authorization.md, line 115 at r2 (raw file):
`DROP` | Database, Table `GRANT` | Database, Table `SELECT` | Table
This is a bit confusing. A reader might interpret this to mean you can't grant SELECT (or the following privileges) on a database, but that is not the case. You can do so, and it means that any new tables created in the database will inherit that grant.
There was a problem hiding this comment.
Reviewable status:
v20.1/authorization.md, line 24 at r1 (raw file):
Previously, taroface (Ryan Kuo) wrote…
Just checking that this renders correctly as an H3?
Done.
v20.1/authorization.md, line 36 at r1 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Why are we adding this note about postgres? Have we seen questions/confusions fromo users? If not, I'd leave it out. If we do include it, though, the expression "database user" should just be "user". Also, in first sentence,
a database usersshould bea database user, I think.
I think it'd be important for people coming from postgres to know that there's a difference. it certainly confused me when I was trying to understand how postgres auth works and differs from CRDB. That's my personal opinion though. Happy to delete the note if you think that's okay.
v20.1/authorization.md, line 41 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
It's odd that we're introducing the relevant SQL statements here as a bulleted list, whereas we introduce them in paragraph form for uses. I like providing a summary of each statement's purpose, as you do for users. Let's do that here as well. Instead of paragraph form, perhaps a table would be better, though.
Done.
v20.1/authorization.md, line 53 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
The
adminrole andpublicroles...should be:
The
adminandpublicroles...For clarity, the second sentences, should probably start:
However, only Enterprise customers can...
Done.
v20.1/authorization.md, line 56 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Just above, you said that Enterprise customers can change the privileges for the
adminrole. If so, should the second sentence here clarify thatadmingets all privileges by default?
Done.
v20.1/authorization.md, line 58 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Do we need to call out an "admin user" entity? Is this something specific, or is this just our description of a user with the admin role? If we don't want to talk about "admin" users, this text could be something like:
Only users belonging to the
adminrole can create and remove additional roles viaCREATE ROLEandDROP ROLE.
"Admin user" is a term that is more prominent for CC than CRDB. Establishing this term here allows me to refer to it in CC docs.
v20.1/authorization.md, line 62 at r2 (raw file):
https://www.cockroachlabs.com/docs/v19.2/sql-name-resolution.html#logical-schemas-and-namespaces
AIUI,publicschema is accessible to all users, and schemas created by users is restricted as per privileges. I might be wrong though -- will check with engg.
Update: Meeting with Solon: "All new users to the public role by default. You can grant and revoke the privileges on public role to manage access to databases and tables." <Link to https://www.cockroachlabs.com/docs/stable/grant.html#make-a-table-readable-to-every-user-in-the-system>
v20.1/authorization.md, line 68 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Can you clarify what "modify role membership" means? Probably just rephrase that to be more specific.
Done.
v20.1/authorization.md, line 70 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
I'd make this a tip.
Done.
v20.1/authorization.md, line 94 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
These aren't all rules, really.
Done.
v20.1/authorization.md, line 99 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
I'd just make this a second sentence in the bullet above. Right now, it's a continuation of the first sentence, which is odd for a callout.
Done.
v20.1/authorization.md, line 120 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
I'd rename this "Authentication best practices". Also, we should probably refactor the security part of the production checklist to cover authentication and authorization best practices like this. http://cockroach-docs-review.s3-website-us-east-1.amazonaws.com/d9eeda7a102794c8e649cabb5d35badf8b1ed7a9/v20.1/recommended-production-settings.html#security
Done. And I agree. Opened an issue to track this.
v20.1/authorization.md, line 124 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
Let's make this active:
Use the
rootuser only for database administration tasks... Do not use therootuser for applications; instead, create users with specific privileges...
Done.
v20.1/authorization.md, line 125 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
I'd revise this as follows:
Enterprise customers: Create roles with specific privileges, create users, and then add the users to the relevant roles.
Done.
v20.1/authorization.md, line 126 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
What does this mean? Is there a wikipedia article to link to or should we just paragraph what this means? I'd make this the second bullet.
Done.
|
v20.1/authorization.md, line 76 at r2 (raw file): Previously, solongordon (Solon) wrote…
@solongordon I tried creating one role and assigning it to another role and it worked 🤔 Here's what I tried: |
There was a problem hiding this comment.
Reviewable status:
v20.1/authorization.md, line 71 at r2 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
privileges across cluster>privileges across the clusterBut I don't know exactly what that's trying to say. Perhaps rephrase to be more specific?
Done.
v20.1/authorization.md, line 115 at r2 (raw file):
Previously, solongordon (Solon) wrote…
This is a bit confusing. A reader might interpret this to mean you can't grant
SELECT(or the following privileges) on a database, but that is not the case. You can do so, and it means that any new tables created in the database will inherit that grant.
Hmm..good point. I wonder what the original docs meant here. Deleting the table for now .
There was a problem hiding this comment.
Fixes for my comments look good! Sorry for the confusion about role hierarchies.
Reviewable status:
v20.1/authorization.md, line 76 at r2 (raw file):
Previously, Amruta-Ranade (Amruta Ranade) wrote…
@solongordon I tried creating one role and assigning it to another role and it worked 🤔
Here's what I tried:
CREATE ROLE dev_ops; create role dev_test; grant dev_test to dev_ops;show grants on role dev_test; role_name | member | is_admin +-----------+---------+----------+ dev_test | dev_ops | false
My mistake!
|
TFTR, @solongordon !! |
There was a problem hiding this comment.
Reviewable status:
v20.1/authorization.md, line 156 at r4 (raw file):
{% include copy-clipboard.html %} ~~~ sql > create user db_admin;
We generally capitalize SQL commands/keywords:
CREATE USER db_admin
Applies to all subsequent steps as well.
There was a problem hiding this comment.
Reviewable status:
v20.1/authorization.md, line 156 at r4 (raw file):
Previously, jseldess (Jesse Seldess) wrote…
We generally capitalize SQL commands/keywords:
CREATE USER db_adminApplies to all subsequent steps as well.
Done.
Closes #5925
To-Do: Backport to 19.2