server: ensure correct authorization for debug pages

[BramGruneir]

Up until now, they have been wide open regardless of the cluster settings.

[cockroach-teamcity]

This change is 

[tamird]

I think you can do this without all the duplication if you use a middleware pattern: write a function that returns a http.HandlerFunc which wraps a passed-in http.HandlerFunc with this authentication logic. Then do all the wrapping in server/server.go where you register all the handlers.

[petermattis]

These need to be tested. See acceptance/debug_remote_test.go.

---

Review status: 0 of 5 files reviewed at latest revision, all discussions resolved, all commit checks successful.

---

Comments from Reviewable

[BramGruneir]

@tamird Good call. Done.
@petermattis. Done.

---

Review status: 0 of 3 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[BramGruneir]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

pkg/server/server.go, line 849 at r1 (raw file):

	s.mux.Handle(statusPrefix, gwMux)
	s.mux.Handle("/health", gwMux)
	s.mux.Handle(statusVars, authorizedHandler(http.HandlerFunc(s.status.handleVars)))

Should statusVars be protected as well? Seems like it should be.

---

Comments from Reviewable

[mberhault]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

---

pkg/server/server.go, line 849 at r1 (raw file):

Previously, BramGruneir (Bram Gruneir) wrote…

Should statusVars be protected as well? Seems like it should be.

Probably not, we still want monitoring to work and shouldn't be exporting metrics with sensitive information.
Even if we do protect it, does that mean we want to disable metrics reporting when we add push-type metrics (eg: graphite)?

---

Comments from Reviewable

[petermattis]

modulo not doing auth for the status vars.

---

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

pkg/server/server.go, line 854 at r1 (raw file):

	s.mux.Handle(certificatesDebugEndpoint, authorizedHandler(http.HandlerFunc(s.status.handleDebugCertificates)))
	s.mux.Handle(networkDebugEndpoint, authorizedHandler(http.HandlerFunc(s.status.handleDebugNetwork)))
	s.mux.Handle(nodesDebugEndpoint, authorizedHandler(http.HandlerFunc(s.status.handleDebugNodes)))

It might be the case that these could have been added to debugServeMux instead of s.mux. Maybe. Probably not worth changing now.

---

Comments from Reviewable

[BramGruneir]

Review status: 0 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

pkg/server/server.go, line 849 at r1 (raw file):

Previously, mberhault (marc) wrote…

Probably not, we still want monitoring to work and shouldn't be exporting metrics with sensitive information.
Even if we do protect it, does that mean we want to disable metrics reporting when we add push-type metrics (eg: graphite)?

Done.

---

pkg/server/server.go, line 854 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

It might be the case that these could have been added to debugServeMux instead of s.mux. Maybe. Probably not worth changing now.

noted. Since I'm going to be moving a lot of these to /_status/ with the move to the admin ui, it might be moot.

---

Comments from Reviewable