Re: [PATCH v3] GSSAPI encryption support

On Thu, Oct 22, 2015 at 1:28 AM, Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> 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.

Hm, and that's why you chose this way of going. My main concern about
this patch is that it adds on top of the existing Postgres protocol a
layer to encrypt and decrypt the messages between server and client
based on GSSAPI. All messages transmitted between client and server
are changed to 'g' messages on the fly and switched back to their
original state at reception. This is symbolized by the four routines
you added in the patch in this purpose, two for frontend and two for
backend, each one for encryption and decryption. I may be wrong of
course, but it seems to me that this approach will not survive
committer-level screening because of the fact that context-level
things invade higher level protocol messages.

IMO, we would live better with something at a lower level, like in
be-secure.c and fe-secure.c to exchange the encrypted and decrypted
packages using the GSSAPI context created at authentication, that's
where the context-level switches are living after all.

So I am thinking that this patch needs a rework, and should be
returned with feedback.
Stephen, others, what do you think?
--
Michael