-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
ui: remove remote_debugging checks #28209
ui: remove remote_debugging checks #28209
Conversation
eeeeabb
to
21bfd3a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about on insecure clusters? Is user login used for them too?
Reviewed 4 of 4 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained
pkg/server/status.go, line 244 at r1 (raw file):
} ctx = propagateGatewayMetadata(ctx)
I believe you can also get rid of all these propagateGatewayMetadata
calls, they were only here to support the remote-allowed checks.
Once cockroachdb#28207 lands, all of these endpoints will be protected by login already, so we can skip the check for the setting remote_debugging.mode. Closes: cockroachdb#24992 Release note (admin ui change): The cluster setting remote_debugging.mode no longer controls access to any web ui API endpoints, since they are already protected behind user login.
21bfd3a
to
dd300ff
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about on insecure clusters?
¯\_(ツ)_/¯ they're insecure anyway? I suppose we could leave this in place for them if we thought it was valuable.
Reviewable status: complete! 0 of 0 LGTMs obtained
pkg/server/status.go, line 244 at r1 (raw file):
Previously, a-robinson (Alex Robinson) wrote…
I believe you can also get rid of all these
propagateGatewayMetadata
calls, they were only here to support the remote-allowed checks.
Ack, done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
¯_(ツ)_/¯ they're insecure? I suppose we could leave this in place for them if we thought it was valuable.
Will other sensitive information be exposed in the 2.1 webui when running in insecure mode? If not, then I really don't think we should open this up.
If new sensitive info will be exposed, then I assume a conversation has already been had about that and the same logic would apply here, so I'll defer to that conclusion.
Reviewable status: complete! 0 of 0 LGTMs obtained
Once #28207 lands, all of these endpoints will be protected by login already,
so we can skip the check for the setting remote_debugging.mode.
Closes: #24992
Release note (admin ui change): The cluster setting remote_debugging.mode no
longer controls access to any web ui API endpoints, since they are already
protected behind user login.