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

Robbie,

Just an initial pass over the patch.

* Robbie Harwood (rharwood@redhat.com) wrote:
> Here's v12, both here and on my github:
> https://github.com/frozencemetery/postgres/tree/feature/gssencrypt12

I've started taking a look at this as it's a capability I've wanted us
to support for a *long* time.

> Subject: [PATCH 1/3] Move common GSSAPI code into its own files

Didn't look too closely at this as it's mostly just moving stuff around.
I'll review it more closely once the other items are addressed though.

> Subject: [PATCH 2/3] Connection encryption support for GSSAPI

> diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c
> index 73d493e..94d95bd 100644
> --- a/src/backend/libpq/auth.c
> +++ b/src/backend/libpq/auth.c
> @@ -596,10 +596,12 @@ sendAuthRequest(Port *port, AuthRequest areq)
>      pq_endmessage(&buf);
>
>      /*
> -     * Flush message so client will see it, except for AUTH_REQ_OK, which need
> -     * not be sent until we are ready for queries.
> +     * In most cases, we do not need to send AUTH_REQ_OK until we are ready
> +     * for queries.  However, if we are doing GSSAPI encryption, that request
> +     * must go out immediately to ensure that all messages which follow the
> +     * AUTH_REQ_OK are not grouped with it and can therefore be encrypted.
>       */
> -    if (areq != AUTH_REQ_OK)
> +    if (areq != AUTH_REQ_OK || port->gss != NULL)
>          pq_flush();
>
>      CHECK_FOR_INTERRUPTS();

Do we actually need to send pq_flush *whenever* port->gss is not null?
Shouldn't this actually be port->gss->encrypt?

> diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
[...]
> +/*
> + * Wrapper function indicating whether we are currently performing GSSAPI
> + * connection encryption.
> + *
> + * gss->encrypt is set when connection parameters are processed, which happens
> + * immediately after AUTH_REQ_OK is sent.
> + */
> +static bool
> +be_gssapi_should_encrypt(Port *port)
> +{
> +    if (port->gss->ctx == GSS_C_NO_CONTEXT)
> +        return false;
> +    return port->gss->encrypt;
> +}

be_gssapi_should_encrypt returns bool, which seems entirely reasonable,
but...

> +be_gssapi_write(Port *port, void *ptr, size_t len)
> +{
> +    OM_uint32 major, minor;
> +    gss_buffer_desc input, output;
> +    ssize_t ret;
> +    int conf;
> +    uint32 netlen;
> +    char lenbuf[4];
> +
> +    ret = be_gssapi_should_encrypt(port);

Why are we storing the result into an ssize_t?

> +    if (ret == -1)
> +        return -1;
> +    else if (ret == 0)
> +        return secure_raw_write(port, ptr, len);

And then testing the result against -1...?  Or a bare 0 for that matter?

> +    /* encrypt the message */
> +    output.value = NULL;
> +    output.length = 0;
> +
> +    input.value = ptr;
> +    input.length = len;
> +
> +    conf = 0;
> +    major = gss_wrap(&minor, port->gss->ctx, 1, GSS_C_QOP_DEFAULT,
> +                     &input, &conf, &output);
> +    if (GSS_ERROR(major))
> +    {
> +        pg_GSS_error(ERROR,
> +                     gettext_noop("GSSAPI wrap error"),
> +                     major, minor);
> +        ret = -1;
> +        goto cleanup;
> +    }
> +    else if (conf == 0)
> +    {
> +        ereport(FATAL, (errmsg("GSSAPI did not provide confidentiality")));
> +        ret = -1;
> +        goto cleanup;
> +    }
> +
> +    /* format for on-wire: 4 network-order bytes of length, then payload */
> +    netlen = htonl(output.length);
> +    memcpy(lenbuf, &netlen, 4);
> +
> +    appendBinaryStringInfo(&port->gss->writebuf, lenbuf, 4);
> +    appendBinaryStringInfo(&port->gss->writebuf, output.value, output.length);

That strikes me as a bit of overkill, we tend to just cast the pointer
to a (char *) rather than memcpy'ing the data just to get a different
pointer out of it.

> +    /* recur to send any buffered data */
> +    gss_release_buffer(&minor, &output);
> +    return be_gssapi_write(port, ptr, len);

This feels a bit odd to be doing, honestly.  We try to take a lot of
care to consider low-memory situation and to be careufl when it comes to
potential for infinite recursion and there's already a way to ask for
this function to be called again, isn't there?

> + cleanup:
> +    if (output.value != NULL)
> +        gss_release_buffer(&minor, &output);
> +
> +    return ret;
> +}

