Re: [PATCH v22] GSSAPI encryption support

Robbie Harwood <rharwood@redhat.com>

From: Robbie Harwood <rharwood@redhat.com>
To: Stephen Frost <sfrost@snowman.net>
Cc: Peter Eisentraut <peter.eisentraut@2ndquadrant.com>, pgsql-hackers@postgresql.org, Nico Williams <nico@cryptonector.com>, Michael Paquier <michael@paquier.xyz>, Alvaro Herrera <alvherre@2ndquadrant.com>, David Steele <david@pgmasters.net>
Date: 2019-04-01T15:13:24Z
Lists: pgsql-hackers
Stephen Frost <sfrost@snowman.net> writes:

> * Robbie Harwood (rharwood@redhat.com) wrote:
>> Stephen Frost <sfrost@snowman.net> writes:
>> 
>> 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.

Yeah, that's definitely a fair point.  We could combine the two
approaches, but I don't really see a reason to if it's unlikely to be an
issue - as you say, this is more readable.  It can always be a follow-on
if needed.

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

Good idea.

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

I'm happy with this!  Appreciate you exploring my concerns.

Thanks,
--Robbie

Commits

  1. GSSAPI encryption support

  2. Fix typo