[NET-4897] net/http host header is now verified and request.host that contains socked now error by jmurret · Pull Request #18129 · hashicorp/consul

jmurret · 2023-07-13T20:16:56Z

Description

This is related to #18124 where we pinned the go versions in CI to 1.20.5 and 1.19.10.

go 1.20.6 and 1.19.11 now validate request host headers for validity, including the hostname cannot be prefixed with slashes.

For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname. Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header. Around the community we are seeing that others are intercepting the req.host and if it starts with a slash or ends with .sock, they changing the host to localhost or another dummy value.

client: define a "dummy" hostname to use for local connections by thaJeztah · Pull Request #45942 · moby/moby

Testing & Reproduction steps

Check CI tests.

Links

updated test coverage
external facing docs updated
appropriate backport labels added
not a security concern

…20.6 introduces an invalid host header error

…-unit.yml. pin the 1.20 api tests to use 1.20.5

…s just testing 1.20

…hen the go-version input is supplied.

…esses./

dhiaayachi

Thank you @jmurret !
I left a comment about the go version change, other than that LGTM!

.github/workflows/go-tests.yml

mkeeler

This PR seems fine as a mitigation for the Go team breaking functionality.

Is the Go team expected to fix this in the future release. It seems like the net/http code that is handling the host header knows about the URLs scheme for the request and could more intelligently handle host headers that look like unix sockets or pipes.

api/api_test.go

nfi-hashicorp · 2023-07-14T17:00:55Z

api/api.go

 	}

