From: Robbie Harwood <rharwood@redhat.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Stephen Frost <sfrost@snowman.net>,
 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
In-Reply-To: <20190216041026.gpkfn4e6wrozliyb@alap3.anarazel.de>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com> <jlgy38mir81.fsf@redhat.com>
 <20190216041026.gpkfn4e6wrozliyb@alap3.anarazel.de>
Date: Mon, 18 Feb 2019 10:02:27 -0500
Message-ID: <jlgzhqtqgvw.fsf@redhat.com>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
	micalg=pgp-sha512; protocol="application/pgp-signature"
Precedence: bulk

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

Andres Freund <andres@anarazel.de> writes:

> 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...

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).

>> diff --git a/src/interfaces/libpq/fe-secure.c b/src/interfaces/libpq/fe-=
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 le=
n)
>>   * 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=20
>> @@ -297,6 +304,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_=
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)?

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.

> 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?

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

    ssize_t pqsecure_write(PGConn *conn, void *ptr, size_t len)
    {
        ssize_t n;

        if (conn->compress)
        {
            do_compression(conn, &ptr, &len);
        }
        if (conn->ssl_in_use)
        {
            n =3D pgtls_write(conn, ptr, len);
        }
        else if (conn->gssenc)
        {
            n =3D pg_GSS_write(conn, ptr, len);
        }
        else
        {
            n =3D pqsecure_raw_write(conn, ptr, len);
        }

        return n;
    }

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

Thanks,
=2D-Robbie

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

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEA5qc6hnelQjDaHWqJTL5F2qVpEIFAlxqyQMACgkQJTL5F2qV
pELRWhAAqiBWRna/YlUHISVAXZqPT1jTJV7os/b7A/f8fs2B1R7Kx0K1qNkpmn8w
3dN48AoQlOLFB8ynOOwybk1dRjwSRqnmF4CWFoVA5yNx0bGWc+11ILS7mFNUehhW
FM+Kt96MEn3XnAEL3hjhH2dclD44b4c6Z3oJ/kSlqCX+QJBd1vt4cB3xUtnZfHjR
VIF7RJGn71YLYrNNyZ3crUih2MrcLEmxcDG9DgVFeJYGF+HzpV6Ym9NmI8M19gES
z01p2AVsd0bGykyXTdzXg1C5i0DV1AEaED2uFvb9NCAXWEIO5XIPM/8UFS7hOysq
jrC5UUC2/OUJohwyiPT+e2yhEy9BsSq0G1UC/z0+ioueLpAKjf3eHX3C4/Dxgybu
biiuQRis9srHQb5uCjsOqbASfy0btAV0VC0QOIMipWRLbdtaT3KdmxRbVMGzqs0T
9jeA3kn0UbpxArqQZlRwHglXWQTF7iTAdI1G2CDLFDeAuA8p6l/gvlGui32dmwDJ
2AOW8Xxj9uyN2jPDXKujiq2ZueC4zKtIPFxO5VanDv8t5MpmJdhmo3l55SjY4O0V
oBr5LS9qG67RAZDYxVgdLDJuWiP/RyyLLqTruBn5UUF6bmhuhTPXM0PoxVK8Escw
x/f7D+EGNpu8Ofn7BOHH76bKAyyNKve4bshVUbN3cX2H01ahufU=
=6T6T
-----END PGP SIGNATURE-----
--=-=-=--