Re: [PATCH v20] GSSAPI encryption support

Greetings!

* Robbie Harwood (rharwood(at)redhat(dot)com) wrote:
> Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > * Peter Eisentraut (peter(dot)eisentraut(at)2ndquadrant(dot)com) wrote:
> >> Or maybe we avoid that, and you rename be-secure-gssapi.c to just
> >> be-gssapi.c and also combine that with the contents of
> >> be-gssapi-common.c.
> >
> > I don't know why we would need to, or want to, combine
> > be-secure-gssapi.c and be-gssapi-common.c, they do have different
> > roles in that be-gssapi-common.c is used even if you aren't doing
> > encryption, while bs-secure-gssapi.c is specifically for handling the
> > encryption side of things.
> >
> > Then again, be-gssapi-common.c does currently only have the one
> > function in it, and it's not like there's an option to compile for
> > *just* GSS authentication (and not encryption), or at least, I don't
> > think that would be a useful option to have.. Robbie?
>
> Yeah, I think I'm opposed to making that an option.

Yeah, I tend to agree, seems silly.

> Worth pointing out here: up until v6, I had this structured differently,
> with all the GSSAPI code in fe-gssapi.c and be-gssapi.c. The current
> separation was suggested by Michael Paquier for ease of reading and to
> keep the code churn down.

I'm still a bit on the fence myself regarding the filenames and where
things exist today. I do agree that it might make sense to move things
around to make the code structure clearer but I also think that doesn't
necessairly have to be done at the same time as this patch.

> >> (And similarly in libpq.)
> >
> > I do agree that we should try to keep the frontend and backend code
> > structures similar in this area, that seems to make sense.
>
> Well, I don't know if it's an argument in either direction, but: on the
> frontend we have about twice as much shared code in fe-gssapi-common.c
> (pg_GSS_have_ccache() and pg_GSS_load_servicename()).

Yeah, that's an interesting point, and I do wonder if we will actually
end up having code that's shared between the frontend and backend
eventually (if we can figure out how to pull out the encryption
algorithm info, for example).

> >> I don't see any tests in the patch. We have a Kerberos test suite at
> >> src/test/kerberos/ and an SSL test suite at src/test/ssl/. You can
> >> get some ideas there.
> >
> > Yeah, I was going to comment on that as well. We definitely should
> > implement tests around this.
>
> Attached. Please note that I don't really speak perl. There's a pile
> of duplicated code in 002_enc.pl that probably should be shared between
> the two. (It would also I think be possible for 001_auth.pl to set up
> the KDC and for 002_enc.pl to then use it.)

I don't think the code duplication between the tests is really all that
much of an issue, though I wouldn't complain if someone wanted to work
on improving that situation. Thanks a lot for adding those test though!

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.

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.

So, please find attached a new version, which also includes the tests
and the other bits that Robbie had sent independent patches for.

Thanks!

Stephen