Обсуждение: Refactoring of connection with password prompt loop for frontends

Hi all,

In six places of the code tree (+ one in psql which is a bit
different), we have the following pattern for frontend tools to
connect to a backend with a password prompt, roughly like that:
do
{
    [...]
    conn = PQconnectdbParams(keywords, values, true);

    [...]

    if (PQstatus(conn) == CONNECTION_BAD &&
        PQconnectionNeedsPassword(conn) &&
    !have_password)
    {
        PQfinish(conn);
        simple_prompt("Password: ", password, sizeof(password), false);
        have_password = true;
        new_pass = true;
    }
} while (new_pass);

Attached is a tentative of patch to consolidate this logic across the
tree.  The patch is far from being in a committable state, and there
are a couple of gotchas:
- pg_dumpall merges connection string parameters, so it is much harder
to plugin that the others, and I left it out on purpose.
- Some code paths spawn a password prompt before the first connection
attempt.  For now I have left this first attempt out of the refactored
logic, but I think that it is possible to consolidate that a bit more
by enforcing a password prompt before doing the first connection
attempt (somewhat related to the XXX portion in the patch).  At the
end it would be nice to not have to have a 100-byte-long field for the
password buffer we have here and there.  Unfortunately this comes with
its limitations in pg_dump as savedPassword needs to be moved around
and may be reused in the context.
- I don't like the routine name connect_with_password_prompt() I
introduced.  Suggestions of better names are welcome :)
- This also brings the point that some of our tools are not able to
handle tri-values for passwords, so we may want to consolidate that as
well.

Among the positive points, this brings a lot of consolidation in terms
of error handling, and this shaves a bit of code:
13 files changed, 190 insertions(+), 280 deletions(-)

This moves the logic into src/fe_utils, which is natural as that's
aimed only for frontends and because this also links to libpq.

Please note that this links a bit with the refactoring of vacuumlo and
oid2name logging I proposed a couple of days back (applying one patch
or the other results in conflicts) because we need to have frontends
initialized for logging in order to be able to log errors in the
refactored routine:
https://www.postgresql.org/message-id/20190820012819.GA8326@paquier.xyz
This one should be merged first IMO, but that's a story for the other
thread.

This compiles, and passes all regression tests so it is possible to
play with it easily, still it needs much more testing and love.

Any thoughts?  I am adding that to the next commit fest.

Thanks,
--
Michael

This patch has been absolutely overlooked by reviewers.  Euler, you're
registered as a reviewer for it.  Will you submit a review soon?  :-)

It does not currently apply, so if we get a rebase, that'd be good too.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Sep 25, 2019 at 05:47:39PM -0300, Alvaro Herrera wrote:
> This patch has been absolutely overlooked by reviewers.  Euler, you're
> registered as a reviewer for it.  Will you submit a review soon?  :-)
>
> It does not currently apply, so if we get a rebase, that'd be good too.

Here you go.  There were four conflicts in total caused by the switch
to the central logging infra for vacuumlo/oid2name and the
introduction of recovery_gen.c.  No actual changes except the rebase.
--
Michael

On 2019-Sep-26, Michael Paquier wrote:

> On Wed, Sep 25, 2019 at 05:47:39PM -0300, Alvaro Herrera wrote:
> > This patch has been absolutely overlooked by reviewers.  Euler, you're
> > registered as a reviewer for it.  Will you submit a review soon?  :-)
> > 
> > It does not currently apply, so if we get a rebase, that'd be good too.
> 
> Here you go.  There were four conflicts in total caused by the switch
> to the central logging infra for vacuumlo/oid2name and the
> introduction of recovery_gen.c.  No actual changes except the rebase.

Hmm, you have an XXX comment that appears to need addressing; and I'd
add an explanation about the looping behavior to the comment atop the
new function.

I see that vacuumlo and scripts/common retain their "have_password"
variables.  That seems to be so that they can reuse one for other
databases.  But how does that work with the new routine?

I notice you changed the historical error message 

> -            pg_log_error("connection to database \"%s\" failed", database);

to

> +        pg_log_error("could not connect to database \"%s\": %s",

The change seems appropriate to me.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Thu, Sep 26, 2019 at 10:06:27AM -0300, Alvaro Herrera wrote:
> Hmm, you have an XXX comment that appears to need addressing; and I'd
> add an explanation about the looping behavior to the comment atop the
> new function.
>
> I see that vacuumlo and scripts/common retain their "have_password"
> variables.  That seems to be so that they can reuse one for other
> databases.  But how does that work with the new routine?

That's the second bullet point I mentioned at the top of the thread
(https://www.postgresql.org/message-id/20190822074558.GG1683@paquier.xyz),
and something I wanted to discuss for this patch:
"Some code paths spawn a password prompt before the first connection
attempt..."

Historically, vacuumlo, scripts/common.c and pg_backup_db.c ask for a
password before doing the first connection attempt, which is different
than any other code paths.  That's the reason why have_password is
still there in the case of the first one.  scripts/common.c and
pg_dump are different issues as we don't want to repeat the password
for multiple database connections. That point is also related to the
XXX comment, because if we make the logic more consistent with the
rest (aka ask for the password the first time after the first
connection attempt), then we could remove completely the extra
handling with saved_password (meaning no more XXX).  That would be
also nicer as we want to keep a fixed size for the password buffer of
100 bytes.

Thinking more about it, keeping connect_with_password_prompt() a
maximum simple would be nicer for future maintenance, and it looks
that we may actually be able to remove allow_password_reuse from
connectDatabase() in common.c, but this needs a rather close lookup as
we should not create any password exposure hazards.

For now I am switching the patch as returned with feedback as there is
much more to consider.  And it did not attract much attention either.
--
Michael