Re: [PATCH v1] GSSAPI encryption support

On Sat, Oct 10, 2015 at 3:10 AM, Robbie Harwood wrote:
> Michael Paquier writes:
>>> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>>> [Andres' comments]
>>
>> Here are some comments on top of what Andres has mentioned.
>>
>> --- a/configure.in
>> +++ b/configure.in
>> @@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support],
>>    krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab"
>>  ])
>>  AC_MSG_RESULT([$with_gssapi])
>> +AC_SUBST(with_gssapi)
>>
>> I think that using a new configure variable like that with a dedicated
>> file fe-secure-gss.c and be-secure-gss.c has little sense done this
>> way, and that it would be more adapted to get everything grouped in
>> fe-auth.c for the frontend and auth.c for the backend, or move all the
>> GSSAPI-related stuff in its own file. I can understand the move
>> though: this is to imitate OpenSSL in a way somewhat similar to what
>> has been done for it with a rather generic set if routines, but with
>> GSSAPI that's a bit different, we do not have such a set of routines,
>> hence based on this argument moving it to its own file has little
>> sense. Now, a move that would make sense though is to move all the
>> GSSAPI stuff in its own file, for example pg_GSS_recvauth and
>> pg_GSS_error for the backend, and you should do the same for the
>> frontend with all the pg_GSS_* routines. This should be as well a
>> refactoring patch on top of the actual feature.
>
> My understanding is that frontend and backend code need to be separate
> (for linking), so it's automatically in two places. I really don't want
> to put encryption-related code in files called "auth.c" and "fe-auth.c"
> since those files are presumably for authentication, not encryption.
>
> I'm not sure what you mean about "rather generic set if routines";
> GSSAPI is a RFC-standardized interface.  I think I also don't understand
> the last half of your above paragraph.

src/interfaces/libpq/fe-auth.c contains the following set of routines
related to GSS (frontend code in libpq):
- pg_GSS_error_int
- pg_GSS_error
- pg_GSS_continue
- pg_GSS_startup
src/backend/libpq/auth.c contains the following routines related to
GSS (backend code):
- pg_GSS_recvauth
- pg_GSS_error
My point would be simply to move all those routines in two new files
dedicated to GSS, then add your new routines for encryption in it.
Still, the only reason why the OpenSSL routines have been moved out of
be-secure.c to be-secure-openssl.c is to allow other libraries to be
plugged into that, the primary target being SChannel on Windows. And
that's not the case of GSS, so I think that the separation done as in
your patch is not adapted.

>> diff --git a/src/interfaces/libpq/fe-secure-gss.c
>> b/src/interfaces/libpq/fe-secure-gss.c
>> new file mode 100644
>> index 0000000..afea9c3
>> --- /dev/null
>> +++ b/src/interfaces/libpq/fe-secure-gss.c
>> @@ -0,0 +1,92 @@
>> +#include <assert.h>
>> You should add a proper header to those new files.
>
> Sorry, what?

All the files in the source tree need to have a header like that:
/*-------------------------------------------------------------------------** file_name.c*      Description** Portions
Copyright(c) 2015, PostgreSQL Global Development Group** IDENTIFICATION*
path/to/file/file_name.c**-------------------------------------------------------------------------*/
-- 
Michael