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

Andres Freund <andres@anarazel.de> writes:

> Hi,

Hi, thanks for the review; I really appreciate your time in going
through this.  I have questions about some of your comments, so I'll
wait a bit before sending a v3.  (By the way, there is a v2 of this I've
already posted, though you seem to have replied to the v1.  The only
difference is in documentation, though.)

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

That's fair.  Can you suggest a better organization?

> On 2015-07-02 14:22:13 -0400, Robbie Harwood wrote:
>> +#include <assert.h>
>
> postgres.h should be the first header included.

Okay, will fix.

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

Got it.

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

Probably better to be cautious about it.  Will fix.

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

Do you have a suggestion about where this code *should* go?  I need to
filter on the message type since some can't be encrypted.  I was unable
to find another place, but I may have missed it.

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

That's fair, will fix.

>>  /* 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.

If that is the style I will conform to it.

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

As previously stated, if there is another organization you prefer,
please suggest it.  As stated in my first email, I have attempted to err
on the side of having too many patches since splitting changesets later
is nontrivial, and squashing is easy.

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

It was not my intent to leave it undocumented; I believe I documented it
as part of my changes.  If there is a place I have missed where it
should be documented, please tell me and I will happily document it there.

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

Where would you prefer they go?  Isn't this what check hooks are for -
checking that it's valid to assign?

Thanks!
--Robbie