From: Robbie Harwood <rharwood@redhat.com>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Stephen Frost <sfrost@snowman.net>
Cc: PostgreSQL mailing lists <pgsql-hackers@postgresql.org>
Cc: Andres Freund <andres@anarazel.de>
Cc: Craig Ringer <craig@2ndquadrant.com>
Subject: Re: [PATCH v3] GSSAPI encryption support
Date: Wed, 21 Oct 2015 12:28:29 -0400
Message-ID: <jlgbnbskwya.fsf@thriss.redhat.com> (raw)
In-Reply-To: <CAB7nPqTL1SjaYn7pwb86mW1M+469=ofyyRt1Oh9dyFC_bAf_iw@mail.gmail.com>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com>
<jlgvbaao0sr.fsf@thriss.redhat.com>
<CAMsr+YE-WaD4kV4vYe0WxQhgD=hJ-EHd7oJ3=ZUF614t1HMPMA@mail.gmail.com>
<jlg1tcw6owo.fsf@thriss.redhat.com>
<CAMsr+YH0jamGmOM=mwziYQg=J-0XVp4BosPEv1n+UMaNMj4aMA@mail.gmail.com>
<20151016133459.GL3685@tamriel.snowman.net>
<CAMsr+YG1+HQ-V_kdBKkFX+XRV+bND0pJOygns-nbAuHEn2pj2w@mail.gmail.com>
<20151017021105.GP3685@tamriel.snowman.net>
<jlgwpuin3fm.fsf@thriss.redhat.com>
<CAB7nPqT+URED7yZUPc_Soj=vfhf_r7MJD7=VonWpOFiiJE6zgg@mail.gmail.com>
<CAB7nPqTL1SjaYn7pwb86mW1M+469=ofyyRt1Oh9dyFC_bAf_iw@mail.gmail.com>
List-Unsubscribe: <mailto:majordomo@postgresql.org?body=unsub%20pgsql-hackers>
Michael Paquier <michael.paquier@gmail.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.
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, michael.paquier@gmail.com, sfrost@snowman.net, andres@anarazel.de, craig@2ndquadrant.com
Subject: Re: [PATCH v3] GSSAPI encryption support
In-Reply-To: <jlgbnbskwya.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