Add support for proxy connect headers #409
Conversation
|
Ok, this is weird. The files use ioutil, but they don't import it. I don't see the change that introduced the usage introducing the import, so it's not like the import was accidentally removed. I don't understand how this even compiles... |
mem
force-pushed
the
mem/proxy_header
branch
from
9599179
to
91b8b0c
Compare
mem
force-pushed
the
mem/proxy_header
branch
from
91b8b0c
to
5762128
Compare
|
This is explicitly for secrets, so it would be a security breach to allow this to be marshalled back. We have to use our |
mem
force-pushed
the
mem/proxy_header
branch
from
5762128
to
b9a6417
Compare
| @@ -48,6 +49,29 @@ func (s Secret) MarshalJSON() ([]byte, error) { | |||
| return json.Marshal(secretToken) | |||
| } | |||
|
|
|||
| type Header map[string][]Secret | |||
There was a problem hiding this comment.
this is added mostly to be able to test the conversion to http.Header and the marshaling / unmarshaling.
I see, you are right. I have modified the implementation. I didn't fully understand the comment about duplicating auth parameters... While the use case is basically https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Proxy-Authorization, I haven't been able to trace why the Go implementation has a separate field for these headers. I imagine the reason is that some proxies pay attention to other headers that aren't about authentication / authorization, e.g. https://everything.curl.dev/usingcurl/proxies/headers and the Go client needs a way to differentiate between headers sent to the proxy and headers sent to the final destination over the tunnel that gets created using CONNECT. |
|
@roidelapluie I would appreciate it if you can take another look. Thanks! |
mem
force-pushed
the
mem/proxy_header
branch
2 times, most recently
from
ddf4a4d
to
7d6e36a
Compare
mem
force-pushed
the
mem/proxy_header
branch
2 times, most recently
from
9141262
to
af85c28
Compare
Done :-) |
Some proxy configurations require additional headers to be able to use them (e.g. authorization token specific to the proxy). Fixes: #402 Co-authored-by: Julien Pivotto <roidelapluie@o11y.eu> Signed-off-by: Marcelo E. Magallon <marcelo.magallon@grafana.com>
mem
force-pushed
the
mem/proxy_header
branch
from
af85c28
to
4a0d730
Compare
|
Thanks! |
| var s []string | ||
| if values != nil { | ||
| s = make([]string, 0, len(values)) | ||
| for _, value := range values { | ||
| s = append(s, string(value)) | ||
| } | ||
| } | ||
| header[name] = s |
There was a problem hiding this comment.
This code has a slightly odd smell to me. Why not simply use header.Add(name, string(value)) (https://pkg.go.dev/net/http#Header.Add)?
The test cases would need to be adjusted, since the bogus items like nil header names or empty value lists won't be added, as they would be illegal headers - and as such I don't see the point of including them as test cases anyway. Header.Add() will also canonicalize the header names.
There was a problem hiding this comment.
oh, yeah, I completely missed that. I'll follow up with another PR.
Some proxy configurations require additional headers to be able to use them (e.g. authorization token specific to the proxy).
Fixes: #402
Signed-off-by: Marcelo E. Magallon marcelo.magallon@grafana.com