Add authenticated debug server #187
Conversation
cmd/launcher/launcher.go
Outdated
| @@ -92,6 +93,11 @@ func main() { | |||
| logFatal(logger, "err", errors.Wrap(err, "detecting platform")) | |||
| } | |||
|
|
|||
| debugTokenPath := filepath.Join(rootDirectory, "debug_token") | |||
| if err := debug.StartDebugServer("localhost:5097", debugTokenPath, logger); err != nil { | |||
There was a problem hiding this comment.
the port should not be hardcoded. We're going to end up failing on a bunch of machines just because. It should be a random available port IMO.
cmd/launcher/launcher.go
Outdated
| @@ -92,6 +93,11 @@ func main() { | |||
| logFatal(logger, "err", errors.Wrap(err, "detecting platform")) | |||
| } | |||
|
|
|||
| debugTokenPath := filepath.Join(rootDirectory, "debug_token") | |||
There was a problem hiding this comment.
I think the debug server should be hidden by a flag. Perhaps it's own flag, or perhaps just --debug.
There was a problem hiding this comment.
I'd like to reason about the possibility of leaving it on by default... We are not binding to any public address, and not exposing any particularly sensitive information. It's not as useful if someone encounters an issue and then cannot turn on the server for a running process.
|
The compromise we came up with was to leave the debug server off by default, but toggle its state by sending |
Gopkg.toml
Outdated
|
|
||
| [[constraint]] | ||
| branch = "master" | ||
| name = "github.com/e-dard/netbug" |
There was a problem hiding this comment.
thoughts on moving the html template into this repo before merging?
debug/debug.go
Outdated
| } | ||
|
|
||
| // The below handler code is adapted from MIT licensed github.com/e-dard/netbug | ||
| func handler(token string, logger log.Logger) http.Handler { |
There was a problem hiding this comment.
You can have http.HandlerFunc as the return arg. That returns a handler but you don't have to cast the function in the return.
debug/debug.go
Outdated
| if r.FormValue("token") == token { | ||
| h.ServeHTTP(w, r) | ||
| } else { | ||
| w.WriteHeader(http.StatusUnauthorized) |
debug/debug.go
Outdated
| serv := http.Server{ | ||
| Handler: r, | ||
| } | ||
| listener, err := net.Listen("tcp", "localhost:0") |
There was a problem hiding this comment.
Can you add a comment explaining :0 ?
debug/debug.go
Outdated
| } | ||
| }() | ||
|
|
||
| addr := fmt.Sprintf("http://%s/debug/?token=%s", listener.Addr().String(), token.String()) |
There was a problem hiding this comment.
What do you think about adding url.Parse on the addr for additional validation?
SIGUSR1(default off)