fix: net/http server-side instrumentation #403
Conversation
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Thanks! The general approach makes sense to me. I haven't reviewed this PR deeply. I'll leave that to the Orchestrion experts. I tried this PR out locally on a Go HTTP server we use in the profiling backend that uses ServeMux (the one that motivated #173). I confirmed that the handler gets instrumented :)
…no instrumentation happend in a package
| } | ||
| cmd.OnClose(func() error { | ||
| log.Debugf("Adding %s file into %q\n", linkdeps.LinkDepsFilename, cmd.Flags.Output) | ||
| child := exec.Command("go", "tool", "pack", "r", cmd.Flags.Output, linkDepsFile) |
There was a problem hiding this comment.
🟠 Code Vulnerability
Check command call and ensure there is no unsanitized data used. The variable `linkDepsFile` may need to be validated (...read more)
In Go, the exec.Command function is used to run external commands. Using this function carelessly can lead to command injection vulnerabilities.
Command injection occurs when untrusted input is passed directly to a system shell, allowing an attacker to execute arbitrary commands. This can result in unauthorized access to the system, data leaks, or other security breaches.
To prevent command injection vulnerabilities when using exec.Command in Go, follow these coding best practices:
- Sanitize User Input: Always validate and sanitize user inputs before passing them to
exec.Command. Avoid executing commands constructed using user-provided data. - Avoid using Shell Expansion: If possible, pass the command and arguments as separate strings to
exec.Command. This prevents the shell from interpreting special characters in a potentially malicious way. - Use Absolute Paths: When specifying the command to be executed, use absolute paths for executables whenever possible. This reduces the risk of inadvertently running a similarly named malicious command from the system's PATH.
- Avoid String Concatenation: Refrain from dynamically constructing commands by concatenating strings. Instead, use the
arg ...stringparameter ofexec.Commandto pass arguments safely. - Limit Privileges: Run commands with the least privilege required to carry out the task. Avoid running commands with elevated privileges unnecessarily.
By following these practices, you can reduce the risk of command injection vulnerabilities when using exec.Command in Go and enhance the security of your application.
| }, | ||
| }, | ||
| }, | ||
| Children: nil, |
There was a problem hiding this comment.
Is it expected that the children are gone here?
There was a problem hiding this comment.
Yes, see in the diff there was a comment // FIXME: this span shouldn't exist
This is because we were somehow double instrumenting something (not sure where exactly).
Before (4 spans):
client -> server (net/http) -> server (gorilla/mux) -> server (net/http)
Now (3 spans):
client -> server (net/http) -> server (gorilla/mux)
| @@ -62,10 +62,56 @@ var weavingSpecialCase = []specialCase{ | |||
| {path: "github.com/DataDog/go-tuf/client", prefix: false, behavior: neverWeave}, | |||
| } | |||
|
|
|||
| func (w Weaver) OnCompile(cmd *proxy.CompileCommand) error { | |||
| func (w Weaver) OnCompile(cmd *proxy.CompileCommand) (result error) { | |||
There was a problem hiding this comment.
…http-server' into rarguellof/APPSEC-55883/fix-net-http-server
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #403 +/- ##
==========================================
+ Coverage 60.36% 60.54% +0.17%
==========================================
Files 181 185 +4
Lines 12707 12858 +151
==========================================
+ Hits 7671 7785 +114
- Misses 4526 4527 +1
- Partials 510 546 +36
|
Resolves #173, resolves #400
This PR changes
net/httpserver-side instrumentation aspects to trace the library itself (similar approach as we are doing today with the client-side instrumentation). The hook where we inject is thehttp.Server.Servemethod, which should be the common method that is always called when the http server is started (and at this point, the Handler should already be set).This fixes problems in the existing instrumentation where we:
http.Handlerimplementations (net/http:Handlerinterface implementation is not always instrumented #173).http.HandlerFunc(func(w http.ResponseWriter, r *http.Request))declarations.srv.Handler = myHandlerare not traced.As a side-effect, this approach removes the ability to use
orchestrion:ignorecomments to skip tracing on http servers (which happens for other aspects as well where we instrument the library code).