---
My goal is to be able to make a request overriding the dialed IP, while
preserving the Server Name Indication (SNI).
Currently, if I specify a request.Host, this value gets used as
ServerName as well (which breaks SNI).
I think this is a bug and that the ServerName should always be the host
of the request.URL.
For Request.Host the documentation states:
// For client requests, Host optionally specifies the server to
// connect to. It may be of the form "host" or "host:port".
// If empty, the value of URL.Host is used.
It does not say that this host will be used as the ServerName.
If this was the intended behavior (I don't see a legitimate usecase),
maybe the documentation should be updated instead.
---
client.go | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
diff --git a/client.go b/client.go
index 7c5c372..3f1e7e9 100644
--- a/client.go+++ b/client.go
@@ -82,17 +82,19 @@ func (c *Client) Do(ctx context.Context, req *Request) (*Response, error) {
req = r
}
+ var addr string // Use request host if provided
- if req.Host != "" {- host, port = splitHostPort(req.Host)- host, err = punycodeHostname(host)+ if req.Host == "" {+ addr = net.JoinHostPort(host, port)+ } else {+ rHost, rPort := splitHostPort(req.Host)+ rHost, err = punycodeHostname(rHost) if err != nil {
return nil, err
}
+ addr = net.JoinHostPort(rHost, rPort) }
- addr := net.JoinHostPort(host, port)- // Connect to the host
conn, err := c.dialContext(ctx, "tcp", addr)
if err != nil {
--
2.42.0
On Thu Aug 24, 2023 at 6:14 AM EDT, oliverpool wrote:
> My goal is to be able to make a request overriding the dialed IP, while> preserving the Server Name Indication (SNI).
I don't quite understand your use case. If you are changing the IP
address that you are using to connect, then shouldn't the SNI change as
well?
> Currently, if I specify a request.Host, this value gets used as> ServerName as well (which breaks SNI).> I think this is a bug and that the ServerName should always be the host> of the request.URL.
The use case for Host is to be able to proxy requests for a Gemini site
(e.g. example.com) to another Gemini server (e.g. proxy.example.net).
To proxy requests, you would set Request.URL to "gemini://example.com"
and Request.Host to "proxy.example.net". The client would then connect
to proxy.example.net over TLS (with SNI indicating proxy.example.net),
and then proxy.example.net would read the URL and proxy the request for
you.
Note that the URL sent to the server need not be a gemini:// URL. A
proxy server might support proxying https:// URLs as Gemini text.
So, in summary, the Host field is meant to facilitate proxies. I admit
that it is kind of confusing. If you have ideas for a better design, let
me know.
> I don't quite understand your use case.
It was for unit testing, with a localhost server. With an http request I can simply:
- point the url to the localhost address
- set the Host to the (fake) hostname I want to serve
- send the request (the connection happens at the localhost address, with the servername mentionned in the Host).
I now understand the purpose of the gemini.Request.Host: it is to use the specified Host as a proxy.
If I understand the specification correctly, the "host customization" should happen directly on the URL (in contrary to HTTP, which customizes the Host header).
To prevent such mistakes for clients, I would recommend renaming the Host field to something else (like ProxyHost).
For my tests, I found a workaround, by overriding the Client.DialContext.
== Regarding servers ==
Just as it was mentioned in https://lists.sr.ht/~adnano/go-gemini-devel/patches/20522, this might actually be a footgun for servers: the req.URL.Host should not be trusted to be the host the client actually connected to. req.ServerName() should be used for this.
I don't know how many users expect req.URL.Host to be different from req.ServerName (I didn't expected it!).
I think it would be much safer to add a Server.IgnoreServerNameHostMismatch:
- if this setting is false (safe default), then just after reading the request, the ServerName would be checked against the req.URL.Host. A mismatch would trigger an error 53 PROXY REQUEST REFUSED.
- if this setting is turned to true, no check happens (so no unnecessary allocation)
What do you think of such a (breaking) change?
I could craft a patch for this.
On Sat Aug 26, 2023 at 7:47 AM EDT, Olivier C wrote:
> > I don't quite understand your use case.>> It was for unit testing, with a localhost server. With an http request I can simply:> - point the url to the localhost address> - set the Host to the (fake) hostname I want to serve> - send the request (the connection happens at the localhost address, with the servername mentionned in the Host).>> I now understand the purpose of the gemini.Request.Host: it is to use the specified Host as a proxy.>> If I understand the specification correctly, the "host customization" should happen directly on the URL (in contrary to HTTP, which customizes the Host header).>> To prevent such mistakes for clients, I would recommend renaming the Host field to something else (like ProxyHost).
Perhaps the Host field should be removed entirely, and the proxy
functionality moved to the Client.
> For my tests, I found a workaround, by overriding the Client.DialContext.
That's probably a better way of doing it.
> == Regarding servers ==>> Just as it was mentioned in https://lists.sr.ht/~adnano/go-gemini-devel/patches/20522, this might actually be a footgun for servers: the req.URL.Host should not be trusted to be the host the client actually connected to. req.ServerName() should be used for this.>> I don't know how many users expect req.URL.Host to be different from req.ServerName (I didn't expected it!).
I agree that this can be confusing, and we should probably address it.
> I think it would be much safer to add a Server.IgnoreServerNameHostMismatch:> - if this setting is false (safe default), then just after reading the request, the ServerName would be checked against the req.URL.Host. A mismatch would trigger an error 53 PROXY REQUEST REFUSED.> - if this setting is turned to true, no check happens (so no unnecessary allocation)>> What do you think of such a (breaking) change?>> I could craft a patch for this.
Hmm. I'd rather not add a configuration option for this. Maybe we should
just make this the default behavior, since that is what most users of
this library will probably want. I'll have to think some more about
this.