Improve errors when setting incorrect bounds for SSL protocols

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

Improve errors when setting incorrect bounds for SSL protocols

Michael Paquier-2
Hi all,
(Daniel G. in CC.)

As discussed on the thread to be able to set the min/max SSL protocols
with libpq, when mixing incorrect bounds the user experience is not
that good:
https://www.postgresql.org/message-id/9CFA34EE-F670-419D-B92C-CB7943A27573@...

It happens that the error generated with incorrect combinations
depends solely on what OpenSSL thinks is fine, and that's the
following:
psql: error: could not connect to server: SSL error: tlsv1 alert
internal error

It is hard for users to understand what such an error means and how to
act on it.

Please note that OpenSSL 1.1.0 has added two routines to be able to
get the min/max protocols set in a context, called
SSL_CTX_get_min/max_proto_version.  Thinking about older versions of
OpenSSL I think that it is better to use
ssl_protocol_version_to_openssl to do the parsing work.  I also found
that it is easier to check for compatible versions after setting both
bounds in the SSL context, so as there is no need to worry about
invalid values depending on the build of OpenSSL used.

So attached is a patch to improve the detection of incorrect
combinations.  Once applied, we get a complain about an incorrect
version at server startup (FATAL) or reload (LOG).  The patch includes
new regression tests.

Thoughts?
--
Michael

