Re: [PATCH v5] GSSAPI encryption support
От | Michael Paquier |
---|---|
Тема | Re: [PATCH v5] GSSAPI encryption support |
Дата | |
Msg-id | CAB7nPqSu55_AgoiYYfhaO-aHkN5qEVMDt_b1-GFQYw9Yvo=F8g@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: [PATCH v5] GSSAPI encryption support (Robbie Harwood <rharwood@redhat.com>) |
Ответы |
Re: [PATCH v5] GSSAPI encryption support
(Robbie Harwood <rharwood@redhat.com>)
|
Список | pgsql-hackers |
On Tue, Feb 16, 2016 at 2:45 AM, Robbie Harwood <rharwood@redhat.com> wrote: > David Steele <david@pgmasters.net> writes: >> On 2/10/16 4:06 PM, Robbie Harwood wrote: >>> Hello friends, >>> >>> For your consideration, here is a new version of GSSAPI encryption >>> support. For those who prefer, it's also available on my github: >>> https://github.com/frozencemetery/postgres/commit/c92275b6605d7929cda5551de47a4c60aab7179e >> >> It tried out this patch and ran into a few problems: >> >> 1) It didn't apply cleanly to HEAD. It did apply cleanly on a455878 >> which I figured was recent enough for testing. I didn't bisect to find >> the exact commit that broke it. > > It applied to head of master (57c932475504d63d8f8a68fc6925d7decabc378a) > for me (`patch -p1 < v4-GSSAPI-encryption-support.patch`). I rebased it > anyway and cut a v5 anyway, just to be sure. It's attached, and > available on github as well: > https://github.com/frozencemetery/postgres/commit/dc10e3519f0f6c67f79abd157dc8ff1a1c293f53 v5 is applying fine for me. There were two diff hunks in routine.sgml but nothing to worry about. >> 2) While I was able to apply the patch and get it compiled it seemed >> pretty flaky - I was only able to logon about 1 in 10 times on average. >> Here was my testing methodology: >> >> a) Build Postgres from a455878 (without your patch), install/configure >> Kerberos and get everything working. I was able the set the auth method >> to gss in pg_hba.conf and logon successfully every time. >> >> b) On the same system rebuild Postgres from a455878 including your patch >> and attempt authentication. >> >> The problems arose after step 2b. Sometimes I would try to logon twenty >> times without success and sometimes it only take five or six attempts. >> I was never able to logon successfully twice in a row. >> >> When not successful the client always output this incomplete message >> (without terminating LF): >> >> psql: expected authentication request from server, but received >> >> From the logs I can see the server is reporting EOF from the client, >> though the client does not core dump and prints the above message before >> exiting. >> >> I have attached files that contain server logs at DEBUG5 and tcpdump >> output for both the success and failure cases. >> >> Please let me know if there's any more information you would like me to >> provide. > > What I can't tell from looking at your methodology is whether both the > client and server were running my patches or no. There's no fallback > here (I'd like to talk about how that should work, with example from > v1-v3, if people have ideas). This means that both the client and the > server need to be running my patches for the moment. Is this your > setup? We need to be careful here, backward-compatibility is critical for both the client and the server, or to put it in other words, an uptables client should still be able to connect a patched server, and vice-versa. This is an area where it is important to not break any third-part tool, either using libpq or reimplementing the frontend protocol. So I finally began to dive into your new patch... And after testing this is breaking the authentication protocol for GSS. I have been able to connect to a server once, but at the second attempt and after connection is failing with the following error: psql: expected authentication request from server, but received ioltas Also, something that is missing is the parametrization that has been discussed the last time this patch was on the mailing list. Where is the capacity to control if a client is connecting to a server that is performing encryption and downgrade to non-ecrypted connection should the server not be able to support it? Enforcing a client to require encryption support using pg_hba.conf was as well a good thing. Some more infrastructure is needed here, I thought that there was an agreement previously regarding that. Also, and to make the review a bit easier, it would be good to split the patch into smaller pieces (thanks for waiting on my input here, this became quite clear after looking at this code). Basically, pg_GSS_error is moved to a new file, bringing with it pg_GSS_recvauth because the error routine is being used by the new ones you are introducing: be_gssapi_write, etc. The split that this patch is doing is a bit confusing, all the GSS-related stuff is put within one single file: - read/write routines - authentication routine - constants - routines for error handling Mixing read/write routines with the authentication routine looks wrong, because the read-write routines just need to create a dependency with for example be-secure.c on the backend. In short, where before authentication and secure read/writes after authentication get linked to each other, and create a dependency that did not exist before. For the sake of clarity I would suggest the following split: - be-gss-common.c, where all the constants and the error handling routine are located. - Let authrecv in auth.c - Move only the read/write routines to the new file be-[secure-]gssapi.c Splitting the patches into one that moves around the routines, and a second that introduces the new logic would bring more clarity. As In understood from the patch, enforcing encryption is not good either, and any patched clients would not be able to 9.5 or older servers. Regarding the parametrization, I liked the previous v1-v3 layout to control the encryption, and this allows to set up the session context on backend and frontend at authentication. The connection errors are also something to look at, I guess that David and I ran into the same thing. I am wondering how this patch is tested to be honest. -- Michael
В списке pgsql-hackers по дате отправления: