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

Consider allowing a user to disable Host header validation #18522

Closed
analogrelay opened this issue Jan 22, 2020 · 17 comments
Closed

Consider allowing a user to disable Host header validation #18522

analogrelay opened this issue Jan 22, 2020 · 17 comments
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel severity-nice-to-have This label is used by an internal tool
Milestone

Comments

@analogrelay
Copy link
Contributor

It appears some HTTP clients (in particular gRPC clients) that connect over Unix Domain Sockets put the path to the socket in the Host header. We should investigate this a little bit. If it's the case that most clients do this, we should consider having an option to allow disabling Host header validation. We could limit this to only cases where we're listening on a Unix Socket to reduce security risk.

@JamesNK
Copy link
Member

JamesNK commented Jan 22, 2020

A UDS with a path is the recommended value in these docs

--csi-address: This is the path to the CSI driver socket (defined above) inside the pod that the node-driver-registrar container will use to issue CSI operations (e.g. /csi/csi.sock).

https://github.com/kubernetes-csi/node-driver-registrar#required-arguments

@blowdart
Copy link
Contributor

We should switch to path style validation, rather than disable altogether.

@Zetanova
Copy link

Zetanova commented Jan 23, 2020

nginx makes the same host validation of the :authority pseudo header

I read now the RFC https://tools.ietf.org/html/rfc3986#section-3.2.2 twice

The bottom line is that it is a "reg-name" and it is a UTF8 free-text

...
This specification does not mandate a particular registered name
   lookup technology and therefore does not restrict the syntax of reg-
   name beyond what is necessary for interoperability.  Instead, it
   delegates the issue of registered name syntax conformance to the
   operating system of each application performing URI resolution, and
   that operating system decides what it will allow for the purpose of
   host identification.  A URI resolution implementation might use DNS,
   host tables, yellow pages, NetInfo, WINS, or any other system for
   lookup of registered names.
...

The reg-name syntax allows percent-encoded octets ... UTF8
I didn't saw that there is some kind of decoding in the current code.

like @blowdart wrote,
In the UDS case a switch to path style validation would be more conform to the RFC

@andresrsanchez
Copy link

andresrsanchez commented Jan 23, 2020

Hi, i was doing something like:

var socket = new Socket(AddressFamily.Unix, SocketType.Stream, ProtocolType.IP);
var endPoint = new UnixDomainSocketEndPoint("/../run/foo/bar.sock");
socket.Connect(endPoint);

Console.Writeline("It's working!");

var channel = GrpcChannel.ForAddress("/../run/foo/bar.sock");

The last part fails with the message "System.ArgumentException: Only 'http' and 'https' schemes are allowed"

Some part of this makes sense? I'm trying to imitate golang things 😟

Thanks!!!

@JamesNK
Copy link
Member

JamesNK commented Jan 23, 2020

GrpcChannel only supports TCP. Support for UDS and pipes is planned for 5.0

@analogrelay
Copy link
Contributor Author

analogrelay commented Jan 24, 2020

Do we know if the UDS client is URL-encoding the Host header? I do see that the RFC allows / but only if it's URL-encoded. When I did a quick test with curl, I saw that both foo/bar and foo%2fbar were rejected.

Perhaps this doesn't need special configuration and we just need to be validating pre decoding. If the UDS clients are encoding the header value, then we can be both spec-compliant and support UDS.

@blowdart If you have concerns there though, I like the idea of doing path-style validation when listening on a UDS.

@JamesNK
Copy link
Member

JamesNK commented Jan 24, 2020

Related to the issue, please consider esoteric Windows named pipes values for when Kestrel is using that transport.

https://docs.microsoft.com/en-us/windows/win32/ipc/pipe-names

@blowdart
Copy link
Contributor

Can we iterate through transports, and have transports decide if the host header is valid for them? That seems a better approach than either on or off.

@analogrelay analogrelay added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jan 24, 2020
@analogrelay analogrelay added this to the Next sprint planning milestone Jan 24, 2020
@analogrelay
Copy link
Contributor Author

This may relate to proposed work on Named Pipes, so @Tratcher can take a look for now

@Zetanova
Copy link

Zetanova commented Mar 9, 2020

Is there any design decision made?

Would need it badly for my kubernetes CSI plugin,
the only other framework or proxy that i found
with a support for h2 over UDS is golang.

@Tratcher
Copy link
Member

@Zetanova even when we do this, it will only be for .NET 5. In the meantime you're going to need a different solution.

@analogrelay
Copy link
Contributor Author

Planning notes: We'll look at allowing validation to be disabled when using UDS. Targeting 5.0, but not the immediate next preview and may be punted. It must be configurable per-endpoint.

@JamesNK
Copy link
Member

JamesNK commented Aug 18, 2020

I've experimented with gRPC over unix domain sockets with Grpc.Net.Client (using HttpClient) calling Grpc.AspNetCore (using Kestrel).

HttpClient will always send a valid host header, even when using a different transport. It is a hard requirement of using HttpClient.

I recommend moving this issue to future milestone. We can judge whether it is needed based on additional demand.

@Zetanova
Copy link

A workaround is to use golang as a sidecar/reverse proxy to overwrite the host header.
https://github.com/Zetanova/grpc-proxy

It is the only way that i found to use dotnet-kestrel for kubelet/kubernetes plugins over UDS

@JamesNK
Copy link
Member

JamesNK commented Aug 19, 2020

I looked into this more. go-grpc's behavior of sending the file path has been causing problems for other servers like nodejs. See grpc/grpc-go#2628

go-grpc has recently been updated to send "localhost" as the authority with unix domain sockets. See grpc/grpc-go#3730. The fix is in 1.31 which is available today. Upgrade to it and retry?

I think Kestrel is doing the right thing here.

@Zetanova
Copy link

@JamesNK thx for the good news.
Yes all proxies (nginx, apache, kestrel, candy) had this issue.

Because kubernetes/kubelet is the gRPC-client,
to try it out, the kubelet BIN would need to use the new version of golang,
I will do it on the next kubernetes version.

@BrennanConroy BrennanConroy modified the milestones: 5.0.0-rc1, Discussions Aug 19, 2020
@JamesNK JamesNK removed their assignment Sep 30, 2020
@pranavkm pranavkm removed the cost: S label Nov 6, 2020
@jkotalik jkotalik added affected-very-few This issue impacts very few customers severity-nice-to-have This label is used by an internal tool labels Nov 12, 2020 — with ASP.NET Core Issue Ranking
@ghost
Copy link

ghost commented Jan 11, 2021

Thank you for contacting us. Due to a lack of activity on this discussion issue we're closing it in an effort to keep our backlog clean. If you believe there is a concern related to the ASP.NET Core framework, which hasn't been addressed yet, please file a new issue.

This issue will be locked after 30 more days of inactivity. If you still wish to discuss this subject after then, please create a new issue!

@ghost ghost closed this as completed Jan 11, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 2021
@amcasey amcasey added area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions and removed area-runtime labels Jun 2, 2023
This issue was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-very-few This issue impacts very few customers area-networking Includes servers, yarp, json patch, bedrock, websockets, http client factory, and http abstractions enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-kestrel severity-nice-to-have This label is used by an internal tool
Projects
None yet
Development

No branches or pull requests

10 participants