ssl-proto-context-v1.patch (5K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Improve errors when setting incorrect bounds for SSL protocols

Daniel Gustafsson
> On 14 Jan 2020, at 04:54, Michael Paquier <[hidden email]> wrote:
>
> Hi all,
> (Daniel G. in CC.)
>
> As discussed on the thread to be able to set the min/max SSL protocols
> with libpq, when mixing incorrect bounds the user experience is not
> that good:
> https://www.postgresql.org/message-id/9CFA34EE-F670-419D-B92C-CB7943A27573@...
>
> It happens that the error generated with incorrect combinations
> depends solely on what OpenSSL thinks is fine, and that's the
> following:
> psql: error: could not connect to server: SSL error: tlsv1 alert
> internal error
>
> It is hard for users to understand what such an error means and how to
> act on it.

Correct, it's an easy mistake to make but based on the error it might take some
time to figure it out.

> Please note that OpenSSL 1.1.0 has added two routines to be able to
> get the min/max protocols set in a context, called
> SSL_CTX_get_min/max_proto_version.  Thinking about older versions of
> OpenSSL I think that it is better to use
> ssl_protocol_version_to_openssl to do the parsing work.  I also found
> that it is easier to check for compatible versions after setting both
> bounds in the SSL context, so as there is no need to worry about
> invalid values depending on the build of OpenSSL used.

I'm not convinced that it's a good idea to check for incompatible protocol
range in the OpenSSL backend.  We've spent a lot of energy to make the TLS code
library agnostic and pluggable, and since identifying a basic configuration
error isn't OpenSSL specific I think it should be in the guc code.  That would
keep the layering as well as ensure that we don't mistakenly treat this
differently should we get a second TLS backend.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: Improve errors when setting incorrect bounds for SSL protocols

Michael Paquier-2
On Tue, Jan 14, 2020 at 11:21:53AM +0100, Daniel Gustafsson wrote:

> On 14 Jan 2020, at 04:54, Michael Paquier <[hidden email]> wrote:
>> Please note that OpenSSL 1.1.0 has added two routines to be able to
>> get the min/max protocols set in a context, called
>> SSL_CTX_get_min/max_proto_version.  Thinking about older versions of
>> OpenSSL I think that it is better to use
>> ssl_protocol_version_to_openssl to do the parsing work.  I also found
>> that it is easier to check for compatible versions after setting both
>> bounds in the SSL context, so as there is no need to worry about
>> invalid values depending on the build of OpenSSL used.
>
> I'm not convinced that it's a good idea to check for incompatible protocol
> range in the OpenSSL backend.  We've spent a lot of energy to make the TLS code
> library agnostic and pluggable, and since identifying a basic configuration
> error isn't OpenSSL specific I think it should be in the guc code.  That would
> keep the layering as well as ensure that we don't mistakenly treat this
> differently should we get a second TLS backend.
Good points.  And the get routines are not that portable in OpenSSL
either even if HEAD supports 1.0.1 and newer versions...  Attached is
an updated patch which uses a GUC check for both parameters, and
provides a hint on top of the original error message.  The SSL context
does not get reloaded if there is an error, so the errors from OpenSSL
cannot be triggered as far as I checked (after mixing a couple of
corrent and incorrect combinations manually).
--
Michael

ssl-proto-context-v2.patch (4K) Download Attachment
signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Improve errors when setting incorrect bounds for SSL protocols

Daniel Gustafsson
> On 15 Jan 2020, at 03:28, Michael Paquier <[hidden email]> wrote:
>
> On Tue, Jan 14, 2020 at 11:21:53AM +0100, Daniel Gustafsson wrote:
>> On 14 Jan 2020, at 04:54, Michael Paquier <[hidden email]> wrote:
>>> Please note that OpenSSL 1.1.0 has added two routines to be able to
>>> get the min/max protocols set in a context, called
>>> SSL_CTX_get_min/max_proto_version.  Thinking about older versions of
>>> OpenSSL I think that it is better to use
>>> ssl_protocol_version_to_openssl to do the parsing work.  I also found
>>> that it is easier to check for compatible versions after setting both
>>> bounds in the SSL context, so as there is no need to worry about
>>> invalid values depending on the build of OpenSSL used.
>>
>> I'm not convinced that it's a good idea to check for incompatible protocol
>> range in the OpenSSL backend.  We've spent a lot of energy to make the TLS code
>> library agnostic and pluggable, and since identifying a basic configuration
>> error isn't OpenSSL specific I think it should be in the guc code.  That would
>> keep the layering as well as ensure that we don't mistakenly treat this
>> differently should we get a second TLS backend.
>
> Good points.  And the get routines are not that portable in OpenSSL
> either even if HEAD supports 1.0.1 and newer versions...  Attached is
> an updated patch which uses a GUC check for both parameters, and
> provides a hint on top of the original error message.  The SSL context
> does not get reloaded if there is an error, so the errors from OpenSSL
> cannot be triggered as far as I checked (after mixing a couple of
> corrent and incorrect combinations manually).

This is pretty much exactly the patch I was intending to write for this, so +1
from me.

cheers ./daniel

Reply | Threaded
Open this post in threaded view
|

Re: Improve errors when setting incorrect bounds for SSL protocols

Michael Paquier-2
On Wed, Jan 15, 2020 at 06:34:39PM +0100, Daniel Gustafsson wrote:
> This is pretty much exactly the patch I was intending to write for this, so +1
> from me.

Thanks for the review.  Let's wait a couple of days to see if others
have objections or more comments about this patch, but I'd like to
fix the issue and backpatch down to 12 where the parameters have been
introduced.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Improve errors when setting incorrect bounds for SSL protocols

Michael Paquier-2
On Thu, Jan 16, 2020 at 10:00:52AM +0900, Michael Paquier wrote:
> Thanks for the review.  Let's wait a couple of days to see if others
> have objections or more comments about this patch, but I'd like to
> fix the issue and backpatch down to 12 where the parameters have been
> introduced.

And committed.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Improve errors when setting incorrect bounds for SSL protocols

Peter Eisentraut-6
In reply to this post by Michael Paquier-2
On 2020-01-15 03:28, Michael Paquier wrote:
> Good points.  And the get routines are not that portable in OpenSSL
> either even if HEAD supports 1.0.1 and newer versions...  Attached is
> an updated patch which uses a GUC check for both parameters, and
> provides a hint on top of the original error message.  The SSL context
> does not get reloaded if there is an error, so the errors from OpenSSL
> cannot be triggered as far as I checked (after mixing a couple of
> corrent and incorrect combinations manually).

The reason this wasn't done originally is that it is not correct to have
GUC check hooks that refer to other GUC variables, because otherwise you
get inconsistent behavior depending on the order of processing of the
assignments.  In this case, I think it would work because you have
symmetric checks for both variables, but in general it is a problematic
strategy.

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