Subject: Re: SSL SNI
To: Michael Paquier <michael@paquier.xyz>, Tom Lane <tgl@sss.pgh.pa.us>
Cc: Jacob Champion <pchampion@vmware.com>,
 "pgsql-hackers@postgresql.org" <pgsql-hackers@postgresql.org>
References: <9407b344-7342-b2b7-004f-d5250687be42@enterprisedb.com>
 <ecb6a57d176a396feb959fbb0c53b3c794e8d8e5.camel@vmware.com>
 <4eaa2b56-c3d0-67b6-4716-010d001c7601@enterprisedb.com>
 <4bf3ae1c-b28e-8960-e89f-c390c5cf01a2@enterprisedb.com>
 <504c276ab6eee000bb23d571ea9b0ced4250774e.camel@vmware.com>
 <14368.1622742108@sss.pgh.pa.us> <14840.1622742761@sss.pgh.pa.us>
 <15936.1622744081@sss.pgh.pa.us>
 <5df46438-c1b4-161c-4669-f6e6ab6797f0@enterprisedb.com>
 <878795.1623080064@sss.pgh.pa.us> <YL8ULDGZaTDw3Swa@paquier.xyz>
From: Peter Eisentraut <peter.eisentraut@enterprisedb.com>
Message-ID: <559ffbc8-05ed-c44c-728a-d2a71d1232a7@enterprisedb.com>
Date: Tue, 8 Jun 2021 16:12:44 +0200
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0)
 Gecko/20100101 Thunderbird/78.10.2
MIME-Version: 1.0
In-Reply-To: <YL8ULDGZaTDw3Swa@paquier.xyz>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Archived-At: 
 <https://www.postgresql.org/message-id/559ffbc8-05ed-c44c-728a-d2a71d1232a7%40enterprisedb.com>
Precedence: bulk

On 08.06.21 08:54, Michael Paquier wrote:
> On Mon, Jun 07, 2021 at 11:34:24AM -0400, Tom Lane wrote:
>> Yeah, I'd include the empty-string test just because it's standard
>> practice in this area of libpq.  Whether those tests are actually
>> triggerable in every case is obscure, but ...
> 
> Checking after a NULL string and an empty one is more libpq-ish.
> 
>> Patch looks sane by eyeball, though I didn't test it.
> 
> I did, and I could not break it.
> 
> +               SSLerrfree(err);
> +               SSL_CTX_free(SSL_context);
> +               return -1;
> It seems to me that there is no need to free SSL_context if
> SSL_set_tlsext_host_name() fails here, except if you'd like to move
> the check for the SNI above SSL_CTX_free() around L1082.  There is no
> harm as SSL_CTX_free() is a no-op on NULL input.

Good point.  Committed that way.