Re: [PATCH v20] GSSAPI encryption support

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