Date: Mon, 18 Feb 2019 10:32:53 -0500
From: Stephen Frost <sfrost@snowman.net>
To: Robbie Harwood <rharwood@redhat.com>
Cc: Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org,
	Nico Williams <nico@cryptonector.com>,
	Michael Paquier <michael@paquier.xyz>,
	Alvaro Herrera <alvherre@2ndquadrant.com>,
	David Steele <david@pgmasters.net>
Subject: Re: [PATCH v20] GSSAPI encryption support
Message-ID: <20190218153253.GE6197@tamriel.snowman.net>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com>
 <jlgy38mir81.fsf@redhat.com>
 <20190216041026.gpkfn4e6wrozliyb@alap3.anarazel.de>
 <jlgzhqtqgvw.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="x9NlMAausKqHAFUv"
Content-Disposition: inline
In-Reply-To: <jlgzhqtqgvw.fsf@redhat.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk


--x9NlMAausKqHAFUv
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

Greetings,

* Robbie Harwood (rharwood@redhat.com) wrote:
> Andres Freund <andres@anarazel.de> writes:
>=20
> > On 2018-12-18 14:12:46 -0500, Robbie Harwood wrote:
> >
> >> Subject: [PATCH] libpq GSSAPI encryption support
> >
> > Could some of these be split into separate patches that could be more
> > eagerly merged? This is a somewhat large patch...
>=20
> What splits do you propose?  (It's been a single patch since v3 as per
> your request in id:20151003161810.GD30738@alap3.anarazel.de and Michael
> Paquier's request in
> id:CAB7nPqTJD-tTrM1Vu8P55_4kKVeDX8DFz9v1D_XsQQvR_xA5qQ@mail.gmail.com).

Yeah, I tend to agree with the earlier comments that this can be a
single patch.  There's a little bit that could maybe be split out (when
it comes to the bits that are mostly just moving things around and
removing things) but I'm not convinced it's necessary or, really,
particularly worth it.

> >> diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/f=
e-secure.c
> >> index a06fc7dc82..f4f196e3b4 100644
> >> --- a/src/interfaces/libpq/fe-secure.c
> >> +++ b/src/interfaces/libpq/fe-secure.c
> >> @@ -220,6 +220,13 @@ pqsecure_read(PGconn *conn, void *ptr, size_t len)
> >>  		n =3D pgtls_read(conn, ptr, len);
> >>  	}
> >>  	else
> >> +#endif
> >> +#ifdef ENABLE_GSS
> >> +	if (conn->gssenc)
> >> +	{
> >> +		n =3D pg_GSS_read(conn, ptr, len);
> >> +	}
> >> +	else
> >>  #endif
> >>  	{
> >>  		n =3D pqsecure_raw_read(conn, ptr, len);
> >> @@ -287,7 +294,7 @@ pqsecure_raw_read(PGconn *conn, void *ptr, size_t =
len)
> >>   * to determine whether to continue/retry after error.
> >>   */
> >>  ssize_t
> >> -pqsecure_write(PGconn *conn, const void *ptr, size_t len)
> >> +pqsecure_write(PGconn *conn, void *ptr, size_t len)
> >>  {
> >>  	ssize_t		n;
> >> =20
> >> @@ -297,6 +304,13 @@ pqsecure_write(PGconn *conn, const void *ptr, siz=
e_t len)
> >>  		n =3D pgtls_write(conn, ptr, len);
> >>  	}
> >>  	else
> >> +#endif
> >> +#ifdef ENABLE_GSS
> >> +	if (conn->gssenc)
> >> +	{
> >> +		n =3D pg_GSS_write(conn, ptr, len);
> >> +	}
> >> +	else
> >>  #endif
> >>  	{
> >>  		n =3D pqsecure_raw_write(conn, ptr, len);
> >
> > Not a fan of this. Seems like we'll grow more and more such branches
> > over time?  Wouldn't the right thing be to have callbacks in PGconn
> > (and similarly in the backend)?
>=20
> Is that really a problem?  Each branch is only seven lines, which is a
> lot less than adding callback support will be.  And we'll only get a new
> branch when we want to support a new encryption protocol - unlike
> authentication, there aren't too many of those.

Considering this is only the second encryption protocol in the project's
lifetime, I agree that using callbacks would be overkill here.  What
other encryption protocols are you thinking we would be adding here?  I
think most would be quite hard-pressed to name a second general-purpose
one beyond TLS/SSL, and those who can almost certainly would say GSS,
but a third?  Certainly OpenSSH has its own, but it's not intended to be
general purpose and I can't see us adding support for OpenSSH's
encryption protocol to PG.

Adding support for different libraries which implement TLS/SSL wouldn't
be changing this part of the code, if that was perhaps what was being
considered..?

> > Seems like if that's done reasonably it'd also make integration of
> > compression easier, because that could just layer itself between
> > encryption and wire?

Building a general-purpose filtering mechanism for streams is actually
quite a bit of work (we did it for pgBackRest, feel free to check it
out- and all that code is in C these days too) and I don't think it's
really necessary when the options are "optionally compress and
optionally encrypt using one of these two protocols".  I don't see any
reason why we'd need to, say, compress a stream multiple times, or
encrypt it multiple times (like with TLS first and then GSS).

> The current interface would allow a compress/decompress call in a way
> that makes sense to me (here for write, ignoring ifdefs):

[...]

> (pqsecure_read would look similarly, with decompression as the last step
> instead of the first.)

That certainly seems reasonable to me.

Thanks!

Stephen

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

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

iQIcBAEBCgAGBQJcatAlAAoJEO1sijiDR2RVmxsQAIJpb6lyg0/Vz6FsDxnmqef2
yxd44b3ftJUb0l8gKTdLU1YBs/sINGaiA9aXRidKW1ZCsmacDyEv2WLBC2JfSers
aPUpguWzfIRrRGm6tWj0DWT8gOwkcWX/iI/4L4xlGS8auE2cAkVyFDN3vEkgfv/b
GkSxwK/50g0QL6t5Lx6b/XghPt2FVwX9krAcNFS4QjmY+6zEqmntAwtXAwIAC1OC
hciWIN6vFZo77bNzoAb2z2apTcX9jTyhbVbrX8x5WfO8K2oI2qN420XAHEYwYePP
bvLuOYtJycG9vlqq9sXSgRyymVzG8t0ArGjRXewYbH7xD/aHHF3qfjgNcEpvxsmi
FHuc7BoSZsIKi4t+NkuOBUAxcB4KSFmRNzk1lvEm+K9VCbUGVH+e8y76nMsXlAJ1
I5IgjnyWA05xdhtO7ik1G71JcjQ8RqdG0o9yYITjnh9YfCMA+MFN8Yo1DyLKPYAR
Jpe0kYWO4S7jtQ7PPSEUA5jEvDd+sWuwWV5KgPlgT6tF49z7iZslCBcYX2Jf+YL5
nYdEs3ERtEb+wDGBO8ZKLhkgMEVJh5uA1JwFu3KlzVJsdMtAfFWB8qwjoGkoLQOy
rQjMpMmb+T9b1L7BWHhWP2DpVuL8UXAAHzFnjqlwRZZdPxFgRblo4wl0GAUjbRJ7
xR4vkI05yzDGumzdZeIN
=aLym
-----END PGP SIGNATURE-----

--x9NlMAausKqHAFUv--