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

server: remove the setting server.remote_debugging.mode #63748

Merged
merged 1 commit into from
Apr 20, 2021

Conversation

knz
Copy link
Contributor

@knz knz commented Apr 15, 2021

Fixes #57072.
Fixes #26283
Fixes #24992

As suggested/requested by @bdarnell.

Release note (security update): Certain HTTP debug endpoints reserved
to admin users now return more details about range start/end keys,
such as the "hot ranges" report.

Release note (security update): The cluster setting
server.remote_debugging.mode has been removed. The debug endpoints
are now available to every client with access to the HTTP port.
All the HTTP URLs previously affected by this setting already have
user authentication and require a user to be logged in that's member
of the admin group, so there was no need for an additional layer of
security.

@knz knz requested review from bdarnell and a team April 15, 2021 18:20
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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.

Reviewed 9 of 9 files at r1.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/cmd/roachprod/install/cockroach.go, line 585 at r1 (raw file):

	url := h.r.NodeURL(h.c, "localhost", h.r.NodePort(h.c, 1))

	// We ignore failures to set remote_debugging.mode, which was

Users may also have scripts that do this. In order to minimize disruption, it may be better to leave the setting in place but make it an error to set it to anything but "any".

As suggested/requested by @bdarnell.

Release note (security update): Certain HTTP debug endpoints reserved
to `admin` users now return more details about range start/end keys,
such as the "hot ranges" report.

Release note (security update): The cluster setting
`server.remote_debugging.mode` has been removed. The debug endpoints
are now available to every client with access to the HTTP port.
All the HTTP URLs previously affected by this setting already have
user authentication and require a user to be logged in that's member
of the `admin` group, so there was no need for an additional layer of
security.
@knz knz force-pushed the 20210415-remote-debugging branch from 5794a97 to 5896578 Compare April 20, 2021 10:18
@knz
Copy link
Contributor Author

knz commented Apr 20, 2021

In order to minimize disruption, it may be better to leave the setting in place but make it an error to set it to anything but "any".

I simply marked the setting as retired - we have logic to do this already. Setting it becomes a no-op, and SHOW CLUSTER SETTINGS hides it.

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 @bdarnell and @knz)


pkg/cmd/roachprod/install/cockroach.go, line 585 at r1 (raw file):

Previously, bdarnell (Ben Darnell) wrote…

Users may also have scripts that do this. In order to minimize disruption, it may be better to leave the setting in place but make it an error to set it to anything but "any".

I'd undo this change now that the setting is marked as retired.


pkg/server/debug/server.go, line 55 at r2 (raw file):

	// This setting definition still exists so as to not break
	// deployment scripts that set it unconditionally.
	v := settings.RegisterStringSetting("server.remote_debugging.mode", "unused", "local")

The default should now say "any" instead of "local" (or maybe just an empty string if the default of a retired setting is never visible anywhere).

@knz
Copy link
Contributor Author

knz commented Apr 20, 2021

TFYR!

bors r=bdarnell

@craig
Copy link
Contributor

craig bot commented Apr 20, 2021

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Apr 20, 2021

Build succeeded:

@RaduBerinde
Copy link
Member

@knz what do you think about backporting this change to 21.1? I'm asking because I'm trying to make roachprod as close as possible between 21.1 and 21.2.

@knz
Copy link
Contributor Author

knz commented Nov 8, 2021

it's not too hard to backport, but @bdarnell's opinion is that we shouldn't pull functionality from under the feet of users in patch releases, so I don't know?

Probably with sufficient PM approval and @bdarnell 's we could make it happen.

@knz knz deleted the 20210415-remote-debugging branch November 8, 2021 22:05
@RaduBerinde
Copy link
Member

Thanks - I don't think it's worth the work if there are no other good reasons. I think I can backport just the roachprod change and things would still work.

@bdarnell
Copy link
Contributor

bdarnell commented Nov 9, 2021

I don't think this is appropriate for a backport - removing this control was safe because of other work we did to improve access control. Backporting it runs the risk of a security regression if we failed to backport any of those other improvements. (and whether there's an actual security regression, it would look like one to anyone watching closely, which would raise eyebrows about the safety of our patches)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants