Horton

Thread

Commits

  1. pg_receivewal, pg_recvlogical: allow canceling initial password prompt.

  2. Allow psql's other uses of simple_prompt() to be interrupted by ^C.

  3. Provide a variant of simple_prompt() that can be interrupted by ^C.

  4. Make psql's \password default to CURRENT_USER, not PQuser(conn).

  1. Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-29T19:45:54Z

    The attached patch responds to the discussion at [1].  That thread
pointed out that if you issue "\password" with no argument, psql tries
to set the password for the role identified by PQuser(conn), that is,
the originally requested login name.  That's problematic because
if you've done SET SESSION AUTHORIZATION or SET ROLE, you may no
longer have permissions to set the password for the original role.
Even if you do, that might not be what you were expecting, especially
since the psql documentation specifically says it sets the password
for "the current user".

The solution adopted here is to do "SELECT CURRENT_USER" to acquire
the correct role name, and to include the role name in the password
prompt so as to make it very clear which password is to be set.

While testing that, I noticed another bit of user-unfriendliness:
there's no obvious way to get out of it if you realize you are
setting the wrong user's password.  simple_prompt() ignores
control-C, and when you give up and press return, you'll just
get the prompt to enter the password again.  If at this point
you have the presence of mind to enter a deliberately different
string, you'll be out of the woods.  If you don't, and just hit
return again, you will get this response from the backend:

NOTICE:  empty string is not a valid password, clearing password

which is just about the worst default behavior I can think of.
If you're superuser, and you meant to set the password for user1
but typed user2 instead, you just clobbered user2's password,
and you have no easy way to undo that.

What I propose here is that we just refuse to let you set an
empty password this way:

postgres=# \password joe
Enter new password for user "joe": 
Empty string is not a valid password.
postgres=# 

If you actually want to clear the password, I don't see anything
wrong with entering "alter user joe password null" explicitly.

It's not clear to me whether we ought to back-patch some or all
of this.  In the other thread I expressed doubt about back-patching
security-relevant behavior changes, and that still seems like a
serious concern; but on the other hand, the behaviors enumerated
above are pretty bad.  Since \password is probably only used
interactively, it seems like we could get away with making these
changes: the addition of the user name to the prompt should be
enough to cue anyone about what's really going to happen.

A compromise position could be to keep PQuser() as the default
target role name in the back branches, but back-patch the other
aspects (the prompt addition and the exit on empty password).

Thoughts?

			regards, tom lane

[1] https://www.postgresql.org/message-id/flat/B340250F-A0E3-43BF-A1FB-2AE36003F68D%40gmail.com

  2. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-10-29T20:45:33Z

    On 10/29/21, 12:47 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> While testing that, I noticed another bit of user-unfriendliness:
> there's no obvious way to get out of it if you realize you are
> setting the wrong user's password.  simple_prompt() ignores
> control-C, and when you give up and press return, you'll just
> get the prompt to enter the password again.  If at this point
> you have the presence of mind to enter a deliberately different
> string, you'll be out of the woods.  If you don't, and just hit
> return again, you will get this response from the backend:
>
> NOTICE:  empty string is not a valid password, clearing password
>
> which is just about the worst default behavior I can think of.
> If you're superuser, and you meant to set the password for user1
> but typed user2 instead, you just clobbered user2's password,
> and you have no easy way to undo that.

Well, as of bf6b9e9, "ALTER ROLE nathan PASSWORD ''" is effectively
the same as "ALTER ROLE nathan PASSWORD NULL".  I agree about the
user-unfriendliness, but maybe simple_prompt() ignoring control-C is
the root-cause of the user-unfriendliness.  I'm not sure that it's
totally unreasonable to expect the password to be cleared if you don't
enter a new one in the prompts.

> A compromise position could be to keep PQuser() as the default
> target role name in the back branches, but back-patch the other
> aspects (the prompt addition and the exit on empty password).

I think it would be okay to back-patch the PQuser() fix.  I would
argue that it's clearly a bug because the docs say it uses the current
user.

Nathan

  3. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-10-30T00:06:58Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/29/21, 12:47 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> While testing that, I noticed another bit of user-unfriendliness:
>> there's no obvious way to get out of it if you realize you are
>> setting the wrong user's password.  simple_prompt() ignores
>> control-C, and when you give up and press return, you'll just
>> get the prompt to enter the password again.

> Well, as of bf6b9e9, "ALTER ROLE nathan PASSWORD ''" is effectively
> the same as "ALTER ROLE nathan PASSWORD NULL".  I agree about the
> user-unfriendliness, but maybe simple_prompt() ignoring control-C is
> the root-cause of the user-unfriendliness.

I was afraid somebody would say that.  I have looked at it, but AFAICS
we'd have to duplicate all of sprompt.c and nearly all of pg_get_line.c
in order to tie it into psql's SIGINT infrastructure, since we wouldn't
dare enable the signal handler except during the innermost fgets() call,
and if we did get a signal we'd still need to clean up the terminal echo
state, so we couldn't just longjmp out of simple_prompt().  The
cost/benefit ratio of that doesn't look very good.

(Note that most callers of simple_prompt() don't need to sweat over
this because they haven't moved SIGINT handling off the default:
they're OK with just terminating on control-C.)

			regards, tom lane

  4. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-11T16:04:02Z

    On 10/29/21, 5:07 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> Well, as of bf6b9e9, "ALTER ROLE nathan PASSWORD ''" is effectively
>> the same as "ALTER ROLE nathan PASSWORD NULL".  I agree about the
>> user-unfriendliness, but maybe simple_prompt() ignoring control-C is
>> the root-cause of the user-unfriendliness.
>
> I was afraid somebody would say that.  I have looked at it, but AFAICS
> we'd have to duplicate all of sprompt.c and nearly all of pg_get_line.c
> in order to tie it into psql's SIGINT infrastructure, since we wouldn't
> dare enable the signal handler except during the innermost fgets() call,
> and if we did get a signal we'd still need to clean up the terminal echo
> state, so we couldn't just longjmp out of simple_prompt().  The
> cost/benefit ratio of that doesn't look very good.

Hm.  Is it really necessary to duplicate all of sprompt.c and
pg_get_line.c?  Would it be possible to teach the existing functions
how to optionally enable SIGINT handling instead?  I wouldn't mind
trying my hand at this if it seems like a reasonable approach.

Nathan

  5. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-11T16:11:51Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> On 10/29/21, 5:07 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> I was afraid somebody would say that.  I have looked at it, but AFAICS
>> we'd have to duplicate all of sprompt.c and nearly all of pg_get_line.c
>> in order to tie it into psql's SIGINT infrastructure, since we wouldn't
>> dare enable the signal handler except during the innermost fgets() call,
>> and if we did get a signal we'd still need to clean up the terminal echo
>> state, so we couldn't just longjmp out of simple_prompt().  The
>> cost/benefit ratio of that doesn't look very good.

> Hm.  Is it really necessary to duplicate all of sprompt.c and
> pg_get_line.c?  Would it be possible to teach the existing functions
> how to optionally enable SIGINT handling instead?  I wouldn't mind
> trying my hand at this if it seems like a reasonable approach.

It seems to me it'd overcomplicate simple_prompt's API for one use-case
... but if you want to try it, step right up.  (I suppose some of that
objection could be overcome by making simple_prompt into a wrapper
around another function not_so_simple_prompt.)

			regards, tom lane

  6. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-12T17:23:58Z

    On 11/11/21, 8:12 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> It seems to me it'd overcomplicate simple_prompt's API for one use-case
> ... but if you want to try it, step right up.  (I suppose some of that
> objection could be overcome by making simple_prompt into a wrapper
> around another function not_so_simple_prompt.)

I haven't started on the SIGINT stuff yet, but I did extract the
PQuser() fix so that we can hopefully get that one out of the way.  I
made some small adjustments in an attempt to keep the branching in
this function from getting too complicated.

Nathan

  7. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-12T19:57:56Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> I haven't started on the SIGINT stuff yet, but I did extract the
> PQuser() fix so that we can hopefully get that one out of the way.  I
> made some small adjustments in an attempt to keep the branching in
> this function from getting too complicated.

Right, it makes sense to consider just this much as the back-patchable
part, and leave the more complicated messing with simple_prompt()
for HEAD only.  I made a couple further cosmetic tweaks and pushed it.

			regards, tom lane

  8. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-12T20:08:59Z

    On 11/12/21, 11:58 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> I haven't started on the SIGINT stuff yet, but I did extract the
>> PQuser() fix so that we can hopefully get that one out of the way.  I
>> made some small adjustments in an attempt to keep the branching in
>> this function from getting too complicated.
>
> Right, it makes sense to consider just this much as the back-patchable
> part, and leave the more complicated messing with simple_prompt()
> for HEAD only.  I made a couple further cosmetic tweaks and pushed it.

Thanks!

Nathan

  9. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-15T17:46:31Z

    On 11/12/21, 11:58 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Right, it makes sense to consider just this much as the back-patchable
> part, and leave the more complicated messing with simple_prompt()
> for HEAD only.  I made a couple further cosmetic tweaks and pushed it.

Here is an attempt at adding control-C support for \password.  With
this patch, we pass sigint_interrupt_jmp and sigint_interrupt_enabled
all the way down to pg_get_line_append(), which is admittedly a bit
more complicated than I think would be ideal.  I see a couple of other
calls to simple_prompt() (e.g., \prompt and \connect), and I think the
same infrastructure could be used for those as well.  I'll send some
follow-up patches for those if this approach seems reasonable.

Nathan

  10. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-15T18:12:44Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> Here is an attempt at adding control-C support for \password.  With
> this patch, we pass sigint_interrupt_jmp and sigint_interrupt_enabled
> all the way down to pg_get_line_append(), which is admittedly a bit
> more complicated than I think would be ideal.  I see a couple of other
> calls to simple_prompt() (e.g., \prompt and \connect), and I think the
> same infrastructure could be used for those as well.  I'll send some
> follow-up patches for those if this approach seems reasonable.

Hm.  It's not as bad as I thought it might be, but I still dislike
importing <setjmp.h> into common/string.h --- that seems like a mighty
ugly dependency to have there.  I guess one idea to avoid that is to
declare SigintInterruptContext.jmpbuf as "void *".  Alternatively we
could push those function declarations into some specialized header.

Some other random observations (not a full review):

* API spec for SigintInterruptContext needs to be a bit more detailed.
Maybe "... via an existing SIGINT signal handler that will longjmp to
the specified place, but only when *enabled is true".

* I don't believe that this bit is necessary, or if it is,
the comment fails to justify it:

-	initStringInfo(&buf);
+	/* make sure buf is palloc'd so we don't lose changes after a longjmp */
+	buf = makeStringInfo();

* You're failing to re-enable sigint_ctx->enabled when looping
around for another fgets call.

* Personally I'd write those assignments like

	*(sigint_ctx->enabled) = true;

as that seems clearer.

			regards, tom lane

  11. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-15T20:30:49Z

    Thanks for the expeditious review.

On 11/15/21, 10:13 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Hm.  It's not as bad as I thought it might be, but I still dislike
> importing <setjmp.h> into common/string.h --- that seems like a mighty
> ugly dependency to have there.  I guess one idea to avoid that is to
> declare SigintInterruptContext.jmpbuf as "void *".  Alternatively we
> could push those function declarations into some specialized header.

I used "void *" for v2.

> * API spec for SigintInterruptContext needs to be a bit more detailed.
> Maybe "... via an existing SIGINT signal handler that will longjmp to
> the specified place, but only when *enabled is true".

Done.

> * I don't believe that this bit is necessary, or if it is,
> the comment fails to justify it:
>
> -       initStringInfo(&buf);
> +       /* make sure buf is palloc'd so we don't lose changes after a longjmp */
> +       buf = makeStringInfo();

My main worry was that buf->data might get repalloc'd via
enlargeStringInfo(), which could cause problems after a longjmp.  We
could also declare it as volatile, but I think you'd unfortunately
have to unvolatize() a bunch.

> * You're failing to re-enable sigint_ctx->enabled when looping
> around for another fgets call.

Oops, fixed.

> * Personally I'd write those assignments like
>
>         *(sigint_ctx->enabled) = true;
>
> as that seems clearer.

Done.

Nathan

  12. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-15T21:03:53Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> On 11/15/21, 10:13 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> * I don't believe that this bit is necessary, or if it is,
>> the comment fails to justify it:
>> 
>> -       initStringInfo(&buf);
>> +       /* make sure buf is palloc'd so we don't lose changes after a longjmp */
>> +       buf = makeStringInfo();

> My main worry was that buf->data might get repalloc'd via
> enlargeStringInfo(), which could cause problems after a longjmp.

So what?  That has nothing to do with whether the buf struct itself
is alloc'd or not.  Besides which, no longjmp is going to happen
during any reallocation.  I'm not entirely sure what scenario
you're worried about, but I don't see how alloc'ing the
StringInfoData struct would make it any safer.  If anything it'd
be less safe, since the StringInfoData is certain to be on the
stack, while a buf pointer variable is likely to be kept in a
register.  But really that doesn't matter anyhow, since this
is a stack level below where the sigsetjmp call is.  AFAIK the
only longjmp clobber risk is to pg_get_line_append's mutable
local variables, of which there are none.

			regards, tom lane

  13. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-15T21:52:42Z

    On 11/15/21, 1:04 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> register.  But really that doesn't matter anyhow, since this
> is a stack level below where the sigsetjmp call is.  AFAIK the
> only longjmp clobber risk is to pg_get_line_append's mutable
> local variables, of which there are none.

*facepalm*

You are right.  I'm not sure what I was thinking.  Attached a v3
with that part removed.

Nathan

  14. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-18T00:14:28Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> You are right.  I'm not sure what I was thinking.  Attached a v3
> with that part removed.

Pushed with a little further tweaking --- mostly, I felt that
explicitly referring to SIGINT in the API names was too
implementation-specific, so I renamed things.

As you mentioned, there are several other simple_prompt() calls
that could usefully be improved.  (I think the one in startup.c
may be OK because we've not set up the SIGINT handler yet,
though.)  I wondered whether it's worth refactoring some more
to have just one function that sets up the context mechanism.

I was also of two minds about whether to add a context option
to pg_get_line_buf().  I stuck with your choice not to, but
it does look a bit inconsistent.

			regards, tom lane

  15. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-18T00:34:25Z

    On 11/17/21, 4:15 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> Pushed with a little further tweaking --- mostly, I felt that
> explicitly referring to SIGINT in the API names was too
> implementation-specific, so I renamed things.

Thanks!

> As you mentioned, there are several other simple_prompt() calls
> that could usefully be improved.  (I think the one in startup.c
> may be OK because we've not set up the SIGINT handler yet,
> though.)  I wondered whether it's worth refactoring some more
> to have just one function that sets up the context mechanism.

I'll get started on these.

> I was also of two minds about whether to add a context option
> to pg_get_line_buf().  I stuck with your choice not to, but
> it does look a bit inconsistent.

Yeah, I figured it'd be simple enough to add that if/when it's needed.

Nathan

  16. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-19T00:41:30Z

    On 11/17/21, 4:38 PM, "Bossart, Nathan" <bossartn@amazon.com> wrote:
> On 11/17/21, 4:15 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> As you mentioned, there are several other simple_prompt() calls
>> that could usefully be improved.  (I think the one in startup.c
>> may be OK because we've not set up the SIGINT handler yet,
>> though.)  I wondered whether it's worth refactoring some more
>> to have just one function that sets up the context mechanism.
>
> I'll get started on these.

Okay, here is an attempt at adding control-C support for \prompt and
\connect.  It was reasonably straightforward.  I did have to teach
simple_prompt_extended() to add a newline after a cancellation when
"echo" is true.

I think that's it for psql.  After a quick glance, I didn't see any
other obvious candidates for control-C support, but I'll look a little
closer to be sure.

Nathan

  17. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-19T17:15:57Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> Okay, here is an attempt at adding control-C support for \prompt and
> \connect.  It was reasonably straightforward.  I did have to teach
> simple_prompt_extended() to add a newline after a cancellation when
> "echo" is true.

LGTM, pushed after very minor fooling with the comments.

> I think that's it for psql.  After a quick glance, I didn't see any
> other obvious candidates for control-C support, but I'll look a little
> closer to be sure.

Hmm, initdb's prompt-for-superuser-password might need it.
I think the rest of our frontend programs don't trap SIGINT,
or at least don't do so while requesting user input.

			regards, tom lane

  18. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-19T23:03:26Z

    On 11/19/21, 9:17 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> Okay, here is an attempt at adding control-C support for \prompt and
>> \connect.  It was reasonably straightforward.  I did have to teach
>> simple_prompt_extended() to add a newline after a cancellation when
>> "echo" is true.
>
> LGTM, pushed after very minor fooling with the comments.

Thanks!

>> I think that's it for psql.  After a quick glance, I didn't see any
>> other obvious candidates for control-C support, but I'll look a little
>> closer to be sure.
>
> Hmm, initdb's prompt-for-superuser-password might need it.
> I think the rest of our frontend programs don't trap SIGINT,
> or at least don't do so while requesting user input.

I'm able to cancel the superuser password prompt in initdb already.
It looks like the signal handlers aren't set up until after
get_su_pwd().  I did find some missing control-C handling in
pg_receivewal/pg_recvlogical, though.  Attached is a patch for those.

Nathan

  19. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-20T21:57:56Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> On 11/19/21, 9:17 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> Hmm, initdb's prompt-for-superuser-password might need it.

> I'm able to cancel the superuser password prompt in initdb already.
> It looks like the signal handlers aren't set up until after
> get_su_pwd().

Right; I misread that code in an overly hasty scan.

> I did find some missing control-C handling in
> pg_receivewal/pg_recvlogical, though.  Attached is a patch for those.

Meh ... I'm inclined to fix those programs by just moving their pqsignal
calls down to after their initial GetConnection calls, as attached.
This'd be simple enough to back-patch, for one thing.

It could be argued that this doesn't provide a nice experience if
(a) somebody changes your password mid-run and (b) you actually
need to make a new connection for some reason and (c) you want
to give up at that point instead of putting in the new password.
But I doubt it's worth so much extra complication to address that
edge case.  We've had about zero field complaints about the existing
behavior in those programs, so the cost/benefit ratio seems poor.

			regards, tom lane

PS: I noticed that StreamLogicalLog leaks its query buffer if
GetConnection fails.  Probably not very exciting, but we might
as well fix that, as included below.

  20. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-20T23:25:43Z

    On 11/20/21, 1:58 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> I did find some missing control-C handling in
>> pg_receivewal/pg_recvlogical, though.  Attached is a patch for those.
>
> Meh ... I'm inclined to fix those programs by just moving their pqsignal
> calls down to after their initial GetConnection calls, as attached.
> This'd be simple enough to back-patch, for one thing.
>
> It could be argued that this doesn't provide a nice experience if
> (a) somebody changes your password mid-run and (b) you actually
> need to make a new connection for some reason and (c) you want
> to give up at that point instead of putting in the new password.
> But I doubt it's worth so much extra complication to address that
> edge case.  We've had about zero field complaints about the existing
> behavior in those programs, so the cost/benefit ratio seems poor.

Yeah, I was looking for a way to simplify this, too.  Your approach
seems reasonable enough to me.

Nathan

  21. Re: Improving psql's \password command

    Tom Lane <tgl@sss.pgh.pa.us> — 2021-11-21T19:14:36Z

    "Bossart, Nathan" <bossartn@amazon.com> writes:
> On 11/20/21, 1:58 PM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
>> Meh ... I'm inclined to fix those programs by just moving their pqsignal
>> calls down to after their initial GetConnection calls, as attached.
>> This'd be simple enough to back-patch, for one thing.

> Yeah, I was looking for a way to simplify this, too.  Your approach
> seems reasonable enough to me.

Hearing no contrary opinions, pushed that way.

			regards, tom lane

  22. Re: Improving psql's \password command

    Nathan Bossart <bossartn@amazon.com> — 2021-11-22T19:29:37Z

    On 11/21/21, 11:15 AM, "Tom Lane" <tgl@sss.pgh.pa.us> wrote:
> "Bossart, Nathan" <bossartn@amazon.com> writes:
>> Yeah, I was looking for a way to simplify this, too.  Your approach
>> seems reasonable enough to me.
>
> Hearing no contrary opinions, pushed that way.

Thanks!

Nathan