Re: [PATCH v5] GSSAPI encryption support

Michael Paquier <michael.paquier@gmail.com> writes:

> On Tue, Feb 16, 2016 at 2:45 AM, Robbie Harwood <rharwood@redhat.com> wrote:
>> David Steele <david@pgmasters.net> writes:
>>> On 2/10/16 4:06 PM, Robbie Harwood wrote:
>>>> Hello friends,
>>>>
>>>> For your consideration, here is a new version of GSSAPI encryption
>>>> support.  For those who prefer, it's also available on my github:
>>>> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e
>>>
>>> It tried out this patch and ran into a few problems:
>>>
>>> 2) While I was able to apply the patch and get it compiled it seemed
>>> pretty flaky - I was only able to logon about 1 in 10 times on average.
>>>
>>> When not successful the client always output this incomplete message
>>> (without  terminating LF):
>>>
>>> psql: expected authentication request from server, but received
>>>
>>> From the logs I can see the server is reporting EOF from the client,
>>> though the client does not core dump and prints the above message before
>>> exiting.
>>>
>>> I have attached files that contain server logs at DEBUG5 and tcpdump
>>> output for both the success and failure cases.
>>>
>>> Please let me know if there's any more information you would like me to
>>> provide.
>>
>> What I can't tell from looking at your methodology is whether both the
>> client and server were running my patches or no.  There's no fallback
>> here (I'd like to talk about how that should work, with example from
>> v1-v3, if people have ideas).  This means that both the client and the
>> server need to be running my patches for the moment.  Is this your
>> setup?
>
> We need to be careful here, backward-compatibility is critical for
> both the client and the server, or to put it in other words, an
> uptables client should still be able to connect a patched server, and
> vice-versa. This is an area where it is important to not break any
> third-part tool, either using libpq or reimplementing the frontend
> protocol.

Which is why in my introduction to the v4 patch I explicitly mentioned
that this was missing.  I wanted to talk about how this should be
implemented, since I feel that I received no feedback on that design
From last time.  It's not hard to port that design over, if desired.

> So I finally began to dive into your new patch... And after testing
> this is breaking the authentication protocol for GSS. I have been able
> to connect to a server once, but at the second attempt and after
> connection is failing with the following error:
> psql: expected authentication request from server, but received ioltas

Interesting.  I will see if I can find anything.  The capture posted
earlier (thanks David!) suggests that the client doesn't expect the
encrypted data.

I suspect what has happened is a race between the client buffering data
From the socket and the client processing the completion of GSSAPI
authentication.  This kind of issue is why my v1 handled GSSAPI at the
protocol level, not at the transport level.  I think we end up with
encrypted data in the buffer that's supposed to be decrypted, and since
the GSSAPI blob starts with \x00, it doesn't know what to do.

I'll cut a v6 with most of the changes we've talked about here.  It
should address this issue, but I suspect that no one will be happy about
how, since the client essentially needs to "un-read" some data.

As a side note, this would also explain why I can't reproduce the issue,
since I'm running in very low-latency environments (three VMs on my
laptop).

> Also, something that is missing is the parametrization that has been
> discussed the last time this patch was on the mailing list. Where is
> the capacity to control if a client is connecting to a server that is
> performing encryption and downgrade to non-ecrypted connection should
> the server not be able to support it? Enforcing a client to require
> encryption support using pg_hba.conf was as well a good thing. Some
> more infrastructure is needed here, I thought that there was an
> agreement previously regarding that.

This does not match my impression of the discussion, but I would be
happy to be wrong about that since it is less work for me.

> Also, and to make the review a bit easier, it would be good to split
> the patch into smaller pieces (thanks for waiting on my input here,
> this became quite clear after looking at this code). Basically,
> pg_GSS_error is moved to a new file, bringing with it pg_GSS_recvauth
> because the error routine is being used by the new ones you are
> introducing: be_gssapi_write, etc. The split that this patch is doing
> is a bit confusing, all the GSS-related stuff is put within one single
> file:
> - read/write routines
> - authentication routine
> - constants
> - routines for error handling
> Mixing read/write routines with the authentication routine looks
> wrong, because the read-write routines just need to create a
> dependency with for example be-secure.c on the backend. In short,
> where before authentication and secure read/writes after
> authentication get linked to each other, and create a dependency that
> did not exist before.
>
> For the sake of clarity I would suggest the following split:
> - be-gss-common.c, where all the constants and the error handling
> routine are located.
> - Let authrecv in auth.c
> - Move only the read/write routines to the new file be-[secure-]gssapi.c
> Splitting the patches into one that moves around the routines, and a
> second that introduces the new logic would bring more clarity.

So... auth.c is now going to depend on be-gss-common.c, and
be-secure-gssapi.c is also going to depend on be-gss-common.c.  That
doesn't seem better in terms of dependencies: now I'm creating two that
didn't exist before.

That said, I would believe that it makes everything clearer.  I will do
that shortly.

> I am wondering how this patch is tested to be honest.

We've already had this conversation; please let's stay constructive.