There's no need for any of this.  This goto will never be reached as
either a pg_GSS_error(ERROR) or an ereport(FATAL) isn't going to return
control to this path.  That's one of the reasons to be careful with
memory allocation and to use appropriate memory contexts, we're going to
longjmp out of this code path and clean up the memory allocations by
free'ing the contexts that we no longer need.  That is, on an ERROR
level failure, on FATAL, we're just going to exit, so we don't have to
worry about memory cleanup in that case.  Note that in some cases we'll
actually promote an ERROR to a FATAL; that's particularly relevant here
as we tend to do that when we're in backend startup.

> +ssize_t
> +be_gssapi_read(Port *port, void *ptr, size_t len)
> +{
> +    OM_uint32 major, minor;
> +    gss_buffer_desc input, output;
> +    ssize_t ret;
> +    int conf = 0;
> +
> +    ret = be_gssapi_should_encrypt(port);
> +
> +    if (ret == -1)
> +        return -1;
> +    else if (ret == 0)
> +        return secure_raw_read(port, ptr, len);

This has the same issue as be_gssapi_write(), noted above.

> +    /* ensure proper behavior under recursion */
> +    if (len == 0)
> +        return 0;
> +
> +    /* report any buffered data, then recur */
> +    if (port->gss->buf.cursor > 0)
> +    {
> +        ret = be_gssapi_read_from_buffer(port, ptr, len);
> +        if (ret > 0)
> +        {
> +            ssize_t r_ret =
> +                be_gssapi_read(port, (char *)ptr + ret, len - ret);
> +            if (r_ret < 0 && errno != EWOULDBLOCK
> +#ifdef EAGAIN
> +                && errno != EAGAIN
> +#endif
> +                )
> +                /* connection is dead in some way */
> +                return r_ret;
> +            else if (r_ret < 0)
> +                /* no more data right now */
> +                return ret;
> +            return ret + r_ret;
> +        }
> +    }

I'm really not excited by all the recursion.  Further, I'd segregate the
variable declaration from such a complicated recursive call and probably
throw in another comment or comment-paragraph in there about what's
going on.

> +    /* our buffer is now empty */
> +    if (port->gss->buf.len < 4)
> +    {
> +        enlargeStringInfo(&port->gss->buf, 4 - port->gss->buf.len);
> +        ret = secure_raw_read(port, port->gss->buf.data + port->gss->buf.len,
> +                              4 - port->gss->buf.len);
> +        if (ret < 0)
> +            return ret;
> +
> +        /* write length to buffer */
> +        port->gss->buf.len += ret;
> +        port->gss->buf.data[port->gss->buf.len] = '\0';
> +        if (port->gss->buf.len < 4)
> +        {
> +            errno = EWOULDBLOCK;
> +            return -1;
> +        }
> +    }
> +
> +    /* we know the length of the packet at this point */
> +    memcpy((char *)&input.length, port->gss->buf.data, 4);
> +    input.length = ntohl(input.length);
> +    enlargeStringInfo(&port->gss->buf, input.length - port->gss->buf.len + 4);

I'm aware that enlargeStringInfo() does check and handle the case where
the length ends up >1G, but that feels a bit grotty to me- are you sure
you want the generic enlargeStringInfo() to handle that case?

> +    ret = be_gssapi_read_from_buffer(port, ptr, len);
> + cleanup:
> +    if (output.value != NULL)
> +        gss_release_buffer(&minor, &output);
> +
> +    return ret;

Again, probably not much use using the goto's.

Similar comments regarding the libpq side of things.

> Subject: [PATCH 3/3] GSSAPI authentication cleanup

Why wouldn't this be part of patch #2?

Otherwise it looked pretty reasonable.

Thanks!

Stephen