From: Robbie Harwood <rharwood@redhat.com>
To: Michael Paquier <michael.paquier@gmail.com>,
 Andres Freund <andres@anarazel.de>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Subject: Re: [PATCH v1] GSSAPI encryption support
In-Reply-To: 
 <CAB7nPqTJD-tTrM1Vu8P55_4kKVeDX8DFz9v1D_XsQQvR_xA5qQ@mail.gmail.com>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com>
 <20151003161810.GD30738@alap3.anarazel.de>
 <CAB7nPqTJD-tTrM1Vu8P55_4kKVeDX8DFz9v1D_XsQQvR_xA5qQ@mail.gmail.com>
User-Agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1
 (x86_64-pc-linux-gnu)
Date: Fri, 09 Oct 2015 14:10:16 -0400
Message-ID: <jlgoag73oc7.fsf@thriss.redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
 micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--=-=-=
Content-Type: text/plain

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

> On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund <andres@anarazel.de> wrote:
>> Hi,
>>
>> I quickly read through the patch, trying to understand what exactly is
>> happening here. To me the way the patch is split doesn't make much sense
>> - I don't mind incremental patches, but right now the steps don't
>> individually make sense.
>
> I agree with Andres. While I looked a bit at this patch, I just had a
> look at them a whole block and not individually.

I'm hearing block from both of you!  Okay, if block is desired, I'll
squish for v3.  Sorry for the inconvenience.

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

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

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJWGAMJAAoJECUy+RdqlaRC67MQAKZeAMVR8G0/RPqNjiHb16bR
rVkHr2pNdde3V7MU6EnATYcNrGX8PuaHjwyHbyeEudRodlFI6POeXrK0Zfoe5gpk
zgWsVoN4moTZjEEnlvzr7jdTAK7wujdctPViCdvEKkiuXkiySt2Q8vIEzkNkh0c8
TOMYL5ewfOPgj+sjPXu4WLM7uq4QGnyJyDTdkbPALR6bs1hFQ3y0LgSXWMV84Wrh
uSERk9vYP7fb53j42yeoegrJF8hcPe1quVL20QW7nzjCgi+M+hirHPdDGoaWDsGw
9LLP2IIZAU9ycZiiRAEk31eIVRMRw1wgZTHo3q9Z6HxHa7Mpij+RSaImkAp9wmyT
PYVM6xf/vuTuEZPsIiKy8IP697V7Ss/FCsEtUpDus0kJzfVAKReuyshEud1HOf3F
6TJ+GB0/BWbHYqNHpsQTuk/IETYIFlWXupwt2xzNBijJ8YFV+ixaZKAQ2VW/bx8i
Uq9YUeWCQkgsY7J3Nfuc7sy2KJ3dD6pHbHMrzrzuuGz8fw7sRgxRxc21zhvkO6Jo
7kACyPzsvV5Q04zOeRFdW5spYPmS7FhfRDeYteWd3QTzS7mmALxmFv03JbDW2gfw
LlEAB9JNhBuXvP2LqOsoElvUW/BxEMs4pCFFqM+4DYXCWcCar/XPmYUHhjHxerlp
76o/QnLNEVqYk/ek5OQt
=YDlH
-----END PGP SIGNATURE-----
--=-=-=--