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

rfcs: rbac extensions and system privileges #80580

Merged

Conversation

RichardJCai
Copy link
Contributor

Release note: None

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@RichardJCai RichardJCai force-pushed the rfc_rbac_extensions_system_privileges branch 2 times, most recently from 50b80f2 to f10f658 Compare April 26, 2022 19:31
@RichardJCai RichardJCai marked this pull request as ready for review April 26, 2022 19:31
@RichardJCai RichardJCai requested a review from a team as a code owner April 26, 2022 19:31
@RichardJCai RichardJCai requested review from a team and adityamaru and removed request for a team April 26, 2022 19:31
@RichardJCai RichardJCai force-pushed the rfc_rbac_extensions_system_privileges branch from f10f658 to 507a490 Compare April 26, 2022 21:55
Copy link
Contributor

@otan otan left a comment

Choose a reason for hiding this comment

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

passing look as i was curious

Another relevant use case is that for Cockroach Cloud we want to be able to define an “operator” user which is a step below admin, (granting admin generally violates the least privilege principle). As of right now, the operator role is given `CREATEROLE CREATELOGIN CREATEDB CONTROLJOB CANCELQUERY VIEWACTIVITY CONTROLCHANGEFEED` role options, the problem is, granting the operator role to a user does not actually allow the user to perform operator abilities due to role options not being inherited. The user must login as the operator role itself.

# Technical Design
Technical Design
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: doubled up on technical design

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


The problem with role options are that they are not inheritable, this can be considered an anti-pattern with regards to RBAC. Generally in the RBAC model, when a role is granted a privilege, granting that role to another role will also give the role that privilege. With role options we currently do not support inheritance due to ambiguous inheritance semantics.

This has led to customer issues: https://github.com/cockroachlabs/support/issues/1217 , https://github.com/cockroachdb/cockroach/issues/79571.
Copy link
Contributor

Choose a reason for hiding this comment

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

