Re: [PATCH v20] GSSAPI encryption support
От | Stephen Frost |
---|---|
Тема | Re: [PATCH v20] GSSAPI encryption support |
Дата | |
Msg-id | 20190316102110.GU6197@tamriel.snowman.net обсуждение исходный текст |
Ответ на | Re: [PATCH v20] GSSAPI encryption support (Robbie Harwood <rharwood@redhat.com>) |
Ответы |
Re: [PATCH v20] GSSAPI encryption support
|
Список | pgsql-hackers |
Greetings! * Robbie Harwood (rharwood@redhat.com) wrote: > Stephen Frost <sfrost@snowman.net> writes: > > * Peter Eisentraut (peter.eisentraut@2ndquadrant.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
Вложения
В списке pgsql-hackers по дате отправления:
Предыдущее
От: Magnus HaganderДата:
Сообщение: Re: Make pg_checksums complain if compiled BLCKSZ and data folder'sblock size differ
Следующее
От: Dean RasheedДата:
Сообщение: Re: [HACKERS] PATCH: multivariate histograms and MCV lists