Re: [PATCH v1] GSSAPI encryption support

Andres Freund <andres(at)anarazel(dot)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(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.

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