we probably shouldnt link customer issues in this public facing doc
also use [#79571](https://github.com/cockroachdb/cockroach/issues/79571)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair, I'll just make a comment of the issue instead

I think we do have code that links to internal issues though but it shouldn't be an issue since it's private anyway

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

# Technical Design
Technical Design
The goal here is to extend our current privilege system to allow more granular grants on objects / entities that are not necessarily backed by a descriptor such as jobs and changefeed objects.
Certain role options should be replaced* with grants. At a high level, we want to introduce “system” or “global” level grants that follow general inheritance rules. This is a concept that exists in many popular non-Postgres databases. (MySQL, Oracle, SQL Server, Db2). We also want to support more granular grants on jobs and bulkio operations.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: replaced* -> replaced

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

## New System Table

We will create a new “system.privileges” table.
`
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: use "```sql"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Results in the row
(foo, 1, “system”, 16384, 0)

The bitmask value for MODIFYCLUSTERSETTING is assumed to be 2 << 13 = 16384 in the above example.
Copy link
Contributor

@otan otan Apr 27, 2022

Choose a reason for hiding this comment

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

nit: do bitmasks usually make more sense as 1 << 14 and 1 << 15?

in my head, visualising 0b1 << n which is easier to visualise than 0b10 << n, unless i'm missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

### Alternate considerations
The system level grants could be stored on a PrivilegeDescriptor on the system database, however this would require us to support mutations on the system database descriptor which we currently do not do and this allows us to not perform a round trip whenever we look for a system table.

We could create descriptors to represent every object we want to grant on. This is not an attractive option at first glance as these descriptors will likely only be used to store privileges and nothing more.
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to store privileges with the object_type, e.g. for jobs, add an extra column on system.jobs with a privilege protobuf? that way everything is tied together, but i guess the logic can get more confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I guess an alternative is to make a new system table for each object type?
I don't think system.jobs makes sense to add privileges to since it's not exactly per job type.

The downside of a new system table per object type is that if another team wants to add a new object type, instead of adding to an enum they'd have to create a whole new table.

The bitmask value for VIEWCLUSTERSETTING is assumed to be 2 << 14 = 32768 in the above example. 32768 + 16384 = 49152.

## Support for grants on virtual tables
One thing worth explicitly calling out is that users have asked for the ability to grant on virtual tables (crdb_internal, pg_catalog). This proposal allows us to support grants on virtual tables and unify the grant experience for tables. Virtual tables have OIDs starting at `MaxUint32 = 1<<32 - 1` counting backwards, an INT (64 bit) column for object id supports this case.
Copy link
Contributor

Choose a reason for hiding this comment

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

are virtual table oids fixed? i don't think they are today and we have shifted them between versions.

in particular, note

const (
CrdbInternalID = math.MaxUint32 - iota
CrdbInternalBackwardDependenciesTableID
CrdbInternalBuildInfoTableID
CrdbInternalBuiltinFunctionsTableID
CrdbInternalClusterContendedIndexesViewID
CrdbInternalClusterContendedKeysViewID
CrdbInternalClusterContendedTablesViewID
CrdbInternalClusterContentionEventsTableID
CrdbInternalClusterDistSQLFlowsTableID
CrdbInternalClusterLocksTableID
CrdbInternalClusterQueriesTableID
CrdbInternalClusterTransactionsTableID
CrdbInternalClusterSessionsTableID
CrdbInternalClusterSettingsTableID
CrdbInternalClusterStmtStatsTableID
CrdbInternalClusterTxnStatsTableID
CrdbInternalCreateSchemaStmtsTableID
CrdbInternalCreateStmtsTableID
CrdbInternalCreateTypeStmtsTableID
CrdbInternalDatabasesTableID
CrdbInternalFeatureUsageID
CrdbInternalForwardDependenciesTableID
CrdbInternalKVNodeLivenessTableID
CrdbInternalGossipNodesTableID
CrdbInternalGossipAlertsTableID
CrdbInternalGossipLivenessTableID
CrdbInternalGossipNetworkTableID
CrdbInternalTransactionContentionEvents
CrdbInternalIndexColumnsTableID
CrdbInternalIndexUsageStatisticsTableID
CrdbInternalInflightTraceSpanTableID
CrdbInternalJobsTableID
CrdbInternalKVNodeStatusTableID
CrdbInternalKVStoreStatusTableID
CrdbInternalLeasesTableID
CrdbInternalLocalContentionEventsTableID
CrdbInternalLocalDistSQLFlowsTableID
CrdbInternalLocalQueriesTableID
CrdbInternalLocalTransactionsTableID
CrdbInternalLocalSessionsTableID
CrdbInternalLocalMetricsTableID
CrdbInternalNodeStmtStatsTableID
CrdbInternalNodeTxnStatsTableID
CrdbInternalPartitionsTableID
CrdbInternalPredefinedCommentsTableID
CrdbInternalRangesNoLeasesTableID
CrdbInternalRangesViewID
CrdbInternalRuntimeInfoTableID
CrdbInternalSchemaChangesTableID
CrdbInternalSessionTraceTableID
CrdbInternalSessionVariablesTableID
CrdbInternalStmtStatsTableID
CrdbInternalTableColumnsTableID
CrdbInternalTableIndexesTableID
CrdbInternalTablesTableID
CrdbInternalTablesTableLastStatsID
CrdbInternalTransactionStatsTableID
CrdbInternalTxnStatsTableID
CrdbInternalZonesTableID
CrdbInternalInvalidDescriptorsTableID
CrdbInternalClusterDatabasePrivilegesTableID
CrdbInternalCrossDbRefrences
CrdbInternalLostTableDescriptors
CrdbInternalClusterInflightTracesTable
CrdbInternalRegionsTable
CrdbInternalDefaultPrivilegesTable
CrdbInternalActiveRangeFeedsTable
CrdbInternalTenantUsageDetailsViewID
CrdbInternalPgCatalogTableIsImplementedTableID
CrdbInternalSuperRegions
InformationSchemaID
InformationSchemaAdministrableRoleAuthorizationsID
InformationSchemaApplicableRolesID
InformationSchemaAttributesTableID
InformationSchemaCharacterSets
InformationSchemaCheckConstraintRoutineUsageTableID
InformationSchemaCheckConstraints
InformationSchemaCollationCharacterSetApplicability
InformationSchemaCollations
InformationSchemaColumnColumnUsageTableID
InformationSchemaColumnDomainUsageTableID
InformationSchemaColumnOptionsTableID
InformationSchemaColumnPrivilegesID
InformationSchemaColumnStatisticsTableID
InformationSchemaColumnUDTUsageID
InformationSchemaColumnsExtensionsTableID
InformationSchemaColumnsTableID
InformationSchemaConstraintColumnUsageTableID
InformationSchemaConstraintTableUsageTableID
InformationSchemaDataTypePrivilegesTableID
InformationSchemaDomainConstraintsTableID
InformationSchemaDomainUdtUsageTableID
InformationSchemaDomainsTableID
InformationSchemaElementTypesTableID
InformationSchemaEnabledRolesID
InformationSchemaEnginesTableID
InformationSchemaEventsTableID
InformationSchemaFilesTableID
InformationSchemaForeignDataWrapperOptionsTableID
InformationSchemaForeignDataWrappersTableID
InformationSchemaForeignServerOptionsTableID
InformationSchemaForeignServersTableID
InformationSchemaForeignTableOptionsTableID
InformationSchemaForeignTablesTableID
InformationSchemaInformationSchemaCatalogNameTableID
InformationSchemaKeyColumnUsageTableID
InformationSchemaKeywordsTableID
InformationSchemaOptimizerTraceTableID
InformationSchemaParametersTableID
InformationSchemaPartitionsTableID
InformationSchemaPluginsTableID
InformationSchemaProcesslistTableID
InformationSchemaProfilingTableID
InformationSchemaReferentialConstraintsTableID
InformationSchemaResourceGroupsTableID
InformationSchemaRoleColumnGrantsTableID
InformationSchemaRoleRoutineGrantsTableID
InformationSchemaRoleTableGrantsID
InformationSchemaRoleUdtGrantsTableID
InformationSchemaRoleUsageGrantsTableID
InformationSchemaRoutinePrivilegesTableID
InformationSchemaRoutineTableID
InformationSchemaSQLFeaturesTableID
InformationSchemaSQLImplementationInfoTableID
InformationSchemaSQLPartsTableID
InformationSchemaSQLSizingTableID
InformationSchemaSchemataExtensionsTableID
InformationSchemaSchemataTableID
InformationSchemaSchemataTablePrivilegesID
InformationSchemaSequencesID
InformationSchemaSessionVariables
InformationSchemaStGeometryColumnsTableID
InformationSchemaStSpatialReferenceSystemsTableID
InformationSchemaStUnitsOfMeasureTableID
InformationSchemaStatisticsTableID
InformationSchemaTableConstraintTableID
InformationSchemaTableConstraintsExtensionsTableID
InformationSchemaTablePrivilegesID
InformationSchemaTablesExtensionsTableID
InformationSchemaTablesTableID
InformationSchemaTablespacesExtensionsTableID
InformationSchemaTablespacesTableID
InformationSchemaTransformsTableID
InformationSchemaTriggeredUpdateColumnsTableID
InformationSchemaTriggersTableID
InformationSchemaTypePrivilegesID
InformationSchemaUdtPrivilegesTableID
InformationSchemaUsagePrivilegesTableID
InformationSchemaUserAttributesTableID
InformationSchemaUserDefinedTypesTableID
InformationSchemaUserMappingOptionsTableID
InformationSchemaUserMappingsTableID
InformationSchemaUserPrivilegesID
InformationSchemaViewColumnUsageTableID
InformationSchemaViewRoutineUsageTableID
InformationSchemaViewTableUsageTableID
InformationSchemaViewsTableID
PgCatalogID
PgCatalogAggregateTableID
PgCatalogAmTableID
PgCatalogAmopTableID
PgCatalogAmprocTableID
PgCatalogAttrDefTableID
PgCatalogAttributeTableID
PgCatalogAuthIDTableID
PgCatalogAuthMembersTableID
PgCatalogAvailableExtensionVersionsTableID
PgCatalogAvailableExtensionsTableID
PgCatalogCastTableID
PgCatalogClassTableID
PgCatalogCollationTableID
PgCatalogConfigTableID
PgCatalogConstraintTableID
PgCatalogConversionTableID
PgCatalogCursorsTableID
PgCatalogDatabaseTableID
PgCatalogDbRoleSettingTableID
PgCatalogDefaultACLTableID
PgCatalogDependTableID
PgCatalogDescriptionTableID
PgCatalogEnumTableID
PgCatalogEventTriggerTableID
PgCatalogExtensionTableID
PgCatalogFileSettingsTableID
PgCatalogForeignDataWrapperTableID
PgCatalogForeignServerTableID
PgCatalogForeignTableTableID
PgCatalogGroupTableID
PgCatalogHbaFileRulesTableID
PgCatalogIndexTableID
PgCatalogIndexesTableID
PgCatalogInheritsTableID
PgCatalogInitPrivsTableID
PgCatalogLanguageTableID
PgCatalogLargeobjectMetadataTableID
PgCatalogLargeobjectTableID
PgCatalogLocksTableID
PgCatalogMatViewsTableID
PgCatalogNamespaceTableID
PgCatalogOpclassTableID
PgCatalogOperatorTableID
PgCatalogOpfamilyTableID
PgCatalogPartitionedTableTableID
PgCatalogPoliciesTableID
PgCatalogPolicyTableID
PgCatalogPreparedStatementsTableID
PgCatalogPreparedXactsTableID
PgCatalogProcTableID
PgCatalogPublicationRelTableID
PgCatalogPublicationTableID
PgCatalogPublicationTablesTableID
PgCatalogRangeTableID
PgCatalogReplicationOriginStatusTableID
PgCatalogReplicationOriginTableID
PgCatalogReplicationSlotsTableID
PgCatalogRewriteTableID
PgCatalogRolesTableID
PgCatalogRulesTableID
PgCatalogSecLabelsTableID
PgCatalogSecurityLabelTableID
PgCatalogSequenceTableID
PgCatalogSequencesTableID
PgCatalogSettingsTableID
PgCatalogShadowTableID
PgCatalogSharedDescriptionTableID
PgCatalogSharedSecurityLabelTableID
PgCatalogShdependTableID
PgCatalogShmemAllocationsTableID
PgCatalogStatActivityTableID
PgCatalogStatAllIndexesTableID
PgCatalogStatAllTablesTableID
PgCatalogStatArchiverTableID
PgCatalogStatBgwriterTableID
PgCatalogStatDatabaseConflictsTableID
PgCatalogStatDatabaseTableID
PgCatalogStatGssapiTableID
PgCatalogStatProgressAnalyzeTableID
PgCatalogStatProgressBasebackupTableID
PgCatalogStatProgressClusterTableID
PgCatalogStatProgressCreateIndexTableID
PgCatalogStatProgressVacuumTableID
PgCatalogStatReplicationTableID
PgCatalogStatSlruTableID
PgCatalogStatSslTableID
PgCatalogStatSubscriptionTableID
PgCatalogStatSysIndexesTableID
PgCatalogStatSysTablesTableID
PgCatalogStatUserFunctionsTableID
PgCatalogStatUserIndexesTableID
PgCatalogStatUserTablesTableID
PgCatalogStatWalReceiverTableID
PgCatalogStatXactAllTablesTableID
PgCatalogStatXactSysTablesTableID
PgCatalogStatXactUserFunctionsTableID
PgCatalogStatXactUserTablesTableID
PgCatalogStatioAllIndexesTableID
PgCatalogStatioAllSequencesTableID
PgCatalogStatioAllTablesTableID
PgCatalogStatioSysIndexesTableID
PgCatalogStatioSysSequencesTableID
PgCatalogStatioSysTablesTableID
PgCatalogStatioUserIndexesTableID
PgCatalogStatioUserSequencesTableID
PgCatalogStatioUserTablesTableID
PgCatalogStatisticExtDataTableID
PgCatalogStatisticExtTableID
PgCatalogStatisticTableID
PgCatalogStatsExtTableID
PgCatalogStatsTableID
PgCatalogSubscriptionRelTableID
PgCatalogSubscriptionTableID
PgCatalogTablesTableID
PgCatalogTablespaceTableID
PgCatalogTimezoneAbbrevsTableID
PgCatalogTimezoneNamesTableID
PgCatalogTransformTableID
PgCatalogTriggerTableID
PgCatalogTsConfigMapTableID
PgCatalogTsConfigTableID
PgCatalogTsDictTableID
PgCatalogTsParserTableID
PgCatalogTsTemplateTableID
PgCatalogTypeTableID
PgCatalogUserMappingTableID
PgCatalogUserMappingsTableID
PgCatalogUserTableID
PgCatalogViewsTableID
PgExtensionSchemaID
PgExtensionGeographyColumnsTableID
PgExtensionGeometryColumnsTableID
PgExtensionSpatialRefSysTableID
MinVirtualID = PgExtensionSpatialRefSysTableID
)
and how we keep it in order and in the past have interrupted this order. how are we going to ensure these don't change over time in future if we are relying on it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it would be a non-issue to fix the IDs moving forward right?

Copy link
Contributor

Choose a reason for hiding this comment

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

no. but preferably in a protobuf or something that makes it clear that changing it is bad.

For backing up individual database objects right now, we check for CONNECT on databases, SELECT on tables and USAGE on schemas and types. We can potentially augment these in the future?

## New Jobs related privileges
The ability to interact with jobs hinges on the coarse grained CONTROLJOB role option. It makes sense to define privileges such as CONTROL on individual job types.
Copy link
Contributor

Choose a reason for hiding this comment

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

we are still supporting the coarse grained CONTROLJOB option too right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Likely yes, no immediate plans to deprecate any role options.

- Status: draft
- Start Date: 2022-04-26
- Authors: Richard Cai
- RFC PR: [80580](https://github.com/cockroachdb/cockroach/pull/80580)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: [#80580]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Results in the row
(foo, 1, “system”, 49152, 0)

The bitmask value for VIEWCLUSTERSETTING is assumed to be 2 << 14 = 32768 in the above example. 32768 + 16384 = 49152.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe 1<<14 | 1<<15=49152 is clearer than the +

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


# Technical Design
Technical Design
The goal here is to extend our current privilege system to allow more granular grants on objects / entities that are not necessarily backed by a descriptor such as jobs and changefeed objects.
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to point out that we might be moving toward having a descriptor for changefeed objects.
Would this proposed system worked if something that was not a descriptor backed object becomes a descriptor backed object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd have two options if something that was not descriptor backed became descriptor backed.

  1. We could just leave it as is and continue to resolve privileges from the system table.
  2. Do a migration, which in theory should be fairly straight forward.

Both of the options hinge on the ID staying consistent from when the object that was not backed by a descriptor becomes backed by one.


`system.object_type` will be an enum that defines the various object types that can support privileges in CockroachDB. This tentatively looks like
```
CREATE TYPE system.object_type AS ENUM('system', 'jobs', 'changefeed_sink')
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, I'm a fan of strong types -- such as this system.object_type.

I have concern however around the fact that doing so would make changes to those types very slow (once per major release) and cumbersome (requires migrations code, etc).

Furthermore, strong types work well when you know exactly what you need to represent and you are pretty
confident that the object model you have captures all of the use cases in sufficient details.

I know that this is just an example, but off top of my head, there bulk io may want to control permissions
to use kms encryption feature; or maybe changefeed may want to have separate permission for metrics labesl.

Reading some existing docs it appears that custom role creation seems to be a thing -- for example google supports ability to create arbitrary roles -- just give it a name, etc (e.g. https://cloud.google.com/iam/docs/creating-custom-roles)

In short: I think this RFC should explore alternatives where system.object_type is not a statically typed enum; but a "thing" that the users of this system may define more/less dynamically. And the RFC should explore why one approach was chosen over an alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a fair concern, I think a valid question is how much value do we get out of this being an enum instead of just a string.

We can always do validation somewhere else in the code.

Copy link
Contributor

@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.

I agree with the motivation for the work but I am a little bit concerned by the idea to place grants into a system table, as opposed to using object descriptors.
There are several parts of my concern:

  • what guardrails are put in place to ensure that backup/restore properly preserves the permissions?
  • what guardrails are put in place to ensure that replication streams properly preserve permissions?
  • what guardrails are put in place to ensure that a range unavailability on the privilege table doesn't block all queries?
  • what is the plan wrt data sovereignty, ensuring that the metadata for a given table/object remains in the same region as the data itself?
  • what is the plan wrt access latency, ensuring that the metadata for a given table/object can be accessed with "local" latency in a multi-az or multi-dc deployment?

(With descriptors, all these questions are trivially answerable.)

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @amruss, @livlobo, @miretskiy, @otan, and @RichardJCai)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 74 at r2 (raw file):

The system level grants could be stored on a PrivilegeDescriptor on the system database, however this would require us to support mutations on the system database descriptor which we currently do not do and this allows us to not perform a round trip whenever we look for a system table.

We could create descriptors to represent every object we want to grant on. This is not an attractive option at first glance as these descriptors will likely only be used to store privileges and nothing more.

I disagree on the "nothing more".

At the very start, it would allow us to reference objects by ID and not by name, and support renaming objects without major rewrites.

It would also make it easier to implement per-object auditing rules.

And more importantly, it would enable us to ensure the descriptors for objects are co-located in the same region as the objects themselves, which the current proposal does not do.

Copy link
Contributor Author

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

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

what guardrails are put in place to ensure that backup/restore properly preserves the permissions?

In the full cluster backup case, this is still trivial. I believe right now, full cluster backup restore is the only case that preserves the permissions, we don't actually preserve privileges when restoring a database or tables IIUC.

See: https://www.cockroachlabs.com/docs/stable/restore.html#databases

If you are not doing a full cluster restore, the table-level privileges need to be granted to the users after the restore is complete. (By default, the user restoring will become the owner of the restored objects.) To grant table-level privileges after a restore, backup the system.users table, restore users and their passwords, and then grant the table-level privileges.

We could add backup/restore logic for certain rows if we deem necessary down the road.

what guardrails are put in place to ensure that replication streams properly preserve permissions?

Could you explain more here?

what guardrails are put in place to ensure that a range unavailability on the privilege table doesn't block all queries?

If only a range is unavailable, I don't see why this would block all queries.
Also, this new table is also pretty similar to the system.users table, we essentially have to query the system.users table (or use the users cache) every time before querying this table. We can also add a cache for these privileges to mitigate some cases if a range becomes unavailable.

what is the plan wrt data sovereignty, ensuring that the metadata for a given table/object remains in the same region as the data itself?

Hadn't thought of this yet, I suppose we could make it regional by row - although I'm not quite sure if a system table can be RBR. Is this 100% necessary? I'm not really well versed with metadata and data sovereignty, if the row contains just an object id / object type and privileges which I'd argue is pure metadata and does not have any identifiable information about a user, it might be unnecessary to keep the metadata to the same region as the object. I'm not sure at all about this though.

what is the plan wrt access latency, ensuring that the metadata for a given table/object can be accessed with "local" latency?

A cache similar to the users cache / role auth cache deals with this. Maybe making the table RBR?

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @amruss, @livlobo, @miretskiy, @otan, and @RichardJCai)

Copy link
Contributor

@bdarnell bdarnell 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 @adityamaru, @amruss, @miretskiy, @otan, and @RichardJCai)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 36 at r2 (raw file):

LOGIN / NOLOGIN | Unchanged
VALIDUNTIL | Unchanged
CONTROLJOB / NOCONTROLJOB | Replace with per job grants Example: GRANT PAUSE ON JOB TYPE BACKUP

Is "job type" the right level of granularity here? For example, in CC we may want to protect the backups that we schedule from being interrupted by the user, but the user should be able to cancel jobs that they schedule (and they should be able to delegate this capability to other customer-defined users)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 62 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

That's a fair concern, I think a valid question is how much value do we get out of this being an enum instead of just a string.

We can always do validation somewhere else in the code.

Thought experiment: would a path-style string be a good structure here? It might help make things look more consistent and give obvious structure for places where wildcards could be used, but I'm not sure that a hierarchy is the right fit for the data model.

Anyway, whether it's path-like or not I do agree that the use of an enum here is likely to cause headaches and it's probably better to use a string.


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 95 at r2 (raw file):

Results in the row
(foo, 1, “system”, 16384, 0)

So there is only ever one "object" for type "system", but there are a lot of "privileges"? This seems less scalable than making an "object" for cluster settings and granting the "update" permission so we can keep the set of bit flags (relatively) small.

Copy link
Contributor Author

@RichardJCai RichardJCai 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 @adityamaru, @amruss, @bdarnell, @miretskiy, @otan, and @RichardJCai)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 36 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Is "job type" the right level of granularity here? For example, in CC we may want to protect the backups that we schedule from being interrupted by the user, but the user should be able to cancel jobs that they schedule (and they should be able to delegate this capability to other customer-defined users)

Is the desired granularity in your example jobs per user?


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 62 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Thought experiment: would a path-style string be a good structure here? It might help make things look more consistent and give obvious structure for places where wildcards could be used, but I'm not sure that a hierarchy is the right fit for the data model.

Anyway, whether it's path-like or not I do agree that the use of an enum here is likely to cause headaches and it's probably better to use a string.

Path style string would mean we remove the id column and do something like object_type/id or parent_object_type/object_type/id?

I'm not convinced we want to go down the wildcard route here though. Ideally we could define privileges such that we can grant parent_object_type or define default privileges instead of doing grant * on something like parent_object_type/*`.


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 95 at r2 (raw file):

So there is only ever one "object" for type "system", but there are a lot of "privileges"?

This is correct

This seems less scalable than making an "object" for cluster settings and granting the "update" permission so we can keep the set of bit flags (relatively) small.

Is the worry here that we may have to define too many privileges? To start we'd only have ~10 privileges at the system level (see the table). What would be considered relatively small? <64?

Copy link
Contributor

@bdarnell bdarnell 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 @adityamaru, @amruss, @miretskiy, @otan, and @RichardJCai)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 36 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Is the desired granularity in your example jobs per user?

That would probably be the least invasive way to do it, but it feels a bit clumsy. Another way to think about it would be to define some sort of container/namespace system for jobs (possibly attaching them to databases or schemas, or defining a new kind of "job folder"), but that's a pretty big change to the current model.

One way to bridge the gap could be to use path-style syntax where jobs today can be addressed as /jobs/user/$USER/backup/$JOBID and we could introduce other namespaces as peers to /jobs/user at some point in the future. Then you could grant permissions on something like /jobs/user/*/backup or /jobs/user/$USER depending on your needs. But now that I've typed that out it doesn't really help CC since they want to protect a certain user, so you'd need exceptions to the ACL, etc, etc.

I don't want to get too hung up on a particular use case here because I'm just speculating about what users might want, but it is important that we design the system for flexibility. I think we can be confident that in a couple of years "job type" won't be the only way we want to control permissions related to jobs.


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 62 at r2 (raw file):

Path style string would mean we remove the id column and do something like object_type/id or parent_object_type/object_type/id?

Yes, something like that.

I'm not convinced we want to go down the wildcard route here though. Ideally we could define privileges such that we can grant parent_object_type or define default privileges instead of doing grant * on something like parent_object_type/*`.

I'm not sure a single hierarchy will work in the long term - what if you want both type and owner to be possible parameters for access grants? Wildcards give you a crude way to handle a few different types of parameters (see the examples in my previous comment), although they're cumbersome as complexity grows.


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 95 at r2 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

So there is only ever one "object" for type "system", but there are a lot of "privileges"?

This is correct

This seems less scalable than making an "object" for cluster settings and granting the "update" permission so we can keep the set of bit flags (relatively) small.

Is the worry here that we may have to define too many privileges? To start we'd only have ~10 privileges at the system level (see the table). What would be considered relatively small? <64?

I'm thinking more about scaling mental models than anything technical. We could have hundreds of bits without causing technical problems, but then you're making all of those bits system-wide and I think that's going to be restrictive.

Copy link
Contributor Author

@RichardJCai RichardJCai 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 @adityamaru, @amruss, @bdarnell, @miretskiy, @otan, and @RichardJCai)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 36 at r2 (raw file):

I don't want to get too hung up on a particular use case here because I'm just speculating about what users might want, but it is important that we design the system for flexibility. I think we can be confident that in a couple of years "job type" won't be the only way we want to control permissions related to jobs.

Fair, I'm that this path-style syntax approach is solid and worth considering. It definitely opens up for flexibility in the future and isn't a big technical lift from the current suggestion.


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 62 at r2 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Path style string would mean we remove the id column and do something like object_type/id or parent_object_type/object_type/id?

Yes, something like that.

I'm not convinced we want to go down the wildcard route here though. Ideally we could define privileges such that we can grant parent_object_type or define default privileges instead of doing grant * on something like parent_object_type/*`.

I'm not sure a single hierarchy will work in the long term - what if you want both type and owner to be possible parameters for access grants? Wildcards give you a crude way to handle a few different types of parameters (see the examples in my previous comment), although they're cumbersome as complexity grows.

Yeah I suppose we can explore the path-style syntax approach without actually implementing wildcards in the time being.

@RichardJCai RichardJCai force-pushed the rfc_rbac_extensions_system_privileges branch from 507a490 to 9be01de Compare May 17, 2022 21:30
@blathers-crl blathers-crl bot requested a review from otan May 17, 2022 21:30
Copy link
Contributor Author

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

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

Addressed all open comments.

Please take another look if interested. I'm leaning towards supporting path style strings now. Still some details to iron out about how the strings will look as well.

- Status: draft
- Start Date: 2022-04-26
- Authors: Richard Cai
- RFC PR: [80580](https://github.com/cockroachdb/cockroach/pull/80580)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


The problem with role options are that they are not inheritable, this can be considered an anti-pattern with regards to RBAC. Generally in the RBAC model, when a role is granted a privilege, granting that role to another role will also give the role that privilege. With role options we currently do not support inheritance due to ambiguous inheritance semantics.

This has led to customer issues: https://github.com/cockroachlabs/support/issues/1217 , https://github.com/cockroachdb/cockroach/issues/79571.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Another relevant use case is that for Cockroach Cloud we want to be able to define an “operator” user which is a step below admin, (granting admin generally violates the least privilege principle). As of right now, the operator role is given `CREATEROLE CREATELOGIN CREATEDB CONTROLJOB CANCELQUERY VIEWACTIVITY CONTROLCHANGEFEED` role options, the problem is, granting the operator role to a user does not actually allow the user to perform operator abilities due to role options not being inherited. The user must login as the operator role itself.

# Technical Design
Technical Design
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

# Technical Design
Technical Design
The goal here is to extend our current privilege system to allow more granular grants on objects / entities that are not necessarily backed by a descriptor such as jobs and changefeed objects.
Certain role options should be replaced* with grants. At a high level, we want to introduce “system” or “global” level grants that follow general inheritance rules. This is a concept that exists in many popular non-Postgres databases. (MySQL, Oracle, SQL Server, Db2). We also want to support more granular grants on jobs and bulkio operations.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

## New System Table

We will create a new “system.privileges” table.
`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Results in the row
(foo, 1, “system”, 16384, 0)

The bitmask value for MODIFYCLUSTERSETTING is assumed to be 2 << 13 = 16384 in the above example.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Results in the row
(foo, 1, “system”, 49152, 0)

The bitmask value for VIEWCLUSTERSETTING is assumed to be 2 << 14 = 32768 in the above example. 32768 + 16384 = 49152.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@RichardJCai RichardJCai force-pushed the rfc_rbac_extensions_system_privileges branch from 9be01de to 1b39be6 Compare May 17, 2022 21:32
@RichardJCai
Copy link
Contributor Author

Ping, anyone have any more comments about the system table approach and specifically using path style strings to identify objects?

Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

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

lgtm overall! just had some considerations that would be good to think about

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @amruss, @bdarnell, @miretskiy, @otan, and @RichardJCai)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 109 at r2 (raw file):

Previously, otan (Oliver Tan) wrote…

no. but preferably in a protobuf or something that makes it clear that changing it is bad.

i think it could be confusing to allow access to some pg_catalog tables but not others. maybe this permission could be across the whole pg_catalog.. then again we don't need to worry about this now


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 27 at r4 (raw file):

## Replacing Relevant Role Options
Note - to preface this, role options will likely not be deprecated, we can keep them side by side with their equivalent form of regular grants due to the overhead not being high enough to warrant a potentially tricky migration. We can emphasize in docs to use system privileges instead.

maybe a pg NOTICE if certain role option is used also


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 29 at r4 (raw file):

Note - to preface this, role options will likely not be deprecated, we can keep them side by side with their equivalent form of regular grants due to the overhead not being high enough to warrant a potentially tricky migration. We can emphasize in docs to use system privileges instead.

In short, role options should be reserved for user administration permissions, anything that reads/writes from the database should be a grant.

we'd also want to make sure we keep good support for the PG compatibility role options


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 65 at r4 (raw file):

The object is a string style path that represents an object.

would we need to have a place where "allowed path templates" are listed? as a way of making sure the data in the privileges table is valid and easy to reference from code


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 158 at r4 (raw file):

Right now we have an issue that we do not have granular controls to enable backup / restore and export / import. For example, right now, admin is required to do a full cluster backup. We could add a system level privilege (ie FULLCLUSTERBACKUP) to enable full cluster backups without admin privileges.

For backing up individual database objects right now, we check for CONNECT on databases, SELECT on tables and USAGE on schemas and types. We can potentially augment these in the future? 

we could make it an OR check. like ((has CONNECT && SELECT && USAGE) || (has FULLCLUSTERBACKUP))

Copy link
Contributor Author

@RichardJCai RichardJCai left a comment

Choose a reason for hiding this comment

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

Thanks @rafiss

If no more comments from anyone, I'm going to merge tomorrow EOD and proceed with implementation.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @amruss, @bdarnell, @miretskiy, @otan, @rafiss, and @RichardJCai)


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 29 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we'd also want to make sure we keep good support for the PG compatibility role options

The ones that PG support we will not be changing at all.


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 65 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

would we need to have a place where "allowed path templates" are listed? as a way of making sure the data in the privileges table is valid and easy to reference from code

Yeah we'd have a table/template somewhere. Engineers shouldn't have to ever interact with the path string directly, I'll make a prototype of how we'll abstract it.


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 158 at r4 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

we could make it an OR check. like ((has CONNECT && SELECT && USAGE) || (has FULLCLUSTERBACKUP))

Yeah privileges that currently work, I don't think we'll remove, at least in the short term so this is the plan.

@RichardJCai
Copy link
Contributor Author

Thanks for reviewing all!

bors r+

@craig
Copy link
Contributor

craig bot commented May 25, 2022

Build succeeded:

@craig craig bot merged commit 85b2ebe into cockroachdb:master May 25, 2022
Copy link
Contributor

@ajwerner ajwerner 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


docs/RFCS/20220426_rbac_extensions_system_privileges.md line 60 at r4 (raw file):

object string NOT NULL,
privileges VARBIT NOT NULL,
with_grant VARBIT NOT NULL,

What values go here? How to the values here map to semantic meaning?

Code quote:

privileges VARBIT NOT NULL,
with_grant VARBIT NOT NULL,

docs/RFCS/20220426_rbac_extensions_system_privileges.md line 65 at r4 (raw file):

Previously, RichardJCai (Richard Cai) wrote…

Yeah we'd have a table/template somewhere. Engineers shouldn't have to ever interact with the path string directly, I'll make a prototype of how we'll abstract it.

I'm interested in what this is going to look like. Consider updating the RFC when you figure it out.

RichardJCai added a commit to RichardJCai/cockroach that referenced this pull request May 25, 2022
First step in upcoming privileges work.
Explained in RFC: cockroachdb#80580

Release note: None
RichardJCai added a commit to RichardJCai/cockroach that referenced this pull request Jun 2, 2022
First step in upcoming privileges work.
Explained in RFC: cockroachdb#80580

Release note: None
RichardJCai added a commit to RichardJCai/cockroach that referenced this pull request Jun 7, 2022
First step in upcoming privileges work.
Explained in RFC: cockroachdb#80580

Release note: None
RichardJCai added a commit to RichardJCai/cockroach that referenced this pull request Jun 15, 2022
First step in upcoming privileges work.
Explained in RFC: cockroachdb#80580

Release note: None
RichardJCai added a commit to RichardJCai/cockroach that referenced this pull request Jun 23, 2022
First step in upcoming privileges work.
Explained in RFC: cockroachdb#80580

Release note: None
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.

9 participants