Re: [PATCH v6] GSSAPI encryption support

On Tue, Mar 15, 2016 at 3:12 PM, David Steele <david(at)pgmasters(dot)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 vagrant(at)PGMASTERS(dot)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