Обсуждение: Inconsistent error handling in the openssl init code

The errorhandling in be_tls_init(), and functions called from it, set the
appropriate elevel by the isServerStart.  ssl_protocol_version_to_openssl() is
however erroring out unconditionally with ERROR on invalid TLS versions.  The
attached patch adds isServerStart handling to the TLS version handling as well,
to make be_tls_init() consistent in its errorhandling.

cheers ./daniel

On Wed, Feb 06, 2019 at 11:18:22PM +0100, Daniel Gustafsson wrote:
> The errorhandling in be_tls_init(), and functions called from it, set the
> appropriate elevel by the isServerStart.  ssl_protocol_version_to_openssl() is
> however erroring out unconditionally with ERROR on invalid TLS versions.  The
> attached patch adds isServerStart handling to the TLS version handling as well,
> to make be_tls_init() consistent in its errorhandling.

(Adding Peter Eisentraut in CC)

Good catch, this is an oversight from commit e73e67c7, which affects
only HEAD.  The comment at the top of ssl_protocol_version_to_openssl
becomes incorrect as the function would not throw an error in a reload
context.

The current comment is that:
 * If a version is passed that is not supported by the current OpenSSL
 * version, then we throw an error, so that subsequent code can assume it's
 * working with a supported version.

Which we could change to that:
..., then we throw an error as FATAL if isServerStart is true so as it
won't return.  Otherwise, errors are logged as LOG level and return -1
to indicate trouble, preserving the old SSL state if any.

Peter, could you take care of it?  Or should I?
--
Michael

> On 7 Feb 2019, at 05:12, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Feb 06, 2019 at 11:18:22PM +0100, Daniel Gustafsson wrote:
>> The errorhandling in be_tls_init(), and functions called from it, set the
>> appropriate elevel by the isServerStart.  ssl_protocol_version_to_openssl() is
>> however erroring out unconditionally with ERROR on invalid TLS versions.  The
>> attached patch adds isServerStart handling to the TLS version handling as well,
>> to make be_tls_init() consistent in its errorhandling.
>
> (Adding Peter Eisentraut in CC)
>
> Good catch, this is an oversight from commit e73e67c7, which affects
> only HEAD.  The comment at the top of ssl_protocol_version_to_openssl
> becomes incorrect as the function would not throw an error in a reload
> context.

Doh, managed to completely overlook that.  The attached updated patch also
fixes the comment, thanks!

cheers ./daniel

On Thu, Feb 07, 2019 at 10:03:30AM +0100, Daniel Gustafsson wrote:
> Doh, managed to completely overlook that.  The attached updated patch also
> fixes the comment, thanks!

That looks fine to me.  Could you just add it to the next CF as a bug
fix so as we don't forget?  I am pretty sure that Peter will look at
that soon.
--
Michael

> On 8 Feb 2019, at 01:10, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Feb 07, 2019 at 10:03:30AM +0100, Daniel Gustafsson wrote:
>> Doh, managed to completely overlook that.  The attached updated patch also
>> fixes the comment, thanks!
>
> That looks fine to me.  Could you just add it to the next CF as a bug
> fix so as we don't forget?  I am pretty sure that Peter will look at
> that soon.

Done, thanks!  I took the liberty to mark you as reviewer since you’ve already
spent time looking at the patch.

cheers ./daniel

On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
> Done, thanks!  I took the liberty to mark you as reviewer since you’ve already
> spent time looking at the patch.

Thanks.  Please note that I can take care of the patch in a couple of
days if need be.
--
Michael

On 08/02/2019 11:01, Michael Paquier wrote:
> On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
>> Done, thanks!  I took the liberty to mark you as reviewer since you’ve already
>> spent time looking at the patch.
> 
> Thanks.  Please note that I can take care of the patch in a couple of
> days if need be.

Fixed, thanks.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 8 Feb 2019, at 12:01, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 08/02/2019 11:01, Michael Paquier wrote:
>> On Fri, Feb 08, 2019 at 09:36:59AM +0100, Daniel Gustafsson wrote:
>>> Done, thanks!  I took the liberty to mark you as reviewer since you’ve already
>>> spent time looking at the patch.
>>
>> Thanks.  Please note that I can take care of the patch in a couple of
>> days if need be.
>
> Fixed, thanks.

Thanks, I’ve closed it in the commitfest with you as committer.

cheers ./daniel