Re: [PATCH v1] GSSAPI encryption support - Mailing list pgsql-hackers
From | Michael Paquier |
---|---|
Subject | Re: [PATCH v1] GSSAPI encryption support |
Date | |
Msg-id | CAB7nPqTZDnRSGCj6JoiHKmXHV71DqoRH3fUajBxAFi9EW7=gDA@mail.gmail.com Whole thread Raw |
In response to | Re: [PATCH v1] GSSAPI encryption support (Robbie Harwood <rharwood@redhat.com>) |
List | pgsql-hackers |
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
pgsql-hackers by date: