Re: [PATCH v20] GSSAPI encryption support

Stephen Frost <sfrost@snowman.net> writes:

> * Robbie Harwood (rharwood@redhat.com) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>>> * Robbie Harwood (rharwood@redhat.com) wrote:
>>>> Stephen Frost <sfrost@snowman.net> writes:
>>>>> * Robbie Harwood (rharwood@redhat.com) wrote:
>>>>>
>>>>>> Attached please find version 20 of the GSSAPI encryption support.
>>>>>> This has been rebased onto master (thanks Stephen for calling out
>>>>>> ab69ea9).
>>>>>
>>>>> I've looked over this again and have been playing with it
>>>>> off-and-on for a while now.  One thing I was actually looking at
>>>>> implementing before we push to commit this was to have similar
>>>>> information to what SSL provides on connection, specifically what
>>>>> printSSLInfo() prints by way of cipher, bits, etc for the
>>>>> client-side and then something like pg_stat_ssl for the server
>>>>> side.
>>>>>
>>>>> I went ahead and added a printGSSENCInfo(), and then
>>>>> PQgetgssSASLSSF(), which calls down into
>>>>> gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF to
>>>>> get the bits (which also required including gssapi_ext.h), and now
>>>>> have psql producing:
>>>>>
>>>>> I don't think these things are absolutely required to push the
>>>>> patch forward, just to be clear, but it's helping my confidence
>>>>> level, at least, and would make it closer to on-par with our
>>>>> current SSL implementation.  They also seem, well, reasonably
>>>>> straight-forward to add and quite useful.
>>>>
>>>> Auditability is definitely reasonable.  I think there are a couple
>>>> additional wrinkles discussed above, but we can probably get them
>>>> sorted.
>>>
>>> Fantastic.  Do you think you'll have time to work through some of
>>> the above with me over the next couple of weeks?  I was just
>>> starting to look into adding things into the beentry to hold
>>> information on the server side.
>>
>> Sure!  I'll go ahead and hack up the checks and lucid stuff and get
>> back to you.
>
> Great!  I'll finish fleshing out the basics of how to have this work
> server-side and once the checks and lucid stuff are in on the psql
> side, it should be pretty straight-forward to copy that same
> information into beentry alongside the SSL info, and expose it through
> pg_stat_get_activity() into a pg_stat_gss view.

When the mech is gss_mech_krb5 under MIT krb5:

    psql (12devel)
    GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha384-192)
    Type "help" for help.

And the same under Heimdal:

    psql (12devel)
    GSSAPI encrypted connection (krb5 using aes256-cts-hmac-sha1-96)
    Type "help" for help

If the mech weren't gss_krb5, or Lucid introspection failed, but we're a
SASL-aware mech (and using MIT):

    psql (12devel)
    GSSAPI encrypted connection (~256 bits)
    Type "help" for help.

The third case (no lucid, no SASL SSF):

    psql (12devel)
    GSSAPI encrypted connection (unknown mechanism)
    Type "help" for help.

Feel free to tweak these strings as needed.

I've also adjusted the layering somewhat and moved the actual printf()
call down into fe-secure-gssapi.c  I don't know whether this model makes
more sense in the long run, but for PoC code it was marginally easier to
reason about.

Another thing I've been thinking about here is whether the SASL SSF
logic is useful.  It'll only get invoked when the mechanism isn't
gss_mech_krb5, and only under MIT.  SPNEGO (krb5), NTLM (gss-ntlmssp),
IAKERB (krb5), and EAP (moonshot) all don't support
GSS_C_SEC_CONTEXT_SASL_SSF; I actually couldn't come up with another
mechanism that does.  I defer to you on whether to remove that, though.

I did some testing with Heimdal since we're using some extensions here,
and found out that the current build machinery doesn't support Heimdal.
(The configure.in detection logic only seems to work for MIT and
Solaris.)  However, it's possible to explicitly pass in CFLAGS/LDFLAGS
and it worked prior to my changes, so I've preserved that behavior.

