Re: [PATCH v3] GSSAPI encryption support

Robbie,

On Wed, Oct 21, 2015 at 3:54 PM, Michael Paquier
<michael(dot)paquier(at)gmail(dot)com> wrote:
> On Tue, Oct 20, 2015 at 3:01 AM, Robbie Harwood wrote:
>> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>>> psql: lost synchronization with server: got message type "S", length 22
>>
>> which unfortunately could be a great many things. I've said this a
>> couple times now, but I really do need more information - a traffic
>> dump, a list of commands that were run, etc.; unfortunately, the surface
>> here is pretty large, and while I totally am willing to believe there
>> are bugs in the code I've written, I do not yet see them.
>
> --- a/src/interfaces/libpq/fe-protocol3.c
> +++ b/src/interfaces/libpq/fe-protocol3.c
> @@ -129,6 +129,58 @@ pqParseInput3(PGconn *conn)
> return;
> }
>
> +#ifdef ENABLE_GSS
> + /* We want to be ready in both IDLE and BUSY states
> for encryption */
> + if (id == 'g' && !conn->gss_disable_enc && conn->gctx)
> + {
> + ssize_t encEnd, next;
> [...]
> + }
> + else if (!conn->gss_disable_enc && conn->gss_auth_done &&
> + !conn->gss_decrypted_cur && id != 'E')
> + /* This could be a sync error, so let's handle
> it as such. */
> + handleSyncLoss(conn, id, msgLength);
> +#endif
>
> Hm. The out-of-sync error I am seeing in my environment is caused by
> this block when parsing 'g' messages coming from the backend that are
> considered as being GSSAPI-encrypted messages. I am still looking at
> that...

@@ -604,6 +604,11 @@ pqPutMsgEnd(PGconn *conn)
memcpy(conn->outBuffer + conn->outMsgStart, &msgLen, 4);
}

+#ifdef ENABLE_GSS
+ if (pggss_encrypt(conn) < 0)
+ return EOF;
+#endif

@@ -1528,10 +1541,20 @@ socket_putmessage(char msgtype, const char *s,
size_t len)
if (internal_putbytes(s, len))
goto fail;
PqCommBusy = false;
+#ifdef ENABLE_GSS
+ /* if we're GSSAPI encrypting, s was allocated in be_gss_encrypt */
+ if (msgtype == 'g')
+ pfree((char *)s);
+#endif

Looking at this patch in more details... Why is it necessary to wrap
all the encrypted messages into a dedicated message type 'g'? Cannot
we rely on the context on client and backend that should be set up
after authentication to perform the encryption and decryption
operations? This patch is enforcing the message type in
socket_putmessage for backend and pggss_encrypt/pqPutMsgEnd for
frontend, this just feels wrong and I think that the patch could be
really simplified, this includes the crafting added in fe-protocol3.c
that should be IMO reserved only for messages received from the
backend and should not be made aware of any protocol-level logic.
--
Michael