Re: [PATCH v6] GSSAPI encryption support
От | Robbie Harwood |
---|---|
Тема | Re: [PATCH v6] GSSAPI encryption support |
Дата | |
Msg-id | jlgtwk7il5x.fsf@thriss.redhat.com обсуждение исходный текст |
Ответ на | Re: [PATCH v6] GSSAPI encryption support (Michael Paquier <michael.paquier@gmail.com>) |
Ответы |
Re: [PATCH v6] GSSAPI encryption support
(Stephen Frost <sfrost@snowman.net>)
|
Список | pgsql-hackers |
Michael Paquier <michael.paquier@gmail.com> writes: > On Tue, Mar 15, 2016 at 3:12 PM, David Steele <david@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. 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@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.
В списке pgsql-hackers по дате отправления:
Следующее
От: Peter GeogheganДата:
Сообщение: Re: [NOVICE] WHERE clause not used when index is used