A non-admin user is incorrectly able to see Cluster Settings from DB Console

[thtruo]

Describe the problem

1. A non-admin user running SHOW CLUSTER SETTINGS; will be hit with an error that they need the MODIFYCLUSTERSETTING privilege to access cluster settings.
2. However, the same non-admin user can go into the DB Console, load the /reports/settings link, and see all cluster settings

Expected behavior
Access to Cluster Settings should be gated by the same MODIFYCLUSTERSETTING privilege. So that a non-admin user who:

• Loads the reports/settings link from DB Console's Advanced Debug page will not see Cluster Settings. They should see the same error text that you need MODIFYCLUSTERSETTING to load that page

Additional data / screenshots
If the problem is SQL-related, include a copy of the SQL query and the schema
of the supporting tables.

Environment:

• Affects all versions of CRDB (e.g. 21.1, 21.2, 22.1)

[thtruo]

cc @abhinavg6 for awareness based on our discussion

@vy-ton looping you in as well - since this is related to Cluster Settings, it looks like this should be triaged through SQL Experience. I'll go ahead and update the team label accordingly - but let us know if you think differently about ownership

[rafiss]

this is more of an issue with the reports/settings endpoint or the AdminServer's Settings endpoint, which isn’t owned by SQL experience. It's more of a DB Server team thing.

The endpoint needs to be updated to check the permissions for the user, probably here:

cockroach/pkg/server/admin.go

Lines 1668 to 1685 in f68a5a7

	_, isAdmin, err := s.getUserAndRole(ctx)
	if err != nil {
	return nil, err
	}
	var lookupPurpose settings.LookupPurpose
	if isAdmin {
	// Root accesses can customize the purpose.
	// This is used by the UI to see all values (local access)
	// and `cockroach zip` to redact the values (telemetry).
	lookupPurpose = settings.LookupForReporting
	if req.UnredactedValues {
	lookupPurpose = settings.LookupForLocalAccess
	}
	} else {
	// Non-root access cannot see the values in any case.
	lookupPurpose = settings.LookupForReporting
	}

[thtruo]

Ack, thanks Rafi. cc @knz @mwang1026 to put on DB Server's radar

[abhinavg6]

@knz @jtsiros - This seems to be a bug / mismatch for cluster settings. The permission MODIFYCLUSTERSETTING works as expected for the SQL shell, but not through the DB Console. Could we address this anytime soon? Thanks.

[koorosh]

@thtruo , as it's documented MODIFYCLUSTERSETTING:

Allow or disallow the user to modify the cluster settings with the sql.defaults prefix.

(https://www.cockroachlabs.com/docs/v21.2/alter-user.html#parameters).
Db Console only shows settings without providing any ability to modify/update settings.

Should we rely on parameter that intended to allow/disallow writing permission for read-only view in Db Console?

In addition, Db Console displays all cluster settings but some sensitive values are hidden (redacted) for non admin users (ie. cluster secrets, organization name, and any new settings by default are considered to be redacted). Related PR #42520

Would it be appropriate to display cluster settings from SQL shell (SHOW CLUSTER SETTINGS;) for non-admin users instead?

---

@solongordon , can you probably clarify the intended behavior for SHOW CLUSTER SETTINGS; for users without MODIFYCLUSTERSETTING parameter set? Should it be possible to execute query or not?
checkPrivilegesForSetting accepts action argument (that can be either set or show) but it is not used within the function to determine permission.

cockroach/pkg/sql/set_cluster_setting.go

Lines 62 to 76 in e1d95fe

	func checkPrivilegesForSetting(ctx context.Context, p *planner, name string, action string) error {
	if settings.AdminOnly(name) {
	return p.RequireAdminRole(ctx, fmt.Sprintf("%s cluster setting '%s'", action, name))
	}
	hasModify, err := p.HasRoleOption(ctx, roleoption.MODIFYCLUSTERSETTING)
	if err != nil {
	return err
	}
	if !hasModify {
	return pgerror.Newf(pgcode.InsufficientPrivilege,
	"only users with the %s privilege are allowed to %s cluster setting '%s'",
	roleoption.MODIFYCLUSTERSETTING, action, name)
	}
	return nil
	}

[knz]

@koorosh Solon is not the person who's going to work on this.

@thtruo @rafiss , Andrii is raising a valid point. The very name of the role option MODIFYCLUSTERSETTING suggest that a user without this permission can VIEW the cluster settings, even if they cannot MODIFY them. The fact that the internal function checkPrivilegesForSetting currently does not distinguish the two is, IMHO, an implementation bug (and, in my view, the UI behavior is correct).

To support my view further, I'd like to point out that we already had extensive conversations about the confidentiality of data in sensitive cluster settings, and the code in the UI is already taking this into account: non-admin user can VIEW most settings, but they cannot view the values of certain settings (those we mark as invalid for reporting in telemetry). So there was design work in that direction.

As to the course of action, I'd suggest the following:

• explain the customer that the name "MODIFYCLUSTERSETTING" does what it says on the label: it prevents writing to a setting, but not reading them.

  • in that view, we should also acknowledge that the current behavior of SHOW CLUSTER SETTING has a bug.

• what the customer seems to want is a new role option "VIEWCLUSTERSETTING" which does not exist yet. We can add this.

  • if/when we add this, we'll need to change the aforementioned API endpoint to check this option alongside the user role.

• in any case there is a documentation bug. The excessive focus on "settings whose name start with sql.defaults." in the doc is misaligned with what the MODIFYCLUSTERSETTING is about.

Finally, while I agree that some of the mechanism here falls under the DB server team, most of the decisions around the semantics of the role option and the desired end result should be taken by the SQL Experience team, with its PM.

[knz]

@thtruo can you coordinate the decision-making here? We need to do the product decision before we ask Andreii (or anyone else in the DB team) to make code changes.

[thtruo]

Thanks for the callout everyone. Agreed that at the very least we should make a docs change describing MODIFYCLUSTERSETTINGS. Will track that shortly.

Cluster settings are generally seen as an admin/operator operation, it would make sense to present a new role option like VIEWCLUSTERSETTINGS to grant to non-admins who may want to view cluster settings.

To summarize and put forward a proposal following @knz's suggestion:

• We can introduce a new VIEWCLUSTERSETTING role option. This allows a user to see the output of SHOW CLUSTER SETTINGS; and to see cluster settings from within the DB Console
• The existing MODIFYCLUSTERSETTINGS role option does not change. We do need to explain that this role allows writing not reading a setting.
• Admin users by default have the VIEWCLUSTERSETTINGS and MODIFYCLUSTERSETTINGS role options
• Non-admin users by default have NOVIEWCLUSTERSETTINGS and NOMODIFYCLUSTERSETTINGS role options set

One question for the SQL Experience folks: @vy-ton @rafiss do we have a precedent for parent/child roles, e.g. making VIEWCLUSTERSETTING a child permission of MODIFYCLUSTERSETTINGS? That way, we don't break the existing SHOW CLUSTER SETTINGS behavior for existing users that are granted MODIFYCLUSTERSETTINGS. If there isn't a precedent, it would imply existing admins and other users that essentially have both MODIFYCLUSTERSETTINGS and NOVIEWCLUSTERSETTING role options can no longer run SHOW CLUSTER SETTINGS commands.

[rafiss]

do we have a precedent for parent/child roles, e.g. making VIEWCLUSTERSETTING a child permission of MODIFYCLUSTERSETTINGS? That way, we don't break the existing SHOW CLUSTER SETTINGS behavior for existing users that are granted MODIFYCLUSTERSETTINGS. If there isn't a precedent, it would imply existing admins and other users that essentially have both MODIFYCLUSTERSETTINGS and NOVIEWCLUSTERSETTING role options can no longer run SHOW CLUSTER SETTINGS commands.

Some of the recent settings are like this. It basically comes down to the order that the settings are checked. For example, @dhartunian added NOSQLLOGIN, which relates to NOLOGIN, in #74706. And @maryliag added VIEWACTIVITYREDACTED, which relates to VIEWACTIVITY, in #75274.