On 22.09.22 01:37, Jacob Champion wrote:
> On Wed, Sep 21, 2022 at 3:36 PM Peter Eisentraut
> <peter.eisentraut@enterprisedb.com> wrote:
>> So let's look at the two TODO comments you have:
>>
>> * TODO: how should !auth_required interact with an incomplete
>> * SCRAM exchange?
>>
>> What specific combination of events are you thinking of here?
>
> Let's say the client is using `require_auth=!password`. If the server
> starts a SCRAM exchange. but doesn't finish it, the connection will
> still succeed with the current implementation (because it satisfies
> the "none" case). This is also true for a client setting of
> `require_auth=scram-sha-256,none`. I think this is potentially
> dangerous, but it mirrors the current behavior of libpq and I'm not
> sure that we should change it as part of this patch.
It might be worth reviewing that behavior for other reasons, but I think
semantics of your patch are correct.
>> In any case, it seems to me that it would be safer to *not*
>> make this assumption at first and then have someone more knowledgeable
>> make the argument that it would be safe.
>
> I think I'm okay with that, regardless. Here's one of the wrinkles:
> right now, both of the following connstrings work:
>
> require_auth=gss gssencmode=require
> require_auth=gss gssencmode=prefer
>
> If we don't treat gssencmode as providing GSS auth, then the first
> case will always fail, because there will be no GSS authentication
> packet over an encrypted connection. Likewise, the second case will
> almost always fail, unless the server doesn't support gssencmode at
> all (so why are you using prefer?).
>
> If you're okay with those limitations, I will rip out the code. The
> reason I'm not too worried about it is, I don't think it makes much
> sense to be strict about your authentication requirements while at the
> same time leaving the choice of transport encryption up to the server.
The way I understand what you explained here is that it would be more
sensible to leave that code in. I would be okay with that.
