Re: [PATCH v22] GSSAPI encryption support

Greetings,

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

Agreed, we could do that later if it seems like it would be helpful.

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

Thanks. :) I definitely like being able to see from the backend side of
things when a connection is encrypted or not, and being able to see the
principal used for authentication is really nice too.

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

Fantastic, thanks so much for working on this over the years! Unless
there's any further comments, I'm going to push this tomorrow.

Thanks again!

Stephen