[PATCH v22] GSSAPI encryption support

Greetings,

* Robbie Harwood (rharwood(at)redhat(dot)com) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
>
> > One of the things that I really didn't care for in this patch was the
> > use of the string buffers, without any real checks (except for "oh,
> > you tried to allocated over 1G"...) to make sure that the other side
> > of the connection wasn't feeding us ridiculous packets, and with the
> > resizing of the buffers, etc, that really shouldn't be necessary.
> > After chatting with Robbie about these concerns while reading through
> > the code, we agreed that we should be able to use fixed buffer sizes
> > and use the quite helpful gss_wrap_size_limit() to figure out how much
> > data we can encrypt without going over our fixed buffer size. As
> > Robbie didn't have time to implement those changes this past week, I
> > did so, and added a bunch more comments and such too, and have now
> > gone through and done more testing. Robbie has said that he should
> > have time this upcoming week to review the changes that I made, and
> > I'm planning to go back and review other parts of the patch more
> > closely now as well.
>
> In general this looks good - there are a couple minor comments inline,
> but it's fine.

Thanks for reviewing!

> I wanted to note a couple things about this approach. It now uses one
> more buffer than before (in contrast to the previous approach, which
> reused a buffer for received data that was encrypted and decrypted).

Yeah, I don't see that as too much of an issue and it certainly seems
cleaner and simpler to reason about to me, which seems worth the modest
additional buffer cost. That's certainly something we could change if
others feel differently.

> Since these are static fixed size buffers, this increases the total
> steady-state memory usage by 16k as opposed to re-using the buffer.
> This may be fine; I don't know how tight RAM is here.

It seems unlikely to be an issue to me- and I would contend that the
prior implementation didn't actually take any steps to prevent the other
side from sending packets of nearly arbitrary size (up to 1G), so while
the steady-state memory usage of the prior implementation was less when
everyone was playing nicely, it could have certainly been abused. I'm a
lot happier having an explicit cap on how much memory will be used, even
if that cap is a bit higher.

> > Note that there's an issue with exporting the context to get the
> > encryption algorithm used that I've asked Robbie to look into, so
> > that's no longer done and instead we just print that the connection is
> > encrypted, if it is. If we can't figure out a way to make that work
> > then obviously I'll pull out that code, and if we can get it to work
> > then I'll update it to be done through libpq properly, as I had
> > suggested earlier. That's really more of a nice to have in any case
> > though, so I may just exclude it for now anyway if it ends up adding
> > any complications.
>
> Correct. Unfortunately I'd overlooked that the lucid interface won't
> meet our needs (destroys the context). So the two options here are:
> SASL SSF (and I'll separately push more mechs to add support for that),
> or nothing at all.

I went ahead and ripped out all of that, including the SASL SSF (which
really didn't seem like it added all that much... but if others feel
like it's useful, then we can add it back in later). That also let me
get rid of the configure/configure.in changes, which was nice. I did
add in a simple pg_stat_gssapi view, modeled on pg_stat_ssl, so that you
can check server-side if GSSAPI was used for authentication and/or
encryption, and what principal was used if GSSAPI was used for
authentication.

I also changed the libpq function to return a boolean to indicate if
encryption was used or not; I don't think it's appropriate to have a
libpq function that will just print stuff to stdout, that should be the
client program's job, so that's handled by psql now.

There were some other bits that had been forgotten to get removed from
when I moved away from using the string buffers, but I went through and
I'm pretty confident that I've cleaned all of that out now.

> If you want a patch for that I can make one, but I think there was code
> already... just needed a ./configure check program for whether the OID
> is defined.
>
> > +ssize_t
> > +be_gssapi_write(Port *port, void *ptr, size_t len)
> > +{
> > + size_t bytes_to_encrypt = len;
> > + size_t bytes_encrypted = 0;
> > +
> > + /*
> > + * Loop through encrypting data and sending it out until
> > + * secure_raw_write() complains (which would likely mean that the socket
> > + * is non-blocking and the requested send() would block, or there was some
> > + * kind of actual error) and then return.
> > + */
> > + while (bytes_to_encrypt || PqGSSSendPointer)
> > + {
>
> I guess it's not a view everyone will share, but I think this block is
> too long. Maybe a helper function around secure_raw_write() would help?
> (The check-and-send part at the start.)

I looked for a way to add a wrapper around secure_raw_write() that would
actually be useful here but honestly, it really didn't seem like it'd
have done anything but make the code unnecessairly complicated. I mean,
there's literally only one call to secure_raw_write() inside of
be_gssapi_write()..

> I have similar nits about the other functions that don't fit on my
> (86-line high) screen, though I guess a lot of this is due to project
> style using a lot of vertical space.

So, I did think about what you wrote here and we did have multiple calls
to be_gssapi_read/write (and similar on the frontend) inside of the
secure_open_gssapi() / pqsecure_open_gss() calls, with each call
location having to handle the various results from read/write. I went
ahead and made wrappers and used them instead, simplifying those
functions and reducing the amount of nearly copy/paste code for handling
return values from read/write. Those wrappers don't work for the
actual secure read/write functions though, since those typically are
just able to pass the actual read/write() return back up to the caller,
unlike the open calls.

> > + if (GSS_ERROR(major))
> > + pg_GSS_error(FATAL, gettext_noop("GSSAPI context error"),
>
> I'd prefer this to be a different error message than the init/accept
> errors - maybe something like "GSSAPI size check error"?
>
> > + if (GSS_ERROR(major))
> > + pg_GSS_error(libpq_gettext("GSSAPI context error"), conn,
>
> Same here.

Ok, I've fixed those.

> Again, these are nits, and I think I'm okay with the changes.

Great, thanks again for reviewing!

Updated patch attached, if you could take another look through it,
that'd be great.

Thanks,

Stephen