Re: [HACKERS] [PATCH v6] GSSAPI encryption support
Stephen Frost writes: > Robbie, > > * Robbie Harwood (rharw...@redhat.com) wrote: >> Michael Paquier writes: >> > - maj_stat = gss_accept_sec_context( >> > - &min_stat, >> > + maj_stat = gss_accept_sec_context(&min_stat, >> > >> > This is just noise. >> >> You're not wrong, though I do think it makes the code more readable by >> enforcing style and this is a "cleanup" commit. I'll take it out if it >> bothers you. > > First, thanks much for working on this, I've been following along the > discussions and hope to be able to help move it to commit, once it's > ready. > > Secondly, generally, speaking, we prefer that 'cleanup' type changes > (particularly whitespace-only ones) are in independent commits which are > marked as just whitespace/indentation changes. We have a number of > organizations which follow our code changes and it makes it more > difficult on them to include whitespace/indentation changes with code > changes. Thanks for the clarification. I'll be sure to take it out! signature.asc Description: PGP signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
Robbie, * Robbie Harwood (rharw...@redhat.com) wrote: > Michael Paquier writes: > > - maj_stat = gss_accept_sec_context( > > - &min_stat, > > + maj_stat = gss_accept_sec_context(&min_stat, > > > > This is just noise. > > You're not wrong, though I do think it makes the code more readable by > enforcing style and this is a "cleanup" commit. I'll take it out if it > bothers you. First, thanks much for working on this, I've been following along the discussions and hope to be able to help move it to commit, once it's ready. Secondly, generally, speaking, we prefer that 'cleanup' type changes (particularly whitespace-only ones) are in independent commits which are marked as just whitespace/indentation changes. We have a number of organizations which follow our code changes and it makes it more difficult on them to include whitespace/indentation changes with code changes. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
Michael Paquier writes:
> On Tue, Mar 15, 2016 at 3:12 PM, David Steele wrote:
>> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>>> Here's yet another version of GSSAPI encryption support.
>>
>> This looks far more stable than last versions, cool to see the
>> progress. pgbench -C does not complain on my side so that's a good
>> thing. This is not yet a detailed review, there are a couple of
>> things that I find strange in what you did and are potential subject
>> to bugs, but I need a bit of time to let that mature a bit. This is
>> not something yet committable, but I really like the direction that
>> the patch is taking.
Thanks! I must admit a preference for receiving all feedback at once
(reduces back-and-forth overhead), but if you feel there are still
design-type problems then that's very reasonable. (I also admit to
feeling the pressure of feature freeze in less than a month.)
> For now, regarding 0002:
> /*
> -* Flush message so client will see it, except for AUTH_REQ_OK, which need
> -* not be sent until we are ready for queries.
> +* In most cases, we do not need to send AUTH_REQ_OK until we are ready
> +* for queries, but if we are doing GSSAPI encryption that request must go
> +* out now.
> */
> - if (areq != AUTH_REQ_OK)
> - pq_flush();
> + pq_flush();
> Er, this sends unconditionally the message without caring about the
> protocol, and so this is incorrect, no?
My impression from reading the old version of the comment above it was
that on other protocols it could go either way. I think last time I
made it conditional; I can do so again if it is desired. It's certainly
not /incorrect/ to send it immediately; there's just a question of
performance by minimizing the number of writes as far as I can tell.
> +#ifdef ENABLE_GSS
> + if (conn->gss->buf.data)
> + pfree(conn->gss->buf.data);
> + if (conn->gss->writebuf.data)
> + pfree(conn->gss->writebuf.data);
> +#endif
> You should use resetStringInfo here.
That will leak since resetStringInfo() doesn't free the underlying
representation.
>> OK, everything seems to be working fine with a 9.6 client and server so
>> next I tested older clients and I got this error:
>>
>> $ /usr/lib/postgresql/9.1/bin/psql -h localhost \
>> -U vagr...@pgmasters.net postgres
>> psql: FATAL: GSSAPI did no provide required flags
>>
>> There's a small typo in the error message, BTW.
Thanks, will fix. I forgot that MIT doesn't provide GSS_C_REPLAY_FLAG
and GSS_C_SEQUENCE_FLAG by default. Removing those from auth.c should
temporarily resolve the problem, which is what I'll do in the next
version. (Tested with MIT krb5.)
On the subject of older code, I realized (one of those wake up in the
middle of the night-type moments) that the old server has a potential
problem with new clients now in that we try to decrypt the "help I don't
recognize connection parameter gss_encrypt" error message. v8 will have
some sort of a fix, though I don't know what yet. The only thing I've
come up with so far is to have the client decrypt check the first time
through for packets beginning with 'E'. Pretty much anything I can do
will end up being a poor substitute for being at the protocol layer
anyway.
> And in 0003, the previous error is caused by that:
> + target_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG |
> + GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG;
> + if ((gflags & target_flags) != target_flags)
> + {
> + ereport(FATAL, (errmsg("GSSAPI did no provide required flags")));
> + return STATUS_ERROR;
> + }
> Yeah, this is a recipe for protocol incompatibility and here the
> connection context is not yet fully defined I believe. We need to be
> careful.
Nope, it's done. This is happening immediately prior to username
checks. By the time we exit the do/while the context is fully
complete.
> - maj_stat = gss_accept_sec_context(
> - &min_stat,
> + maj_stat = gss_accept_sec_context(&min_stat,
>
> This is just noise.
You're not wrong, though I do think it makes the code more readable by
enforcing style and this is a "cleanup" commit. I'll take it out if it
bothers you.
signature.asc
Description: PGP signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
On Tue, Mar 15, 2016 at 3:12 PM, David Steele wrote:
> On 3/8/16 5:44 PM, Robbie Harwood wrote:
>> Here's yet another version of GSSAPI encryption support.
This looks far more stable than last versions, cool to see the
progress. pgbench -C does not complain on my side so that's a good
thing. This is not yet a detailed review, there are a couple of things
that I find strange in what you did and are potential subject to bugs,
but I need a bit of time to let that mature a bit. This is not
something yet committable, but I really like the direction that the
patch is taking.
For now, regarding 0002:
/*
-* Flush message so client will see it, except for AUTH_REQ_OK, which need
-* not be sent until we are ready for queries.
+* In most cases, we do not need to send AUTH_REQ_OK until we are ready
+* for queries, but if we are doing GSSAPI encryption that request must go
+* out now.
*/
- if (areq != AUTH_REQ_OK)
- pq_flush();
+ pq_flush();
Er, this sends unconditionally the message without caring about the
protocol, and so this is incorrect, no?
+#ifdef ENABLE_GSS
+ if (conn->gss->buf.data)
+ pfree(conn->gss->buf.data);
+ if (conn->gss->writebuf.data)
+ pfree(conn->gss->writebuf.data);
+#endif
You should use resetStringInfo here.
> OK, everything seems to be working fine with a 9.6 client and server so
> next I tested older clients and I got this error:
>
> $ /usr/lib/postgresql/9.1/bin/psql -h localhost \
> -U vagr...@pgmasters.net postgres
> psql: FATAL: GSSAPI did no provide required flags
>
> There's a small typo in the error message, BTW.
And in 0003, the previous error is caused by that:
+ target_flags = GSS_C_MUTUAL_FLAG | GSS_C_REPLAY_FLAG |
+ GSS_C_SEQUENCE_FLAG | GSS_C_CONF_FLAG | GSS_C_INTEG_FLAG;
+ if ((gflags & target_flags) != target_flags)
+ {
+ ereport(FATAL, (errmsg("GSSAPI did no provide required flags")));
+ return STATUS_ERROR;
+ }
Yeah, this is a recipe for protocol incompatibility and here the
connection context is not yet fully defined I believe. We need to be
careful.
- maj_stat = gss_accept_sec_context(
- &min_stat,
+ maj_stat = gss_accept_sec_context(&min_stat,
This is just noise.
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
On 3/8/16 5:44 PM, Robbie Harwood wrote: > Here's yet another version of GSSAPI encryption support. OK, everything seems to be working fine with a 9.6 client and server so next I tested older clients and I got this error: $ /usr/lib/postgresql/9.1/bin/psql -h localhost \ -U vagr...@pgmasters.net postgres psql: FATAL: GSSAPI did no provide required flags There's a small typo in the error message, BTW. Here's the output from the server logs: DEBUG: forked new backend, pid=7746 socket=9 DEBUG: postgres child[7746]: starting with ( DEBUG: postgres DEBUG: ) DEBUG: InitPostgres DEBUG: my backend ID is 2 DEBUG: StartTransaction DEBUG: name: unnamed; blockState: DEFAULT; state: INPROGR, xid/subid/cid: 0/1/0, nestlvl: 1, children: DEBUG: Processing received GSS token of length 667 DEBUG: gss_accept_sec_context major: 0, minor: 0, outlen: 161, outflags: 1b2 DEBUG: sending GSS response token of length 161 DEBUG: sending GSS token of length 161 FATAL: GSSAPI did no provide required flags DEBUG: shmem_exit(1): 1 before_shmem_exit callbacks to make DEBUG: shmem_exit(1): 6 on_shmem_exit callbacks to make DEBUG: proc_exit(1): 3 callbacks to make DEBUG: exit(1) DEBUG: shmem_exit(-1): 0 before_shmem_exit callbacks to make DEBUG: shmem_exit(-1): 0 on_shmem_exit callbacks to make DEBUG: proc_exit(-1): 0 callbacks to make DEBUG: reaping dead processes DEBUG: server process (PID 7746) exited with exit code 1 I got the same result with a 9.4 client so didn't try any others. -- -David da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
On 3/14/16 7:20 PM, Robbie Harwood wrote: > David Steele writes: > >> >> Strange timing since I was just testing this. Here's what I got: >> >> $ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres >> conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179 >> psql (9.6devel) >> Type "help" for help. >> >> postgres=> > > Thanks, that certainly is interesting! I did finally manage to > reproduce the issue on my end, but the rate of incidence is much lower > than what you and Michael were seeing: I have to run connections in a > loop for about 10-20 minutes before it makes itself apparent (and no, > it's not due to entropy). Apparently I just wasn't patient enough. I'm running client and server on the same VM - perhaps that led to less latency than your setup? > All that is to say: thank you very much for investigating that! My pleasure! -- -David da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
David Steele writes: > On 3/14/16 4:10 PM, Robbie Harwood wrote: > >> David Steele writes: >> >>> On 3/8/16 5:44 PM, Robbie Harwood wrote: >>> Here's yet another version of GSSAPI encryption support. It's also available for viewing on my github: >>> >>> psql simply hangs and never returns. I have attached a pcap of the >>> psql/postgres session generated with: >> >> Please disregard my other email. I think I've found the issue; will >> post a new version in a moment. > > Strange timing since I was just testing this. Here's what I got: > > $ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres > conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179 > psql (9.6devel) > Type "help" for help. > > postgres=> Thanks, that certainly is interesting! I did finally manage to reproduce the issue on my end, but the rate of incidence is much lower than what you and Michael were seeing: I have to run connections in a loop for about 10-20 minutes before it makes itself apparent (and no, it's not due to entropy). Apparently I just wasn't patient enough. > This was after commenting out: > > // appendBinaryPQExpBuffer(&conn->gwritebuf, > //conn->inBuffer + conn->inStart, > //conn->inEnd - conn->inStart); > // conn->inEnd = conn->inStart; > > The good news I can log on every time now! Since conn->inStart == conn->inEnd in the case you were testing, the lines you commented out would have been a no-op anyway (that's the normal case of operation, as far as I can tell). That said, the chances of hitting the race for me seemed very dependent on how much code wants to run in that conditional: I got it up to 30-40 minutes when I added a lot of printf()s (can't just run in gdb because it's nondeterministic and rr has flushing bugs at the moment). All that is to say: thank you very much for investigating that! signature.asc Description: PGP signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
On 3/14/16 4:10 PM, Robbie Harwood wrote: > David Steele writes: > >> Hi Robbie, >> >> On 3/8/16 5:44 PM, Robbie Harwood wrote: >>> Hello friends, >>> >>> Here's yet another version of GSSAPI encryption support. It's also >>> available for viewing on my github: >> >> The build went fine but when testing I was unable to logon at all. I'm >> using the same methodology as in >> http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except >> that I'm running against 51c0f63 and using the v6 patch set. >> >> psql simply hangs and never returns. I have attached a pcap of the >> psql/postgres session generated with: >> >> tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap >> >> If you would like me to capture more information please let me know >> specifically how you would like me to capture it. > > Please disregard my other email. I think I've found the issue; will > post a new version in a moment. Strange timing since I was just testing this. Here's what I got: $ pg/bin/psql -h localhost -U vagr...@pgmasters.net postgres conn->inStart = 179, conn->inEnd = 179, conn->inCursor = 179 psql (9.6devel) Type "help" for help. postgres=> This was after commenting out: // appendBinaryPQExpBuffer(&conn->gwritebuf, // conn->inBuffer + conn->inStart, // conn->inEnd - conn->inStart); // conn->inEnd = conn->inStart; The good news I can log on every time now! -- -David da...@pgmasters.net signature.asc Description: OpenPGP digital signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
David Steele writes: > Hi Robbie, > > On 3/8/16 5:44 PM, Robbie Harwood wrote: >> Hello friends, >> >> Here's yet another version of GSSAPI encryption support. It's also >> available for viewing on my github: > > The build went fine but when testing I was unable to logon at all. I'm > using the same methodology as in > http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except > that I'm running against 51c0f63 and using the v6 patch set. > > psql simply hangs and never returns. I have attached a pcap of the > psql/postgres session generated with: > > tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap > > If you would like me to capture more information please let me know > specifically how you would like me to capture it. Please disregard my other email. I think I've found the issue; will post a new version in a moment. signature.asc Description: PGP signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
David Steele writes: > On 3/8/16 5:44 PM, Robbie Harwood wrote: >> >> Here's yet another version of GSSAPI encryption support. It's also >> available for viewing on my github: > > I got this warning when applying the first patch in the set: > > ../other/v6-0001-Move-common-GSSAPI-code-into-its-own-files.patch:245: > new blank line at EOF. > + > warning: 1 line adds whitespace errors. Hah, so it does. Thanks for catching it; will fix. > The build went fine but when testing I was unable to logon at all. I'm > using the same methodology as in > http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except > that I'm running against 51c0f63 and using the v6 patch set. > > psql simply hangs and never returns. I have attached a pcap of the > psql/postgres session generated with: > > tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap > > If you would like me to capture more information please let me know > specifically how you would like me to capture it. Thank you for the pcap! (I'm using wireshark so formats it can open are greatly appreciated.) This suggests that the hang is my client code's fault, but just in case: I assume nothing unusual was logged on the server? v6-0002-Connection-encryption-support-for-GSSAPI.patch in fe-connect.c at around line 2518 adds a call to appendBinaryPQExpBuffer and sets conn->inEnd. Can you try without those lines? Can you also (e.g., with gdb or by adding printf calls) tell me what the values of conn->inStart, conn->inEnd, and conn->inCursor any time (should only be once) that those lines are triggered? > I reverted to v5 and got the same behavior I was seeing with v4 and v5, > namely that I can only logon occasionally and usually get this error: > > psql: expected authentication request from server, but received > > Using a fresh build from 51c0f63 I can logon reliably every time so I > don't think there's an issue in my environment. Agreed, I'm sure I've caused it somehow, though I don't know what's wrong yet. (And if it weren't my fault but you didn't get useful errors out, that'd be my fault anyway for not checking enough stuff!) I don't know if this would say anything relevant, but it might be interesting to see what the results are of applying [1] to the v5 code. It's the same approach to solving the problem, though it happens at a different time due to the aforementioned protocol change between v5 and v6. Thanks, --Robbie [1] https://github.com/frozencemetery/postgres/commit/82c89227a6b499ac9273044f91cff747c154629f signature.asc Description: PGP signature
Re: [HACKERS] [PATCH v6] GSSAPI encryption support
Hi Robbie, On 3/8/16 5:44 PM, Robbie Harwood wrote: > Hello friends, > > Here's yet another version of GSSAPI encryption support. It's also > available for viewing on my github: I got this warning when applying the first patch in the set: ../other/v6-0001-Move-common-GSSAPI-code-into-its-own-files.patch:245: new blank line at EOF. + warning: 1 line adds whitespace errors. I know it's minor but I'm always happier when patches apply cleanly. The build went fine but when testing I was unable to logon at all. I'm using the same methodology as in http://www.postgresql.org/message-id/56be0ff9.70...@pgmasters.net except that I'm running against 51c0f63 and using the v6 patch set. psql simply hangs and never returns. I have attached a pcap of the psql/postgres session generated with: tcpdump -i lo -nnvvXSs 1514 port 5432 -w gssapi.pcap If you would like me to capture more information please let me know specifically how you would like me to capture it. I reverted to v5 and got the same behavior I was seeing with v4 and v5, namely that I can only logon occasionally and usually get this error: psql: expected authentication request from server, but received Using a fresh build from 51c0f63 I can logon reliably every time so I don't think there's an issue in my environment. -- -David da...@pgmasters.net gssapi.pcap Description: Binary data signature.asc Description: OpenPGP digital signature
[HACKERS] [PATCH v6] GSSAPI encryption support
Hello friends, Here's yet another version of GSSAPI encryption support. It's also available for viewing on my github: https://github.com/frozencemetery/postgres/tree/feature/gssencrypt6 Let me hit the highlights of this time around: - Fallback code is back! It's almost unchanged from early versions of this patchset. Corresponding doc changes for this and the next item are of course included. - Minor protocol change. I did not realize that connection parameters were not read until after auth was complete, which means that in this version I go back to sending the AUTH_REQ_OK in the clear. Though I found this initially irritating since it required re-working the should_crypto conditions, it ends up being a net positive since I can trade a library call for a couple variables. - Client buffer flush on completion of authentication. This should prevent the issue with the client getting unexpected message type of NUL due to encrypted data not getting decrypted. I continue to be unable to replicate this issue, but since the codepath triggers in the "no data buffered case" all the math is sound. (Famous last words I'm sure.) - Code motion is its own patch. This was requested and hopefully clarifies what's going on. - Some GSSAPI authentication fixes have been applied. I've been staring at this code too long now and writing this made me feel better. If it should be a separate change that's fine and easy to do. Thanks! From 5674aa74effab4931bac1044f32dee83d915aa90 Mon Sep 17 00:00:00 2001 From: Robbie Harwood Date: Fri, 26 Feb 2016 16:07:05 -0500 Subject: [PATCH 1/3] Move common GSSAPI code into its own files On both the frontend and backend, prepare for GSSAPI encryption suport by moving common code for error handling into a common file. Other than build-system changes, no code changes occur in this patch. --- configure | 2 + configure.in| 1 + src/Makefile.global.in | 1 + src/backend/libpq/Makefile | 4 ++ src/backend/libpq/auth.c| 63 +-- src/backend/libpq/be-gssapi-common.c| 75 + src/include/libpq/be-gssapi-common.h| 26 src/interfaces/libpq/Makefile | 4 ++ src/interfaces/libpq/fe-auth.c | 48 + src/interfaces/libpq/fe-gssapi-common.c | 63 +++ src/interfaces/libpq/fe-gssapi-common.h | 21 + 11 files changed, 199 insertions(+), 109 deletions(-) create mode 100644 src/backend/libpq/be-gssapi-common.c create mode 100644 src/include/libpq/be-gssapi-common.h create mode 100644 src/interfaces/libpq/fe-gssapi-common.c create mode 100644 src/interfaces/libpq/fe-gssapi-common.h diff --git a/configure b/configure index b3f3abe..a5bd629 100755 --- a/configure +++ b/configure @@ -713,6 +713,7 @@ with_systemd with_selinux with_openssl krb_srvtab +with_gssapi with_python with_perl with_tcl @@ -5491,6 +5492,7 @@ $as_echo "$with_gssapi" >&6; } + # # Kerberos configuration parameters # diff --git a/configure.in b/configure.in index 0bd90d7..4fd8f05 100644 --- a/configure.in +++ b/configure.in @@ -636,6 +636,7 @@ PGAC_ARG_BOOL(with, gssapi, no, [build with GSSAPI support], krb_srvtab="FILE:\$(sysconfdir)/krb5.keytab" ]) AC_MSG_RESULT([$with_gssapi]) +AC_SUBST(with_gssapi) AC_SUBST(krb_srvtab) diff --git a/src/Makefile.global.in b/src/Makefile.global.in index e94d6a5..3dbc5c2 100644 --- a/src/Makefile.global.in +++ b/src/Makefile.global.in @@ -183,6 +183,7 @@ with_perl = @with_perl@ with_python = @with_python@ with_tcl = @with_tcl@ with_openssl = @with_openssl@ +with_gssapi = @with_gssapi@ with_selinux = @with_selinux@ with_systemd = @with_systemd@ with_libxml = @with_libxml@ diff --git a/src/backend/libpq/Makefile b/src/backend/libpq/Makefile index 09410c4..a8cc9a0 100644 --- a/src/backend/libpq/Makefile +++ b/src/backend/libpq/Makefile @@ -21,4 +21,8 @@ ifeq ($(with_openssl),yes) OBJS += be-secure-openssl.o endif +ifeq ($(with_gssapi),yes) +OBJS += be-gssapi-common.o +endif + include $(top_srcdir)/src/backend/common.mk diff --git a/src/backend/libpq/auth.c b/src/backend/libpq/auth.c index 57c2f48..73d493e 100644 --- a/src/backend/libpq/auth.c +++ b/src/backend/libpq/auth.c @@ -136,11 +136,7 @@ bool pg_krb_caseins_users; * */ #ifdef ENABLE_GSS -#if defined(HAVE_GSSAPI_H) -#include -#else -#include -#endif +#include "libpq/be-gssapi-common.h" static int pg_GSS_recvauth(Port *port); #endif /* ENABLE_GSS */ @@ -715,63 +711,6 @@ recv_and_check_password_packet(Port *port, char **logdetail) */ #ifdef ENABLE_GSS -#if defined(WIN32) && !defined(WIN32_ONLY_COMPILER) -/* - * MIT Kerberos GSSAPI DLL doesn't properly export the symbols for MingW - * that contain the OIDs required. Redefine here, values copied -
