Re: [PATCH v20] GSSAPI encryption support

Andres Freund <andres(at)anarazel(dot)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(dot)GD30738(at)alap3(dot)anarazel(dot)de and Michael
Paquier's request in
id:CAB7nPqTJD-tTrM1Vu8P55_4kKVeDX8DFz9v1D_XsQQvR_xA5qQ(at)mail(dot)gmail(dot)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 = pgtls_read(conn, ptr, len);
>> }
>> else
>> +#endif
>> +#ifdef ENABLE_GSS
>> + if (conn->gssenc)
>> + {
>> + n = pg_GSS_read(conn, ptr, len);
>> + }
>> + else
>> #endif
>> {
>> n = 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;
>>
>> @@ -297,6 +304,13 @@ pqsecure_write(PGconn *conn, const void *ptr, size_t len)
>> n = pgtls_write(conn, ptr, len);
>> }
>> else
>> +#endif
>> +#ifdef ENABLE_GSS
>> + if (conn->gssenc)
>> + {
>> + n = pg_GSS_write(conn, ptr, len);
>> + }
>> + else
>> #endif
>> {
>> n = 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 = pgtls_write(conn, ptr, len);
}
else if (conn->gssenc)
{
n = pg_GSS_write(conn, ptr, len);
}
else
{
n = pqsecure_raw_write(conn, ptr, len);
}

return n;
}

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

Thanks,
--Robbie