Re: [PATCH v12] GSSAPI encryption support

Stephen Frost <sfrost(at)snowman(dot)net> writes:

> Just an initial pass over the patch.

Thanks! In the interest of brevity, if I haven't replied to something,
I plan to fix it.

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

I need to flush this any time we might be doing encryption because it
needs to be in a separate request to _secure_write() from what follows
it. We don't know whether we should be doing encryption until
connection parameters are parsed; to put it another way,
port->gss->encrypt will never be true here because it hasn't been parsed
out of port->gss->gss_encrypt yet.

I could parse it earlier, but then I need another variable in the struct
(i.e., to hold whether AUTH_REQ_OK has been sent yet) to check as well.
Additionally, it seemed to me that there might be some value
security-wise in delaying parsing of connection parameters until after
auth is complete, though of course for just a bool this may not be as
important.

>> + /* 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?

This call should be okay because (1) it's a tail call and (2) we know
the buffer isn't empty.

That said, the other recursion is excessively complicated and I think
it's simpler to eliminate it entirely in favor of being called again.
I'll see what I can do.

>> + /* 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?

This is a good point. We definitely have to draw the line somewhere; 1G
is a high upper bound. Digging around the server code I don't see a
precedent for what's a good size to stop at. There's
PG_MAX_AUTH_TOKEN_LENGTH, which is 65k, and the password length in
auth.c is 1k. Beyond that, SocketBackend() calls pq_getmessage() with
no maximum length, which causes the enlargeStringInfo() size
restrictions to be the only control (wrapped in a PG_TRY()).

I think what I'm trying to get at here is that I'm open to suggestion,
but don't see a clearly better way to do this. We could use the
PG_TRY() approach here to preserve sync, though I'm not sure it's worth
it considering PQ_RECV_BUFFER_SIZE and PQ_SEND_BUFFER_SIZE are both 8k.

>> Subject: [PATCH 3/3] GSSAPI authentication cleanup
>
> Why wouldn't this be part of patch #2?

It's a cleanup of existing code, not my new code, so I thought the
separation would make it easier to understand. I'm fine with it as part
of #2 so long as it happens, but the impression I had gotten from
earlier reviews was that it should have even more division (i.e., move
the gss_display_status() wrappers into their own patch), not less.

Thanks,
--Robbie