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.

Sign up for GitHub

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

Jump to bottom

db-console: Allow users with MODIFIFYCLUSTERSETTING to view unredacted cluster settings #137698

Open
kyle-a-wong opened this issue Dec 18, 2024 · 0 comments · May be fixed by #138688
Open

db-console: Allow users with MODIFIFYCLUSTERSETTING to view unredacted cluster settings #137698

kyle-a-wong opened this issue Dec 18, 2024 · 0 comments · May be fixed by #138688
Assignees
@kyle-a-wong
Labels
branch-master Failures and bugs on the master branch. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability

Comments

@kyle-a-wong
Copy link
Contributor

kyle-a-wong commented Dec 18, 2024 •
edited
Loading

#115851 Introduced logic to allow users with MODIFYCLUSTERSETTING privilege to view sensitive cluster settings when viewing cluster settings via SQL. This same logic doesn't hold true for the cluster settings page in DB-console and sensitive settings are always redacted, unless the user is an admin. Based on this conversations, DB-console should have the same rules and should allow users with MODIFYCLUSTERSETTING to be able to view sensitive cluster settings.

The reason why db-console doesn't show sensitive cluster setting values is because it implements its own authorization and redaction logic and accesses settings via the settings registry directly, instead of querying the crdb_internal table. Possible solutions:

  1. Update the admin server to query crdb_internal, which will take care of privilege checking
  2. Update the auth checking to allow users with MODIFYCLUSTERSETTING to view sensitive settings

slack conversation for some more context: https://cockroachlabs.slack.com/archives/C063CP41TG9/p1734041508464959
Jira issue: CRDB-45709
@kyle-a-wong kyle-a-wong added C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. branch-master Failures and bugs on the master branch. T-observability labels Dec 18, 2024
kyle-a-wong added a commit to kyle-a-wong/cockroach that referenced this issue Jan 8, 2025
@kyle-a-wong
server: fix admin server Settings RPC redaction logic
e8f3727 
Previously admin.Settings only allowed admins to view
all cluster settings without redaction. If the
requester was not an admin, would use the isReportable
field on settings to determine if the setting should be
redacted or not. This API also had outdated logic, as
users with the MODIFYCLUSTERSETTINGS should also be
able to view all cluster settings (See cockroachdb#115356 for
more discussions on this).

This patch respects this new role, and no longer
uses the `isReportable` setting flag to determine
if a setting should be redacted. This is implemented
by query `crdb_internal.cluster_settings` directly,
allowing the sql layer to permission check.

This commit also removes the `unredacted_values` from
the request entity as well, since it is no longer
necessary.

Ultimately, this commit updates the Settings RPC
to have the same redaction logic as querying
`crdb_internal.cluster_settings` or using
`SHOW CLUSTER SETTINGS`.

Epic: None
Fixes: cockroachdb#137698
Release note (general change): The /_admin/v1/settings
API now returns cluster settings using the same redaction
logic as querying `SHOW CLUSTER SETTINGS` and
`crdb_internal.cluster_settings`. This means that only
settings flagged as "sensitive" will be redacted, all
other settings will be visible. The same authorization
is required for this endpoint, meaning the user must
be an admin or have MODIFYCLUSTERSETTINGS or
VIEWCLUSTERSETTINGS roles to hit this API. The exception
is that if the user has VIEWACTIVITY or
VIEWACTIVITYREDACTED, they will see console only settings.
@kyle-a-wong kyle-a-wong linked a pull request Jan 8, 2025 that will close this issue
Open
kyle-a-wong added a commit to kyle-a-wong/cockroach that referenced this issue Jan 9, 2025
@kyle-a-wong
server: fix admin server Settings RPC redaction logic
28748df 
Previously admin.Settings only allowed admins to view
all cluster settings without redaction. If the
requester was not an admin, would use the isReportable
field on settings to determine if the setting should be
redacted or not. This API also had outdated logic, as
users with the MODIFYCLUSTERSETTINGS should also be
able to view all cluster settings (See cockroachdb#115356 for
more discussions on this).

This patch respects this new role, and no longer
uses the `isReportable` setting flag to determine
if a setting should be redacted. This is implemented
by query `crdb_internal.cluster_settings` directly,
allowing the sql layer to permission check.

This commit also removes the `unredacted_values` from
the request entity as well, since it is no longer
necessary.

Ultimately, this commit updates the Settings RPC
to have the same redaction logic as querying
`crdb_internal.cluster_settings` or using
`SHOW CLUSTER SETTINGS`.

Epic: None
Fixes: cockroachdb#137698
Release note (general change): The /_admin/v1/settings
API now returns cluster settings using the same redaction
logic as querying `SHOW CLUSTER SETTINGS` and
`crdb_internal.cluster_settings`. This means that only
settings flagged as "sensitive" will be redacted, all
other settings will be visible. The same authorization
is required for this endpoint, meaning the user must
be an admin or have MODIFYCLUSTERSETTINGS or
VIEWCLUSTERSETTINGS roles to hit this API. The exception
is that if the user has VIEWACTIVITY or
VIEWACTIVITYREDACTED, they will see console only settings.
kyle-a-wong added a commit to kyle-a-wong/cockroach that referenced this issue Jan 9, 2025
@kyle-a-wong
server: fix admin server Settings RPC redaction logic
7534bd4 
Previously admin.Settings only allowed admins to view
all cluster settings without redaction. If the
requester was not an admin, would use the isReportable
field on settings to determine if the setting should be
redacted or not. This API also had outdated logic, as
users with the MODIFYCLUSTERSETTINGS should also be
able to view all cluster settings (See cockroachdb#115356 for
more discussions on this).

This patch respects this new role, and no longer
uses the `isReportable` setting flag to determine
if a setting should be redacted. This is implemented
by query `crdb_internal.cluster_settings` directly,
allowing the sql layer to permission check.

This commit also removes the `unredacted_values` from
the request entity as well, since it is no longer
necessary.

Ultimately, this commit updates the Settings RPC
to have the same redaction logic as querying
`crdb_internal.cluster_settings` or using
`SHOW CLUSTER SETTINGS`.

Epic: None
Fixes: cockroachdb#137698
Release note (general change): The /_admin/v1/settings
API now returns cluster settings using the same redaction
logic as querying `SHOW CLUSTER SETTINGS` and
`crdb_internal.cluster_settings`. This means that only
settings flagged as "sensitive" will be redacted, all
other settings will be visible. The same authorization
is required for this endpoint, meaning the user must
be an admin or have MODIFYCLUSTERSETTINGS or
VIEWCLUSTERSETTINGS roles to hit this API. The exception
is that if the user has VIEWACTIVITY or
VIEWACTIVITYREDACTED, they will see console only settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@kyle-a-wong kyle-a-wong

Labels
branch-master Failures and bugs on the master branch. C-bug Code not up to spec/doc, specs & docs deemed correct. Solution expected to change code/behavior. T-observability
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

server: fix admin server Settings RPC redaction logic kyle-a-wong/cockroach
1 participant
@kyle-a-wong