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

Requests proxied from API Server to APIService obfuscate original request hostname #107435

Open
rynowak opened this issue Jan 9, 2022 · 14 comments
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@rynowak
Copy link

rynowak commented Jan 9, 2022

What happened?

Requests that are proxied by the APIServer to a custom APIService (aggregation) don't generate the X-Forwarded-Host header correctly. Each request will have both Host and X-Forwarded-Host set to ClusterIP IP address of their service. X-Forwarded-Host should be set to the original Host value sent by the client.

The result is that the APIService will not be able to determine the original host and scheme used to make the request. An APIService that wants to generate URLs that are routable for an external client cannot do so.


As an example here's some network psuedocode that reflects what I observe:

Client makes an HTTP request to the API Server using a public API Server endpoint. Let's say mycluster.mycloud.net:6443.

Initial request:

GET /apis/api.example.com/v1alpha1
Host: mycluster.mycloud.net:6443

This is routed to aggregation layer of the API Server which then reverse-proxies the request. In this case the APIService is using a ClusterIP service, which happens to have Cluster IP 10.43.176.179.

Request received by APIService:

GET /apis/api.example.com/v1alpha1
Host: 10.43.176.179:443
X-Forwarded-Host: 10.43.176.179:443
<other headers added by API Server>

The thing I am saying is the bug is that X-Forwarded-Host is 10.43.176.179:443 when it should be mycluster.mycloud.net:6443.

What did you expect to happen?

I would expect X-Forwarded-Host to reflect the original hostname.

Expected: (request sent to APIService)

GET /apis/api.example.com/v1alpha1
Host: 10.43.176.179:443
X-Forwarded-Host: mycluster.mycloud.net:6443
<other headers added by API Server>

Actual: (request sent to APIService)

GET /apis/api.example.com/v1alpha1
Host: 10.43.176.179:443
X-Forwarded-Host: 10.43.176.179:443
<other headers added by API Server>

How can we reproduce it (as minimally and precisely as possible)?

