The socket parameter is sent as-is to the UNIX domain socket opened via http.send

[michivi]

Short description

It seems that a call to the built-in http.send function targeting a UNIX domain socket is also sending over the socket parameter used by http.send to identify the path to the socket itself. While probably not a big issue in most use cases, our very own use case is failing as all sent parameters are validated by our listening program. The socket parameter being an OPA parameter, our program doesn't recognize it and fails.

Our environment:

• OPA 0.45.0
• OS X 12.6

Steps To Reproduce

To reproduce the error:

1. Run nc -l -U /tmp/test.sock
2. In another shell, run opa eval 'http.send({ "method": "GET", "url": "unix://localhost/hello?param1=value1&socket=%2ftmp%2ftest.sock" })'
3. Observe the incoming request: GET /hello?param1=value1&socket=%2ftmp%2ftest.sock HTTP/1.1. It contains both the voluntary parameters and the socket parameter targeted at http.send

Expected behavior

One might expect that the original query is untouched (parameters order included), except for the socket parameter which would be removed. So in our case: GET /hello?param1=value1 HTTP/1.1.

Though I guess we should also consider what would happen if there's multiple socket parameters. We don't have that use cases, so any chosen behavior (so long as one of them is picked to determine the path to the UNIX domain socket deterministically and then removed) would do.

Additional context

I'm guessing the related code is this:

opa/topdown/http.go

Lines 252 to 264 in fff3e0a

	v, err := url.ParseQuery(u.RawQuery)
	if err != nil {
	return false, rawURL, nil
	}
	tr := http.DefaultTransport.(*http.Transport).Clone()
	tr.DialContext = func(ctx context.Context, network, addr string) (net.Conn, error) {
	return http.DefaultTransport.(*http.Transport).DialContext(ctx, u.Scheme, v.Get("socket"))
	}
	tr.TLSClientConfig = tlsConfig
	tr.DisableKeepAlives = true
	rawURL = strings.Replace(rawURL, "unix:", "http:", 1)

We can see that the socket parameter is used to determine the path to the UNIX domain socket, but it is not removed. The only change in the URL is the change of the scheme.

[srenatus]

Thanks for the thorough writeup! And I think you're quite correct there, the socket parameter should be removed!

Though I guess we should also consider what would happen if there's multiple socket parameters.

I'd check what others following the same convention do in this case... I bet this doesn't come out of nowhere, but I cannot recall the discussions from back then without git history dumpster diving.

[srenatus]

☝️ It's a good issue to work on, I think, because the code change should be minimal. What's more important, though, is figuring out, via some research, what the right thing to do is with multiple such query params.

[michivi]

#5355 is an attempt to tackle this issue and is working fine for our use case.

It does the following:

• Pick the first value for the socket parameter, ignoring any other values (if any)
• Delete all socket parameters to prevent HTTP parameter pollution (any forged request trying to change the socket file path by trying to override the socket parameter in the original query). The downside of this is that the listening socket will never receive any socket parameter if that was a legitimate parameter. Is that acceptable?
• The implementation may reorder the global http parameters because of parsing and re-encoding. Not ideal, but there is no standard telling us how the order matters so I guess it's kind of a best effort. Parsing and re-encoding the query string while preserving order and modifying it on the fly requires much effort. I guess we also need to decide on whether we accept that.

I did not find why the original implementation used the socket parameter to determine the socket's path. The only programs that I know of and which are using UNIX domain sockets are getting the socket's path through other means (e.g. extra CLI parameters for curl or netcat). Perhaps it would be possible for OPA but it would require an API breaking change (ignore the query parameter and send it as-is to the listening socket, retrieve the socket path from an extra object parameter in http.send). But perhaps it would be the way to go so that we don't have any of the caveats indicated previously?