Re: [PATCH v1] GSSAPI encryption support

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 <assert.h>

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(&minor, MyProcPort->gss->ctx, &input, &output,
> + &conf, 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 *)&msglen, ((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(&minor, &output);
> +
> + 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, &s, 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
> + },
> + &gss_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
should error out when not supported. As is you're going to get linker
errors around gss_encrypt, assign_gss_encrypt.

> From e55795e0638ca37447ef200f21f74db80b07ebf4 Mon Sep 17 00:00:00 2001
> From: "Robbie Harwood (frozencemetery)" <rharwood(at)redhat(dot)com>
> Date: Fri, 12 Jun 2015 13:27:50 -0400
> Subject: Error when receiving plaintext on GSS-encrypted connections

I don't see why this makes sense as a separate patch.

> Subject: server: hba option for requiring GSSAPI encryption
>
> Also includes logic for failing clients that do not request encryption
> in the startup packet when encryption is required.
> ---
> src/backend/libpq/hba.c | 9 +++++++++
> src/backend/utils/init/postinit.c | 7 ++++++-
> src/backend/utils/misc/guc.c | 12 +++++++++++-
> src/include/libpq/hba.h | 1 +
> 4 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/src/backend/libpq/hba.c b/src/backend/libpq/hba.c
> index 23c8b5d..90fe57f 100644
> --- a/src/backend/libpq/hba.c
> +++ b/src/backend/libpq/hba.c
> @@ -1570,6 +1570,15 @@ parse_hba_auth_opt(char *name, char *val, HbaLine *hbaline, int line_num)
> else
> hbaline->include_realm = false;
> }
> + else if (strcmp(name, "require_encrypt") == 0)
> + {
> + if (hbaline->auth_method != uaGSS)
> + INVALID_AUTH_OPTION("require_encrypt", "gssapi");
> + if (strcmp(val, "1") == 0)
> + hbaline->require_encrypt = true;
> + else
> + hbaline->require_encrypt = false;
> + }

So this is a new, undocumented, option that makes a connection require
encryption? But despite the generic name, it's gss specific?

> @@ -1628,7 +1629,7 @@ static struct config_bool ConfigureNamesBool[] =
> NULL,
> GUC_NOT_IN_SAMPLE | GUC_DISALLOW_IN_FILE
> },
> - &gss_encrypt, false, NULL, assign_gss_encrypt, NULL
> + &gss_encrypt, false, check_gss_encrypt, assign_gss_encrypt, NULL
> },
>
> /* End-of-list marker */
> @@ -10133,4 +10134,13 @@ assign_gss_encrypt(bool newval, void *extra)
> gss_encrypt = newval;
> }
>
> +static bool
> +check_gss_encrypt(bool *newval, void **extra, GucSource source)
> +{
> + if (MyProcPort && MyProcPort->hba && MyProcPort->hba->require_encrypt &&
> + !*newval)
> + return false;
> + return true;
> +}

Doing such checks in a guc assign hook seems like a horrible idea. Yes,
there's some precedent, but still.

Greetings,

Andres Freund