Обсуждение: un-revert the MAINTAIN privilege and the pg_maintain predefined role

Thanks to Jeff's recent work with commits 2af07e2 and 59825d1, the issue
that led to the revert of the MAINTAIN privilege and the pg_maintain
predefined role (commit 151c22d) should now be resolved.  Specifically,
there was a concern that roles with the MAINTAIN privilege could use
search_path tricks to run arbitrary code as the table owner.  Jeff's work
prevents this by restricting search_path to a known safe value when running
maintenance commands.  (This approach and others were discussed on the
lists quite extensively, and it was also brought up at the developer
meeting at FOSDEM [0] earlier this year.)

Given this, I'd like to finally propose un-reverting MAINTAIN and
pg_maintain.  I created a commitfest entry for this [1] a few weeks ago and
attached it to Jeff's search_path thread, but I figured it would be good to
create a dedicated thread for this, too.  The attached patch is a straight
revert of commit 151c22d except for the following small changes:

* The catversion bump has been removed for now.  The catversion will need
  to be bumped appropriately if/when this is committed.

* The OID for the pg_maintain predefined role needed to be changed.  The
  original OID has been reused for something else since this feature was
  reverted.

* The change in AdjustUpgrade.pm needed to be updated to check for
  "$old_version < 17" instead of "$old_version < 16".

Thoughts?

[0]
https://wiki.postgresql.org/wiki/FOSDEM/PGDay_2024_Developer_Meeting#The_Path_to_un-reverting_the_MAINTAIN_privilege
[1] https://commitfest.postgresql.org/47/4836/

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, Mar 05, 2024 at 10:12:35AM -0600, Nathan Bossart wrote:
> Thanks to Jeff's recent work with commits 2af07e2 and 59825d1, the issue
> that led to the revert of the MAINTAIN privilege and the pg_maintain
> predefined role (commit 151c22d) should now be resolved.  Specifically,
> there was a concern that roles with the MAINTAIN privilege could use
> search_path tricks to run arbitrary code as the table owner.  Jeff's work
> prevents this by restricting search_path to a known safe value when running
> maintenance commands.  (This approach and others were discussed on the
> lists quite extensively, and it was also brought up at the developer
> meeting at FOSDEM [0] earlier this year.)
> 
> Given this, I'd like to finally propose un-reverting MAINTAIN and
> pg_maintain.  I created a commitfest entry for this [1] a few weeks ago and
> attached it to Jeff's search_path thread, but I figured it would be good to
> create a dedicated thread for this, too.  The attached patch is a straight
> revert of commit 151c22d except for the following small changes:
> 
> * The catversion bump has been removed for now.  The catversion will need
>   to be bumped appropriately if/when this is committed.
> 
> * The OID for the pg_maintain predefined role needed to be changed.  The
>   original OID has been reused for something else since this feature was
>   reverted.
> 
> * The change in AdjustUpgrade.pm needed to be updated to check for
>   "$old_version < 17" instead of "$old_version < 16".

Given all of this code was previously reviewed and committed, I am planning
to forge ahead and commit this early next week, provided no objections or
additional feedback materialize.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Thu, Mar 07, 2024 at 10:50:00AM -0600, Nathan Bossart wrote:
> Given all of this code was previously reviewed and committed, I am planning
> to forge ahead and commit this early next week, provided no objections or
> additional feedback materialize.

Jeff Davis and I spent some additional time looking at this patch.  There
are existing inconsistencies among the privilege checks for the various
maintenance commands, and the MAINTAIN privilege just builds on the status
quo, with one exception.  In the v1 patch, I proposed skipping privilege
checks when VACUUM recurses to TOAST tables, which means that a user may be
able to process a TOAST table for which they've concurrent lost privileges
on the main relation (since each table is vacuumed in a separate
transaction).  It's easy enough to resolve this inconsistency by sending
down the parent OID when recursing to a TOAST table and using that for the
privilege checks.  AFAICT this avoids any kind of cache lookup hazards
because we hold a session lock on the main relation in this case.  I've
done this in the attached v2.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

On Tue, 2024-03-12 at 16:05 -0500, Nathan Bossart wrote:
> It's easy enough to resolve this inconsistency by sending
> down the parent OID when recursing to a TOAST table and using that
> for the
> privilege checks.  AFAICT this avoids any kind of cache lookup
> hazards
> because we hold a session lock on the main relation in this case. 
> I've
> done this in the attached v2.

Looks good to me. Thank you for expanding on the comment, as well.

Regards,
    Jeff Davis

On Wed, Mar 13, 2024 at 09:49:26AM -0700, Jeff Davis wrote:
> Looks good to me. Thank you for expanding on the comment, as well.

Thanks for reviewing!  Committed.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com