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

[David Steele]

On 3/29/16 5:05 PM, Robbie Harwood wrote:
> David Steele  writes:
> 
>> On 3/20/16 12:09 AM, Robbie Harwood wrote:
>>
>>> A new version of my GSSAPI encryption patchset is available
>>
>> Here's a more thorough review:
> 
> Thanks for the review!  To keep this a manageable size, I'm going to
> trim pretty heavily.  If I haven't replied to something, please
> interpret it to mean that I think it's a good suggestion and will
> fix/change in v9.
> 
>> +++ b/doc/src/sgml/runtime.sgml
>>
>> I think you mean postgresql.conf?
> 
> Sorry, what does this review comment go with?

Oops! That was a comment I made then realized I had misunderstood what
was going on.  I removed the code but not the comment apparently.

Thanks for the other clarifications.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

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

[Robbie Harwood]

David Steele  writes:

> On 3/20/16 12:09 AM, Robbie Harwood wrote:
>
>> A new version of my GSSAPI encryption patchset is available
>
> Here's a more thorough review:

Thanks for the review!  To keep this a manageable size, I'm going to
trim pretty heavily.  If I haven't replied to something, please
interpret it to mean that I think it's a good suggestion and will
fix/change in v9.

> +++ b/doc/src/sgml/runtime.sgml
>
> I think you mean postgresql.conf?

Sorry, what does this review comment go with?

> +++ b/src/backend/libpq/auth.c
>
> +  * In most cases, we do not need to send AUTH_REQ_OK until we are ready
> +  * for queries, but if we are doing GSSAPI encryption that request must 
> go
> +  * out now.
>
> Why?

Because otherwise we will send the connection spew (connection
parameters and such) unencrypted, since it will get grouped with the
AUTH_REQ_OK packet.  I'll note this in the comment.

> + ret = be_gssapi_should_crypto(port);
>
> Add LF here.
>
>
> + port->gss->writebuf.cursor += ret;
>
> And here.
>
> + /* need to be called again */
>
> And here.  Well, you get the idea.

Sorry, what is LF?  ASCII newline?

> * PATCH 3 - GSSAPI authentication cleanup
>
> +++ b/src/backend/libpq/auth.c
>
> +  * GSS_C_REPLAY_FLAG and GSS_C_SEQUENCE_FLAG are missing for 
> compatability
> +  * with older clients and should be added in as soon as possible.
>
> Please elaborate here.  Why should they be added and what functionality 
> is missing before they are.