+	if strings.HasPrefix(r.url.Host, "/") {
+		r.url.Host = "localhost"


Like the moby folks remarked, it's too bad we couldn't just leave this blank.

So . or - may be accepted (see moby/moby#45942 (review)), but yeah, empty would've been most transparent.

api/api_test.go

nfi-hashicorp

Some nits, but good enough to 🚢

jmurret · 2023-07-14T17:19:59Z

Sorry for the late comment but I just noticed that we don't explicitly set the go-version in go-test-sdk-1-20. I think that we should be for consistency with the other jobs.

consul/.github/workflows/go-tests.yml

Lines 431 to 435 in f8578b0

with:

directory: sdk

runs-on: ${{ needs.setup.outputs.compute-xl }}

repository-name: ${{ github.repository }}

go-tags: "${{ github.event.repository.name == 'consul-enterprise' && 'consulent consulprem consuldev' || '' }}"

This is fixed in 19634a4

… contains socked now error (#18129) ### Description This is related to #18124 where we pinned the go versions in CI to 1.20.5 and 1.19.10. go 1.20.6 and 1.19.11 now validate request host headers for validity, including the hostname cannot be prefixed with slashes. For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname. Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header. Around the community we are seeing that others are intercepting the req.host and if it starts with a slash or ends with .sock, they changing the host to localhost or another dummy value. [client: define a "dummy" hostname to use for local connections by thaJeztah · Pull Request #45942 · moby/moby](moby/moby#45942) ### Testing & Reproduction steps Check CI tests. ### Links * [ ] updated test coverage * [ ] external facing docs updated * [ ] appropriate backport labels added * [ ] not a security concern

@jmurret

…st.host that contains socked now error into release/1.13.x (#18142) ## Backport This PR is auto-generated from #18129 to be assessed for backporting due to the inclusion of the label backport/1.13. :rotating_light: >**Warning** automatic cherry-pick of commits failed. If the first commit failed, you will see a blank no-op commit below. If at least one commit succeeded, you will see the cherry-picked commits up to, _not including_, the commit where the merge conflict occurred. The person who merged in the original PR is: @jmurret This person should manually cherry-pick the original PR into a new backport PR, and close this one when the manual backport PR is merged in. > merge conflict error: POST https://api.github.com/repos/hashicorp/consul/merges: 409 Merge conflict [] The below text is copied from the body of the original PR. --- ### Description This is related to #18124 where we pinned the go versions in CI to 1.20.5 and 1.19.10. go 1.20.6 and 1.19.11 now validate request host headers for validity, including the hostname cannot be prefixed with slashes. For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname. Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header. Around the community we are seeing that others are intercepting the req.host and if it starts with a slash or ends with .sock, they changing the host to localhost or another dummy value. [client: define a "dummy" hostname to use for local connections by thaJeztah · Pull Request #45942 · moby/moby](moby/moby#45942) ### Testing & Reproduction steps Check CI tests. ### Links * [ ] updated test coverage * [ ] external facing docs updated * [ ] appropriate backport labels added * [ ] not a security concern --- <details> <summary> Overview of commits </summary> - 747195f - 5164924 - f4d6ca1 - a474071 - 8c03b36 - c50b17c - cc8eaf8 - ce10138 - 133c7ec - b0bd440 - 8f22308 - f8578b0 - 4452224 - 19634a4 </details> --------- Co-authored-by: temp <[email protected]> Co-authored-by: John Murret <[email protected]>

… contains socked now error (#18129) ### Description This is related to #18124 where we pinned the go versions in CI to 1.20.5 and 1.19.10. go 1.20.6 and 1.19.11 now validate request host headers for validity, including the hostname cannot be prefixed with slashes. For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname. Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header. Around the community we are seeing that others are intercepting the req.host and if it starts with a slash or ends with .sock, they changing the host to localhost or another dummy value. [client: define a "dummy" hostname to use for local connections by thaJeztah · Pull Request #45942 · moby/moby](moby/moby#45942) ### Testing & Reproduction steps Check CI tests. ### Links * [ ] updated test coverage * [ ] external facing docs updated * [ ] appropriate backport labels added * [ ] not a security concern

@jmurret

…st.host that contains socked now error into release/1.15.x (#18144) ## Backport This PR is auto-generated from #18129 to be assessed for backporting due to the inclusion of the label backport/1.15. :rotating_light: >**Warning** automatic cherry-pick of commits failed. If the first commit failed, you will see a blank no-op commit below. If at least one commit succeeded, you will see the cherry-picked commits up to, _not including_, the commit where the merge conflict occurred. The person who merged in the original PR is: @jmurret This person should manually cherry-pick the original PR into a new backport PR, and close this one when the manual backport PR is merged in. > merge conflict error: POST https://api.github.com/repos/hashicorp/consul/merges: 409 Merge conflict [] The below text is copied from the body of the original PR. --- ### Description This is related to #18124 where we pinned the go versions in CI to 1.20.5 and 1.19.10. go 1.20.6 and 1.19.11 now validate request host headers for validity, including the hostname cannot be prefixed with slashes. For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname. Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header. Around the community we are seeing that others are intercepting the req.host and if it starts with a slash or ends with .sock, they changing the host to localhost or another dummy value. [client: define a "dummy" hostname to use for local connections by thaJeztah · Pull Request #45942 · moby/moby](moby/moby#45942) ### Testing & Reproduction steps Check CI tests. ### Links * [ ] updated test coverage * [ ] external facing docs updated * [ ] appropriate backport labels added * [ ] not a security concern --- <details> <summary> Overview of commits </summary> - 747195f - 5164924 - f4d6ca1 - a474071 - 8c03b36 - c50b17c - cc8eaf8 - ce10138 - 133c7ec - b0bd440 - 8f22308 - f8578b0 - 4452224 - 19634a4 </details> --------- Co-authored-by: temp <[email protected]> Co-authored-by: John Murret <[email protected]>

@jmurret

…st.host that contains socked now error into release/1.14.x (#18143) ## Backport This PR is auto-generated from #18129 to be assessed for backporting due to the inclusion of the label backport/1.14. :rotating_light: >**Warning** automatic cherry-pick of commits failed. If the first commit failed, you will see a blank no-op commit below. If at least one commit succeeded, you will see the cherry-picked commits up to, _not including_, the commit where the merge conflict occurred. The person who merged in the original PR is: @jmurret This person should manually cherry-pick the original PR into a new backport PR, and close this one when the manual backport PR is merged in. > merge conflict error: POST https://api.github.com/repos/hashicorp/consul/merges: 409 Merge conflict [] The below text is copied from the body of the original PR. --- ### Description This is related to #18124 where we pinned the go versions in CI to 1.20.5 and 1.19.10. go 1.20.6 and 1.19.11 now validate request host headers for validity, including the hostname cannot be prefixed with slashes. For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname. Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header. Around the community we are seeing that others are intercepting the req.host and if it starts with a slash or ends with .sock, they changing the host to localhost or another dummy value. [client: define a "dummy" hostname to use for local connections by thaJeztah · Pull Request #45942 · moby/moby](moby/moby#45942) ### Testing & Reproduction steps Check CI tests. ### Links * [ ] updated test coverage * [ ] external facing docs updated * [ ] appropriate backport labels added * [ ] not a security concern --- <details> <summary> Overview of commits </summary> - 747195f - 5164924 - f4d6ca1 - a474071 - 8c03b36 - c50b17c - cc8eaf8 - ce10138 - 133c7ec - b0bd440 - 8f22308 - f8578b0 - 4452224 - 19634a4 </details> --------- Co-authored-by: temp <[email protected]> Co-authored-by: John Murret <[email protected]>

…st.host that contains socked now error into release/1.16.x (#18145) * no-op commit due to failed cherry-picking * [NET-4897] net/http host header is now verified and request.host that contains socked now error (#18129) ### Description This is related to #18124 where we pinned the go versions in CI to 1.20.5 and 1.19.10. go 1.20.6 and 1.19.11 now validate request host headers for validity, including the hostname cannot be prefixed with slashes. For local communications (npipe://, unix://), the hostname is not used, but we need valid and meaningful hostname. Prior versions go Go would clean the host header, and strip slashes in the process, but go1.20.6 and go1.19.11 no longer do, and reject the host header. Around the community we are seeing that others are intercepting the req.host and if it starts with a slash or ends with .sock, they changing the host to localhost or another dummy value. [client: define a "dummy" hostname to use for local connections by thaJeztah · Pull Request #45942 · moby/moby](moby/moby#45942) ### Testing & Reproduction steps Check CI tests. ### Links * [ ] updated test coverage * [ ] external facing docs updated * [ ] appropriate backport labels added * [ ] not a security concern --------- Co-authored-by: temp <[email protected]> Co-authored-by: John Murret <[email protected]>

jmurret added 8 commits July 13, 2023 11:41

see if go 1.26.4 fails in ci for compatibility and upgrade tests

747195f

pin CI for compatibility and upgrade tests to go version 1.20.5 as 1.…

5164924

…20.6 introduces an invalid host header error

allow optionally specifying a specific go-version when using reusable…

f4d6ca1

…-unit.yml. pin the 1.20 api tests to use 1.20.5

updating if logic on steps so they can be parsed.

a474071

fix go-tests-1.19 to actually pass 1.19 as the version. previously wa…

8c03b36

…s just testing 1.20

fix reusable unit to specify go-version rather than go-version-file w…

c50b17c

…hen the go-version input is supplied.

detect slashes in hostname and replace with localhost for socket addr…

cc8eaf8

…esses./

remove pinning of 1.20.5

ce10138

github-actions bot added theme/api Relating to the HTTP API interface theme/contributing Additions and enhancements to community contributing materials type/ci Relating to continuous integration (CI) tooling for testing or releases labels Jul 13, 2023

Merge branch 'main' into jm/req-host-val

133c7ec

jmurret added pr/no-changelog PR does not need a corresponding .changelog entry backport/1.13 backport/1.15 This release series is no longer active on CE. Use backport/ent/1.15. backport/1.16 This release series is no longer active on CE. Use backport/ent/1.16. labels Jul 13, 2023

jmurret marked this pull request as ready for review July 13, 2023 21:32

jmurret requested review from cthain, dhiaayachi, kisunji, mkeeler, nfi-hashicorp and rboyer July 13, 2023 21:34

Merge branch 'main' into jm/req-host-val

b0bd440

vercel bot deployed to Preview – consul July 14, 2023 13:55 View deployment

dhiaayachi approved these changes Jul 14, 2023

View reviewed changes

.github/workflows/go-tests.yml Show resolved Hide resolved

mkeeler approved these changes Jul 14, 2023

View reviewed changes

kisunji reviewed Jul 14, 2023

View reviewed changes

api/api_test.go Outdated Show resolved Hide resolved

fix test

8f22308

nfi-hashicorp reviewed Jul 14, 2023

View reviewed changes

api/api_test.go Outdated Show resolved Hide resolved

nfi-hashicorp approved these changes Jul 14, 2023

View reviewed changes

jmurret added 2 commits July 14, 2023 11:15

add comment. remove test

4452224

explicitly set version on go sdk 1.20 tests

19634a4

kisunji approved these changes Jul 14, 2023

View reviewed changes

jmurret merged commit 5af7390 into main Jul 14, 2023

jmurret deleted the jm/req-host-val branch July 14, 2023 20:53

zalimeni mentioned this pull request Jul 24, 2023

[NET-4865] security: Upgrade Go and net/http CVE-2023-29406 hashicorp/consul-dataplane#219

Merged

hc-github-team-consul-core added a commit that referenced this pull request Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NET-4897] net/http host header is now verified and request.host that contains socked now error#18129

[NET-4897] net/http host header is now verified and request.host that contains socked now error#18129
jmurret merged 14 commits intomainfrom
jm/req-host-val

jmurret commented Jul 13, 2023 •

edited

Loading

Uh oh!

dhiaayachi left a comment

Uh oh!

Uh oh!

mkeeler left a comment

Uh oh!

Uh oh!

nfi-hashicorp Jul 14, 2023

Uh oh!

thaJeztah Jul 14, 2023

Uh oh!

Uh oh!

nfi-hashicorp left a comment

Uh oh!

jmurret commented Jul 14, 2023

Uh oh!

Conversation

jmurret commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Testing & Reproduction steps

Links

Uh oh!

dhiaayachi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mkeeler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nfi-hashicorp Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

thaJeztah Jul 14, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nfi-hashicorp left a comment

Choose a reason for hiding this comment

Uh oh!

jmurret commented Jul 14, 2023

Uh oh!

jmurret commented Jul 13, 2023 •

edited

Loading