Re: [PATCH v8] GSSAPI encryption support

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 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
- <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
</listitem>
</varlistentry>

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

+++ 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 = &GSS_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?

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.

--
-David
david(at)pgmasters(dot)net