From: Stephen Frost <sfrost@snowman.net>
To: Robbie Harwood <rharwood@redhat.com>
Cc: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org, Nico Williams <nico@cryptonector.com>
Cc: Michael Paquier <michael@paquier.xyz>
Cc: Alvaro Herrera <alvherre@2ndquadrant.com>
Cc: David Steele <david@pgmasters.net>
Subject: Re: [PATCH v20] GSSAPI encryption support
Date: Mon, 18 Feb 2019 10:32:53 -0500
Message-ID: <20190218153253.GE6197@tamriel.snowman.net> (raw)
In-Reply-To: <jlgzhqtqgvw.fsf@redhat.com>
References: <jlg1tgq1ktm.fsf@thriss.redhat.com>
<jlgy38mir81.fsf@redhat.com>
<20190216041026.gpkfn4e6wrozliyb@alap3.anarazel.de>
<jlgzhqtqgvw.fsf@redhat.com>
Greetings,
* Robbie Harwood (rharwood@redhat.com) wrote:
> 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).
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/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.
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
Attachments:
signature.asc (application/pgp-signature, 818B) download
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: sfrost@snowman.net, rharwood@redhat.com, andres@anarazel.de, nico@cryptonector.com, michael@paquier.xyz, alvherre@2ndquadrant.com, david@pgmasters.net
Subject: Re: [PATCH v20] GSSAPI encryption support
In-Reply-To: <20190218153253.GE6197@tamriel.snowman.net>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox