On Tue, Mar 15, 2016 at 3:12 PM, David Steele <da...@pgmasters.net> 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