On Tue, 2019-09-17 at 16:04 +0900, Michael Paquier wrote:
> basically maps into checking after FE_SCRAM_FINISHED. Instead of
> those two flags, wouldn't it be cleaner to add an extra routine to
> fe-auth-scram.c which does the same sanity checks, say
> pg_fe_scram_check_state()? This needs to be careful about three
> things, taking in input an opaque pointer to fe_scram_state if
> channel
> binding is required:
> - The data is not NULL.
> - The sasl mechanism selected is SCRAM-SHA-256-PLUS.
> - The state is FE_SCRAM_FINISHED.
Yes, I think this does come out a bit cleaner, thank you.
> What do you think? There is no need to save down the connection
> parameter value into fe_scram_state.
I'm not sure I understand this comment, but I removed the extra boolean
flags.
> Nit here as there are only two mechanisms handled: I would rather
> cause the error if the selected mechanism does not match
> SCRAM-SHA-256-PLUS, instead of complaining if the selected mechanism
> matches SCRAM-SHA-256. Either way is actually fine :)
Done.
> + printfPQExpBuffer(&conn->errorMessage,
> + libpq_gettext("Channel binding required but
> not
> offered by server\n"));
> + result = false;
> Should that be "completed by server" instead?
Done.
> is required". Or you have in mind that this error message is better?
I felt it would be a more useful error message.
> I think that pgindent would complain with the comment block in
> check_expected_areq().
Changed.
Regards,
Jeff Davis