Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: Ignore the host header before forwarding #162

Closed
wants to merge 6 commits into from

Conversation

kahowell
Copy link

@kahowell kahowell commented Mar 4, 2021

With some hosts behind shared TLS (using SNI), keeping the Host header was causing the target host to be misidentified.

I also observed issues related to TLS verification (as seen in #156).

Fixes #156

@winklerm
Copy link

I have also experienced problems with the host header when smee-client is forwarding a request with the host header value without a port number to a Jenkins master running in OpenShift, resulting in HTTP 503 response status returned to smee-client.

For that reason, removing the host header would help for this issue as well.

[1] openshift/origin#13355

@winklerm

This comment has been minimized.

winklerm added a commit to winklerm/kie-ci that referenced this pull request Mar 25, 2021
Add workaround for [1] by building smee-client from a fork with the fix
[2].

[1] probot/smee-client#156
[2] probot/smee-client#162
mbiarnes pushed a commit to kiegroup/kie-ci that referenced this pull request Mar 25, 2021
Add workaround for [1] by building smee-client from a fork with the fix
[2].

[1] probot/smee-client#156
[2] probot/smee-client#162
@Ginxo
Copy link

Ginxo commented Aug 16, 2021

Hi @kahowell @JasonEtco @tcbyrd any news here? Thanks!

@tcbyrd
Copy link
Contributor

tcbyrd commented Aug 30, 2021

I definitely think the change makes sense. Given this comment in #156 (comment)

superagent@^3.8.3 to ^5.0.2 (this is most probably the one which is introducing the error since is the library they use for pushing events).

Would anyone be willing to add a test here? Since the regression was most likely introduced in a dependabot upgrade and people are blocked by this, it would probably be best to have even a basic test to ensure upgrading dependencies doesn't re-introduce the problem.

@eidsnessje-nih
Copy link

eidsnessje-nih commented Jul 16, 2022

I would like to ask that the bug fix be released before someone waits too much longer trying to design a universally valid test for TLS validation problems applicable to private CI/CD servers receiving github hooks, with internally signed certificates, behind firewalls. That's just the type of oddball situation that this project appeals to. Maybe there's some other project that would make a good dev dependency to create such a test scenario, but the perfect test is looking like an enemy to this simple bug fix. I get the impression TLS is only going to be more and more common on internal networks and will undermine an increasing number of use cases for smee and smee-client.

The test is a good idea, don't get me wrong, but it could come following a 2nd issue in the tracker. I was afraid to try & count how many of the 100 forks have fixes for this faulty assumption about the "Host" property in data.

@unphased
Copy link

unphased commented Aug 12, 2022

@eidsnessje-nih I noticed that I did not run into this issue when testing today, and I saw that superagent has been updated to v7.1.3 in smee-client v1.2.3, does it resolve the issue for you?

Ah, ok, my test is probably not one of

hosts behind shared TLS (using SNI)

I don't even know what that is.

@Ginxo
Copy link

Ginxo commented Oct 18, 2022

I tried to solve this issue #156 (comment) by testing this one and it works great. Would be nice to merge this one. Are you planning to do so?

@Ginxo
Copy link

Ginxo commented Oct 18, 2022

can you please @kahowell merge upstream/master with this? This way you will have latest, we are using your proposal on production, otherwise I will produce mine 🤔
#222

With some hosts behind shared TLS (using SNI), keeping the Host header
was causing the target host to be misidentified.

I also observed issues related to TLS verification (as seen in probot#156).

Fixes probot#156
@kahowell
Copy link
Author

@Ginxo I've rebased against latest as requested.

@Ginxo
Copy link

Ginxo commented Oct 19, 2022

just notice PR #222 (based on this PR) additionally fixes the commander error, added commander test coverage and github actions workflow. Thanks @kahowell

index.ts Fixed Show resolved Hide resolved
@wolfy1339
Copy link
Contributor

This seems good to me.

Is it possible to add a test to make sure it doesn't happen again?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 3, 2024

Its basically impossible to write a useful unit test for this.

@simensen
Copy link

simensen commented Jan 4, 2024

I want to make sure #181 is considered as an alternative to this. In my case, the host header is required to have the correct host since the web server or reverse proxy makes decisions based on the host header. Removing the host header entirely may fix cases where the default server is the intended target, but it will make it impossible to correctly target a specific host.

Also, #181 has a test that I believe shows that it works as expected. I think? :)

Is there any reason to believe #181 won't work for addressing #156?

@Uzlopak
Copy link
Collaborator

Uzlopak commented Jan 4, 2024

@simensen

I guess, that the issue is not existent anymore. We use now fetch, which does not take host header as it is a header which should not be manipulated.

@Uzlopak Uzlopak changed the title Remove the host header before forwarding test: Remove the host header before forwarding Jan 4, 2024
@Uzlopak Uzlopak changed the title test: Remove the host header before forwarding test: Ignore the host header before forwarding Jan 4, 2024
@Uzlopak Uzlopak closed this Feb 3, 2024
@topikachu
Copy link

@simensen

I guess, that the issue is not existent anymore. We use now fetch, which does not take host header as it is a header which should not be manipulated.

I still meet this issue on 2.0.1.
It seems the host header is still forwarding to the target which causes

  1. TLS verification error.
  2. L7 proxy can't forward the request to the right target using the host header.

It's safe to delete the host header and fetch can add it back from the url.

@Uzlopak
Copy link
Collaborator

Uzlopak commented May 7, 2024

@topikachu

Do you want to provide a PR?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
10 participants