Re: [PATCH v19] GSSAPI encryption support

Greetings Robbie,

* Dmitry Dolgov (9erthalion6(at)gmail(dot)com) wrote:
> > On Tue, Oct 2, 2018 at 11:12 PM Robbie Harwood <rharwood(at)redhat(dot)com> wrote:
> >
> > Michael Paquier <michael(at)paquier(dot)xyz> writes:
> >
> > > On Mon, Aug 06, 2018 at 05:23:28PM -0400, Robbie Harwood wrote:
> > >> If you're in a position where you're using Kerberos (or most other
> > >> things from the GSSAPI) for authentication, the encryption comes at
> > >> little to no additional setup cost. And then you get all the security
> > >> benefits outlined in the docs changes.
> > >
> > > Please note that the last patch set does not apply anymore, so I have
> > > moved it to CF 2018-11 and marked it as waiting on author.
> >
> > Indeed. Please find v19 attached. It's just a rebase; no architecture
> > changes.
>
> Unfortunately, patch needs another rebase, could you do this? In the meantime
> I'll move it to the next CF.

This patch needs a few minor changes to get it back to working, but I'm
happy to say that with those changes, it seems to be working rather well
for me.

I'm working on a more comprehensive review but I wanted to go ahead and
get these first minor items out of the way:

Needs the same treatment done in ab69ea9, for all the WaitLatchOrSocket
calls in be-secure-gssapi.c:

- WaitLatchOrSocket(MyLatch, WL_SOCKET_WRITEABLE, port->sock, 0,
+ WaitLatchOrSocket(MyLatch, WL_SOCKET_WRITEABLE | WL_EXIT_ON_PM_DEATH, port->sock, 0,
WAIT_EVENT_GSS_OPEN_SERVER);

(and I have to wonder- if we want nearly all callers of
WaitLatch/WaitLatchOrSocket to use WL_EXIT_ON_PM_DEATH, maybe we should
make that the default and allow it to be overridden..? Also, does
ModifyWaitEvent() need to also do some checking here..? Should
be_tls_read()'s waitfor settings also include WL_EXIT_ON_PM_DEATH?)

Needed a minor adjustment in src/interfaces/libpq/fe-connect.c due to
conflict with 6e5f8d4.

Otherwise, it's building and running with all flavors of client and
server-side GSS-related options (require/disable/prefer and
host/hostgss/hostnogss).

Not surprisingly, trying to connect from a newer psql w/
PGGSSMODE=require (explicitly requesting encryption from the server)
with an older server blows up, but there's not much we can do for that.
Using prefer works, with an extra roundtrip to discover the server
doesn't understand GSSAPI encryption and then falling back.

Also, when connecting from an older psql to a newer server, if
pg_hba.conf has 'host' or 'hostnogss', everything works great (though
without GSSAPI encryption, of course), while an entry of 'hostgss'
returns the typical 'no pg_hba.conf entry found', as you'd expect.

Just in general, it seems like the code could use a lot more
comments. :) Also, it needs some pgindent run over it.

That's about all I have time to cover for today, but maybe you could try
to spruce up the comments (I'm at least a fan of function-level
comments, in particular, explaining how they're to be used, et al..),
see if you can get pgindent run over the changes and make the
above-mentioned fixes and then perhaps we can get cfbot to do its thing,
ask the other folks on the thread who were having issues before to retry
with this patch, if possible, and I'll start doing a more thorough code
review later this week.

Thanks!

Stephen