It's request reply detection and out-of-sequence detection.  We can't
require them because old clients don't request them, and so would be
unable to connect.  (There's some history of this in the last couple
versions I've posted, but it's not really interesting beyond "it doesn't
work".)  I will clarify this comment.

> +++ b/src/backend/libpq/be-gssapi-common.c
>
> -#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
>
> Can you explain why it's OK to remove this now?  I can see you've 
> replaced it in gss_init_sec_context() with GSS_C_MUTUAL_FLAG.  Have you 
> tested this on Win32?

This comment is old enough that it references sources from Athena.  If
this is in fact a current krb5 bug (which I doubt, since MIT tests
windows rather heavily), then it should be filed against krb5 instead of
kludged around here.  I do not however have win32 machines to test this
with.  (GSS_C_MUTUAL_FLAG is unrelated; it just requests that the client
and server are both authenticated to each other.)

Thanks,
--Robbie


signature.asc
Description: PGP signature

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

[David Steele]

Hi Robbie,

On 3/20/16 12:09 AM, Robbie Harwood wrote:


A new version of my GSSAPI encryption patchset is available


Here's a more thorough review:

* PATCH - Move common GSSAPI code into its own files

diff --git a/src/backend/libpq/be-gssapi-common.c

+   if (msg_ctx)
+
+   /*
+* More than one message available. XXX: Should we loop and 
read all
+* messages? (same below)
+*/

It seems like it would be a good idea to pull all error messages if only 
for debugging purposes.


+   /* Fetch mechanism minor status message */
+   msg_ctx = 0;
+   gss_display_status(_s, min_stat, GSS_C_MECH_CODE,
+  GSS_C_NO_OID, _ctx, );
+   strlcpy(msg_minor, gmsg.value, sizeof(msg_minor));
+   gss_release_buffer(_s, );
+
+   if (msg_ctx)
+   ereport(WARNING,
+   (errmsg_internal("incomplete GSS minor error 
report")));

Same here.

* PATCH - Connection encryption support for GSSAPI

+++ b/doc/src/sgml/client-auth.sgml

 data sent over the database connection will be sent unencrypted unless
-SSL is used.
+SSL is used, or GSSAPI encryption
+is in use.

Perhaps "... unless SSL or GSSAPI encryption is used."

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

Some rewording:

Require GSSAPI encryption.  The default is off which enables GSSAPI 
encryption only when available and requested to maintain compatability 
with older clients.  This setting should be enabled unless older clients 
are present.


+++ b/doc/src/sgml/libpq.sgml

@@ -1356,6 +1356,18 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
   
  

+  gss_enc_require
+  
+   
+If set, whether to require GSSAPI encryption support from the 
remote
+server.  Defaults to unset, which will cause the client to fall 
back

+to not using GSSAPI encryption if the server does not support
+encryption through GSSAPI.
+   

Some rewording:

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.


+++ b/doc/src/sgml/runtime.sgml

I think you mean postgresql.conf?

+ 
+  GSSAPI connections can also encrypt all data sent across the network.
+  In the pg_hba.conf file, the GSSAPI authentication 
method

+  has a parameter to require encryption; otherwise connections will be
+  encrypted if available and requested by the client.  On the 
client side,
+  there is also a parameter to require GSSAPI encryption support 
from the

+  server.
+ 

I think a link to the client parameter would be valuable here.

+++ b/src/backend/libpq/auth.c

+* In most cases, we do not need to send AUTH_REQ_OK until we are ready
+* for queries, but if we are doing GSSAPI encryption that request must 
go
+* out now.

Why?

+++ b/src/backend/libpq/be-secure-gssapi.c

Some of the functions in this file are missing comment headers and 
should have more inline comments for each major section.


+   ret = be_gssapi_should_crypto(port);

Add LF here.

+   port->gss->writebuf.cursor += ret;

And here.

+   /* need to be called again */

And here.  Well, you get the idea.

These functions could all use more whitespace and comments.

+++ b/src/backend/utils/misc/guc.c

+   {
+   {"gss_encrypt", PGC_USERSET, CONN_AUTH_SECURITY,
+gettext_noop("Whether client wants encryption for this 
connection."),

Perhaps, "Require encryption for all GSSAPI connections."

+++ b/src/interfaces/libpq/fe-connect.c

+   if (n > 0)
+   conn->inEnd += n;
+   /*
+* If n < 0, then this wasn't a 
full request and
+* either more data will be 
available later to
+* complete it or we will error 
out then.
+*/

Shouldn't this comment block go above the if?

+   /*
+* We tried to request GSSAPI 
encryption, but the
+* server doesn't support it.  
Retries are permitted
+* here, so hang up and try 
again.  A connection that
+* doesn't rupport appname will 

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

[Michael Paquier]

On Fri, Mar 25, 2016 at 10:10 PM, David Steele  wrote:
> Excellent, Robbie!  I've run this patch through my test cases and
> everything works.
>
> Now that it's working I'll be writing up an actual review so expect that
> by Monday.

(I haven't given up on this patch yet, sorry for the low activity on
this thread)
-- 
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 v8] GSSAPI encryption support

[David Steele]

On 3/20/16 12:09 AM, Robbie Harwood wrote:
> 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/gssencrypt8

Excellent, Robbie!  I've run this patch through my test cases and
everything works.

Now that it's working I'll be writing up an actual review so expect that
by Monday.

-- 
-David
da...@pgmasters.net



signature.asc
Description: OpenPGP digital signature

[HACKERS] [PATCH v8] GSSAPI encryption support

[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/gssencrypt8

What changed:

- Fixed fallback in the new server/old client case.  The server flag
  checking has been reduced.  In the (distant) future when all old
  clients are gone, the checking should be increased in strength once
  again.

- Fixed fallback in the old server/new client case.  This consists of
  checking whether the first message we receive after auth is done is an
  error, and raising it without decrypting if it is so that reconnection
  can happen if desired.  The quality of the fallback path has also
  generally been improved.

- Removed overzealous whitespace cleanup.  I'm a packager and have been
  bitten by upstreams doing this; I should have known better.

- Made flushing the AUTH_REQ_OK message conditional again.

- Fixed typo in server error message for insufficient GSSAPI protection.

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],
-