From: Robbie Harwood <rharwood@redhat.com>
To: Michael Paquier <michael.paquier@gmail.com>
Cc: Stephen Frost <sfrost@snowman.net>,
 PostgreSQL mailing lists <pgsql-hackers@postgresql.org>,
 Andres Freund <andres@anarazel.de>, Craig Ringer <craig@2ndquadrant.com>
Subject: Re: [PATCH v3] GSSAPI encryption support
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>
User-Agent: Notmuch/0.18.2 (http://notmuchmail.org) Emacs/24.4.1
 (x86_64-pc-linux-gnu)
Date: Wed, 21 Oct 2015 12:28:29 -0400
Message-ID: <jlgbnbskwya.fsf@thriss.redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
 micalg=pgp-sha1; protocol="application/pgp-signature"
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

--=-=-=
Content-Type: text/plain

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.

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQIcBAEBAgAGBQJWJ70uAAoJECUy+RdqlaRCYeoP+wUGpaWF9pLYmq+6H+aQSsaX
+d1rrbGZ/WITbPRFx7uydDrn5dmJmxNZDV8uV96MH5V9jFjR5C1avcQ82qitijiP
oXuQJFu726Yp9pvbLFk8YrltBx43jejNfev+u4v1uf107Twf2R3akgx7Ne0knnyt
6qZQt4ZhnsGmNfgpBwX0D/sMPzi8y4pCoREtEiJYnKhPIEs9fTyqahM6n3mcX63c
xV5foq24NeHPN+mLC1zlwinddtETsyi38i5Sp4shS579p4caIlUab2MJ74zfP+K9
Yfjb/14CytVJCAm/4cWVYZpej7/07v5Ai/BPUX8D9pf3sIaecZxgA70I80R5KtKs
gs3kkhfn14QSSOe2tsPmH7DD59u2dex7tEXMCn7jQMirHWC5hVWFx3HKjoo4IROm
mSHRaO/jpjCMAOxmzelqYX3BnRctNTBjEeb76nXBNIZoSMtXUfXxd1qfW9Van6kA
kgt4GgiWMtbe6tbgpakDTYcTX4iUS9bYmOaNRPnYhwCi1nJoLhqnaI+y4YdoXU18
Ny1oes2qkjAhymj1LK7IX3kxuwz2u31IP1YSH5qgkbrfA8RqMd+PZ3TbF5GJ8SMa
MsSUPlQXc3lOPNf5Q7PrS3QK1ILyjO+SNEa5xXpx5OXb/ZcLSlzbEwlOELCc21FP
F+DNDDtJgtrHvuwvIsy0
=4aOC
-----END PGP SIGNATURE-----
--=-=-=--