~adnano/go-gemini-devel

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
6 3

[PATCH go-gemini] Set req.Host to SNI hostname when available

Details
Message ID
<20210225010507.203071-1-belak@coded.io>
DKIM signature
pass
Download raw message
Patch: +7 -1
---
Setting Request.Host will allow Handlers to properly handle different virtual
hosts in the same process.

I actually wanted to clean up server.go a bit, but I'll leave that for another
patch. I think it's cleaner to use a plain net.Listener and create a tls.Conn
farther down the line, because that makes it easier to pass the tls.Conn through
the call chain. Unfortunately, there's an exposed method, ServeConn, which would
mean breaking backwards compatibility to do this so I left it alone for now.

 request.go | 4 +++-
 server.go  | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/request.go b/request.go
index f15d891..00bf265 100644
--- a/request.go
+++ b/request.go
@@ -20,7 +20,9 @@ type Request struct {
	// For international domain names, Host may be in Punycode or
	// Unicode form. Use golang.org/x/net/idna to convert it to
	// either format if needed.
	// This field is ignored by the Gemini server.
	//
	// The Gemini server in this package sets Host to the SNI hostname
	// before invoking a handler.
	Host string

	// For client requests, Certificate optionally specifies the
diff --git a/server.go b/server.go
index a8ff77b..2576ee6 100644
--- a/server.go
+++ b/server.go
@@ -373,6 +373,10 @@ func (srv *Server) goServeConn(ctx context.Context, conn net.Conn) error {
	}
	req.conn = conn

	if tlsConn, ok := conn.(*tls.Conn); ok {
		req.Host = tlsConn.ConnectionState().ServerName
	}

	h := srv.Handler
	if h == nil {
		w.WriteHeader(StatusNotFound, "Not found")
-- 
2.30.1

[go-gemini/patches/.build.yml] build success

builds.sr.ht <builds@sr.ht>
Details
Message ID
<C9I7IO2CVBYD.QDIB7MUUGZGY@cirno>
In-Reply-To
<20210225010507.203071-1-belak@coded.io> (view parent)
DKIM signature
missing
Download raw message
go-gemini/patches/.build.yml: SUCCESS in 47s

[Set req.Host to SNI hostname when available][0] from [Kaleb Elwert][1]

[0]: https://lists.sr.ht/~adnano/go-gemini-devel/patches/20522
[1]: belak@coded.io

✓ #438656 SUCCESS go-gemini/patches/.build.yml https://builds.sr.ht/~adnano/job/438656
Details
Message ID
<C9I8H2U3NF57.GUW5GAC4HO85@nitro>
In-Reply-To
<20210225010507.203071-1-belak@coded.io> (view parent)
DKIM signature
pass
Download raw message
On Wed Feb 24, 2021 at 8:05 PM EST, Kaleb Elwert wrote:
> ---
> Setting Request.Host will allow Handlers to properly handle different virtual
> hosts in the same process.

I had considered setting Request.Host before, but I was not sure whether
the overhead of tls.Conn.ConnectionState could affect the performance.
Request used to have a TLS field like in net/http, but it was moved to a
method so that it would not be allocated if it was not needed.

Currently those who need to access SNI information can use
TLS().ServerName. For the majority of use cases I don't think that
accessing the ServerName is necessary, as the certificate.Store struct
handles automatic creation of certificates and will not create a
certificate for a host that has not been registered. Do you have an
example where this could be necessary?

> I actually wanted to clean up server.go a bit, but I'll leave that for another
> patch. I think it's cleaner to use a plain net.Listener and create a tls.Conn
> farther down the line, because that makes it easier to pass the tls.Conn through
> the call chain. Unfortunately, there's an exposed method, ServeConn, which would
> mean breaking backwards compatibility to do this so I left it alone for now.

We don't necessarily want to impose the choice of *tls.Conn on Server
users, as they might prefer another TLS implementation, or they might
use an unencrypted connection if they are sitting behind a TLS reverse
proxy.
Details
Message ID
<facb754e-8de8-43e0-94ed-c555214b6f05@www.fastmail.com>
In-Reply-To
<C9I8H2U3NF57.GUW5GAC4HO85@nitro> (view parent)
DKIM signature
pass
Download raw message
> I had considered setting Request.Host before, but I was not sure whether
> the overhead of tls.Conn.ConnectionState could affect the performance.
> Request used to have a TLS field like in net/http, but it was moved to a
> method so that it would not be allocated if it was not needed.
> 
> Currently those who need to access SNI information can use
> TLS().ServerName. For the majority of use cases I don't think that
> accessing the ServerName is necessary, as the certificate.Store struct
> handles automatic creation of certificates and will not create a
> certificate for a host that has not been registered. Do you have an
> example where this could be necessary?

The reason for adding this isn't for limiting what hosts that can be connected to,
but routing to different locations for different hosts on in a single service. You are
right that it could be done by calling .TLS().ServerName, however I would argue that
either exposing the *tls.ConnectionState by default or at least copying over the
ServerName makes sense, maybe even both. If we're exposing ServerName,
it would make more sense to just store the whole ConnectionState on the request
anyway, as we've already generated it to get the ServerName.

Additionally, it's needed to properly parse the incoming URL to detect and deny proxy
requests (see https://github.com/go-gemini/gemini/blob/main/server.go#L195-L215 and
https://github.com/go-gemini/gemini/blob/main/utils.go#L64-L83). I generally think it's
best to optimize for correctness first, followed by performance and this is one of those
instances where an extra allocation for the ConnectionState shouldn't matter that much.
I would be willing to bet that the crypto caused by using a TLS connection (and
the network syscalls themselves) are way more expensive than the extra allocation.

As you mentioned, the http package already does this (See serve() at
https://golang.org/src/net/http/server.go#L1817) and even though Gemini tries to be
lighter weight than HTTP, I believe it still applies.

> We don't necessarily want to impose the choice of *tls.Conn on Server
> users, as they might prefer another TLS implementation, or they might
> use an unencrypted connection if they are sitting behind a TLS reverse
> proxy.

This is the first I've heard this argument used with Go - are there other TLS
implementations commonly used in Go? If this is an actual argument, doesn't
the call to Request.TLS() also break?
Details
Message ID
<C9IQLMCEZEKO.2CSNUBNR7V183@nitro>
In-Reply-To
<facb754e-8de8-43e0-94ed-c555214b6f05@www.fastmail.com> (view parent)
DKIM signature
pass
Download raw message
On Wed Feb 24, 2021 at 11:56 PM EST, Kaleb Elwert wrote:
> The reason for adding this isn't for limiting what hosts that can be connected to,
> but routing to different locations for different hosts on in a single service. You are
> right that it could be done by calling .TLS().ServerName, however I would argue that
> either exposing the *tls.ConnectionState by default or at least copying over the
> ServerName makes sense, maybe even both. If we're exposing ServerName,
> it would make more sense to just store the whole ConnectionState on the request
> anyway, as we've already generated it to get the ServerName.

Right. I just want to avoid populating the request with too much
unnecessary information that lightweight handlers might not need.
See https://github.com/golang/go/issues/5465#issue-51283668

It might be necessary to populate Request.Host to make it easy to
disallow proxy requests. ServeMux should probably be where proxy
requests are disallowed.

Perhaps we could have an accessor method (say, ServerName) that would
generate return TLS().ServerName. We should also probably cache the
*tls.ConnectionState on the first call to ServerName so that we don't
repeatedly generate it. Or perhaps storing it in Request.Host is cleaner
since it matches with the usage of Request.Host on the client side.

Maybe the server should have its own Request type as mentioned in the
issue above.

> > We don't necessarily want to impose the choice of *tls.Conn on Server
> > users, as they might prefer another TLS implementation, or they might
> > use an unencrypted connection if they are sitting behind a TLS reverse
> > proxy.
>
> This is the first I've heard this argument used with Go - are there other TLS
> implementations commonly used in Go? If this is an actual argument, doesn't
> the call to Request.TLS() also break?

See https://github.com/golang/go/issues/21753
The call to Request.TLS won't break, it will just return nil.
Details
Message ID
<C9KJP4MJM5QM.XPQFBLOV1N1I@nitro>
In-Reply-To
<C9IQLMCEZEKO.2CSNUBNR7V183@nitro> (view parent)
DKIM signature
pass
Download raw message
I've added the ServerName helper method to Request to make it easier to
access the TLS server name.
Details
Message ID
<15417e9a-2134-4969-8603-c4e25c248091@www.fastmail.com>
In-Reply-To
<C9KJP4MJM5QM.XPQFBLOV1N1I@nitro> (view parent)
DKIM signature
pass
Download raw message
On Sat, Feb 27, 2021, at 11:04 AM, Adnan Maolood wrote:
> I've added the ServerName helper method to Request to make it easier to
> access the TLS server name.

That seems like a good compromise. Thanks!
Reply to thread Export thread (mbox)