networkservicemesh / deployments-k8s

Apache License 2.0
41 stars 32 forks source link

Add configuration for turning profiling on/off #12045

Open VitalyGushin opened 1 month ago

VitalyGushin commented 1 month ago
denis-tingaikin commented 1 month ago

Hi @VitalyGushin, @szvincze 

I just got a thought that probably it is better to use build flags for enabling or disabling pprof. The flags can be used for CI and RC images. This approach allows to exclude http and pproff deps in the release binaries. 

Thoughts?

VitalyGushin commented 1 month ago

@szvincze When importing the net/http/pprof package, the profiler's HTTP endpoints are automatically exposed, which causes gosec warning G108: "Profiling endpoint is automatically exposed on /debug/pprof", which I disable with the #nosec tag in this task. This doesn't matter when the HTTP server is not running, but it may still matter if someone adds a wrapper with their own HTTP server. Is this not critical for you?

szvincze commented 1 month ago

it may still matter if someone adds a wrapper with their own HTTP server. Is this not critical for you?

@VitalyGushin Can you please elaborate a bit more when and how it could cause a problem?

VitalyGushin commented 1 month ago

@szvincze You can read more about the G108 threat here. This is a security hole for which we are turning off the warning. The threat may appear if someone adds an http server that will run constantly. It is difficult to say what specific problems this may cause (or may not). In the current implementation, if the profiler is turned off, then there is no threat.

szvincze commented 1 month ago

@VitalyGushin I have read that page and considered all the listed potential security issues. By default the profiling will be disabled, it will only be enabled on our request to the customer and the profiling will be done as a controlled procedure. So, I think we can go and live with it.

denis-tingaikin commented 1 month ago

@szvincze, @VitalyGushin,

It looks like the issue is with NSM apps that rely on http. You can actually find a list of all http-based apps on GitHub https://github.com/search?q=org%3Anetworkservicemesh%20%22net%2Fhttp%22&type=code

These could be things like webhook, dashboard backend and etc.

We need to head up though, if we will use pprof for these apps, the profile endpoint will always be on. That's because importing packages with import _ "package" automatically runs the init function from that package.

Basically, this approach wouldn't be ideal since HTTP-based apps would always have a gap that we can't turn off.

szvincze commented 1 month ago

@denis-tingaikin Good point! I think we just added pprof to the most important apps. For the rest we can control it by build flags and use the proper image on demand.

VitalyGushin commented 2 weeks ago

@szvincze We have several tag name options for additional images with profiling enabled:

cmd-nsmgr:v1.13.1-rc.1-debug
cmd-nsmgr:v1.13.1-rc.1-profiler
cmd-nsmgr:v1.13.1-rc.1-pprof

Which one do you like best? Or maybe you have another option?

szvincze commented 2 weeks ago

I would vote for cmd-nsmgr:v1.13.1-rc.1-pprof because it clearly represents the tool we should use.