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(&lmin_s, min_stat, GSS_C_MECH_CODE,
+                                          GSS_C_NO_OID, &msg_ctx, &gmsg);
+       strlcpy(msg_minor, gmsg.value, sizeof(msg_minor));
+       gss_release_buffer(&lmin_s, &gmsg);
+       if (msg_ctx)
+               ereport(WARNING,
+                               (errmsg_internal("incomplete GSS minor error 

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
-    <acronym>SSL</acronym> is used.
+    <acronym>SSL</acronym> is used, or <acronym>GSSAPI</acronym> encryption
+    is in use.

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

+      <term><literal>require_encrypt</literal></term>
+      <listitem>
+       <para>
+        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

+      <term><literal>gss_enc_require</literal></term>
+      <listitem>
+       <para>
+ 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.
+       </para>

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?

+     <para>
+      GSSAPI connections can also encrypt all data sent across the network.
+ In the <filename>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.
+     </para>

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 
+        * out now.


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

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 
also not support
+                                                * GSSAPI encryption.
+                                                */

typo - "doesn't support appname".

+++ b/src/interfaces/libpq/fe-secure-gssapi.c

Comments are a bit better here than in be-secure-gssapi.c but could still be better. And again, more whitespace.

* PATCH 3 - GSSAPI authentication cleanup

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

+        * GSS_C_REPLAY_FLAG and GSS_C_SEQUENCE_FLAG are missing for 
+        * 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.

+++ 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"};

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?

Things look pretty good in general but fe-secure-gssapi.c and be-secure-gssapi.c should both be reworked with better comments and more whitespace before this patch goes to a committer.


Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to