Re: [PATCH v1] GSSAPI encryption support - Mailing list pgsql-hackers

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@redhat.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