Finally, as this patch touches configure.in, configure needs to be
regenerated; I'm not sure what project policy is on when that happens
(and it produces rather a lot of churn on my machine).

Patch attached after the break; apply on top of -20.

Thanks,
--Robbie

From 0f32efdb9b201a3b2d25d3fc41c9758790c84b93 Mon Sep 17 00:00:00 2001
From: Robbie Harwood <rharwood@redhat.com>
Date: Wed, 20 Feb 2019 17:23:06 -0500
Subject: [PATCH] Log encryption strength in libpq GSSAPI connections

---
 configure.in                            |  20 ++++
 src/bin/psql/command.c                  |   2 +
 src/include/pg_config.h.in              |   6 ++
 src/interfaces/libpq/exports.txt        |   1 +
 src/interfaces/libpq/fe-secure-gssapi.c | 127 ++++++++++++++++++++++++
 src/interfaces/libpq/fe-secure.c        |  11 ++
 src/interfaces/libpq/libpq-fe.h         |   3 +
 7 files changed, 170 insertions(+)

diff --git a/configure.in b/configure.in
index 89a0fb2470..be011778f3 100644
--- a/configure.in
+++ b/configure.in
@@ -1191,6 +1191,26 @@ if test "$with_gssapi" = yes ; then
   if test "$PORTNAME" != "win32"; then
     AC_SEARCH_LIBS(gss_init_sec_context, [gssapi_krb5 gss 'gssapi -lkrb5 -lcrypto'], [],
                    [AC_MSG_ERROR([could not find function 'gss_init_sec_context' required for GSSAPI])])
+
+    AC_COMPILE_IFELSE(
+      [AC_LANG_PROGRAM(
+        [#include <krb5.h>],
+        [[(void)krb5_enctype_to_name]]
+      )],
+      [AC_DEFINE(HAVE_KRB5_ENCTYPE_TO_NAME, 1,
+        [Define to 1 if you have support for krb5_enctype_to_name],
+      []
+    )])
+
+    AC_COMPILE_IFELSE(
+      [AC_LANG_PROGRAM(
+        [#include <gssapi/gssapi_krb5.h>],
+        [[gss_OID g = GSS_C_SEC_CONTEXT_SASL_SSF]]
+      )],
+      [AC_DEFINE(HAVE_GSS_C_SEC_CONTEXT_SASL_SSF, 1,
+        [Define to 1 if you have support for GSS_C_SEC_CONTEXT_SASL_SSF],
+      []
+    )])
   else
     LIBS="$LIBS -lgssapi32"
   fi
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 55315fe43b..042e585c20 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -621,6 +621,7 @@ exec_command_conninfo(PsqlScanState scan_state, bool active_branch)
                            db, PQuser(pset.db), host, PQport(pset.db));
             }
             printSSLInfo();
+            PQprintGSSENCInfo(pset.db);
         }
     }
 
@@ -3184,6 +3185,7 @@ connection_warnings(bool in_startup)
         checkWin32Codepage();
 #endif
         printSSLInfo();
+        PQprintGSSENCInfo(pset.db);
     }
 }
 
diff --git a/src/include/pg_config.h.in b/src/include/pg_config.h.in
index 76bd81e9bf..903e1aad56 100644
--- a/src/include/pg_config.h.in
+++ b/src/include/pg_config.h.in
@@ -290,6 +290,9 @@
 /* Define to 1 if you have the <gssapi.h> header file. */
 #undef HAVE_GSSAPI_H
 
+/* Define to 1 if you have support for GSS_C_SEC_CONTEXT_SASL_SSF */
+#undef HAVE_GSS_C_SEC_CONTEXT_SASL_SSF
+
 /* Define to 1 if you have the <history.h> header file. */
 #undef HAVE_HISTORY_H
 
@@ -332,6 +335,9 @@
 /* Define to 1 if you have isinf(). */
 #undef HAVE_ISINF
 
+/* Define to 1 if you have support for krb5_enctype_to_name */
+#undef HAVE_KRB5_ENCTYPE_TO_NAME
+
 /* Define to 1 if you have the <langinfo.h> header file. */
 #undef HAVE_LANGINFO_H
 
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index cc9ee9ce6b..2075ebde0c 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -174,3 +174,4 @@ PQresultVerboseErrorMessage 171
 PQencryptPasswordConn     172
 PQresultMemorySize        173
 PQhostaddr                174
+PQprintGSSENCInfo         175
diff --git a/src/interfaces/libpq/fe-secure-gssapi.c b/src/interfaces/libpq/fe-secure-gssapi.c
index 5d9c1f8f5b..821dd2ac9e 100644
--- a/src/interfaces/libpq/fe-secure-gssapi.c
+++ b/src/interfaces/libpq/fe-secure-gssapi.c
@@ -17,6 +17,11 @@
 #include "libpq-int.h"
 #include "fe-gssapi-common.h"
 
+#include "port/pg_bswap.h"
+
+#include <gssapi/gssapi_krb5.h>
+#include <krb5.h>
+
 /*
  * Require encryption support, as well as mutual authentication and
  * tamperproofing measures.
@@ -24,6 +29,11 @@
 #define GSS_REQUIRED_FLAGS GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG | \
     GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG
 
+static ssize_t send_buffered_data(PGconn *conn, size_t len);
+static ssize_t read_from_buffer(PGconn *conn, void *ptr, size_t len);
+static ssize_t load_packet_length(PGconn *conn);
+static ssize_t load_packet(PGconn *conn, size_t len);
+
 /*
  * Helper function to write any pending data.  Returns len when it has all
  * been written; otherwise, sets errno appropriately.  Assumes there is data
@@ -392,3 +402,120 @@ pqsecure_open_gss(PGconn *conn)
     gss_release_buffer(&minor, &output);
     return PGRES_POLLING_WRITING;
 }
+
+#ifndef HAVE_KRB5_ENCTYPE_TO_NAME
+/* Heimdal doesn't (yet) support krb5_enctype_to_name(), but its
+ * krb5_enctype_to_string() has similar behavior.  (MIT's
+ * krb5_enctype_to_string() produces very verbose output that we don't want,
+ * and has different calling convention.)  This wrapper gives us approximate
+ * parity between the two.
+ *
+ * Heimdal issue: https://github.com/heimdal/heimdal/issues/525 */
+static krb5_error_code
+krb5_enctype_to_name(krb5_enctype enctype, krb5_boolean shortest,
+                     char *buffer, size_t buflen)
+{
+    krb5_error_code ret;
+    krb5_context ctx;
+    char       *outstr = NULL;
+
+    ret = krb5_init_context(&ctx);
+    if (ret != 0)
+        return ret;
+
+    ret = krb5_enctype_to_string(ctx, enctype, &outstr);
+    if (ret != 0)
+        goto cleanup;
+
+    strncpy(buffer, outstr, buflen);
+
+cleanup:
+    free(outstr);
+    krb5_free_context(ctx);
+    return ret;
+}
+#endif
+
+/*
+ * Prints information about the current GSS-Encrypted connection, if GSS
+ * encryption is in use.
+ */
+void
+PQprintGSSENCInfo(PGconn *conn)
+{
+    OM_uint32    major,
+                minor;
+    gss_OID        mech = GSS_C_NO_OID;
+    gss_krb5_lucid_context_v1_t *lucid = NULL;
+    gss_buffer_set_t bufset = GSS_C_NO_BUFFER_SET;
+    char        enctype_buf[128];
+    krb5_error_code ret;
+    void       *lptr;
+
+    if (!conn || !conn->gctx)
+        return;
+
+    /* Get underlying GSS mechanism. */
+    (void) gss_inquire_context(&minor, conn->gctx, NULL, NULL, NULL, &mech,
+                               NULL, NULL, NULL);
+    if (gss_oid_equal(mech, gss_mech_krb5))
+    {
+        /* Preferred case - use lucid interface to get underlying enctype. */
+        major = gss_krb5_export_lucid_sec_context(&minor, &conn->gctx, 1,
+                                                  &lptr);
+        if (major == GSS_S_COMPLETE)
+            lucid = lptr;
+        if (major == GSS_S_COMPLETE && lucid->protocol == 1)
+        {
+            ret = krb5_enctype_to_name(lucid->cfx_kd.ctx_key.type, 0,
+                                       enctype_buf, sizeof(enctype_buf));
+            if (ret == 0)
+            {
+                printf(_("GSSAPI encrypted connection (krb5 using %s)\n"),
+                       enctype_buf);
+                goto cleanup;
+            }
+        }
+    }
+
+#ifdef HAVE_GSS_C_SEC_CONTEXT_SASL_SSF
+    {
+        uint32        ssf_result = 0,
+                    tmp4;
+
+        /*
+         * Fall back to trying for SASL SSF - query the value and range-check
+         * the result.  Not all GSSAPI mechanisms can implement this
+         * extension, and it's not supported by the Heimdal GSSAPI
+         * implementation.
+         */
+        major = gss_inquire_sec_context_by_oid(&minor, conn->gctx,
+                                               GSS_C_SEC_CONTEXT_SASL_SSF,
+                                               &bufset);
+        if (major == GSS_S_COMPLETE && bufset->elements[0].length == 4)
+        {
+            /*
+             * gss_inquire_sec_context_by_oid() for GSS_C_SEC_CONTEXT_SASL_SSF
+             * returns a 32bit integer in network byte-order, so we need to
+             * adjust for that and then print the integer into our text
+             * string.
+             */
+            memcpy(&tmp4, bufset->elements[0].value, 4);
+            ssf_result = pg_ntoh32(tmp4);
+            if (ssf_result >= 56 && ssf_result <= 256)
+            {
+                printf(_("GSSAPI encrypted connection (~%d bits)\n"),
+                       ssf_result);
+                goto cleanup;
+            }
+        }
+    }
+#endif
+
+    printf(_("GSSAPI encrypted connection (unknown mechanism)\n"));
+
+cleanup:
+    if (lucid)
+        gss_krb5_free_lucid_sec_context(&minor, lucid);
+    gss_release_buffer_set(&minor, &bufset);
+}
diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-secure.c
index f4f196e3b4..f18fc94e1c 100644
--- a/src/interfaces/libpq/fe-secure.c
+++ b/src/interfaces/libpq/fe-secure.c
@@ -434,6 +434,17 @@ PQsslAttributeNames(PGconn *conn)
 }
 #endif                            /* USE_SSL */
 
+/* Dummy versions of GSSAPI logging function, when built without GSS support */
+#ifndef ENABLE_GSS
+
+void
+PQprintGSSENCInfo(PGconn *conn)
+{
+    return;
+}
+
+#endif                            /* ENABLE_GSS */
+
 
 #if defined(ENABLE_THREAD_SAFETY) && !defined(WIN32)
 
diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h
index e8e0a9529a..e41fc3d5ec 100644
--- a/src/interfaces/libpq/libpq-fe.h
+++ b/src/interfaces/libpq/libpq-fe.h
@@ -347,6 +347,9 @@ extern void PQinitSSL(int do_init);
 /* More detailed way to tell libpq whether it needs to initialize OpenSSL */
 extern void PQinitOpenSSL(int do_ssl, int do_crypto);
 
+/* GSSAPI logging function */
+extern void PQprintGSSENCInfo(PGconn *conn);
+
 /* Set verbosity for PQerrorMessage and PQresultErrorMessage */
 extern PGVerbosity PQsetErrorVerbosity(PGconn *conn, PGVerbosity verbosity);
 
-- 
2.20.1