Re: [HACKERS] [PATCH v1] GSSAPI encryption support

2016-03-31 Thread Michael Paquier 
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

2016-03-31 Thread Michael Paquier 
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

Re: [HACKERS] [PATCH v1] GSSAPI encryption support

2016-03-30 Thread Michael Paquier 
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

2016-03-29 Thread Robbie Harwood 
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 = _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

Re: [HACKERS] [PATCH v1] GSSAPI encryption support

2015-10-12 Thread Michael Paquier 
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

2015-10-09 Thread Robbie Harwood 
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

2015-10-09 Thread Robbie Harwood 
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(, MyProcPort->gss->ctx, , ,
>> +   , 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 *), ((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, , 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, NULL
>>

Re: [HACKERS] [PATCH v1] GSSAPI encryption support

2015-10-06 Thread Michael Paquier 
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

2015-10-03 Thread Andres Freund 
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(, MyProcPort->gss->ctx, , ,
> +, 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 *), ((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(, );
> +
> + 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, , 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
> + },
> + _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 be present, not just when gss is built in. It

Re: [HACKERS] [PATCH v1] GSSAPI encryption support

2015-08-21 Thread Michael Paquier 
On Sat, Aug 22, 2015 at 4:06 AM, Robbie Harwood wrote:

 Michael Paquier michael.paqu...@gmail.com 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

2015-08-21 Thread Michael Paquier 
On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood rharw...@redhat.com 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:
+   para
+If the client has probed acronymGSSAPI/acronym encryption support
and
+the connection is acronymGSSAPI/acronym-authenticated, then after
the
+server sends AuthenticationOk, all traffic between the client and
server
+will be acronymGSSAPI/acronym-encrypted. Because
+acronymGSSAPI/acronym does not provide framing,
+acronymGSSAPI/acronym-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.
+   /para
Message formats should be described in protocol.sgml in the section for
message formats.

+  network. In the filenamepg_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

2015-08-21 Thread Robbie Harwood 
Michael Paquier michael.paqu...@gmail.com writes:

 On Fri, Jul 3, 2015 at 3:22 AM, Robbie Harwood rharw...@redhat.com 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.

 +   para
 +If the client has probed acronymGSSAPI/acronym encryption support and
 +the connection is acronymGSSAPI/acronym-authenticated, then after the
 +server sends AuthenticationOk, all traffic between the client and server
 +will be acronymGSSAPI/acronym-encrypted. Because
 +acronymGSSAPI/acronym does not provide framing,
 +acronymGSSAPI/acronym-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.
 +   /para

 Message formats should be described in protocol.sgml in the section for
 message formats.

ACK.  In next version of patch.

 +  network. In the filenamepg_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

2015-07-06 Thread Stephen Frost 
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

2015-07-02 Thread Robbie Harwood 
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) rharw...@redhat.com
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) rharw...@redhat.com
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