Re: [HACKERS] [PATCH v1] GSSAPI encryption support
On Thu, Mar 31, 2016 at 4:48 PM, Michael Paquier wrote: > [long review] Note as well that I have switched the patch as "waiting on author" for the time being. Missing symbols on Windows as well as crashes are pointing out that this should be returned with feedback for now (sorry for the late reviews btw). -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
On Thu, Mar 31, 2016 at 2:14 PM, Michael Paquier wrote: > On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood wrote: >> A new version of my GSSAPI encryption patchset is available, both in >> this email and on my github: >> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9 >> >> This version is intended to address David's review suggestions: >> >> - The only code change (not counting SGML and comments) is that I've >> resolved the pre-existing "XXX: Should we loop and read all messages?" >> comment in the affirmative by... looping and reading all messages in >> pg_GSS_error. Though commented on as part of the first patch, this is >> bundled with the rest of the auth cleanup since the first patch is >> code motion only. >> >> - Several changes to comments, documentation rewordings, and whitespace >> additions. I look forward to finding out what needs even more of the >> same treatment. Of all the changesets I've posted thus far, this >> might be the one for which it makes the most sense to see what changed >> by diffing from the previous changeset. > > Thanks for the new versions. For now I have been having a look at only 0001. > > The first thing I have noticed is that 0001 breaks the Windows build > using Visual Studio. When building without GSSAPI, fe-gssapi-common.c > and be-gssapi-common.c should be removed from the list of files used > so that's simple enough to fix. Now when GSSAPI build is enabled, > there are some declaration conflicts with your patch, leading to many > compilation errors. This took me some time to figure out but the cause > is this diff in be-gssapi-common.c: > +#include "libpq/be-gssapi-common.h" > + > +#include "postgres.h" > > postgres.h should be declared *before* be-gssapi-common.h. As I > suggested the refactoring and I guess you don't have a Windows > environment at hand, attached is a patch that fixes the build with and > without gssapi for Visual Studio, that can be applied on top of your > 0001. Feel free to rebase using it. > > Note for others: I had as well to patch the MSVC scripts because in > the newest installations of Kerberos: > - inc/ does not exist anymore, include/ does > - The current VS scripts ignore completely x64 builds, the libraries > linked to are for x86 unconditionally. > I am not sure if there are *any* people building the code with > Kerberos on Windows except I, but as things stand those scripts are > rather broken in my view. (Lonely feeling after typing that) > Now looking at 0002 and 0003... (Thanks for the split btw, this is far easier to look at) Patch 0002 has the same problems, because it introduces the gssapi-secure stuff (see attached patch that can be applied on top of 0002 fixing the windows build). When gss build is not enabled, compilation is able to work, now when gss is enabled... See below. + + gss_enc_require + Perhaps renaming that gss_require_encrypt? +++ b/src/backend/libpq/be-secure-gssapi.c [...] +#include "libpq/be-gssapi-common.h" + +#include "postgres.h" Those should be reversed. +Require GSSAPI encryption. Defaults to off, which enables GSSAPI +encryption only when both available and requested to maintain +compatability with old clients. This setting should be enabled unless +old clients are present. s/compatability/compatibility/ + +Require GSSAPI encryption support from the remote server when set. By +default, clients will fall back to not using GSSAPI encryption if the +server does not support encryption through GSSAPI. + Nitpick: the use of the markup for GSSAPI may be more adapted (I know, the docs are a bit lazy on that). + iov[0].iov_base = lenbuf; + iov[0].iov_len = 4; + iov[1].iov_base = output.value; + iov[1].iov_len = output.length; + + ret = writev(port->sock, iov, 2); writev and iovec are not present on Windows, so this code would never compile there, and it does not sound that this patch is a reason sufficient enough to drop support of GSSAPI on Windows. +static ssize_t +be_gssapi_should_crypto(Port *port) +{ + if (port->gss->ctx == GSS_C_NO_CONTEXT) + return 0; + return gss_encrypt; +} This should return a boolean, gss_encrypt being one. + if (!gss_encrypt && port->hba->require_encrypt) + { + ereport(FATAL, (errcode(ERRCODE_INVALID_AUTHORIZATION_SPECIFICATION), + errmsg("GSSAPI encryption required from user \"%s\"", + port->user_name))); + } Could be clearer here, with some error detail message, like: "User has requested GSSAPI encryption, but server disallows it". +static void +assign_gss_encrypt(bool newval, void *extra) +{ + gss_encrypt = newval; +} Assigning a variable is done by the GUC machinery, you don't need that. + { + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY, +gettext_noop("Require encryption for all GSSAPI connections."), +NULL, +GUC_NO
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
On Wed, Mar 30, 2016 at 1:01 PM, Robbie Harwood wrote: > A new version of my GSSAPI encryption patchset is available, both in > this email and on my github: > https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9 > > This version is intended to address David's review suggestions: > > - The only code change (not counting SGML and comments) is that I've > resolved the pre-existing "XXX: Should we loop and read all messages?" > comment in the affirmative by... looping and reading all messages in > pg_GSS_error. Though commented on as part of the first patch, this is > bundled with the rest of the auth cleanup since the first patch is > code motion only. > > - Several changes to comments, documentation rewordings, and whitespace > additions. I look forward to finding out what needs even more of the > same treatment. Of all the changesets I've posted thus far, this > might be the one for which it makes the most sense to see what changed > by diffing from the previous changeset. Thanks for the new versions. For now I have been having a look at only 0001. The first thing I have noticed is that 0001 breaks the Windows build using Visual Studio. When building without GSSAPI, fe-gssapi-common.c and be-gssapi-common.c should be removed from the list of files used so that's simple enough to fix. Now when GSSAPI build is enabled, there are some declaration conflicts with your patch, leading to many compilation errors. This took me some time to figure out but the cause is this diff in be-gssapi-common.c: +#include "libpq/be-gssapi-common.h" + +#include "postgres.h" postgres.h should be declared *before* be-gssapi-common.h. As I suggested the refactoring and I guess you don't have a Windows environment at hand, attached is a patch that fixes the build with and without gssapi for Visual Studio, that can be applied on top of your 0001. Feel free to rebase using it. Note for others: I had as well to patch the MSVC scripts because in the newest installations of Kerberos: - inc/ does not exist anymore, include/ does - The current VS scripts ignore completely x64 builds, the libraries linked to are for x86 unconditionally. I am not sure if there are *any* people building the code with Kerberos on Windows except I, but as things stand those scripts are rather broken in my view. Now looking at 0002 and 0003... -- Michael diff --git a/src/backend/libpq/be-gssapi-common.c b/src/backend/libpq/be-gssapi-common.c index eab68a5..dc27fa8 100644 --- a/src/backend/libpq/be-gssapi-common.c +++ b/src/backend/libpq/be-gssapi-common.c @@ -13,10 +13,10 @@ *- */ -#include "libpq/be-gssapi-common.h" - #include "postgres.h" +#include "libpq/be-gssapi-common.h" + #if defined(WIN32) && !defined(WIN32_ONLY_COMPILER) /* * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW diff --git a/src/tools/msvc/Mkvcbuild.pm b/src/tools/msvc/Mkvcbuild.pm index dbc09b8..923e339 100644 --- a/src/tools/msvc/Mkvcbuild.pm +++ b/src/tools/msvc/Mkvcbuild.pm @@ -163,12 +163,17 @@ sub mkvcbuild $postgres->AddLibrary('wldap32.lib') if ($solution->{options}->{ldap}); $postgres->FullExportDLL('postgres.lib'); - # The OBJS scraper doesn't know about ifdefs, so remove be-secure-openssl.c - # if building without OpenSSL + # The OBJS scraper doesn't know about ifdefs, so remove be-secure-openssl.c + # if building without OpenSSL, and be-gssapi-common.c when building with + # GSSAPI. if (!$solution->{options}->{openssl}) { $postgres->RemoveFile('src/backend/libpq/be-secure-openssl.c'); } + if (!$solution->{options}->{gss}) + { + $postgres->RemoveFile('src/backend/libpq/be-gssapi-common.c'); + } my $snowball = $solution->AddProject('dict_snowball', 'dll', '', 'src/backend/snowball'); @@ -218,12 +223,17 @@ sub mkvcbuild 'src/interfaces/libpq/libpq.rc'); $libpq->AddReference($libpgport); - # The OBJS scraper doesn't know about ifdefs, so remove fe-secure-openssl.c - # if building without OpenSSL + # The OBJS scraper doesn't know about ifdefs, so remove fe-secure-openssl.c + # if building without OpenSSL, and fe-gssapi-common.c when building with + # GSSAPI if (!$solution->{options}->{openssl}) { $libpq->RemoveFile('src/interfaces/libpq/fe-secure-openssl.c'); } + if (!$solution->{options}->{gss}) + { + $libpq->RemoveFile('src/interfaces/libpq/fe-gssapi-common.c'); + } my $libpqwalreceiver = $solution->AddProject('libpqwalreceiver', 'dll', '', -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
Hello friends, A new version of my GSSAPI encryption patchset is available, both in this email and on my github: https://github.com/frozencemetery/postgres/tree/feature/gssencrypt9 This version is intended to address David's review suggestions: - The only code change (not counting SGML and comments) is that I've resolved the pre-existing "XXX: Should we loop and read all messages?" comment in the affirmative by... looping and reading all messages in pg_GSS_error. Though commented on as part of the first patch, this is bundled with the rest of the auth cleanup since the first patch is code motion only. - Several changes to comments, documentation rewordings, and whitespace additions. I look forward to finding out what needs even more of the same treatment. Of all the changesets I've posted thus far, this might be the one for which it makes the most sense to see what changed by diffing from the previous changeset. Thanks! From 3b62e99de16f2c4600d0bb02f3626e5157ecdc6c Mon Sep 17 00:00:00 2001 From: Robbie Harwood Date: Fri, 26 Feb 2016 16:07:05 -0500 Subject: [PATCH 1/3] Move common GSSAPI code into its own files On both the frontend and backend, prepare for GSSAPI encryption suport by moving common code for error handling into a common file. Other than build-system changes, no code changes occur in this patch. --- configure | 2 + configure.in| 1 + src/Makefile.global.in | 1 + src/backend/libpq/Makefile | 4 ++ src/backend/libpq/auth.c| 63 +--- src/backend/libpq/be-gssapi-common.c| 74 + src/include/libpq/be-gssapi-common.h| 26 src/interfaces/libpq/Makefile | 4 ++ src/interfaces/libpq/fe-auth.c | 48 + src/interfaces/libpq/fe-gssapi-common.c | 63 src/interfaces/libpq/fe-gssapi-common.h | 21 ++ 11 files changed, 198 insertions(+), 109 deletions(-) create mode 100644 src/backend/libpq/be-gssapi-common.c create mode 100644 src/include/libpq/be-gssapi-common.h create mode 100644 src/interfaces/libpq/fe-gssapi-common.c create mode 100644 src/interfaces/libpq/fe-gssapi-common.h diff --git a/configure b/configure index b3f3abe..a5bd629 100755 --- a/configure +++ b/configure @@ -713,6 +713,7 @@ with_systemd with_selinux with_openssl krb_srvtab +with_gssapi with_python with_perl with_tcl @@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; } + # # Kerberos configuration parameters # diff --git a/configure.in b/configure.in index 0bd90d7..4fd8f05 100644 --- 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) AC_SUBST(krb_srvtab) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index e94d6a5..3dbc5c2 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -183,6 +183,7 @@ with_perl = @with_perl@ with_python = @with_python@ with_tcl = @with_tcl@ with_openssl = @with_openssl@ +with_gssapi = @with_gssapi@ with_selinux = @with_selinux@ with_systemd = @with_systemd@ with_libxml = @with_libxml@ diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile index 09410c4..a8cc9a0 100644 --- a/src/backend/libpq/Makefile +++ b/src/backend/libpq/Makefile @@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes) OBJS += be-secure-openssl.o endif +ifeq ($(with_gssapi),yes) +OBJS += be-gssapi-common.o +endif + include $(top_srcdir)/src/backend/common.mk diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 57c2f48..73d493e 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -136,11 +136,7 @@ bool pg_krb_caseins_users; * */ #ifdef ENABLE_GSS -#if defined(HAVE_GSSAPI_H) -#include -#else -#include -#endif +#include "libpq/be-gssapi-common.h" static int pg_GSS_recvauth(Port *port); #endif /* ENABLE_GSS */ @@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail) */ #ifdef ENABLE_GSS -#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER) -/* - * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW - * that contain the OIDs required. Redefine here, values copied - * from src/athena/auth/krb5/src/lib/gssapi/generic/gssapi_generic.c - */ -static const gss_OID_desc GSS_C_NT_USER_NAME_desc = -{10, (void *) "\x2a\x86\x48\x86\xf7\x12\x01\x02\x01\x02"}; -static GSS_DLLIMP gss_OID GSS_C_NT_USER_NAME = &GSS_C_NT_USER_NAME_desc; -#endif - - -static void -pg_GSS_error(int severity, char *errmsg, OM_uint32 maj_stat, OM_uint32 min_stat) -{ - gss_buffer_desc gmsg; - OM_uint32 lmin_s, -msg_ctx; - char msg_major[128], -msg_minor[128]; - - /* Fetch major status message */ - msg_ct
Re: [HACKERS] [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 000..afea9c3 >> --- /dev/null >> +++ b/src/interfaces/libpq/fe-secure-gss.c >> @@ -0,0 +1,92 @@ >> +#include >> 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
Michael Paquier writes: > On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund 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 000..afea9c3 > --- /dev/null > +++ b/src/interfaces/libpq/fe-secure-gss.c > @@ -0,0 +1,92 @@ > +#include > You should add a proper header to those new files. Sorry, what? signature.asc Description: PGP signature
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
Andres Freund writes: > Hi, Hi, thanks for the review; I really appreciate your time in going through this. I have questions about some of your comments, so I'll wait a bit before sending a v3. (By the way, there is a v2 of this I've already posted, though you seem to have replied to the v1. The only difference is in documentation, though.) > 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. That's fair. Can you suggest a better organization? > On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote: >> +#include > > postgres.h should be the first header included. Okay, will fix. >> +size_t >> +be_gss_encrypt(Port *port, char msgtype, const char **msgptr, size_t len) >> +{ >> +OM_uint32 major, minor; >> +gss_buffer_desc input, output; >> +uint32 len_n; >> +int conf; >> +char *ptr = *((char **)msgptr); > > trivial nitpick, missing spaces... Got it. >> +int >> +be_gss_inplace_decrypt(StringInfo inBuf) >> +{ >> +OM_uint32 major, minor; >> +gss_buffer_desc input, output; >> +int qtype, conf; >> +size_t msglen = 0; >> + >> +input.length = inBuf->len; >> +input.value = inBuf->data; >> +output.length = 0; >> +output.value = NULL; >> + >> +major = gss_unwrap(&minor, MyProcPort->gss->ctx, &input, &output, >> + &conf, NULL); >> +if (GSS_ERROR(major)) >> +{ >> +pg_GSS_error(ERROR, gettext_noop("wrapping GSS message failed"), >> + major, minor); >> +return -1; >> +} >> +else if (conf == 0) >> +{ >> +ereport(COMMERROR, >> +(errcode(ERRCODE_PROTOCOL_VIOLATION), >> + errmsg("Expected GSSAPI confidentiality but it >> was not received"))); >> +return -1; >> +} > > Hm. Aren't we leaking the gss buffer here? > >> +qtype = ((char *)output.value)[0]; /* first byte is message type */ >> +inBuf->len = output.length - 5; /* message starts */ >> + >> +memcpy((char *)&msglen, ((char *)output.value) + 1, 4); >> +msglen = ntohl(msglen); >> +if (msglen - 4 != inBuf->len) >> +{ >> +ereport(COMMERROR, >> +(errcode(ERRCODE_PROTOCOL_VIOLATION), >> + errmsg("Length value inside GSSAPI-encrypted >> packet was malformed"))); >> +return -1; >> +} > > and here? > > Arguably it doesn't matter that much, since we'll usually disconnect > around here, but ... Probably better to be cautious about it. Will fix. >> diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c >> index a4b37ed..5a929a8 100644 >> --- a/src/backend/libpq/pqcomm.c >> +++ b/src/backend/libpq/pqcomm.c >> @@ -1485,6 +1485,19 @@ socket_putmessage(char msgtype, const char *s, size_t >> len) >> { >> if (DoingCopyOut || PqCommBusy) >> return 0; >> + >> +#ifdef ENABLE_GSS >> +/* Do not wrap auth requests. */ >> +if (MyProcPort->hba->auth_method == uaGSS && gss_encrypt && >> +msgtype != 'R' && msgtype != 'g') >> +{ >> +len = be_gss_encrypt(MyProcPort, msgtype, &s, len); >> +if (len < 0) >> +goto fail; >> +msgtype = 'g'; >> +} >> +#endif > > Putting encryption specific code here feels rather wrong to me. Do you have a suggestion about where this code *should* go? I need to filter on the message type since some can't be encrypted. I was unable to find another place, but I may have missed it. >> diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h >> index 6171ef3..58712fc 100644 >> --- a/src/include/libpq/libpq-be.h >> +++ b/src/include/libpq/libpq-be.h >> @@ -30,6 +30,8 @@ >> #endif >> >> #ifdef ENABLE_GSS >> +#include "lib/stringinfo.h" >> + > > Conditionally including headers providing generic infrastructure strikes > me as a recipe for build failures in different configurations. That's fair, will fix. >> /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */ >> diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h >> index c408e5b..e788cc8 100644 >> --- a/src/include/libpq/libpq.h >> +++ b/src/include/libpq/libpq.h >> @@ -99,4 +99,8 @@ extern char *SSLCipherSuites; >> extern char *SSLECDHCurve; >> extern bool SSLPreferServerCiphers; >> >> +#ifdef ENABLE_GSS >> +extern bool gss_encrypt; >> +#endif > >> --- a/src/backend/utils/misc/guc.c >> +++ b/src/backend/utils/misc/guc.c >> >> +#ifdef ENABLE_GSS >> +static void assign_gss_encrypt(bool newval, void *extra); >> +#endif >> + >> >> /* >> * Options for enum values defined in this module. >> @@ -1618,6 +1622,15 @@ static struct config_bool ConfigureNamesBool[] = >> NULL, NULL
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
On Sun, Oct 4, 2015 at 1:18 AM, Andres Freund 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. > 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. diff --git a/src/interfaces/libpq/fe-secure-gss.c b/src/interfaces/libpq/fe-secure-gss.c new file mode 100644 index 000..afea9c3 --- /dev/null +++ b/src/interfaces/libpq/fe-secure-gss.c @@ -0,0 +1,92 @@ +#include You should add a proper header to those new files. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
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. On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote: > +#include postgres.h should be the first header included. > +size_t > +be_gss_encrypt(Port *port, char msgtype, const char **msgptr, size_t len) > +{ > + OM_uint32 major, minor; > + gss_buffer_desc input, output; > + uint32 len_n; > + int conf; > + char *ptr = *((char **)msgptr); trivial nitpick, missing spaces... > +int > +be_gss_inplace_decrypt(StringInfo inBuf) > +{ > + OM_uint32 major, minor; > + gss_buffer_desc input, output; > + int qtype, conf; > + size_t msglen = 0; > + > + input.length = inBuf->len; > + input.value = inBuf->data; > + output.length = 0; > + output.value = NULL; > + > + major = gss_unwrap(&minor, MyProcPort->gss->ctx, &input, &output, > +&conf, NULL); > + if (GSS_ERROR(major)) > + { > + pg_GSS_error(ERROR, gettext_noop("wrapping GSS message failed"), > + major, minor); > + return -1; > + } > + else if (conf == 0) > + { > + ereport(COMMERROR, > + (errcode(ERRCODE_PROTOCOL_VIOLATION), > + errmsg("Expected GSSAPI confidentiality but it > was not received"))); > + return -1; > + } Hm. Aren't we leaking the gss buffer here? > + qtype = ((char *)output.value)[0]; /* first byte is message type */ > + inBuf->len = output.length - 5; /* message starts */ > + > + memcpy((char *)&msglen, ((char *)output.value) + 1, 4); > + msglen = ntohl(msglen); > + if (msglen - 4 != inBuf->len) > + { > + ereport(COMMERROR, > + (errcode(ERRCODE_PROTOCOL_VIOLATION), > + errmsg("Length value inside GSSAPI-encrypted > packet was malformed"))); > + return -1; > + } and here? Arguably it doesn't matter that much, since we'll usually disconnect around here, but ... > + memcpy(inBuf->data, ((char *)output.value) + 5, inBuf->len); > + inBuf->data[inBuf->len] = '\0'; /* invariant */ > + gss_release_buffer(&minor, &output); > + > + return qtype; > +} > diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c > index a4b37ed..5a929a8 100644 > --- a/src/backend/libpq/pqcomm.c > +++ b/src/backend/libpq/pqcomm.c > @@ -1485,6 +1485,19 @@ socket_putmessage(char msgtype, const char *s, size_t > len) > { > if (DoingCopyOut || PqCommBusy) > return 0; > + > +#ifdef ENABLE_GSS > + /* Do not wrap auth requests. */ > + if (MyProcPort->hba->auth_method == uaGSS && gss_encrypt && > + msgtype != 'R' && msgtype != 'g') > + { > + len = be_gss_encrypt(MyProcPort, msgtype, &s, len); > + if (len < 0) > + goto fail; > + msgtype = 'g'; > + } > +#endif Putting encryption specific code here feels rather wrong to me. > diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h > index 6171ef3..58712fc 100644 > --- a/src/include/libpq/libpq-be.h > +++ b/src/include/libpq/libpq-be.h > @@ -30,6 +30,8 @@ > #endif > > #ifdef ENABLE_GSS > +#include "lib/stringinfo.h" > + Conditionally including headers providing generic infrastructure strikes me as a recipe for build failures in different configurations. > /* TCP keepalives configuration. These are no-ops on an AF_UNIX socket. */ > diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h > index c408e5b..e788cc8 100644 > --- a/src/include/libpq/libpq.h > +++ b/src/include/libpq/libpq.h > @@ -99,4 +99,8 @@ extern char *SSLCipherSuites; > extern char *SSLECDHCurve; > extern bool SSLPreferServerCiphers; > > +#ifdef ENABLE_GSS > +extern bool gss_encrypt; > +#endif > --- a/src/backend/utils/misc/guc.c > +++ b/src/backend/utils/misc/guc.c > > +#ifdef ENABLE_GSS > +static void assign_gss_encrypt(bool newval, void *extra); > +#endif > + > > /* > * Options for enum values defined in this module. > @@ -1618,6 +1622,15 @@ static struct config_bool ConfigureNamesBool[] = > NULL, NULL, NULL > }, > > + { > + {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY, > + gettext_noop("Whether client wants encryption for this > connection."), > + NULL, > + GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE > + }, > + &gss_encrypt, false, NULL, assign_gss_encrypt, NULL > + }, > + > /* End-of-list marker */ > { > {NULL, 0, 0, NULL, NULL}, NULL, false, NULL, NULL, NULL > @@ -10114,4 +10127,10 @@ show_log_file_mode(void) > return buf; > } The guc should always
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
On Sat, Aug 22, 2015 at 4:06 AM, Robbie Harwood wrote: > > Michael Paquier writes: > > Going through the docs, the overall approach taken by the patch looks neat, > > and the default values as designed for both the client and the server are > > good things to do. Now actually looking at the code I am suspecting that > > some code portions could be largely simplified in the authentication > > protocol code, though I don't have the time yet to look at that in details. > > If there are ways to make it simpler without sacrificing clarity, I > welcome them. Fresh eyes could definitely help with that! I'll look at that more at next week or the week after. > > Also, when trying to connect with GSSAPI, I found the following problem: > > psql: lost synchronization with server: got message type "S", length 22 > > This happens whatever the value of require_encrypt on server-side is, > > either 0 or 1. > > Well that's not good! Since I'm not seeing this failure (even after > rebuilding my setup with patches applied to master), can you give me > more information here? Since it's independent of require_encrypt, can > you verify it doesn't happen on master without my patches? Well, I imagine that I have done nothing complicated... I have simply set up a Kerberos KDC on a dev box, created necessary credentials on this box in a keytab file that I have used afterwards to initialize a Kerberos context with kinit for the psql client. On master things worked fine, I was able to connect via gssapi. But with your patch the communication protocol visibly lost track of the messages. I took a memo about that, it's a bit rough, does not use pg_ident, but if that can help: http://michael.otacoo.com/manuals/postgresql/kerberos/ > What messages went over the wire to/from the server before this occurred (and > what was it trying to send at the time)? I haven't checked what were the messages sent over the network yet. > Did you have valid credentials? Yep. I just tried on master before switching to a build with your patch that failed. After moving back to master things worked again. -- Michael -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
Michael Paquier writes: > On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood wrote: > >> There are 8 commits in this series; I have tried to err on the side of >> creating too much separation rather than too little. A patch for each >> is attached. This is v1 of the series. > > I just had a quick look at this patch, and here are some comments: Hi! Thanks for taking it for a spin. > + > +If the client has probed GSSAPI encryption support and > +the connection is GSSAPI-authenticated, then after the > +server sends AuthenticationOk, all traffic between the client and server > +will be GSSAPI-encrypted. Because > +GSSAPI does not provide framing, > +GSSAPI-encrypted messages are modeled after protocol-3 > +messages: the first byte is the caracter g, then four bytes of length, > and > +then an encrypted message. > + > > Message formats should be described in protocol.sgml in the section for > message formats. ACK. In next version of patch. > + network. In the pg_hba.conf file, the GSS authenticaion > + method has a parameter to require encryption; otherwise, connections > + will be encrypted if available and requiested by the client. On the > s/authenticaion/authentication > s/requiested/requested > > +Whether to require GSSAPI encryption. Default is off, which causes > +GSSAPI encryption to be enabled if available and requested for > +compatability with old clients. It is recommended to set this > unless > +old clients are present. > s/compatability/compatibility Thanks for catching these. They'll be included in a new version of the series, which I'll post once you and I have resolved the issue at the bottom. > Going through the docs, the overall approach taken by the patch looks neat, > and the default values as designed for both the client and the server are > good things to do. Now actually looking at the code I am suspecting that > some code portions could be largely simplified in the authentication > protocol code, though I don't have the time yet to look at that in details. If there are ways to make it simpler without sacrificing clarity, I welcome them. Fresh eyes could definitely help with that! > Also, when trying to connect with GSSAPI, I found the following problem: > psql: lost synchronization with server: got message type "S", length 22 > This happens whatever the value of require_encrypt on server-side is, > either 0 or 1. Well that's not good! Since I'm not seeing this failure (even after rebuilding my setup with patches applied to master), can you give me more information here? Since it's independent of require_encrypt, can you verify it doesn't happen on master without my patches? What messages went over the wire to/from the server before this occurred (and what was it trying to send at the time)? Did you have valid credentials? signature.asc Description: PGP signature
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood wrote: > Hello -hackers, > > As previously discussed on this list, I have coded up GSSAPI encryption > support. If it is easier for anyone, this code is also available for > viewing on my github: > > https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt > > Fallback support is present in both directions for talking to old client > and old servers; GSSAPI encryption is by default auto-upgraded to where > available (for compatibility), but both client and server contain > settings for requiring it. > > There are 8 commits in this series; I have tried to err on the side of > creating too much separation rather than too little. A patch for each > is attached. This is v1 of the series. > I just had a quick look at this patch, and here are some comments: + +If the client has probed GSSAPI encryption support and +the connection is GSSAPI-authenticated, then after the +server sends AuthenticationOk, all traffic between the client and server +will be GSSAPI-encrypted. Because +GSSAPI does not provide framing, +GSSAPI-encrypted messages are modeled after protocol-3 +messages: the first byte is the caracter g, then four bytes of length, and +then an encrypted message. + Message formats should be described in protocol.sgml in the section for message formats. + network. In the pg_hba.conf file, the GSS authenticaion + method has a parameter to require encryption; otherwise, connections + will be encrypted if available and requiested by the client. On the s/authenticaion/authentication s/requiested/requested +Whether to require GSSAPI encryption. Default is off, which causes +GSSAPI encryption to be enabled if available and requested for +compatability with old clients. It is recommended to set this unless +old clients are present. s/compatability/compatibility Going through the docs, the overall approach taken by the patch looks neat, and the default values as designed for both the client and the server are good things to do. Now actually looking at the code I am suspecting that some code portions could be largely simplified in the authentication protocol code, though I don't have the time yet to look at that in details. Also, when trying to connect with GSSAPI, I found the following problem: psql: lost synchronization with server: got message type "S", length 22 This happens whatever the value of require_encrypt on server-side is, either 0 or 1. Regards, -- Michael
Re: [HACKERS] [PATCH v1] GSSAPI encryption support
Robbie, * Robbie Harwood (rharw...@redhat.com) wrote: > As previously discussed on this list, I have coded up GSSAPI encryption > support. If it is easier for anyone, this code is also available for > viewing on my github: > https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt > > Fallback support is present in both directions for talking to old client > and old servers; GSSAPI encryption is by default auto-upgraded to where > available (for compatibility), but both client and server contain > settings for requiring it. > > There are 8 commits in this series; I have tried to err on the side of > creating too much separation rather than too little. A patch for each > is attached. This is v1 of the series. Excellent, thanks! I've got other things to tend to at the moment, but this is definitely something I'm interested in and will work on (likely in August). If we could get a reviewer or two to take a look and take the patch out for a test-drive, at least, that would be a huge help. Thanks again! Stephen signature.asc Description: Digital signature
[HACKERS] [PATCH v1] GSSAPI encryption support
Hello -hackers, As previously discussed on this list, I have coded up GSSAPI encryption support. If it is easier for anyone, this code is also available for viewing on my github: https://github.com/postgres/postgres/compare/master...frozencemetery:feature/gssencrypt Fallback support is present in both directions for talking to old client and old servers; GSSAPI encryption is by default auto-upgraded to where available (for compatibility), but both client and server contain settings for requiring it. There are 8 commits in this series; I have tried to err on the side of creating too much separation rather than too little. A patch for each is attached. This is v1 of the series. Thanks! From f506ba6ab6755f56c8aadba7d72a8839d5fbc0d9 Mon Sep 17 00:00:00 2001 From: "Robbie Harwood (frozencemetery)" Date: Mon, 8 Jun 2015 19:27:45 -0400 Subject: build: Define with_gssapi for use in Makefiles This is needed in order to control build of GSSAPI components. --- configure | 2 ++ configure.in | 1 + src/Makefile.global.in | 1 + 3 files changed, 4 insertions(+) diff --git a/configure b/configure index 0407c4f..b9bab06 100755 --- a/configure +++ b/configure @@ -711,6 +711,7 @@ with_uuid with_selinux with_openssl krb_srvtab +with_gssapi with_python with_perl with_tcl @@ -5452,6 +5453,7 @@ $as_echo "$with_gssapi" >&6; } + # # Kerberos configuration parameters # diff --git a/configure.in b/configure.in index 1de41a2..113bd65 100644 --- a/configure.in +++ b/configure.in @@ -635,6 +635,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) AC_SUBST(krb_srvtab) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index c583b44..e50c87d 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -167,6 +167,7 @@ with_perl = @with_perl@ with_python = @with_python@ with_tcl = @with_tcl@ with_openssl = @with_openssl@ +with_gssapi = @with_gssapi@ with_selinux = @with_selinux@ with_libxml = @with_libxml@ with_libxslt = @with_libxslt@ -- 2.1.4 From d5b973752968f87c9bb2ff9434d523657eb4ba67 Mon Sep 17 00:00:00 2001 From: "Robbie Harwood (frozencemetery)" Date: Mon, 8 Jun 2015 20:16:42 -0400 Subject: client: Disable GSS encryption on old servers --- src/interfaces/libpq/fe-connect.c | 34 -- src/interfaces/libpq/fe-protocol3.c | 5 + src/interfaces/libpq/libpq-int.h| 1 + 3 files changed, 38 insertions(+), 2 deletions(-) diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c index a45f4cb..c6c551a 100644 --- a/src/interfaces/libpq/fe-connect.c +++ b/src/interfaces/libpq/fe-connect.c @@ -91,8 +91,9 @@ static int ldapServiceLookup(const char *purl, PQconninfoOption *options, * application_name in a startup packet. We hard-wire the value rather * than looking into errcodes.h since it reflects historical behavior * rather than that of the current code. + * Servers that do not support GSS encryption will also return this error. */ -#define ERRCODE_APPNAME_UNKNOWN "42704" +#define ERRCODE_UNKNOWN_PARAM "42704" /* This is part of the protocol so just define it */ #define ERRCODE_INVALID_PASSWORD "28P01" @@ -2552,6 +2553,35 @@ keep_going: /* We will come back to here until there is if (res->resultStatus != PGRES_FATAL_ERROR) appendPQExpBufferStr(&conn->errorMessage, libpq_gettext("unexpected message from server during startup\n")); +#ifdef ENABLE_GSS + else if (!conn->gss_disable_enc) + { + /* + * We tried to request GSS encryption, but the server + * doesn't support it. Hang up and try again. A + * connection that doesn't support appname will also + * not support GSSAPI encryption, so this check goes + * before that check. See comment below. + */ + const char *sqlstate; + + sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE); + if (sqlstate && + strcmp(sqlstate, ERRCODE_UNKNOWN_PARAM) == 0) + { + OM_uint32 minor; + + PQclear(res); + conn->gss_disable_enc = true; + /* Must drop the old connection */ + pqDropConnection(conn); + conn->status = CONNECTION_NEEDED; + gss_delete_sec_context(&minor, &conn->gctx, + GSS_C_NO_BUFFER); + goto keep_going; + } + } +#endif else if (conn->send_appname && (conn->appname || conn->fbappname)) { @@ -2569,7 +2599,7 @@ keep_going: /* We will come back to here until there is sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE); if (sqlstate && - strcmp(sqlstate, ERRCODE_APPNAME_UNKNOWN) == 0) + strcmp(sqlstate, ERRCODE_UNKNOWN_PARAM) == 0) { PQclear(res); conn->send_appname = false; diff --git a/src/interfaces/libpq/fe-protocol3.c b/src/interfaces/libpq/fe-protocol3.c index a847f08