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

net/http/pprof: disallow package to register to the default mux in Go 2 #42834

Open
rakyll opened this issue Nov 25, 2020 · 6 comments
Open

net/http/pprof: disallow package to register to the default mux in Go 2 #42834

rakyll opened this issue Nov 25, 2020 · 6 comments

Comments

@rakyll
Copy link
Contributor

@rakyll rakyll commented Nov 25, 2020

net/http/pprof registers handlers to the default mux at init time. In order to register the handlers on a custom mux, you still have to import to package and have the debug handlers registered to the default mux. This creates the situation everyone who has a direct or transient dependency to the net/http/pprof package has the debug handles registered.

This creates security issues and long-term maintenance problems where you want to 100% avoid the use of the default mux to make sure debug endpoints are never exposed to the Internet accidentally. Instead of the current model, export a new API that registers these handlers to the default mux if users want to opt in.

(I remember seeing a similar issue but couldn't find it, filing another one but please close if it's a duplicate.)

@cagedmantis cagedmantis changed the title Disallow net/http/pprof to register to the default mux in Go 2 net/http/pprof: disallow package to register to the default mux in Go 2 Dec 2, 2020
@cagedmantis cagedmantis added this to the Go 2 milestone Dec 2, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Dec 2, 2020

/cc @rsc

Loading

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Jan 26, 2021

Loading

@cagedmantis cagedmantis removed this from the Go 2 milestone Jan 26, 2021
@cagedmantis cagedmantis added this to the Go2 milestone Jan 26, 2021
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented Jan 26, 2021

It's been changed @kolyshkin. Thanks.

Loading

@leandrosansilva
Copy link

@leandrosansilva leandrosansilva commented Oct 5, 2021

One intermediate step to achieve the aim of this request is providing some function to allow registering the debug handlers to any mux instead of asking the user to write them manually.

Here is a proposal in details:

net/http/pprof would continue registering them in the default mux as init(), but also offer the possibility of using a custom mux if the user prefers.

In any non trivial project I've worked so far I've never used the default mux (or globals in general), always being explicit in the mux I use.

If we assume that the most projects hardly (or never?) use the default mux in addition to one or more custom muxes, adding such handlers in the default mux is not a problem, as there's no ListenAndServe() using it so it's never exposed.

But I confess this is a strong assumption.

I imagine a call such as (on net/http/pprof), which basically is the current init(), but passing the mux as an argument:

// RegisterHandlers registers handlers with the mux
func RegisterHandlers(mux *http.ServeMux) {
       mux.HandleFunc("/debug/pprof/", Index)
       mux.HandleFunc("/debug/pprof/cmdline", Cmdline)
       mux.HandleFunc("/debug/pprof/profile", Profile)
       mux.HandleFunc("/debug/pprof/symbol", Symbol)
       mux.HandleFunc("/debug/pprof/trace", Trace)
}

Which could be called on init() with the http.DefaultServerMux, keeping the current expected behaviour.

A second step could be move such init() into a InitDefaultServerMux() and telling the users to explicit use it in case they are using the default mux.

Finally InitDefaultServerMux() would be deprecated and tell the user about the need to explicit register the handlers on whatever mux they are using.

Any thoughts? I would be glad on contributing withe adding RegisterHandlers() and use it on init(). It'd already remove some code duplication in some of my projects :-)

Loading

@kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Oct 5, 2021

@cagedmantis perhaps it makes sense to also remove https://github.com/golang/go/milestone/175, otherwise someone else will make the same mistake.

Loading

@pedramteymoori
Copy link

@pedramteymoori pedramteymoori commented Oct 16, 2021

I also wanted to +1 this change. We also had the exact security issue in our system, and pprof data was exposed on our main port, although we defined another ServerMux for it. Defining handlers in the init function can cause this severe security issue, as almost everyone uses DefaultServerMux to expose primary handlers.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants