Re: [PATCH v6] GSSAPI encryption support

Michael Paquier <michael(dot)paquier(at)gmail(dot)com> writes:

> 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.

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 vagrant(at)PGMASTERS(dot)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.