Re: [PATCH v5] GSSAPI encryption support

On Fri, Feb 26, 2016 at 5:02 AM, Robbie Harwood <rharwood@redhat.com> wrote:
> Michael Paquier <michael.paquier@gmail.com> writes:
>> 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.

I gave my opinion about the parametrization here a couple of months
back, and thought that it was rather a neat design. I still think so:
http://www.postgresql.org/message-id/CAB7nPqRGQ60PWLb-CLLrvMoQwPAbnu-961W+xGPvG62RMSkcZQ@mail.gmail.com
In short:
1) Introduction of new pg_hba parameter require_encrypt, defaulting to
off, enforcing the clients to have encryption. This way an
administrator of a new server can prevent connections of old clients
if he/she wants to have all the connections excrypted.
2) Client-side parameter, that you named previously gss_encrypt, to
let a client decide if he wishes to do encryption or not.

>> 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.

Let's be sure that we come out with something rock-solid here, the
code paths taken for authentication do not require an authenticated
user, so any bugs introduced could have dangerous consequences for the
backend.

> 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).

I'm doing my tests in single VM, with both krb5kdc and Postgres
running together.

>> 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.

OK, good to know. I had the opposite impression actually :) See above.

>> 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.

That's a personal suggestion on the matter. If you feel that the way
you did the separation is better, nothing prevents you from submitting
the patch as such. IMO making a clear separation between the
authentication code path and the read/write code path matters, and
this separation exists today. In any case, a patch doing the
refactoring, and a second patch adding the feature would greatly
facilitate the review. After looking at the patch once this is pretty
clear.
-- 
Michael