I have put together a sample that demonstrates this by echoing the values it observes for the Host and X-Forwarded-Host headers:

  1. Clone https://github.com/rynowak/apiservice-repro
  2. (option) If you prefer, build a container from src/ and edit deploy.yaml to use yours
  3. (with a Kubernetes cluster you don't mind getting dirty)

run:

kubectl apply -f deploy.yaml
  1. Make sure APIService starts successfully

run:

kubectl get apiservice v1alpha1.api.example.com

You should see:

NAME                       SERVICE                   AVAILABLE   AGE
v1alpha1.api.example.com   default/demo-apiservice   True        1m
  1. Open a local proxy

run:

kubectl proxy
  1. Make a request

run:

curl http://127.0.0.1:8001/apis/api.example.com/v1alpha1

you should see something like (you will have different IP addresses):

{"host":"10.43.176.179:443","forwardedHost":"10.43.176.179:443"}

Based on this repro (kubectl proxy) I would expect 127.0.0.1:8001 to be the value of X-Forwarded-Host

  1. Check your service

run:

kubectl get service demo-apiservice

you should see:

NAME              TYPE        CLUSTER-IP      EXTERNAL-IP   PORT(S)   AGE
demo-apiservice   ClusterIP   10.43.176.179   <none>        443/TCP   5m

Note that your value for CLUSTER-IP should match the IP addresses from step 6. This confirms that the APIServer proxy is using the downstream value for X-Forwarded-Host instead of the original value.

Based on this repro (kubectl proxy) I would expect 127.0.0.1:8001 to be the value of X-Forwarded-Host

This issue reproduces with and without kubectl proxy. I chose kubectl proxy for the repro steps here because it simpler to set up a repro.

Anything else we need to know?

I suspect this root cause with this code:

https://github.com/kubernetes/kube-aggregator/blob/578a094fa7fa8cdba22411e9ab008757f3ef1cb2/pkg/apiserver/handler_proxy.go#L156

At Line 156 the a new request is created with the downstream as its destination URL

At Line 167 a roundtripper is created that understands how to rewrite host headers to match the downstream for proxying. It seems like the request should use the original host value, so that it can be rewritten by the roundtripper, not the value of the downstream.

Kubernetes version

Using k3d (have also reproduced with Kind)

Client Version: version.Info{Major:"1", Minor:"23", GitVersion:"v1.23.1", GitCommit:"86ec240af8cbd1b60bcc4c03c20da9b98005b92e", GitTreeState:"clean", BuildDate:"2021-12-16T11:33:37Z", GoVersion:"go1.17.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"21", GitVersion:"v1.21.7+k3s1", GitCommit:"ac70570999c566ac3507d2cc17369bb0629c1cc0", GitTreeState:"clean", BuildDate:"2021-11-29T16:40:13Z", GoVersion:"go1.16.10", Compiler:"gc", Platform:"linux/amd64"}
WARNING: version difference between client (1.23) and server (1.21) exceeds the supported minor version skew of +/-1

Cloud provider

(n/a)

OS version

(n/a)

Install tools

(n/a)

Container runtime (CRI) and and version (if applicable)

(n/a)

Related plugins (CNI, CSI, ...) and versions (if applicable)

(n/a)

@rynowak rynowak added the kind/bug Categorizes issue or PR as related to a bug. label Jan 9, 2022
@k8s-ci-robot k8s-ci-robot added needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 9, 2022
@rynowak
Copy link
Author

rynowak commented Jan 9, 2022

/sig api-machinery

@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jan 9, 2022
@aojea
Copy link
Member

aojea commented Jan 11, 2022

IIUIC the header is added by this roundtripper

if len(t.Host) > 0 {
req.Header.Set("X-Forwarded-Host", t.Host)
}

The roundtripper is added in

if h.Transport == nil || h.WrapTransport {
h.Transport = h.defaultProxyTransport(req.URL, h.Transport)
}

by

handler := proxy.NewUpgradeAwareHandler(location, proxyRoundTripper, true, upgrade, &responder{w: w})

I think you are right, but my naive question is, is there any value in the X-Forwarded-Host header?
in a kubernetes cluster all these requests will be forwarded by the apiserver, is there any case you have to discriminate by the forwarded host?

ping @liggitt for historical context, the code seems to have more than 7 years

@rynowak
Copy link
Author

rynowak commented Jan 11, 2022

in a kubernetes cluster all this request will be forwarded by the apiserver, is there any case you have to discriminate by the forwareded host

The scenario that I can think of where this matters is for an APIService that needs to generate a URL that the client can understand. That's what I was trying to do but I'll readily admit that it was a niche scenario.

The blocking issue is that the server has no knowledge what host values are routable for the client. The APIService only sees the internally routable hostname (in this case the ClusterIP of its own service) which isn't meaningful outside the cluster.

@liggitt
Copy link
Member

liggitt commented Jan 11, 2022

Tracked back to original addition here: https://github.com/kubernetes/kubernetes/pull/3612/files

The original motivation was a lot like what you described, though the main use I'm aware of at the time was for backing pods returning html pages to browser-based applications. That was never very successful, and dropped off sharply when basic auth support was removed.

Additionally, there's no guarantee the API host is the one the client contacted, so even if the API server forwarded the request host, that may or may not be the original one the client contacted, right?

@fedebongio
Copy link
Contributor

/cc @deads2k @lavalamp
/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jan 11, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle stale
  • Mark this issue or PR as rotten with /lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 11, 2022
@lavalamp
Copy link
Member

FWIW I don't see any reason not to fix this? It'd be reasonable to set X-Forwarded-Host to the Host value from the original request, I think. Regardless of whether that e.g. names an apiserver or a load balancer.

If we're setting both to the same thing that clearly seems like a bug.

@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Close this issue or PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

@k8s-ci-robot k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 12, 2022
@k8s-triage-robot
Copy link

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

@k8s-ci-robot
Copy link
Contributor

@k8s-triage-robot: Closing this issue.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

  • Reopen this issue or PR with /reopen
  • Mark this issue or PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@lavalamp
Copy link
Member

/reopen
/lifecycle frozen

@k8s-ci-robot k8s-ci-robot reopened this Jun 13, 2022
@k8s-ci-robot
Copy link
Contributor

@lavalamp: Reopened this issue.

In response to this:

/reopen
/lifecycle frozen

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@k8s-ci-robot k8s-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. labels Jun 13, 2022
xmen4xp pushed a commit to vmware-tanzu/graph-framework-for-microservices that referenced this issue May 14, 2023
…served via the api-gw

currently due to k8s apiserver issue: kubernetes/kubernetes#107435 it modifies the x-forwarded-host
and the k8sapiserver also rewrites the html response and updates href section

removing the rewrite from k8s apiserver(staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go) file and running make release-image locally
pushed the result image to gcr.io/nsx-sm
bumping up tag for nexus runtime

Closes NPT-656

See merge request nsx-allspark_users/nexus-sdk/cli!174
xmen4xp pushed a commit to vmware-tanzu/graph-framework-for-microservices that referenced this issue May 14, 2023
…served via the api-gw

currently due to k8s apiserver issue: kubernetes/kubernetes#107435 it modifies the x-forwarded-host
and the k8sapiserver also rewrites the html response and updates href section

removing the rewrite from k8s apiserver(staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go) file and running make release-image locally
pushed the result image to gcr.io/nsx-sm
bumping up tag for nexus runtime

Closes NPT-656

See merge request nsx-allspark_users/nexus-sdk/cli!174
xmen4xp pushed a commit to vmware-tanzu/graph-framework-for-microservices that referenced this issue Jun 1, 2023
…served via the api-gw

currently due to k8s apiserver issue: kubernetes/kubernetes#107435 it modifies the x-forwarded-host
and the k8sapiserver also rewrites the html response and updates href section

removing the rewrite from k8s apiserver(staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go) file and running make release-image locally
pushed the result image to gcr.io/nsx-sm

Closes NPT-656

See merge request nsx-allspark_users/nexus-sdk/nexus-runtime-manifests!33
xmen4xp pushed a commit to vmware-tanzu/graph-framework-for-microservices that referenced this issue Jun 1, 2023
…served via the api-gw

currently due to k8s apiserver issue: kubernetes/kubernetes#107435 it modifies the x-forwarded-host
and the k8sapiserver also rewrites the html response and updates href section

removing the rewrite from k8s apiserver(staging/src/k8s.io/apimachinery/pkg/util/proxy/transport.go) file and running make release-image locally
pushed the result image to gcr.io/nsx-sm

Closes NPT-656

See merge request nsx-allspark_users/nexus-sdk/nexus-runtime-manifests!33
@k8s-triage-robot
Copy link

This issue has not been updated in over 1 year, and should be re-triaged.

You can:

  • Confirm that this issue is still relevant with /triage accepted (org members only)
  • Close this issue with /close

For more details on the triage process, see https://www.kubernetes.dev/docs/guide/issue-triage/

/remove-triage accepted

@k8s-ci-robot k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. and removed triage/accepted Indicates an issue or PR is ready to be actively worked on. labels Jun 13, 2023
@alexzielenski
Copy link
Contributor

/triage accepted

To mark issue as still relevant. Tagging current api machinery leads to make them aware of it again to see if it is worth fixing soon:
/cc @jpbetz @deads2k

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Jun 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Categorizes issue or PR as related to a bug. lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

8 participants