User-Agent: Cyrus-JMAP/3.5.0-alpha0-141-gf094924a34-fm-20210210.001-gf094924a
Mime-Version: 1.0
Message-Id: <facb754e-8de8-43e0-94ed-c555214b6f05@www.fastmail.com>
In-Reply-To: <C9I8H2U3NF57.GUW5GAC4HO85@nitro>
References: <C9I8H2U3NF57.GUW5GAC4HO85@nitro>
Date: Wed, 24 Feb 2021 20:56:11 -0800
From: "Kaleb Elwert" <belak@coded.io>
To: "Adnan Maolood" <me@adnano.co>, ~adnano/go-gemini-devel@lists.sr.ht
Subject: Re: [PATCH go-gemini] Set req.Host to SNI hostname when available
Content-Type: text/plain

> 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?