On 06/06/2017 06:09 AM, Michael Paquier wrote:
> On Thu, Jun 1, 2017 at 4:58 AM, Heikki Linnakangas <hlinnaka@iki.fi> wrote:
>> To fix, I suppose we can do what you did for SASL in your patch, and move
>> the cleanup of conn->gctx from closePGconn to pgDropConnection. And I
>> presume we need to do the same for the SSPI state too, but I don't have a
>> Windows set up to test that at the moment.
>
> SSPI does not complain with sslmode=prefer as each time
> pg_SSPI_startup() is called conn->sspictx is enforced to NULL. This
> looks wrong to me by the way as pg_SSPI_startup() is invoked only once
> per authentication, and it leaks memory this way. That's also
> inconsistent with SASL and GSS. At the same time this inconsistency is
> not causing actual problems except a leak with SSPI in libpq, so not
> doing anything except on HEAD looks fine to me.
Ok, I committed your patch, with some minor changes. I added a line to
also clear "conn->usesspi". Without that, if the server on first attempt
asked for SSPI authentication, but GSS on the second attempt, we would
incorrectly try to continue SSPI authentication during the second
attempt. Also, I kept the existing code to discard the input and output
data together, and added the new code after that, instead of in the
middle. And added some newlines to pqDropConnection for beauty.
BTW, multiple connection attempts if "host" is a list of hostnames,
which is now possible in version 10, also had the same issue. On master,
that was the easiest way to reproduce this.
I decided to backpatch this down to 9.3, after all. It is clearly a bug,
although unlikely to be hit in typical configurations. One configuration
where this can be reproduced, is if you have separate "hostnossl" and
"hostssl" lines in pg_hba.conf, for Kerberos authentication, but with
different options. If the options are such that the first
authentication, with SSL, fails, but the second one should succeed,
before this fix the second attempt would nevertheless fail with the
"duplicate authentication request".
The code in 9.2 was sufficiently different that I didn't backport it
there, out of conservatism (ok, laziness).
- Heikki