Re: [PATCH v3] GSSAPI encryption support

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> Robbie,
>
> +#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.

Well, it's not strictly necessary in the general case, but it makes
debugging a *lot* easier to have it present, and it simplifies some of
the handling logic. For instance, in the part quoted above, with
socket_putmessage() and socket_putmessage_noblock(), we need some way to
tell whether a message blob has already been encrypted.

Some enforcement of message type *will need to be carried out* anyway to
avoid security issues with tampering on the wire, whether that be by
sanity-checking the stated message type and then handling accordingly,
or trying to decrypt and detonating the connection if it fails.

GSSAPI does not define a wire protocol for the transport of messages the
way, e.g., TLS does, so there must be some handling of incomplete
messages, multiple messages at once, etc. There is already adequate
handling of these present in postgres already, so I have structured the
code to take advantage of it. That said, I could absolutely reimplement
them - but it would not be simpler, I'm reasonably sure.