Re: [PATCH v5] GSSAPI encryption support

On Tue, Feb 16, 2016 at 2:45 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> David Steele <david(at)pgmasters(dot)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:
>>
>> 1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878
>> which I figured was recent enough for testing. I didn't bisect to find
>> the exact commit that broke it.
>
> It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a)
> for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`). I rebased it
> anyway and cut a v5 anyway, just to be sure. It's attached, and
> available on github as well:
> https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53

v5 is applying fine for me. There were two diff hunks in routine.sgml
but nothing to worry about.

>> 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.
>> Here was my testing methodology:
>>
>> a) Build Postgres from a455878 (without your patch), install/configure
>> Kerberos and get everything working. I was able the set the auth method
>> to gss in pg_hba.conf and logon successfully every time.
>>
>> b) On the same system rebuild Postgres from a455878 including your patch
>> and attempt authentication.
>>
>> The problems arose after step 2b. Sometimes I would try to logon twenty
>> times without success and sometimes it only take five or six attempts.
>> I was never able to logon successfully twice in a row.
>>
>> 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.

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

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.

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.

As In understood from the patch, enforcing encryption is not good
either, and any patched clients would not be able to 9.5 or older
servers. Regarding the parametrization, I liked the previous v1-v3
layout to control the encryption, and this allows to set up the
session context on backend and frontend at authentication. The
connection errors are also something to look at, I guess that David
and I ran into the same thing. I am wondering how this patch is tested
to be honest.
--
Michael