From: Robbie Harwood <rharwood@redhat.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: pgsql-hackers@postgresql.org
Subject: Re: [PATCH v12] GSSAPI encryption support
Date: Wed, 06 Apr 2016 15:22:15 -0400
Message-ID: <jlg1t6isftk.fsf@thriss.redhat.com> (raw)
In-Reply-To: <20160406112105.GG10850@tamriel.snowman.net>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com>
<jlgy48ssyvl.fsf@thriss.redhat.com>
<20160406112105.GG10850@tamriel.snowman.net>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Stephen Frost <sfrost@snowman.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
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: rharwood@redhat.com, sfrost@snowman.net
Subject: Re: [PATCH v12] GSSAPI encryption support
In-Reply-To: <jlg1t6isftk.fsf@thriss.redhat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox