feat: Promote "Host" header if present. Fixes #12690

[philBrown]

Fixes #12690

Motivation

When communicating with a proxy or load balancer that does not do Host header rewriting, it may be required to override the default request Host header.

Go's HTTP request does not do this by default when passed a Host header value.

Modifications

Checks for any Host header in the HTTP Facade and promotes it to the request Host field if found.

Verification

This is a very simple change and since there were no existing headers tests, it didn't seem necessary.

[agilgur5]

What was your intent for this -- did you mean to modify the CLI? Because the apiclient is used for a few things (an SDK, the CLI, some internal API calls).

It may make sense to limit this change to only the CLI if that was your intent

[philBrown]

@agilgur5 the intent was for the CLI.

I didn't see anywhere else where this could possibly be done though. The request construction is completely opaque outside the Facade

[philBrown]

Unit test failure doesn't seem related and I cannot reproduce it locally. Counting files in /tmp seems flakey at best

[philBrown]

FYI we're using my fork and it's working great for us. It would be fantastic if this could be incorporated into the official release

[philBrown]

Hi @agilgur5, is there anything holding this PR up?

There's really not much to it. Go's HTTP module ignores Host headers which is a problem when you need them present in the request. The simple solution is to set the req.Host instead.

I wish the Go HTTP module was different but this is what we have to live with 😅

[agilgur5]

Hi @agilgur5, is there anything holding this PR up?

Sorry I realized I never explained. Since it affects more than just the CLI, my primary concern is if this potentially ends up opening up any vulnerabilities via spoofed headers, similar to my concern in #13609 (comment).
As a result, I'd need to take a pretty deep look to be confident it doesn't, which I unfortunately don't have too much time to do these days on top of other higher priority things (and features are lower priority too since 3.6 was iceboxed for some time due to 3.5 instability. there's an RC now though)

Go's HTTP module ignores Host headers which is a problem when you need them present in the request. The simple solution is to set the req.Host instead.

I also imagine there's some rationale for this, I don't know off the top of my head or from a quick look though. golang/go#29865 from your issue unfortunately doesn't explain any rationale either.

[philBrown]

via spoofed headers

I'm not sure I understand. The CLI already accepts literally any extra header. The problem here is that the underlying Golang HTTP module just ignores Host

I also imagine there's some rationale for this

I provided a use-case in the issue that this PR solves: #12690

My specific case is that I must send the request to a front proxy that then routes requests based on headers, eg

argo submit -s front-proxy -H 'Host: actual-argo-server'

Here, the extra Host header is not added to the request due to the HTTP Go module specifics (golang/go#29865).

This is entirely possible with any other HTTP client like curl

curl \
  -H 'Host: actual-argo-server' \
  -H 'Content-type: application/json' \
  -d '{...}' \
  https://front-proxy/api/v1/workflows/my-namespace/submit

[agilgur5]

The CLI already accepts literally any extra header.

Again, this change doesn't just impact the CLI. If it were CLI only, this would be easy to approve, but it isn't unfortunately.

I also imagine there's some rationale for this

I provided a use-case in the issue that this PR solves: #12690

Rationale for why the HTTP Go module ignores the header. That is what I was referencing in context.

[philBrown]

Rationale for why the HTTP Go module ignores the header

Ah sorry, I misunderstood. I have no idea why Go does it that way either. Looking back through the request.go history it's really unclear why that and certain other headers like user-agent are treated specially

[mwhittington21]

@agilgur5 in order to make this change only affect the CLI, and thus reduce the surface area that spoofing is an issue to almost nil, what are your thoughts on modifying this PR to change construction of the API client only used within the CLI commands to pass a new option, enabling this header override behaviour?

We could then pass the new option all the way through here and into the facade, configuring this behaviour only for the CLI. Would that be enough to alleviate your concerns?

[agilgur5]

It's a workaround, but yes that would suffice as it would then be purely used client side in the CLI.

It's possible the current approach is fine too, I'm just not sure without taking a deep look at all the other usage. You're welcome to go through that other usage yourself as well to see if you can concretely prove or disprove any spoofing possibility.

[shuangkun]

/retest