Re: List of all* PostgreSQL EXTENSIONs in the world

[Joe Conway]

On 2/23/22 09:33, Euler Taveira wrote:

On Wed, Feb 23, 2022, at 6:00 AM, Joel Jacobson wrote:

On Fri, Feb 11, 2022, at 04:46, Noah Misch wrote:
> How did you make the list?  (I'd imagine doing it by searching for
> repositories containing evidence like \bpgxs\b matches.)

Searching Github for repos with a *.control file in the root dir and a 
Makefile containing ^PGXS

Interesting. What's an extension? It is something that contains user-defined
objects. It would be good if your list was expanded to contain addons 
(modules)
that are basically plugins that don't create additional objects in the 
database

e.g. an output plugin or a module that uses any hooks (such as auth_delay).
They generally don't provide control file (for example, wal2json). I 
don't know
if can only rely on PGXS check because there are client programs that 
uses the

PGXS infrastructure to build it.

Hmm, now that you say it, maybe the ^PGXS regex should be 
case-insensitive,

if pgxs can be written in e.g. lower case?
Makefile variable names are case-sensitive. You cannot write pgxs or 
PgXs; it

should be PGXS.



What about scanning for "PG_MODULE_MAGIC"?

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: List of all* PostgreSQL EXTENSIONs in the world

[Joe Conway]

On 2/23/22 09:52, Aleksander Alekseev wrote:

 > What about scanning for "PG_MODULE_MAGIC"?

An extension can be written without using C at all. BTW some extensions 
[1] are written in Rust these days.


Sure, but scanning for PG_MODULE_MAGIC may well pick up repos that would 
otherwise have been missed. I didn't say that should be the only filter 
used ¯\_(ツ)_/¯


Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: [PATCH v2] use has_privs_for_role for predefined roles

[Joe Conway]

On 3/3/22 11:26, Joshua Brindle wrote:

On Thu, Feb 10, 2022 at 2:37 PM Joe Conway  wrote:


On 2/10/22 14:28, Nathan Bossart wrote:
> On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>> On 2/9/22 13:13, Nathan Bossart wrote:
>>> I do wonder if users find the differences between predefined roles and role
>>> attributes confusing.  INHERIT doesn't govern role attributes, but it will
>>> govern predefined roles when this patch is applied.  Maybe the role
>>> attribute system should eventually be deprecated in favor of using
>>> predefined roles for everything.  Or perhaps the predefined roles should be
>>> converted to role attributes.
>>
>> Yep, I was suggesting that the latter would have been preferable to me while
>> Robert seemed to prefer the former. Honestly I could be happy with either of
>> those solutions, but as I alluded to that is probably a discussion for the
>> next development cycle since I don't see us doing that big a change in this
>> one.
>
> I agree.  I still think Joshua's proposed patch is a worthwhile improvement
> for v15.

+1

I am planning to get into it in detail this weekend. So far I have
really just ensured it merges cleanly and passes make world.


Rebased patch to apply to master attached.


Well longer than I planned, but finally took a closer look.

I made one minor editorial fix to Joshua's patch, rebased to current 
master, and added two missing call sites that presumably are related to 
recent commits for pg_basebackup.


On that last note, I did not find basebackup_to_shell.required_role 
documented at all, and did not attempt to fix that.


When this thread petered out, it seemed that Robert was at least neutral 
on the patch, and everyone else was +1 on applying it to master for pg15.


As such, if there are any other issues, complaints, etc., please speak 
real soon now...


Thanks,

JoeUse has_privs_for_roles for predefined role checks

Generally if a role is granted membership to another role with NOINHERIT they must use SET ROLE to access the privileges of that role, however with predefined roles the membership and privilege is conflated. Fix that by replacing is_member_of_role with has_privs_for_role for predefined roles. Patch does not remove is_member_of_role from acl.h, but it does add a warning not to use that function for privilege checking. Not backpatched based on hackers list discussion.

Author: Joshua Brindle
Reviewed-by: Stephen Frost, Nathan Bossart, Joe Conway
Discussion: https://postgr.es/m/flat/cagb+vh4zv_tvkt2tv3qns6tum_f_9icmuj0zjywwcgvi4pa...@mail.gmail.com

diff --git a/contrib/adminpack/adminpack.c b/contrib/adminpack/adminpack.c
index d7d84d0..03addf1 100644
*** a/contrib/adminpack/adminpack.c
--- b/contrib/adminpack/adminpack.c
*** convert_and_check_filename(text *arg)
*** 79,85 
  	 * files on the server as the PG user, so no need to do any further checks
  	 * here.
  	 */
! 	if (is_member_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
  		return filename;
  
  	/*
--- 79,85 
  	 * files on the server as the PG user, so no need to do any further checks
  	 * here.
  	 */
! 	if (has_privs_of_role(GetUserId(), ROLE_PG_WRITE_SERVER_FILES))
  		return filename;
  
  	/*
diff --git a/contrib/basebackup_to_shell/basebackup_to_shell.c b/contrib/basebackup_to_shell/basebackup_to_shell.c
index d82cb6d..f0ddef1 100644
*** a/contrib/basebackup_to_shell/basebackup_to_shell.c
--- b/contrib/basebackup_to_shell/basebackup_to_shell.c
*** _PG_init(void)
*** 90,96 
  }
  
  /*
!  * We choose to defer sanity sanity checking until shell_get_sink(), and so
   * just pass the target detail through without doing anything. However, we do
   * permissions checks here, before any real work has been done.
   */
--- 90,96 
  }
  
  /*
!  * We choose to defer sanity checking until shell_get_sink(), and so
   * just pass the target detail through without doing anything. However, we do
   * permissions checks here, before any real work has been done.
   */
*** shell_check_detail(char *target, char *t
*** 103,109 
  
  		StartTransactionCommand();
  		roleid = get_role_oid(shell_required_role, true);
! 		if (!is_member_of_role(GetUserId(), roleid))
  			ereport(ERROR,
  	(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
  	 errmsg("permission denied to use basebackup_to_shell")));
--- 103,109 
  
  		StartTransactionCommand();
  		roleid = get_role_oid(shell_required_role, true);
! 		if (!has_privs_of_role(GetUserId(), roleid))
  			ereport(ERROR,
  	(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
  	 errmsg("permission denied to use basebackup_to_shell")));
diff --git a/contrib/file_fdw/expected/file_fdw.out b/contrib/file_fdw/expected/file_fdw.out
index 0ac6e4e..14acdb2 100644
*** a/contrib/file_fdw/expected/file_fdw.out
--- b/contrib/file_fdw/expe

Re: [PATCH v2] use has_privs_for_role for predefined roles

[Joe Conway]

On 3/20/22 12:31, Joshua Brindle wrote:

On Sun, Mar 20, 2022 at 12:27 PM Joe Conway  wrote:


On 3/3/22 11:26, Joshua Brindle wrote:
> On Thu, Feb 10, 2022 at 2:37 PM Joe Conway  wrote:
>>
>> On 2/10/22 14:28, Nathan Bossart wrote:
>> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
>> >> On 2/9/22 13:13, Nathan Bossart wrote:
>> >>> I do wonder if users find the differences between predefined roles and 
role
>> >>> attributes confusing.  INHERIT doesn't govern role attributes, but it 
will
>> >>> govern predefined roles when this patch is applied.  Maybe the role
>> >>> attribute system should eventually be deprecated in favor of using
>> >>> predefined roles for everything.  Or perhaps the predefined roles should 
be
>> >>> converted to role attributes.
>> >>
>> >> Yep, I was suggesting that the latter would have been preferable to me 
while
>> >> Robert seemed to prefer the former. Honestly I could be happy with either 
of
>> >> those solutions, but as I alluded to that is probably a discussion for the
>> >> next development cycle since I don't see us doing that big a change in 
this
>> >> one.
>> >
>> > I agree.  I still think Joshua's proposed patch is a worthwhile improvement
>> > for v15.
>>
>> +1
>>
>> I am planning to get into it in detail this weekend. So far I have
>> really just ensured it merges cleanly and passes make world.
>
> Rebased patch to apply to master attached.

Well longer than I planned, but finally took a closer look.

I made one minor editorial fix to Joshua's patch, rebased to current
master, and added two missing call sites that presumably are related to
recent commits for pg_basebackup.


FWIW I pinged Stephen when I saw the basebackup changes included
is_member_of and he didn't think they necessarily needed to be changed
since they aren't accessible to human and you can't SET ROLE on a
replication connection in order to access the role where inheritance
was blocked.


Maybe worth a discussion, but it seems strange to me to treat them 
differently when the whole purpose of this patch is to make things 
consistent ¯\_(ツ)_/¯


Joe

Re: [PATCH v2] use has_privs_for_role for predefined roles

[Joe Conway]

On 3/20/22 12:38, Stephen Frost wrote:

Greetings,

On Sun, Mar 20, 2022 at 18:31 Joshua Brindle 
mailto:joshua.brin...@crunchydata.com>> 
wrote:


On Sun, Mar 20, 2022 at 12:27 PM Joe Conway mailto:m...@joeconway.com>> wrote:
 >
 > On 3/3/22 11:26, Joshua Brindle wrote:
 > > On Thu, Feb 10, 2022 at 2:37 PM Joe Conway mailto:m...@joeconway.com>> wrote:
 > >>
 > >> On 2/10/22 14:28, Nathan Bossart wrote:
 > >> > On Wed, Feb 09, 2022 at 04:39:11PM -0500, Joe Conway wrote:
 > >> >> On 2/9/22 13:13, Nathan Bossart wrote:
 > >> >>> I do wonder if users find the differences between
predefined roles and role
 > >> >>> attributes confusing.  INHERIT doesn't govern role
attributes, but it will
 > >> >>> govern predefined roles when this patch is applied.  Maybe
the role
 > >> >>> attribute system should eventually be deprecated in favor
of using
 > >> >>> predefined roles for everything.  Or perhaps the
predefined roles should be
 > >> >>> converted to role attributes.
 > >> >>
 > >> >> Yep, I was suggesting that the latter would have been
preferable to me while
 > >> >> Robert seemed to prefer the former. Honestly I could be
happy with either of
 > >> >> those solutions, but as I alluded to that is probably a
discussion for the
 > >> >> next development cycle since I don't see us doing that big
a change in this
 > >> >> one.
 > >> >
 > >> > I agree.  I still think Joshua's proposed patch is a
worthwhile improvement
 > >> > for v15.
 > >>
 > >> +1
 > >>
 > >> I am planning to get into it in detail this weekend. So far I have
 > >> really just ensured it merges cleanly and passes make world.
 > >
 > > Rebased patch to apply to master attached.
 >
 > Well longer than I planned, but finally took a closer look.
 >
 > I made one minor editorial fix to Joshua's patch, rebased to current
 > master, and added two missing call sites that presumably are
related to
 > recent commits for pg_basebackup.

FWIW I pinged Stephen when I saw the basebackup changes included
is_member_of and he didn't think they necessarily needed to be changed
since they aren't accessible to human and you can't SET ROLE on a
replication connection in order to access the role where inheritance
was blocked.

Yeah, though that should really be very clearly explained in comments 
around that code as anything that uses is_member should really be viewed 
as an exception.  I also wouldn’t be against using has_privs there 
anyway and saying that you shouldn’t be trying to connect as one role on 
a replication connection and using the privs of another when you don’t 
automatically inherit those rights, but on the assumption that the 
committer intended is_member there because SET ROLE isn’t something one 
does on replication connections, I’m alright with it being as is.



Robert -- any opinion on this? If I am not mistaken it is code that you 
are actively working on.


Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: documentation fix for SET ROLE

[Joe Conway]

On 2/17/21 2:12 PM, David G. Johnston wrote:
> On Wednesday, February 17, 2021, Bossart, Nathan  > wrote:
> 
> 
>     postgres=# ALTER ROLE test1 SET ROLE test2;
>     ALTER ROLE
> 
> 
> I would not have expected this to work - “role” isn’t a
> configuration_parameter.  Its actually cool that it does, but this doc fix
> should address this oversight as well.


I was surprised this worked too.

But the behavior is consistent with other GUCs. In other words, when you "ALTER
ROLE ... SET ..." you change the default value for the session, and therefore a
RESET just changes to that value.

-- login as postgres
nmx=# show work_mem;
 work_mem
--
 200MB
(1 row)

nmx=# set work_mem = '42MB';
SET
nmx=# show work_mem;
 work_mem
--
 42MB
(1 row)

nmx=# reset work_mem;
RESET
nmx=# show work_mem;
 work_mem
--
 200MB
(1 row)

ALTER ROLE test1 SET work_mem = '42MB';

-- login as test1
nmx=> show work_mem;
 work_mem
--
 42MB
(1 row)

nmx=> reset work_mem;
RESET
nmx=> show work_mem;
 work_mem
--
 42MB
(1 row)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/3/21 8:50 AM, David Steele wrote:
> On 1/29/21 4:56 AM, Joe Conway wrote:
>> On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote:
>>> 2021年1月28日(木) 17:18 Peter Eisentraut:
>>>  I'm not convinced the current behavior is wrong.  Is there some
>>>  practical use case that is affected by this behavior?
>>>   
>>> I was poking around at the function with a view to using it for something 
>>> and was
>>> curious what it did with bad input.
>>>
>>> Providing the column name of a dropped column:
>>>
>>>      Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of 
>>> my
>>> table 'foo'?"
>>>      Pg: "That column doesn't even exist - here, have an error instead."
>>>      Me: "Hey Postgres, does some other less-privileged user have 
>>> privileges on the
>>>           dropped column 'bar' of my table 'foo'?
>>>      Pg: "That column doesn't even exist - here, have an error instead."
>>>
>>> Providing the attnum of a dropped column:
>>>
>>>      Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does 
>>> some
>>>           other less-privileged user have privileges on that column?"
>>>      Pg: "That column doesn't even exist - here, have a NULL".
>>>      Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on 
>>> this table
>>>           I own, do I have privileges on that column?"
>>>      Pg: "Yup. And feel free to throw any other smallint at me, I'll 
>>> pretend that
>>>           represents a column too even if it never existed.".
>>>
>>> Looking at the code, particularly the cited comment, it does seems the 
>>> intent was
>>> to return NULL in all cases where an invalid attnum was provided, but that 
>>> gets
>>> short-circuited by the assumption table owner = has privilege on any column.
>> 
>> Nicely illustrated :-)
>> 
>>> Not the most urgent or exciting of issues, but seems inconsistent to me.
>> 
>> +1
> 
> Peter, did Ian's explanation answer your concerns?
> 
> Joe (or Peter), any interest in reviewing/committing this patch?


Sure, I'll take a look

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/3/21 9:43 AM, Joe Conway wrote:
> On 3/3/21 8:50 AM, David Steele wrote:
>> On 1/29/21 4:56 AM, Joe Conway wrote:
>>> On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote:
>>>> 2021年1月28日(木) 17:18 Peter Eisentraut:
>>>>  I'm not convinced the current behavior is wrong.  Is there some
>>>>  practical use case that is affected by this behavior?
>>>>   
>>>> I was poking around at the function with a view to using it for something 
>>>> and was
>>>> curious what it did with bad input.
>>>>
>>>> Providing the column name of a dropped column:
>>>>
>>>>      Me: "Hey Postgres, do I have privileges on the dropped column 'bar' 
>>>> of my
>>>> table 'foo'?"
>>>>      Pg: "That column doesn't even exist - here, have an error instead."
>>>>      Me: "Hey Postgres, does some other less-privileged user have 
>>>> privileges on the
>>>>           dropped column 'bar' of my table 'foo'?
>>>>      Pg: "That column doesn't even exist - here, have an error instead."
>>>>
>>>> Providing the attnum of a dropped column:
>>>>
>>>>      Me: "Hey Postgres, here's the attnum of the dropped column 'bar', 
>>>> does some
>>>>           other less-privileged user have privileges on that column?"
>>>>      Pg: "That column doesn't even exist - here, have a NULL".
>>>>      Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on 
>>>> this table
>>>>           I own, do I have privileges on that column?"
>>>>      Pg: "Yup. And feel free to throw any other smallint at me, I'll 
>>>> pretend that
>>>>           represents a column too even if it never existed.".
>>>>
>>>> Looking at the code, particularly the cited comment, it does seems the 
>>>> intent was
>>>> to return NULL in all cases where an invalid attnum was provided, but that 
>>>> gets
>>>> short-circuited by the assumption table owner = has privilege on any 
>>>> column.
>>> 
>>> Nicely illustrated :-)
>>> 
>>>> Not the most urgent or exciting of issues, but seems inconsistent to me.
>>> 
>>> +1
>> 
>> Peter, did Ian's explanation answer your concerns?
>> 
>> Joe (or Peter), any interest in reviewing/committing this patch?
> 
> Sure, I'll take a look

Based on Tom's commit here:

  https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=3d0f68dd3

it appears to me that the intent is to return NULL.

However I was not to happy with the way the submitted patch added an argument to
column_privilege_check(). It seems to me that it is better to just reorder the
checks in column_privilege_check() a bit, as in the attached.

I wasn't entirely sure it was ok to split up the check for dropped attribute and
pg_attribute_aclcheck like I did, but when I tested the race condition (by
pausing at pg_attribute_aclcheck and dropping the column in another session)
everything seemed to work fine.

Any comments?

Thanks,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/7/21 2:35 PM, Zhihong Yu wrote:
> Joe:
> I don't seem to find attachment.
> 
> Maybe attach again ?


Oops -- I did forget that, didn't I. This time patch is attached :-)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/utils/adt/acl.c b/src/backend/utils/adt/acl.c
index c7f029e..152d1da 100644
*** a/src/backend/utils/adt/acl.c
--- b/src/backend/utils/adt/acl.c
*** column_privilege_check(Oid tableoid, Att
*** 2460,2484 
  		return -1;
  
  	/*
! 	 * First check if we have the privilege at the table level.  We check
! 	 * existence of the pg_class row before risking calling pg_class_aclcheck.
! 	 * Note: it might seem there's a race condition against concurrent DROP,
! 	 * but really it's safe because there will be no syscache flush between
! 	 * here and there.  So if we see the row in the syscache, so will
! 	 * pg_class_aclcheck.
! 	 */
! 	if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(tableoid)))
! 		return -1;
! 
! 	aclresult = pg_class_aclcheck(tableoid, roleid, mode);
! 
! 	if (aclresult == ACLCHECK_OK)
! 		return true;
! 
! 	/*
! 	 * No table privilege, so try per-column privileges.  Again, we have to
! 	 * check for dropped attribute first, and we rely on the syscache not to
! 	 * notice a concurrent drop before pg_attribute_aclcheck fetches the row.
  	 */
  	attTuple = SearchSysCache2(ATTNUM,
  			   ObjectIdGetDatum(tableoid),
--- 2460,2468 
  		return -1;
  
  	/*
! 	 * We have to check for dropped attribute first, and we rely on the
! 	 * syscache not to notice a concurrent drop before pg_attribute_aclcheck
! 	 * fetches the row.
  	 */
  	attTuple = SearchSysCache2(ATTNUM,
  			   ObjectIdGetDatum(tableoid),
*** column_privilege_check(Oid tableoid, Att
*** 2493,2498 
--- 2477,2499 
  	}
  	ReleaseSysCache(attTuple);
  
+ 	/*
+ 	 * Now check if we have the privilege at the table level. We check
+ 	 * existence of the pg_class row before risking calling pg_class_aclcheck.
+ 	 * Note: it might seem there's a race condition against concurrent DROP,
+ 	 * but really it's safe because there will be no syscache flush between
+ 	 * here and there.  So if we see the row in the syscache, so will
+ 	 * pg_class_aclcheck.
+ 	 */
+ 	if (!SearchSysCacheExists1(RELOID, ObjectIdGetDatum(tableoid)))
+ 		return -1;
+ 
+ 	aclresult = pg_class_aclcheck(tableoid, roleid, mode);
+ 
+ 	if (aclresult == ACLCHECK_OK)
+ 		return true;
+ 
+ 	/* no table privilege, so try per-column privilege */
  	aclresult = pg_attribute_aclcheck(tableoid, attnum, roleid, mode);
  
  	return (aclresult == ACLCHECK_OK);
diff --git a/src/test/regress/expected/privileges.out b/src/test/regress/expected/privileges.out
index 46a69fc..d60ea53 100644
*** a/src/test/regress/expected/privileges.out
--- b/src/test/regress/expected/privileges.out
*** select has_column_privilege('mytable','.
*** 1362,1368 
  select has_column_privilege('mytable',2::int2,'select');
   has_column_privilege 
  --
!  t
  (1 row)
  
  revoke select on table mytable from regress_priv_user3;
--- 1362,1374 
  select has_column_privilege('mytable',2::int2,'select');
   has_column_privilege 
  --
!  
! (1 row)
! 
! select has_column_privilege('mytable',99::int2,'select');
!  has_column_privilege 
! --
!  
  (1 row)
  
  revoke select on table mytable from regress_priv_user3;
diff --git a/src/test/regress/sql/privileges.sql b/src/test/regress/sql/privileges.sql
index 6277140..766eeae 100644
*** a/src/test/regress/sql/privileges.sql
--- b/src/test/regress/sql/privileges.sql
*** alter table mytable drop column f2;
*** 836,841 
--- 836,842 
  select has_column_privilege('mytable','f2','select');
  select has_column_privilege('mytable','pg.dropped.2','select');
  select has_column_privilege('mytable',2::int2,'select');
+ select has_column_privilege('mytable',99::int2,'select');
  revoke select on table mytable from regress_priv_user3;
  select has_column_privilege('mytable',2::int2,'select');
  drop table mytable;

Re: [PATCH] pg_permissions

[Joe Conway]

On 3/6/21 2:03 PM, Joel Jacobson wrote:
> ...but to answer the question...
> 
>    - What permissions are there for a specific role in the database?
> 
> you need to manually query all relevant pg_catalog or
> information_schema.*_privileges views,
> which is a O(n) mental effort, while the first question is mentally O(1).
> 
> I think this can be improved by providing humans a single pg_permissions 
> system view
> which can be queried to answer the second question. This should save a lot of
> keyboard punches.

While this is interesting and probably useful for troubleshooting, it does not
provide the complete picture if what you care about is something like "what
stuff can joel do in my database".

The reasons for this include default grants to PUBLIC and role membership, and
even that is convoluted by INHERIT/NOINHERIT role attributes.

I won't try to describe all the implications here, but a while back I wrote a
fairly comprehensive blog[1] about it.

FWIW in the blog I reference an extension that I wrote to facilitate object and
role privilege inspection[2]. I have toyed with the idea of morphing that into a
feature I can submit for pg15, but I don't want to spend effort on the morphing
unless there is both sufficient interest and lack of conceptual objections to
the feature. I'd love to hear from both sides of that scale.

Joe

[1]
http://blog.crunchydata.com/blog/postgresql-defaults-and-impact-on-security-part-1
[2] https://github.com/CrunchyData/crunchy_check_access

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: Procedures versus the "fastpath" API

[Joe Conway]

On 3/9/21 2:15 PM, Tom Lane wrote:
> So the question on the table is what to do about this.  As far as
> window functions go, it seems clear that fastpath.c should just reject
> any attempt to call a window function that way (or an aggregate for
> that matter; aggregates fail already, but with relatively obscure
> error messages).  Perhaps there's also an argument that window
> functions should have run-time tests, not just assertions, that
> they're called in a valid way.
> 
> As for procedures, I'm of the opinion that we should just reject those
> too, but some other security team members were not happy with that
> idea.  Conceivably we could attempt to make the case actually work,
> but is it worth the trouble?  Given the lack of field complaints
> about the "invalid transaction termination" failure, it seems unlikely
> that it's occurred to anyone to try to call procedures this way.
> We'd need special infrastructure to test the case, too, since psql
> offers no access to fastpath calls.

+1

> A compromise suggestion was to prohibit calling procedures via
> fastpath as of HEAD, but leave existing releases as they are,
> in case anyone is using a case that happens to work.
> 
> Thoughts?

My vote would be reject using fastpath for procedures in all relevant branches.
If someday someone cares enough to make it work, it is a new feature for a new
major release.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: documentation fix for SET ROLE

[Joe Conway]

On 3/12/21 1:16 PM, Bossart, Nathan wrote:

On 3/12/21, 6:35 AM, "Laurenz Albe"  wrote:

On Fri, 2021-03-12 at 10:16 +0100, I wrote:

After sleeping on it, I have come to think that it is excessive to write
so much documentation for a feature that is that unimportant.

It takes some effort to come up with a good use case for it.

I think we can add a few lines to ALTER ROLE, perhaps ALTER DATABASE
(although I don't see what sense it could make to set that on the database 
level),
and briefly explain the difference between RESET ROLE and SET ROLE NONE.

I think adding too much detail will harm - anyone who needs to know the
exact truth can resort to the implementation.

I'll try to come up with a proposal later.


Attached is my idea of the documentation change.

I think that ALTER DATABASE ... SET ROLE can remain undocumented, because
I cannot imagine that it could be useful.

I am unsure if specifying "role" in a libpq connect string might be
worth documenting.  Can you think of a use case?


My main goal of this thread is to get the RESET ROLE documentation
fixed.  I don't have a terribly strong opinion on documenting these
special uses of "role".  I lean in favor of adding it, but I wouldn't
be strongly opposed to simply leaving it out for now.  But if we're
going to add it, I think we might as well add it everywhere.



Looking back at the commit history it seems to me that this only works 
accidentally. Perhaps it would be best to fix RESET ROLE and be done with it.


Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/16/21 1:42 AM, Chengxi Sun wrote:

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   not tested
Documentation:not tested

I tested the patch and it works well.


Was that my patch (i.e. the latest on this thread) or Ian's? It is not clear 
since you did not CC me...



And according to the comment, checking existence of relation and
pg_class_aclcheck() won't influenced by concurrent DROP. So I think it's safe
to just reorder the checking existence of column and
pg_attribute_aclcheck().


"Code never lies, comments sometimes do"

That said, I did at least a basic test of that assumption and it seems to be 
true.

Ian, or anyone else, any comments/complaints on my changes? If not I will commit 
and push that version sooner rather than later.


Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: Should we document IS [NOT] OF?

[Joe Conway]

On 11/19/20 2:03 AM, Tom Lane wrote:
> "David G. Johnston"  writes:
>> Is there a feature code? I skimmed the standard and non-standard tables in
>> our appendix and couldn’t find this in either.
> 
> a19d9d3c4 seems to have thought it was S151.

Here is a link to previous list discussions:

https://www.postgresql.org/message-id/45DA44F3.3010401%40joeconway.com

HTH,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Should we document IS [NOT] OF?

[Joe Conway]

On 11/19/20 11:06 AM, Tom Lane wrote:
> Let's just rip it out and be done.  If anyone is ever
> motivated to make it work per spec, they can resurrect
> whatever seems useful from the git history.

+1

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Should we document IS [NOT] OF?

[Joe Conway]

On 11/19/20 12:08 PM, Tom Lane wrote:
> Bruce Momjian  writes:
>> On Thu, Nov 19, 2020 at 11:15:33AM -0500, Joe Conway wrote:
>>> On 11/19/20 11:06 AM, Tom Lane wrote:
>>>> Let's just rip it out and be done.  If anyone is ever
>>>> motivated to make it work per spec, they can resurrect
>>>> whatever seems useful from the git history.
> 
>>> +1
> 
>> +1
> 
> Here's a proposed patch for that.  I was amused to discover that we have
> a couple of regression test cases making use of IS OF.


I didn't check but those might be my fault ;-)


> However, I think using pg_typeof() is actually better for those tests anyway, 
> since
> printing the regtype result is clearer, and easier to debug if the test
> ever goes wrong.

Looks good to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 1/29/21 12:13 AM, Ian Lawrence Barwick wrote:
> 2021年1月28日(木) 17:18 Peter Eisentraut:
> I'm not convinced the current behavior is wrong.  Is there some
> practical use case that is affected by this behavior?
> 
>  
> I was poking around at the function with a view to using it for something and 
> was
> curious what it did with bad input.
> 
> Providing the column name of a dropped column:
> 
>     Me: "Hey Postgres, do I have privileges on the dropped column 'bar' of my
> table 'foo'?"
>     Pg: "That column doesn't even exist - here, have an error instead."
>     Me: "Hey Postgres, does some other less-privileged user have privileges 
> on the
>          dropped column 'bar' of my table 'foo'?
>     Pg: "That column doesn't even exist - here, have an error instead."
> 
> Providing the attnum of a dropped column:
> 
>     Me: "Hey Postgres, here's the attnum of the dropped column 'bar', does 
> some
>          other less-privileged user have privileges on that column?"
>     Pg: "That column doesn't even exist - here, have a NULL".
>     Me: "Hey Postgres, here's the attnum of the dropped column 'bar' on this 
> table
>          I own, do I have privileges on that column?"
>     Pg: "Yup. And feel free to throw any other smallint at me, I'll pretend 
> that
>          represents a column too even if it never existed.".
> 
> Looking at the code, particularly the cited comment, it does seems the intent 
> was
> to return NULL in all cases where an invalid attnum was provided, but that 
> gets
> short-circuited by the assumption table owner = has privilege on any column.

Nicely illustrated :-)

> Not the most urgent or exciting of issues, but seems inconsistent to me.

+1

Joe


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/16/21 2:45 PM, Joe Conway wrote:

Ian, or anyone else, any comments/complaints on my changes? If not I will commit
and push that version sooner rather than later.


Any thoughts on back-patching this?

On one hand, in my view it is clearly a bug. On the other hand, no one has 
complained before and this does change user visible behavior.


I guess I am leaning toward not back-patching...

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/21/21 12:27 PM, Tom Lane wrote:

I think we may have to adjust the acl.c APIs, or maybe better provide new
entry points, so that we can have variants of pg_xxx_aclcheck that won't
throw a hard error upon not finding the row.  We cheesily tried to avoid
adjusting those APIs to support the semantics we need here, and we now see
that it didn't really work.


Ok, I took a shot at that; see attached.

Questions:

1. I confined the changes to just pg_class_aclcheck/mask
   and pg_attribute_aclcheck/mask -- did you intend
   that we do this same change across the board? Or
   perhaps do the rest of them once we open up pg15
   development?

2. This seems more invasive than something we would want
   to back patch -- agreed?

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index add3d14..b5a6b3a 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** AclMode
*** 3706,3711 
--- 3706,3719 
  pg_attribute_aclmask(Oid table_oid, AttrNumber attnum, Oid roleid,
  	 AclMode mask, AclMaskHow how)
  {
+ 	return pg_attribute_aclmask_ext(table_oid, attnum, roleid,
+ 	mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_attribute_aclmask_ext(Oid table_oid, AttrNumber attnum, Oid roleid,
+ 		 AclMode mask, AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	classTuple;
  	HeapTuple	attTuple;
*** pg_attribute_aclmask(Oid table_oid, Attr
*** 3723,3740 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Throw error on dropped columns, too */
  	if (attributeForm->attisdropped)
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
--- 3731,3768 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
! 
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Check dropped columns, too */
  	if (attributeForm->attisdropped)
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			ReleaseSysCache(attTuple);
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
*** AclMode
*** 3791,3796 
--- 3819,3831 
  pg_class_aclmask(Oid table_oid, Oid roleid,
   AclMode mask, AclMaskHow how)
  {
+ 	return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
+ 	 AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	tuple;
  	Form_pg_class classForm;
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3804,3813 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_TABLE),
!  errmsg("relation with OID %u does not exist",
! 		table_oid)));
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
--- 3839,3858 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_TABLE),
! 	 errmsg("relation with OID %u does not exist",
! 			table_oid)));
! 	}
! 
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
*** AclResult
*** 4468,4474 
  pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
  	  Oid roleid, AclMode mode)
  {
! 	if (pg_attribute_aclmask(table_oid, attnum, roleid, mode, ACLMASK_ANY) != 0)
  		return ACLCHECK_OK;
  	else
  		return ACLCHECK_NO_PRIV;
--- 4513,4527 
  pg_attribute_aclcheck(Oid table_oid, AttrNumber attnum,
  	  Oid roleid, AclMode mode)
  

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/30/21 3:37 PM, Joe Conway wrote:

On 3/21/21 12:27 PM, Tom Lane wrote:

I think we may have to adjust the acl.c APIs, or maybe better provide new
entry points, so that we can have variants of pg_xxx_aclcheck that won't
throw a hard error upon not finding the row.  We cheesily tried to avoid
adjusting those APIs to support the semantics we need here, and we now see
that it didn't really work.


Ok, I took a shot at that; see attached.


Heh, I missed the forest for the trees it seems.

That version undid the changes fixing what Ian was originally complaining about.

New version attached that preserves the fixed behavior.


Questions:
1. I confined the changes to just pg_class_aclcheck/mask
 and pg_attribute_aclcheck/mask -- did you intend
 that we do this same change across the board? Or
 perhaps do the rest of them once we open up pg15
 development?

2. This seems more invasive than something we would want
 to back patch -- agreed?


The questions remain...

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/aclchk.c b/src/backend/catalog/aclchk.c
index add3d14..b5a6b3a 100644
*** a/src/backend/catalog/aclchk.c
--- b/src/backend/catalog/aclchk.c
*** AclMode
*** 3706,3711 
--- 3706,3719 
  pg_attribute_aclmask(Oid table_oid, AttrNumber attnum, Oid roleid,
  	 AclMode mask, AclMaskHow how)
  {
+ 	return pg_attribute_aclmask_ext(table_oid, attnum, roleid,
+ 	mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_attribute_aclmask_ext(Oid table_oid, AttrNumber attnum, Oid roleid,
+ 		 AclMode mask, AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	classTuple;
  	HeapTuple	attTuple;
*** pg_attribute_aclmask(Oid table_oid, Attr
*** 3723,3740 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Throw error on dropped columns, too */
  	if (attributeForm->attisdropped)
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_COLUMN),
!  errmsg("attribute %d of relation with OID %u does not exist",
! 		attnum, table_oid)));
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
--- 3731,3768 
  			   ObjectIdGetDatum(table_oid),
  			   Int16GetDatum(attnum));
  	if (!HeapTupleIsValid(attTuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
! 
  	attributeForm = (Form_pg_attribute) GETSTRUCT(attTuple);
  
! 	/* Check dropped columns, too */
  	if (attributeForm->attisdropped)
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			ReleaseSysCache(attTuple);
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_COLUMN),
! 	 errmsg("attribute %d of relation with OID %u does not exist",
! 			attnum, table_oid)));
! 	}
  
  	aclDatum = SysCacheGetAttr(ATTNUM, attTuple, Anum_pg_attribute_attacl,
  			   &isNull);
*** AclMode
*** 3791,3796 
--- 3819,3831 
  pg_class_aclmask(Oid table_oid, Oid roleid,
   AclMode mask, AclMaskHow how)
  {
+ 	return pg_class_aclmask_ext(table_oid, roleid, mask, how, NULL);
+ }
+ 
+ AclMode
+ pg_class_aclmask_ext(Oid table_oid, Oid roleid, AclMode mask,
+ 	 AclMaskHow how, bool *is_missing)
+ {
  	AclMode		result;
  	HeapTuple	tuple;
  	Form_pg_class classForm;
*** pg_class_aclmask(Oid table_oid, Oid role
*** 3804,3813 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 		ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_TABLE),
!  errmsg("relation with OID %u does not exist",
! 		table_oid)));
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
--- 3839,3858 
  	 */
  	tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(table_oid));
  	if (!HeapTupleIsValid(tuple))
! 	{
! 		if (is_missing != NULL)
! 		{
! 			/* return "no privileges" instead of throwing an error */
! 			*is_missing = true;
! 			return 0;
! 		}
! 		else
! 			ereport(ERROR,
! 	(errcode(ERRCODE_UNDEFINED_TABLE),
! 	 errmsg("relation with OID %u does not exist",
! 			table_oid)));
! 	}
! 
  	classForm = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
*

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/30/21 6:22 PM, Tom Lane wrote:

Joe Conway  writes:

Heh, I missed the forest for the trees it seems.
That version undid the changes fixing what Ian was originally complaining about.


Duh, right.  It would be a good idea for there to be a code comment
explaining this, because it's *far* from obvious.  Say like

* Check for column-level privileges first.  This serves in
* part as a check on whether the column even exists, so we
* need to do it before checking table-level privilege.


Will do.


My gripe about providing API-spec comments for the new aclchk.c
entry points still stands.  Other than that, I think it's good
to go.


Yeah, I was planning to put something akin to this in all four spots:
8<---
/*
 * Exported routine for checking a user's access privileges to a table
 *
 * Does the bulk of the work for pg_class_aclcheck(), and allows other
 * callers to avoid the missing relation ERROR when is_missing is non-NULL.
 */
AclResult
pg_class_aclcheck_ext(Oid table_oid, Oid roleid,
  AclMode mode, bool *is_missing)
...
8<---

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: "has_column_privilege()" issue with attnums and non-existent columns

[Joe Conway]

On 3/30/21 8:17 PM, Joe Conway wrote:

On 3/30/21 6:22 PM, Tom Lane wrote:

Joe Conway  writes:

Heh, I missed the forest for the trees it seems.
That version undid the changes fixing what Ian was originally complaining about.


Duh, right.  It would be a good idea for there to be a code comment
explaining this, because it's *far* from obvious.  Say like

* Check for column-level privileges first.  This serves in
* part as a check on whether the column even exists, so we
* need to do it before checking table-level privilege.


Will do.


My gripe about providing API-spec comments for the new aclchk.c
entry points still stands.  Other than that, I think it's good
to go.


Yeah, I was planning to put something akin to this in all four spots:
8<---
/*
   * Exported routine for checking a user's access privileges to a table
   *
   * Does the bulk of the work for pg_class_aclcheck(), and allows other
   * callers to avoid the missing relation ERROR when is_missing is non-NULL.
   */
AclResult
pg_class_aclcheck_ext(Oid table_oid, Oid roleid,
  AclMode mode, bool *is_missing)
...
8<---



Pushed that way.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: policies with security definer option for allowing inline optimization

[Joe Conway]

On 4/2/21 9:57 AM, Isaac Morland wrote:
Views already run security definer, allowing them to be used for some of the 
same information-hiding purposes as RLS. But I just found something strange: 
current_user/_role returns the user's role, not the view owner's role:



postgres=# set role to t1;
SET
postgres=> table tt;
ERROR:  permission denied for table tt
postgres=> table tv;
  ?column? | current_user
--+--
         5 | t1
(1 row)

postgres=>

Note that even though current_user is t1 "inside" the view, it is still able to 
see the contents of table tt. Shouldn't current_user/_role return the view owner 
in this situation? By contrast security definer functions work properly:


That is because while VIEWs are effectively SECURITY DEFINER for table access, 
functions running as part of the view are still SECURITY INVOKER if they were 
defined that way. And "current_user" is essentially just a special grammatical 
interface to a SECURITY INVOKER function:


postgres=# \df+ current_user
List of functions
-[ RECORD 1 ]---+--
Schema  | pg_catalog
Name| current_user
Result data type| name
Argument data types |
Type| func
Volatility  | stable
Parallel| safe
Owner   | postgres
Security| invoker
Access privileges   |
Language| internal
Source code | current_user
Description | current user name

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: policies with security definer option for allowing inline optimization

[Joe Conway]

On 4/2/21 10:23 AM, Stephen Frost wrote:

Greetings,

* Joe Conway (m...@joeconway.com) wrote:

On 4/2/21 9:57 AM, Isaac Morland wrote:
>Views already run security definer, allowing them to be used for some of
>the same information-hiding purposes as RLS. But I just found something
>strange: current_user/_role returns the user's role, not the view owner's
>role:

>postgres=# set role to t1;
>SET
>postgres=> table tt;
>ERROR:  permission denied for table tt
>postgres=> table tv;
>  ?column? | current_user
>--+--
>         5 | t1
>(1 row)
>
>postgres=>
>
>Note that even though current_user is t1 "inside" the view, it is still
>able to see the contents of table tt. Shouldn't current_user/_role return
>the view owner in this situation? By contrast security definer functions
>work properly:

That is because while VIEWs are effectively SECURITY DEFINER for table
access, functions running as part of the view are still SECURITY INVOKER if
they were defined that way. And "current_user" is essentially just a special
grammatical interface to a SECURITY INVOKER function:


Right- and what I was really getting at is that it'd sometimes be nice
to have the view run as 'security invoker' for table access.  In
general, it seems like it'd be useful to be able to control each piece
and define if it's to be security invoker or security definer.  We're
able to do that for functions, but not other parts of the system.


+1

Agreed -- I have opined similarly in the past

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: documentation fix for SET ROLE

[Joe Conway]

On 4/2/21 10:21 AM, Laurenz Albe wrote:

On Mon, 2021-03-15 at 17:09 +, Bossart, Nathan wrote:

On 3/15/21, 7:06 AM, "Laurenz Albe"  wrote:
> On Fri, 2021-03-12 at 21:41 +, Bossart, Nathan wrote:
> > On 3/12/21, 11:14 AM, "Joe Conway"  wrote:
> > > Looking back at the commit history it seems to me that this only works
> > > accidentally. Perhaps it would be best to fix RESET ROLE and be done with 
it.
> > 
> > That seems reasonable to me.
> 
> +1 from me too.


Here's my latest attempt.  I think it's important to state that it
sets the role to the current session user identifier unless there is a
connection-time setting.  If there is no connection-time setting, it
will reset the role to the current session user, which might be
different if you've run SET SESSION AUTHORIZATION.

diff --git a/doc/src/sgml/ref/set_role.sgml b/doc/src/sgml/ref/set_role.sgml
index 739f2c5cdf..f02babf3af 100644
--- a/doc/src/sgml/ref/set_role.sgml
+++ b/doc/src/sgml/ref/set_role.sgml
@@ -53,9 +53,16 @@ RESET ROLE
   

   
-   The NONE and RESET forms reset the 
current
-   user identifier to be the current session user identifier.
-   These forms can be executed by any user.
+   SET ROLE NONE sets the current user identifier to the
+   current session user identifier, as returned by
+   session_user.  RESET ROLE sets the
+   current user identifier to the connection-time setting specified by the
+   command-line options,
+   ALTER ROLE, or
+   ALTER DATABASE,
+   if any such settings exist.  Otherwise, RESET ROLE sets
+   the current user identifier to the current session user identifier.  These
+   forms can be executed by any user.
   
  


Actually, SET ROLE NONE is defined by the SQL standard:

   18.3 

   [...]

   If NONE is specified, then
   Case:
   i) If there is no current user identifier, then an exception condition is 
raised:
  invalid role specification.
   ii) Otherwise, the current role name is removed.

This is reflected in a comment in src/backend/commands/variable.c:

   /*
* SET ROLE
*
* The SQL spec requires "SET ROLE NONE" to unset the role, so we hardwire
* a translation of "none" to InvalidOid.  Otherwise this is much like
* SET SESSION AUTHORIZATION.
*/

On the other hand, RESET (according to src/backend/utils/misc/README)
does something different:

   Prior values of configuration variables must be remembered in order to deal
   with several special cases: RESET (a/k/a SET TO DEFAULT)

So I think it is intentional that RESET ROLE does something else than
SET ROLE NONE, and we should not change that.

So I think that documenting this is the way to go.  I'll mark it as
"ready for committer".


pushed

Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: PL/R regression on windows, but not linux with master.

[Joe Conway]

On 4/11/21 10:13 AM, Tom Lane wrote:

Andrew Dunstan  writes:

Well, plr.h does this:

#define WARNING 19
#define ERROR   20



The coding pattern in plr.h looks quite breakable.


Meh -- that code has gone 18+ years before breaking.


Indeed.  elog.h already provides a "PGERROR" macro to use for restoring
the value of ERROR.  We have not heard of a need to do anything special
for WARNING though --- maybe that's R-specific?


R also defines WARNING in its headers. If I remember correctly there are (or at 
least were, it *has* been 18+ years since I looked at this particular thing) 
some odd differences in the R headers under Windows and Linux.


In any case we would be happy to use "PGERROR".

Would an equivalent "PGWARNING" be something we are open to adding and 
back-patching?


Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: PL/R regression on windows, but not linux with master.

[Joe Conway]

On 4/11/21 12:51 PM, Dave Cramer wrote:



On Sun, 11 Apr 2021 at 12:43, Tom Lane <mailto:t...@sss.pgh.pa.us>> wrote:


I wrote:
     > Joe Conway mailto:m...@joeconway.com>> writes:
 >> Would an equivalent "PGWARNING" be something we are open to adding and
 >> back-patching?

 > It's not real obvious how pl/r could solve this in a reliable way
 > otherwise, so adding that would be OK with me, but I wonder whether
 > back-patching is going to help you any.  You'd still need to compile
 > against older headers I should think.  So I'd suggest
 > (1) add PGWARNING in HEAD only

Concretely, maybe like the attached?


+1 from me.
I especially like the changes to the comments as it's more apparent what they 
should be used for.


+1

Looks great to me.

Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: security_context_t marked as deprecated in libselinux 3.1

[Joe Conway]

On 8/13/20 1:22 AM, Michael Paquier wrote:
> On Wed, Aug 12, 2020 at 10:50:21PM -0400, Tom Lane wrote:
>> Ummm ... aren't you going to get some cast-away-const warnings now?
>> Or are all of the called functions declared as taking "const char *"
>> not just "char *"?
> 
> Let me see..  The function signatures we use have been visibly changed
> in 9eb9c932, which comes down to a point between 2.2.2 and 2.3, and
> there are two of them we care about, both use now "const char *":
> - security_check_context_raw()
> - security_compute_create_name_raw()
> We claim in the docs that the minimum version of libselinux supported
> is 2.1.10 (7a86fe1a from march 2012).
> 
> Then, the only buildfarm animal I know of testing selinux is
> rhinoceros, that uses CentOS 7.1, and this visibly already bundles
> libselinux 2.5 that was released in 2016 (2b69984), per the RPM list
> here:
> http://mirror.centos.org/centos/7/
> Joe, what's the version of libselinux used in rhinoceros?  2.5?


rpm -q libselinux
libselinux-2.5-15.el7.x86_64


Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [RFC] speed up count(*)

[Joe Conway]

On 10/20/21 2:33 PM, John Naylor wrote:


On Wed, Oct 20, 2021 at 2:23 PM Tomas Vondra 
mailto:tomas.von...@enterprisedb.com>> 
wrote:

 >
 > Couldn't we simply inspect the visibility map, use the index data only
 > for fully visible/summarized ranges, and inspect the heap for the
 > remaining pages? That'd still be a huge improvement for tables with most
 > only a few pages modified recently, which is a pretty common case.
 >
 > I think the bigger issue is that people rarely do COUNT(*) on the whole
 > table. There are usually other conditions and/or GROUP BY, and I'm not
 > sure how would that work.

Right. My (possibly hazy) recollection is that people don't have quite 
as high an expectation for queries with more complex predicates and/or 
grouping. It would be interesting to see what the balance is.


I think you are exactly correct. People seem to understand that with a 
predicate it is harder, but they expect


 select count(*) from foo;

to be nearly instantaneous, and they don't really need it to be exact. 
The stock answer for that has been to do


 select reltuples from pg_class
 where relname = 'foo';

But that is unsatisfying because the problem is often with some 
benchmark or another that cannot be changed.


I'm sure this idea will be shot down in flames suit>, but what if we had a default "off" GUC which could be turned on 
causing the former to be transparently rewritten into the latter 
?


Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: [RFC] speed up count(*)

[Joe Conway]

On 10/21/21 4:06 PM, Robert Haas wrote:

On Thu, Oct 21, 2021 at 9:09 AM Joe Conway  wrote:

I think you are exactly correct. People seem to understand that with a
predicate it is harder, but they expect

  select count(*) from foo;

to be nearly instantaneous, and they don't really need it to be exact.
The stock answer for that has been to do

  select reltuples from pg_class
  where relname = 'foo';

But that is unsatisfying because the problem is often with some
benchmark or another that cannot be changed.


I would also expect it to almost always give the wrong answer.



That is a grossly overstated position. When I have looked, it is often 
not that terribly far off. And for many use cases that I have heard of 
at least, quite adequate.


Joe

--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: [RFC] speed up count(*)

[Joe Conway]

On 10/21/21 4:23 PM, Robert Haas wrote:

On Thu, Oct 21, 2021 at 4:19 PM Joe Conway  wrote:

That is a grossly overstated position. When I have looked, it is often
not that terribly far off. And for many use cases that I have heard of
at least, quite adequate.


I don't think it's grossly overstated. If you need an approximation it
may be good enough, but I don't think it will often be exactly correct
- probably only if the table is small and rarely modified.


meh -- the people who expect this to be impossibly fast don't typically 
need or expect it to be exactly correct, and there is no way to make it 
"exactly correct" in someone's snapshot without doing all the work.


That is why I didn't suggest making it the default. If you flip the 
switch, you would get a very fast approximation. If you care about 
accuracy, you accept it has to be slow.


Joe
--
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 9/6/19 11:26 AM, Yuli Khodorkovskiy wrote:
> On Fri, Sep 6, 2019 at 10:40 AM Stephen Frost  wrote:
>> There are actual reasons why the 'DELETE' privilege is *not* the same as
>> 'TRUNCATE' in PostgreSQL and I'm really not convinced that we should
>> just be tossing that distinction out the window for users of SELinux.  A
>> pretty obvious one is that DELETE triggers don't get fired for a
>> TRUNCATE command, but TRUNCATE also doesn't follow the same MVCC rules
>> that the rest of the system does.
> 
> I do agree with you there should be a distinction between TRUNCATE and
> DELETE in the SELinux perms. I'll wait a few days for more discussion
> and send an updated patch.


+1 - I don't think there is any question about it.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 9/6/19 2:18 PM, Tom Lane wrote:
> Yuli Khodorkovskiy  writes:
>> On Fri, Sep 6, 2019 at 11:57 AM Tom Lane  wrote:
>>> Well, the larger question, independent of the regression tests, is
>>> will the new policy work at all on older SELinux?  If not, that
>>> doesn't seem very acceptable.
> 
>> The default SELinux policy on Fedora ships with deny_unknown set to 0.
>> Deny_unknown was added to the kernel in 2.6.24, so unless someone is
>> using RHEL 5.x, which is in ELS, they will have the ability to
>> override the default behavior on CentOS/RHEL.
> 
> OK, that sounds like it will work.
> 
>> On RHEL 6, which goes into ELS in 2020, it's a bit more complicated
>> and requires rebuilding the base SELinux module from source.
> 
> sepgsql hasn't worked on RHEL6 in a long time, if ever; it requires
> a newer version of libselinux than what ships in RHEL6.  So I'm not
> concerned about that.  We do need to worry about RHEL7, and whatever
> is the oldest version of Fedora that is running the sepgsql tests
> in the buildfarm.


I could be wrong, but as far as I know rhinoceros is the only buildfarm
animal running sepgsql tests.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 9/6/19 2:13 PM, Yuli Khodorkovskiy wrote:
> As Joe Conway pointed out to me out of band, the build animal for RHEL
> 7 has handle_unknown set to `0`. Are there any other concerns with
> this approach?


You mean deny_unknown I believe.

"Allow unknown object class / permissions. This will set the returned AV
  with all 1's."

As I understand it, this would make the sepgsql behavior unchanged from
before if the policy does not support the new permission.

Joe

> On Fri, Sep 6, 2019 at 1:00 PM Yuli Khodorkovskiy wrote:
>> The default SELinux policy on Fedora ships with deny_unknown set to 0.
>> Deny_unknown was added to the kernel in 2.6.24, so unless someone is
>> using RHEL 5.x, which is in ELS, they will have the ability to
>> override the default behavior on CentOS/RHEL.



-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 9/6/19 8:07 PM, Tom Lane wrote:
> Joe Conway  writes:
>> On 9/6/19 2:18 PM, Tom Lane wrote:
>>> sepgsql hasn't worked on RHEL6 in a long time, if ever; it requires
>>> a newer version of libselinux than what ships in RHEL6.  So I'm not
>>> concerned about that.  We do need to worry about RHEL7, and whatever
>>> is the oldest version of Fedora that is running the sepgsql tests
>>> in the buildfarm.
> 
>> I could be wrong, but as far as I know rhinoceros is the only buildfarm
>> animal running sepgsql tests.
> 
> It seems reasonable to define RHEL7 as the oldest SELinux version we
> still care about.  But it'd be a good idea for somebody to be running
> a fairly bleeding-edge Fedora animal with sepgsql enabled, so we get
> coverage of the other end of the scale.


Yeah -- I was planning to eventually register a RHEL8 animal, but I
should probably do one for Fedora as well. I'll bump the priority for
that on my personal TODO.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 9/25/19 3:56 PM, Alvaro Herrera wrote:
> Hello
> 
> On 2019-Sep-09, Yuli Khodorkovskiy wrote:
> 
>> I have included an updated version of the sepgql patch. The
>> Truncate-Hook patch is unchanged from the last version.
> 
> This patch no longer applies.  Can you please rebase?
> 
> Joe, do you plan on being committer for this patch?  There seems to be
> substantial agreement on it.


I should be able to do that.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 9/25/19 4:47 PM, Joe Conway wrote:
> On 9/25/19 3:56 PM, Alvaro Herrera wrote:
>> Hello
>> 
>> On 2019-Sep-09, Yuli Khodorkovskiy wrote:
>> 
>>> I have included an updated version of the sepgql patch. The
>>> Truncate-Hook patch is unchanged from the last version.
>> 
>> This patch no longer applies.  Can you please rebase?
>> 
>> Joe, do you plan on being committer for this patch?  There seems to be
>> substantial agreement on it.
> 
> 
> I should be able to do that.


I am not sure I will get to this today. I assume it is ok for me to move
it forward e.g. next weekend, or is that not in line with commitfest rules?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: get_controlfile() can leak fds in the backend

[Joe Conway]

On 2/27/19 10:26 AM, Joe Conway wrote:
> On 2/27/19 2:47 AM, Michael Paquier wrote:
>> Hi all,
>> (CC-ing Joe as of dc7d70e)

> According to that comment BasicOpenFile does not seem to solve the issue
> you are pointing out (leaking of file descriptor on ERROR). Perhaps
> OpenTransientFile() is more appropriate? I am on the road at the moment
> so have not looked very deeply at this yet though.


It seems to me that OpenTransientFile() is more appropriate. Patch done
that way attached.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/common/controldata_utils.c b/src/common/controldata_utils.c
index abfe706..c3b1934 100644
*** a/src/common/controldata_utils.c
--- b/src/common/controldata_utils.c
***
*** 27,32 
--- 27,35 
  #include "catalog/pg_control.h"
  #include "common/controldata_utils.h"
  #include "port/pg_crc32c.h"
+ #ifndef FRONTEND
+ #include "storage/fd.h"
+ #endif
  
  /*
   * get_controlfile(char *DataDir, const char *progname, bool *crc_ok_p)
*** get_controlfile(const char *DataDir, con
*** 51,63 
  	ControlFile = palloc(sizeof(ControlFileData));
  	snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
  
- 	if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1)
  #ifndef FRONTEND
  		ereport(ERROR,
  (errcode_for_file_access(),
   errmsg("could not open file \"%s\" for reading: %m",
  		ControlFilePath)));
  #else
  	{
  		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
  progname, ControlFilePath, strerror(errno));
--- 54,67 
  	ControlFile = palloc(sizeof(ControlFileData));
  	snprintf(ControlFilePath, MAXPGPATH, "%s/global/pg_control", DataDir);
  
  #ifndef FRONTEND
+ 	if ((fd = OpenTransientFile(ControlFilePath, O_RDONLY | PG_BINARY)) == -1)
  		ereport(ERROR,
  (errcode_for_file_access(),
   errmsg("could not open file \"%s\" for reading: %m",
  		ControlFilePath)));
  #else
+ 	if ((fd = open(ControlFilePath, O_RDONLY | PG_BINARY, 0)) == -1)
  	{
  		fprintf(stderr, _("%s: could not open file \"%s\" for reading: %s\n"),
  progname, ControlFilePath, strerror(errno));
*** get_controlfile(const char *DataDir, con
*** 95,101 
--- 99,109 
  #endif
  	}
  
+ #ifndef FRONTEND
+ 	CloseTransientFile(fd);
+ #else
  	close(fd);
+ #endif
  
  	/* Check the CRC. */
  	INIT_CRC32C(crc);


signature.asc
Description: OpenPGP digital signature

Re: get_controlfile() can leak fds in the backend

[Joe Conway]

On 2/27/19 7:54 PM, Michael Paquier wrote:
> On Wed, Feb 27, 2019 at 07:45:11PM -0500, Joe Conway wrote:
>> It seems to me that OpenTransientFile() is more appropriate. Patch done
>> that way attached.
> 
> Works for me, thanks for sending a patch!  While on it, could you
> clean up the comment on top of get_controlfile()?  "char" is mentioned
> instead of "const char" for DataDir which is incorrect.  I would
> remove the complete set of arguments from the description and just
> keep the routine name.

Sure, will do. What are your thoughts on backpatching? This seems
unlikely to be a practical concern in the field, so my inclination is a
master only fix.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Row Level Security − leakproof-ness and performance implications

[Joe Conway]

On 2/28/19 9:12 AM, Robert Haas wrote:
> On Wed, Feb 27, 2019 at 6:03 PM Joe Conway  wrote:
>> Patch for discussion attached.
> 
> So... you're just going to replace ALL error messages of any kind with
> "ERROR: missing error text" when this option is enabled?  That sounds
> unusable.  I mean if I'm reading it right this would get not only
> messages from SQL-callable functions but also things like "deadlock
> detected" and "could not read block %u in file %s" and "database is
> not accepting commands to avoid wraparound data loss in database with
> OID %u".  You can't even shut it off conveniently, because the way
> you've designed it it has to be PGC_POSTMASTER to avoid TOCTTOU
> vulnerabilities.  Maybe I'm misreading the patch?

You have it correct.

I completely disagree that is is unusable though. The way I envision
this is that you enable force_leakproof on your development machine
without suppress_client_messages being turned on. Do your debugging there.

On production, both are turned on. You still get full unredacted
messages in your pg log. The client on a prod system does not need these
details. If you *really* need to, you can restart to turn it on for a
short while on prod, but hopefully you have a non prod system where you
reproduce issues for debugging anyway.

I am not married to making this only changeable via restart though --
that's why I posted the patch for discussion. Perhaps a superuserset
would be better so debugging could be done on one session only on the
prod machine.

> I don't think it would be crazy to have a mode where we try to redact
> the particular error messages that might leak information, but I think
> we'd need to make it only those.  A wild idea might be to let
> proleakproof take on three values: yes, no, and maybe.  When 'maybe'
> functions are involved, we tell them whether or not the current query
> involves any security barriers, and if so they self-censor.

Again, I disagree. See above -- you have all you need in the server logs.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Row Level Security − leakproof-ness and performance implications

[Joe Conway]

On 2/28/19 11:03 AM, Joshua Brindle wrote:
> On Thu, Feb 28, 2019 at 10:49 AM Tom Lane  wrote:
>>
>> Joshua Brindle  writes:
>> > On Thu, Feb 28, 2019 at 9:12 AM Robert Haas  wrote:
>> >> So... you're just going to replace ALL error messages of any kind with
>> >> "ERROR: missing error text" when this option is enabled?  That sounds
>> >> unusable.  I mean if I'm reading it right this would get not only
>> >> messages from SQL-callable functions but also things like "deadlock
>> >> detected" and "could not read block %u in file %s" and "database is
>> >> not accepting commands to avoid wraparound data loss in database with
>> >> OID %u".  You can't even shut it off conveniently, because the way
>>
>> > This makes complete sense to me. The client side of a client/server
>> > protocol doesn't have any way to fix 'could not read block %u in file
>> > %s', the client doesn't need that kind of detailed information about a
>> > server, and in fact that information could be security sensitive.
>>
>> I agree with Robert that this idea is a nonstarter.  Not only is it
>> a complete disaster from a usability standpoint, but *it does not
>> fix the problem*.  The mere fact that an error occurred, or didn't,
>> is already an information leak.  Sure, you can only extract one bit
>> per query that way, but a slow leak is still a leak.  See the Spectre
>> vulnerability for a pretty exact parallel.
> 
> How is leakproof defined WRT Postgres? Generally speaking a 1 bit
> error path would be considered a covert channel on most systems and
> are relatively slow even compared to e.g., timing channels.

Yes, I am pretty sure there are plenty of very security conscious
environments that would be willing to make this tradeoff in order to get
reliable RLS performance.

> Redacting error information, outside of the global leakproof setting,
> seems useful to prevent data leakage to a client on another system,
> such as Robert's example above "could not read block %u in file %s".

True

> Although, and Joe may hate me for saying this, I think only the
> non-constants should be redacted to keep some level of usability for
> regular SQL errors. Maybe system errors like the above should be
> removed from client messages in general.

I started down this path and it looked fragile. I guess if there is
generally enough support to think this might be viable I could open up
that door again, but I don't want to waste time if the approach is
really a non-starter as stated upthread :-/.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Row Level Security − leakproof-ness and performance implications

[Joe Conway]

On 2/28/19 11:37 AM, Robert Haas wrote:
> On Thu, Feb 28, 2019 at 11:14 AM Joe Conway  wrote:
>> > Although, and Joe may hate me for saying this, I think only the
>> > non-constants should be redacted to keep some level of usability for
>> > regular SQL errors. Maybe system errors like the above should be
>> > removed from client messages in general.
>>
>> I started down this path and it looked fragile. I guess if there is
>> generally enough support to think this might be viable I could open up
>> that door again, but I don't want to waste time if the approach is
>> really a non-starter as stated upthread :-/.
> 
> Hmm.  It seems to me that if there's a function that sometimes throws
> an error and other times does not, and if that behavior is dependent
> on the input, then even redacting the error message down to 'ERROR:
> error' does not remove the leak.  So it seems to me that regardless of
> what one thinks about the proposal from a usability perspective, it's
> probably not correct from a security standpoint.  Information that
> couldn't be leaked until present rules would leak with this change,
> when the new GUCs were turned on.
> 
> Am I wrong?


No, and Tom stated as much too, but life is all about tradeoffs. Some
people will find this an acceptable compromise. For those that don't
they don't have to use it. IMHO we tend toward too much nannyism too often.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Row Level Security − leakproof-ness and performance implications

[Joe Conway]

On 2/28/19 11:50 AM, Robert Haas wrote:
> On Thu, Feb 28, 2019 at 11:44 AM Joe Conway  wrote:
>> No, and Tom stated as much too, but life is all about tradeoffs. Some
>> people will find this an acceptable compromise. For those that don't
>> they don't have to use it. IMHO we tend toward too much nannyism too often.
> 
> Well, I agree with that, too.
> 
> Hmm.  I don't think there's anything preventing you from implementing
> this in "userspace," is there?  A logging hook could suppress all
> error message text, and you could just mark all functions leakproof
> after that, and you'd have this exact behavior in an existing release
> with no core code changes, I think.

I think that would affect the server logs too, no? Worth thinking about
though...

Also manually marking all functions leakproof is far less convenient
than turning off the check as this patch effectively allows. You would
want to keep track of the initial condition and be able to restore it if
needed. Doable but much uglier. Perhaps we could tolerate a hook that
would allow an extension to do this though?

> If you do that, or just stick this patch into your own distro, I would
> be interested to hear some experiences from customers (and those who
> support them) after some time had gone by.  I find it hard to imagine
> delivering customer support in an environment configured this way, but
> sometimes my imagination is limited.

Again, remember that the actual messages are available in the server
logs. The presumption is that the server logs are kept secure, and it is
ok to leak information into them. How the customer does or does not
decide to pass some of that information on to a support group becomes a
problem to deal with on a case by case basis.

Also, as mentioned up-thread, in many cases there is or should be a
non-production instance available to use for reproducing problems to
debug them. Presumably the data on such a system is faked or has already
been cleaned up for a wider audience.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Row Level Security − leakproof-ness and performance implications

[Joe Conway]

On 2/28/19 12:28 PM, Robert Haas wrote:
> Mmmph.  If your customers always have a non-production instance where
> problems from production can be easily reproduced, your customers are
> not much like our customers.

Well I certainly did not mean to imply that this is always the case ;-)

But I think it is fair to tell customers that have these tradeoffs in
front of them that it would be even more wise in the case they decided
to use this capability.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: get_controlfile() can leak fds in the backend

[Joe Conway]

On 2/28/19 7:20 AM, Michael Paquier wrote:
> On Thu, Feb 28, 2019 at 07:11:04AM -0500, Joe Conway wrote:
>> Sure, will do. What are your thoughts on backpatching? This seems
>> unlikely to be a practical concern in the field, so my inclination is a
>> master only fix.
> 
> I agree that this would unlikely become an issue as an error on the
> control file would most likely be a PANIC when updating it, so fixing
> only HEAD sounds fine to me.  Thanks for asking!


Committed and push that way.

By the way, while looking at this, I noted at least a couple of places
where OpenTransientFile() is being passed O_RDWR when the usage is
pretty clearly intended to be read-only. For example at least two
instances in slru.c -- SlruPhysicalReadPage() and
SimpleLruDoesPhysicalPageExist(). Is it worth while searching for and
fixing those instances?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Tighten error control for OpenTransientFile/CloseTransientFile

[Joe Conway]

On 2/28/19 9:33 PM, Michael Paquier wrote:
> Hi all,
> 
> Joe's message here has reminded me that we have lacked a lot of error
> handling around CloseTransientFile():
> https://www.postgresql.org/message-id/c49b69ec-e2f7-ff33-4f17-0eaa4f2ce...@joeconway.com
> 
> This has been mentioned by Alvaro a couple of months ago (cannot find
> the thread about that at quick glance), and I just forgot about it at
> that time.  Anyway, attached is a patch to do some cleanup for all
> that:
> - Switch OpenTransientFile to read-only where sufficient.
> - Add more error handling for CloseTransientFile
> A major take of this patch is to make sure that the new error messages
> generated have an elevel consistent with their neighbors.
> 
> Just on time for this last CF.  Thoughts?

Seems like it would be better to modify the arguments to
CloseTransientFile() to include the filename being closed, errorlevel,
and fail_on_error or something similar. Then all the repeated ereport
stanzas could be eliminated.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Row Level Security − leakproof-ness and performance implications

[Joe Conway]

On 3/18/19 3:52 PM, Peter Eisentraut wrote:
> On 2019-02-28 00:03, Joe Conway wrote:
>> What if we provided an option to redact all client messages (leaving
>> logged messages as-is). Separately we could provide a GUC to force all
>> functions to be resolved as leakproof. Depending on your requirements,
>> having both options turned on could be perfectly acceptable.
> 
> There are two commit fest entries for this thread, one in Pierre's name
> and one in yours.  Is your entry for the error message redacting
> functionality?  I think that approach has been found not to actually
> satisfy the leakproofness criteria.


It is a matter of opinion with regard to what the criteria actually is,
and when it ought to apply. But in any case the clear consensus was
against me, so I guess I'll assume "my patch was rejected by PostgreSQL
all I got was this tee shirt" (...I know I have one that says something
like that somewhere...) ;-)

I have no idea what the other entry is all about as I have not had the
time to look.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: PostgreSQL pollutes the file system

[Joe Conway]

On 3/29/19 11:44 AM, Daniel Gustafsson wrote:
> On Friday, March 29, 2019 4:41 PM, Tom Lane  wrote:
> 
>> Christoph Berg m...@debian.org writes:
>>
>> > What might possibly make sense is to add options to psql to
>> > facilitate common tasks:
>>
>> > psql --createdb foo
>> > psql --createuser bar --superuser
>> > psql --reindex foo
>>
>> That's a thought. Or perhaps better, allow pg_ctl to grow new
>> subcommands for those tasks?
> 
> +1 on using pg_ctl rather than psql, should we go down this path.


Agreed -- another +1 here

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: PostgreSQL pollutes the file system

[Joe Conway]

On 3/29/19 3:01 PM, Pavel Stehule wrote:
> But psql has safe escaping via :"xxx" notation. So some like
> 
> psql -c 'create role :"role"' -v role='my role' ...
> 
> But what I know the psql variables are not evaluated for -c query

You can do this:
echo "create role :\"role\"" | psql -v role='my role'
CREATE ROLE

echo "\password :\"role\"" | psql -v role='my role'
Enter new password:
Enter it again:

That said, this is kind of off the topic of this thread.
I like Tom's last suggestion of:

  pg_util  

Of course that does not lend itself to symlinking for backward
compatibility, does it? If there is a way I am not familiar with it.

I guess the alternative would be an alias, but can packages install an
alias? Or something else I am not thinking about?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: PostgreSQL pollutes the file system

[Joe Conway]

On 3/29/19 3:43 PM, Christoph Berg wrote:
> Re: Joe Conway 2019-03-29 <48e5efaf-7ea2-ed70-a803-949bbfec8...@joeconway.com>
>> echo "\password :\"role\"" | psql -v role='my role'
>> Enter new password:
>> Enter it again:
>> 
>> That said, this is kind of off the topic of this thread.
> 
> It is on-topic because the reason we can't just tell people to replace
>   createuser $foo
> with
>   psql -c "create user $foo"
> is because $foo might need escaping.
> 
> IMHO if we find an way to do that which is acceptable for sh scripts,
> the createuser/... commands could go.

I think these commands *were* once (at least some of them) shell scripts
and we went to executable C in order to make them work on Windows, IIRC.

>> I like Tom's last suggestion of:
>> 
>>   pg_util  
>> 
>> Of course that does not lend itself to symlinking for backward
>> compatibility, does it? If there is a way I am not familiar with it.
> 
> We could symlink createuser -> pg_util. It is pretty common for
> commands to act differently based on the name the were invoked as.

Yeah, I forgot about that. Does that also go for Windows?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Joe Conway]

On 7/27/19 3:02 PM, Sehrope Sarkuni wrote:
> More generally, without a cryptographic MAC I don't think it's
> possible to provide any meaningful malicious tamper detection. And
> even that would have to be off-page to deal with page replay (which I
> think is out of scope).
> 
> [1]: https://en.wikipedia.org/wiki/CRC-32#Data_integrity

Yes, exactly -- pretty sure I made that point down thread but who knows;
I know I at least thought it ;-P

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Joe Conway]

On 7/29/19 6:11 PM, Sehrope Sarkuni wrote:
> On Mon, Jul 29, 2019 at 4:15 PM Alvaro Herrera  > wrote:
> 
> On 2019-Jul-27, Sehrope Sarkuni wrote:
> 
> > Given the non-cryptographic nature of CRC and its 16-bit size, I'd
> > round down the malicious tamper detection it provides to zero. At best
> > it catches random disk errors so might as well keep it in plain text
> > and checkable offline.
> 
> But what attack are we protecting against?  We fear that somebody will
> steal a disk or a backup.  We don't fear that they will *write* data.
> The CRC is there to protect against data corruption.  So whether or not
> the CRC protects against malicious tampering is beside the point.
> 
> 
> That was in response to using an encrypted CRC for tamper detection. I
> agree that it does not provide meaningful protection so there is no
> point in adding complexity to use it for that.
> 
> I agree it's better to leave the CRC as-is for detecting corruption
> which also has the advantage of playing nice with existing checksum tooling.
>  
> 
> If we were trying to protect against an attacker having access to
> *writing* data in the production server, this encryption scheme is
> useless: they could just as well read unencrypted data from shared
> buffers anyway.
> 
> 
> The attack situation is someone being able to modify pages at the
> storage tier. They cannot necessarily read server memory or the
> encryption key, but they could make changes to existing data or an
> existing backup that would be subsequently read by the server.
> 
> Dealing with that is way out of scope but similar to the replica
> promotion I think it should be kept track of and documented.
>  
> 
> I think trying to protect against malicious data tampering is a second
> step *after* this one is done.
> 
> 
> +1

Well said; +1

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Joe Conway]

On 8/19/19 8:51 AM, Ahsan Hadi wrote:
> I have shared a calendar invite for TDE/KMS weekly meeting with the
> members who expressed interest of joining the meeting in this chain.
> Hopefully I haven't missed anyone.
> 
> I am not aware of everyone's timezone but I have tried to setup a time
> that's not very inconvenient. It won't be ideal for everyone as we are
> dealing with multiple timezone but do let me know It is too bizarre for
> you and I will try to find another slot.    
> 
> I will share a zoom link for the meeting on the invite in due course.


Please add me as well. I would like to join when I can.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)

[Joe Conway]

On 8/26/19 2:53 AM, Masahiko Sawada wrote:
> I guess that this depends on the number of encryption keys we use. If
> we have encryption keys per tablespace or database the number of keys
> would be at most several dozen or several hundred. It's enough to have
> them in flat-file format on the disk and to load them to the hash
> table on the shared memory. We would not need a complex mechanism.
> OTOH if we have keys per tables, we would need to consider indexes and
> buffering as they might not fit in the memory.

Master key(s) need to be kept in memory, but derived keys (using KDF)
would be calculated at time of use, I would think.

>> Some kind of flat-file based approach with a temp file and renaming of
>> files using durable_rename(), like what we used to do with
>> pg_shadow/authid, and now do with replorigin_checkpoint and such?
> 
> The PoC patch I created does that for the keyring file. When key
> rotation, the correspond WAL contains all re-encrypted keys with the
> master key identifier, and the recovery restores the keyring file. One
> good point of this approach is that external tools and startup process
> read it easier. It doesn't require backend codes such as system cache
> and heap functions.

That sounds like a good approach.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

RFC: seccomp-bpf support

[Joe Conway]

SECCOMP ("SECure COMPuting with filters") is a Linux kernel syscall
filtering mechanism which allows reduction of the kernel attack surface
by preventing (or at least audit logging) normally unused syscalls.

Quoting from this link:
https://www.kernel.org/doc/Documentation/prctl/seccomp_filter.txt

   "A large number of system calls are exposed to every userland process
with many of them going unused for the entire lifetime of the
process. As system calls change and mature, bugs are found and
eradicated. A certain subset of userland applications benefit by
having a reduced set of available system calls. The resulting set
reduces the total kernel surface exposed to the application. System
call filtering is meant for use with those applications."

Recent security best-practices recommend, and certain highly
security-conscious organizations are beginning to require, that SECCOMP
be used to the extent possible. The major web browsers, container
runtime engines, and systemd are all examples of software that already
support seccomp.

-
A seccomp (bpf) filter is comprised of a default action, and a set of
rules with actions pertaining to specific syscalls (possibly with even
more specific sets of arguments). Once loaded into the kernel, a filter
is inherited by all child processes and cannot be removed. It can,
however, be overlaid with another filter. For any given syscall match,
the most restrictive (a.k.a. highest precedence) action will be taken by
the kernel. PostgreSQL has already been run "in the wild" under seccomp
control in containers, and possibly systemd. Adding seccomp support into
PostgreSQL itself mitigates issues with these approaches, and has
several advantages:

* Container seccomp filters tend to be extremely broad/permissive,
  typically allowing about 6 out 7 of all syscalls. They must do this
  because the use cases for containers vary widely.
* systemd does not implement seccomp filters by default. Packagers may
  decide to do so, but there is no guarantee. Adding them post install
  potentially requires cooperation by groups outside control of
  the database admins.
* In the container and systemd case there is no particularly good way to
  inspect what filters are active. It is possible to observe actions
  taken, but again, control is possibly outside the database admin
  group. For example, the best way to understand what happened is to
  review the auditd log, which is likely not readable by the DBA.
* With built-in support, it is possible to lock down backend processes
  more tightly than the postmaster.
* With built-in support, it is possible to lock down different backend
  processes differently than each other, for example by using ALTER ROLE
  ... SET or ALTER DATABASE ... SET.
* With built-in support, it is possible to calculate and return (in the
  form of an SRF) the effective filters being applied to the postmaster
  and the current backend.
* With built-in support, it could be possible (this part not yet
  implemented) to have separate filters for different backend types,
  e.g. autovac workers, background writer, etc.

-
Attached is a patch for discussion, adding support for seccomp-bpf
(nowadays generally just called seccomp) syscall filtering at
configure-time using libseccomp. I would like to get this in shape to be
committed by the end of the November CF if possible.

The code itself has been through several rounds of revision based on
discussions I have had with the author of libseccomp as well as a few
other folks. However as of the moment:

* Documentation - general discussion missing entirely
* No regression tests

-
For convenience, here are a couple of additional links to relevant
information regarding seccomp:
https://en.wikipedia.org/wiki/Seccomp
https://github.com/seccomp/libseccomp

-
Specific feedback requested:
1. Placement of pg_get_seccomp_filter() in
   src/backend/utils/adt/genfile.c
   originally made sense but after several rewrites no longer does.
   Ideas where it *should* go?
2. Where should a general discussion section go in the docs, if at all?
3. Currently this supports a global filter at the postmaster level,
   which is inherited by all child processes, and a secondary filter
   at the client backend session level. It likely makes sense to
   support secondary filters for other types of child processes,
   e.g. autovacuum workers, etc. Add that now (pg13), later release,
   or never?
4. What is the best way to approach testing of this feature? Tap
   testing perhaps?
5. Default GUC values - should we provide "starter" lists, or only a
   procedure for generating a list (as below).

-
Notes on usage:
===
In order to determine your minimally required allow lists, do something
like the following on a non-production server with the same architecture
as production:

0. Setup:
 * install libseccomp, libseccomp-dev, and seccomp
 * install auditd if not already installed
 * confi

Re: RFC: seccomp-bpf support

[Joe Conway]

On 8/28/19 1:03 PM, Peter Eisentraut wrote:
> On 2019-08-28 17:13, Joe Conway wrote:
>> * systemd does not implement seccomp filters by default. Packagers may
>>   decide to do so, but there is no guarantee. Adding them post install
>>   potentially requires cooperation by groups outside control of
>>   the database admins.
> 
> Well, if we are interested in this, why don't we just ask packagers to
> do so?  That seems like a much easier solution.


For the reason listed below

>> * In the container and systemd case there is no particularly good way to
>>   inspect what filters are active. It is possible to observe actions
>>   taken, but again, control is possibly outside the database admin
>>   group. For example, the best way to understand what happened is to
>>   review the auditd log, which is likely not readable by the DBA.
> 
> Why does one need to know what filters are active (other than,
> obviously, checking that the filters one has set were actually
> activated)?  What decisions would a DBA or database user be able to make
> based on whether a filter is set or not?

So that when an enforement action happens, you can understand why it
happened. Perhaps it was a bug (omission) in your allow list, or perhaps
it was an intentional rule to prevent abuse (say blocking certain
syscalls from plperlu), or it was because someone is trying to
compromise you system (e.g. some obscure and clearly not needed syscall).

>> * With built-in support, it is possible to lock down backend processes
>>   more tightly than the postmaster.
> 
> Also the other way around?

As I stated in the original email, the filters can add restrictions but
never remove them.

>> * With built-in support, it is possible to lock down different backend
>>   processes differently than each other, for example by using ALTER ROLE
>>   ... SET or ALTER DATABASE ... SET.
> 
> What are use cases for this?

For example to allow a specific user to use plperlu to exec shell code
while others cannot.

>> Attached is a patch for discussion, adding support for seccomp-bpf
>> (nowadays generally just called seccomp) syscall filtering at
>> configure-time using libseccomp. I would like to get this in shape to be
>> committed by the end of the November CF if possible.
> 
> To compute the initial set of allowed system calls, you need to have
> fantastic test coverage.  What you don't want is some rarely used error
> recovery path to cause a system crash.  I wouldn't trust our current
> coverage for this.


So if you are worried about that make your default action 'log' and
watch audit.log. There will be no errors or crashes of postgres caused
by that because there will be no change in postgres visible behavior.

And if returning an error from a syscall causes a crash that would be a
serious bug and we should fix it.


> Also, the list of system calls in use changes all the time.  The same
> function call from PostgreSQL could be a system call or a glibc
> implementation, depending on the specific installed packages or run-time
> settings.

True. That is why I did not provide an initial list and believe folks
who want to use this should develop their own.

> Extensions would need to maintain their own permissions list, and they
> would need to be merged manually into the respective existing settings.

People would have to generate their own list for extensions -- I don't
believe it is the extension writers' problem.

> Without good answers to these, I suspect that this feature would go
> straight to the top of the list of "if in doubt, turn off".

That is fine. Perhaps most people never use this, but when needed (and
increasingly will be required) it is available.

> Overall, I think this sounds like a maintenance headache, and the
> possible benefits are unclear.

The only people who will incur the maintenance headache are those who
need to use it. The benefits are compliance with requirements.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: RFC: seccomp-bpf support

[Joe Conway]

On 8/28/19 12:47 PM, David Fetter wrote:
> On Wed, Aug 28, 2019 at 11:13:27AM -0400, Joe Conway wrote:
>> SECCOMP ("SECure COMPuting with filters") is a Linux kernel syscall
>> filtering mechanism which allows reduction of the kernel attack surface
>> by preventing (or at least audit logging) normally unused syscalls.
>> 
>> Quoting from this link:
>> https://www.kernel.org/doc/Documentation/prctl/seccomp_filter.txt
>> 
>>"A large number of system calls are exposed to every userland process
>> with many of them going unused for the entire lifetime of the
>> process. As system calls change and mature, bugs are found and
>> eradicated. A certain subset of userland applications benefit by
>> having a reduced set of available system calls. The resulting set
>> reduces the total kernel surface exposed to the application. System
>> call filtering is meant for use with those applications."
>> 
>> Recent security best-practices recommend, and certain highly
>> security-conscious organizations are beginning to require, that SECCOMP
>> be used to the extent possible. The major web browsers, container
>> runtime engines, and systemd are all examples of software that already
>> support seccomp.
> 
> Neat!
> 
> Are the seccomp interfaces for other kernels arranged in a manner
> similar enough to have a unified interface in PostgreSQL, or is this
> more of a Linux-only feature?


As far as I know libseccomp is Linux specific, at least at the moment.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: RFC: seccomp-bpf support

[Joe Conway]

On 8/28/19 4:07 PM, Peter Eisentraut wrote:
> On 2019-08-28 21:38, Joshua Brindle wrote:
>> I think we need to reign in the thread somewhat. The feature allows
>> end users to define some sandboxing within PG. Nothing is being forced
>> on anyone
> 
> Features come with a maintenance cost.  If we ship it, then people are
> going to try it out.  Then weird things will happen.  They will report
> mysterious bugs.  They will complain to their colleagues.  It all comes
> with a cost.
> 
>> but we would like the capability to harden a PG installation
>> for many reasons already stated.
> 
> Most if not all of those reasons seem to have been questioned.


Clearly Joshua and I disagree, but understand that the consensus is not
on our side. It is our assessment that PostgreSQL will be subject to
seccomp willingly or not (e.g., via docker, systemd, etc.) and the
community might be better served to get out in front and have first
class support.

But I don't want to waste any more of anyone's time on this topic,
except to ask if two strategically placed hooks are asking too much?

Specifically hooks to replace these two stanzas in the patch:

8<--
diff --git a/src/backend/postmaster/postmaster.c
b/src/backend/postmaster/postmaster.c
index 62dc93d..2216d49 100644
*** a/src/backend/postmaster/postmaster.c
--- b/src/backend/postmaster/postmaster.c
*** PostmasterMain(int argc, char *argv[])
*** 963,968 
--- 963,982 

[...]

diff --git a/src/backend/utils/init/postinit.c
b/src/backend/utils/init/postinit.c
index 43b9f17..aac1940 100644
*** a/src/backend/utils/init/postinit.c
--- b/src/backend/utils/init/postinit.c
*** InitPostgres(const char *in_dbname, Oid
*** 1056,1061 
--- 1056,1076 

[...]

8<--


We will continue to pursue this development for customers that require
it and plan to provide an update on our analysis and results.

We thank you for your comments and suggestions.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: RFC: seccomp-bpf support

[Joe Conway]

On 8/29/19 10:00 AM, Tom Lane wrote:
> Joe Conway  writes:
>> Clearly Joshua and I disagree, but understand that the consensus is not
>> on our side. It is our assessment that PostgreSQL will be subject to
>> seccomp willingly or not (e.g., via docker, systemd, etc.) and the
>> community might be better served to get out in front and have first
>> class support.
> 
> Sure, but ...
> 
>> But I don't want to waste any more of anyone's time on this topic,
>> except to ask if two strategically placed hooks are asking too much?
> 
> ... hooks are still implying a design with the filter control inside
> Postgres.  Which, as I said before, seems like a fundamentally incorrect
> architecture.  I'm not objecting to having such control, but I think
> it has to be outside the postmaster, or it's just not a credible
> security improvement.

I disagree. Once a filter is loaded there is no way to unload it short
of a postmaster restart. That is an easily detected event that can be
alerted upon, and that is definitely a security improvement.

Perhaps that is a reason to also set the session level GUC to
PGC_POSTMASTER, but that is an easy change if deemed necessary.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: A space-efficient, user-friendly way to store categorical data

[Joe Conway]

On 02/11/2018 10:06 PM, Thomas Munro wrote:
> On Mon, Feb 12, 2018 at 12:24 PM, Andrew Dunstan
>  wrote:
>> On Mon, Feb 12, 2018 at 9:10 AM, Tom Lane  wrote:
>>> Andrew Kane  writes:
 A better option could be a new "dynamic enum" type, which would have
 similar storage requirements as an enum, but instead of labels being
 declared ahead of time, they would be added as data is inserted.
>>>
>>> You realize, of course, that it's possible to add labels to an enum type
>>> today.  (Removing them is another story.)
>>>
>>> You haven't explained exactly what you have in mind that is going to be
>>> able to duplicate the advantages of the current enum implementation
>>> without its disadvantages, so it's hard to evaluate this proposal.
>>>
>>
>>
>> This sounds rather like the idea I have been tossing around in my head
>> for a while, and in sporadic discussions with a few people, for a
>> dictionary object. The idea is to have an append-only list of labels
>> which would not obey transactional semantics, and would thus help us
>> avoid the pitfalls of enums - there wouldn't be any rollback of an
>> addition.  The use case would be for a jsonb representation which
>> would replace object keys with the oid value of the corresponding
>> dictionary entry rather like enums now. We could have a per-table
>> dictionary which in most typical json use cases would be very small,
>> and we know from some experimental data that the compression in space
>> used from such a change would often be substantial.
>>
>> This would have to be modifiable dynamically rather than requiring
>> explicit additions to the dictionary, to be of practical use for the
>> jsonb case, I believe.
>>
>> I hadn't thought about this as a sort of super enum that was usable
>> directly by users, but it makes sense.
>>
>> I have no idea how hard or even possible it would be to implement.
> 
> I have had thoughts over the years about something similar, but going
> the other way and hiding it from the end user.  If you could declare a
> column to have a special compressed property (independently of the
> type) then it could either automatically maintain a dictionary, or at
> least build a new dictionary for your when you next run some kind of
> COMPRESS operation.  There would be no user visible difference except
> footprint.  In ancient DB2 they had a column property along those
> lines called "VALUE COMPRESSION" (they also have a row-level version,
> and now they have much more advanced kinds of adaptive compression
> that I haven't kept up with).  In some ways it'd be a bit like toast
> with shared entries, but I haven't seriously looked into how such a
> thing might be implemented.

For what it is worth, there is a similar concept in R called "factors".
When categorical data is stored in a data.frame (R language equivalent
to relations) it is transparently and automatically converted. I believe
this is both for storage compression and to facilitate some of the
analytics. In R you can also explicitly specify to *not* convert strings
to factors as a performance optimization, because that conversion does
have a noticeable impact during ingestion and is not always needed.

I can also envision usefulness of this type of mechanism in other
security related scenarios.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 11/8/19 9:02 AM, Yuli Khodorkovskiy wrote:
> On Thu, Nov 7, 2019 at 7:46 PM Michael Paquier  wrote:
>>
>> On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote:
>> > On 2019-Sep-30, Joe Conway wrote:
>> >
>> > > I am not sure I will get to this today. I assume it is ok for me to move
>> > > it forward e.g. next weekend, or is that not in line with commitfest 
>> > > rules?
>> >
>> > You can commit whatever patch whenever you feel like it.  I will
>> > probably move this patch to the next commitfest before that, but you can
>> > mark it committed there as soon as you commit it.
>>
>> One month later, nothing has happened here.  Joe, are you planning to
>> look at this patch?
>>
>> The last patch I found does not apply properly, so please provide a
>> rebase.  I am switching the patch as waiting on author.
> 
> Michael,
> 
> I was able to apply the latest patches in the thread (9/25/19) on top
> of master. I have attached them for convenience.


Yes, I will look when I am able. Hopefully this weekend, almost
certainly before the end of this commitfest.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 11/8/19 9:16 AM, Joe Conway wrote:
> On 11/8/19 9:02 AM, Yuli Khodorkovskiy wrote:
>> On Thu, Nov 7, 2019 at 7:46 PM Michael Paquier  wrote:
>>>
>>> On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote:
>>> > On 2019-Sep-30, Joe Conway wrote:
>>> >
>>> > > I am not sure I will get to this today. I assume it is ok for me to move
>>> > > it forward e.g. next weekend, or is that not in line with commitfest 
>>> > > rules?
>>> >
>>> > You can commit whatever patch whenever you feel like it.  I will
>>> > probably move this patch to the next commitfest before that, but you can
>>> > mark it committed there as soon as you commit it.
>>>
>>> One month later, nothing has happened here.  Joe, are you planning to
>>> look at this patch?
>>>
>>> The last patch I found does not apply properly, so please provide a
>>> rebase.  I am switching the patch as waiting on author.
>> 
>> Michael,
>> 
>> I was able to apply the latest patches in the thread (9/25/19) on top
>> of master. I have attached them for convenience.
> 
> Yes, I will look when I am able. Hopefully this weekend, almost
> certainly before the end of this commitfest.

I tested this successfully on Rhinoceros, both with and without
"db_table: { truncate }" loaded in the policy. Updated patches attached
here with some editorialization. If there are no objections I will
commit/push both in about a day or two.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Add object TRUNCATE hook

All operations with acl permissions checks should have a corresponding hook
so that, for example, mandatory access control (MAC) may be enforced by an
extension. The command TRUNCATE is missing this hook, so add it. Patch by
Yuli Khodorkovskiy with some editorialization by me. Based on the discussion
not back-patched. A separate patch will exercise the hook in the sepgsql
extension.

Author: Yuli Khodorkovskiy
Reviewed-by: Joe Conway
Discussion: https://postgr.es/m/CAFL5wJcomybj1Xdw7qWmPJRpGuFukKgNrDb6uVBaCMgYS9dkaA%40mail.gmail.com

---
diff --git a/src/backend/catalog/objectaccess.c b/src/backend/catalog/objectaccess.c
index b1619b8..d51d01f 100644
*** a/src/backend/catalog/objectaccess.c
--- b/src/backend/catalog/objectaccess.c
***
*** 11,16 
--- 11,17 
  #include "postgres.h"
  
  #include "catalog/objectaccess.h"
+ #include "catalog/pg_class.h"
  #include "catalog/pg_namespace.h"
  #include "catalog/pg_proc.h"
  
*** RunObjectDropHook(Oid classId, Oid objec
*** 65,70 
--- 66,87 
  }
  
  /*
+  * RunObjectTruncateHook
+  *
+  * It is the entrypoint of OAT_TRUNCATE event
+  */
+ void
+ RunObjectTruncateHook(Oid objectId)
+ {
+ 	/* caller should check, but just in case... */
+ 	Assert(object_access_hook != NULL);
+ 
+ 	(*object_access_hook) (OAT_TRUNCATE,
+ 		   RelationRelationId, objectId, 0,
+ 		   NULL);
+ }
+ 
+ /*
   * RunObjectPostAlterHook
   *
   * It is entrypoint of OAT_POST_ALTER event
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 45aae59..5440eb9 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** truncate_check_rel(Oid relid, Form_pg_cl
*** 1937,1942 
--- 1937,1944 
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
   errmsg("permission denied: \"%s\" is a system catalog",
  		relname)));
+ 
+ 	InvokeObjectTruncateHook(relid);
  }
  
  /*
diff --git a/src/include/catalog/objectaccess.h b/src/include/catalog/objectaccess.h
index 0170f1f..9824adf 100644
*** a/src/include/catalog/objectaccess.h
--- b/src/include/catalog/objectaccess.h
***
*** 37,42 
--- 37,46 
   * creation or altering, because OAT_POST_CREATE or OAT_POST_ALTER are
   * sufficient for extensions to track these kind of checks.
   *
+  * OAT_TRUNCATE should be invoked just before truncation of objects. This
+  * event is equivalent to truncate permission on a relation under the
+  * default access control mechanism.
+  *
   * Other types may be added in the future.
   */
  typedef enum ObjectAccessType
*** typedef enum ObjectAccessType
*** 45,51 
  	OAT_DROP,
  	OAT_POST_ALTER,
  	OAT_NAMESPACE_SEARCH,
! 	OAT_FUNCTION_EXECUTE
  } ObjectAccessType;
  
  /*
--- 49,56 
  	OAT_DROP,
  	OAT_POST_ALTER,
  	OAT_NAMESPACE_SEARCH,
! 	OAT_FUNCTION_EXECUTE,
! 	OAT_TRUNCATE
  } ObjectAccessType;
  
  /*
*** extern void RunObjectPostCreateHook(Oid
*** 131,136 
--- 136,142 
  	bool is_internal);
  extern void RunObjectDropHook(Oid classId, Oid objectId, int subId,
  			  int d

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 11/20/19 2:30 PM, Joe Conway wrote:
> On 11/8/19 9:16 AM, Joe Conway wrote:
>> On 11/8/19 9:02 AM, Yuli Khodorkovskiy wrote:
>>> On Thu, Nov 7, 2019 at 7:46 PM Michael Paquier  wrote:
>>>>
>>>> On Mon, Sep 30, 2019 at 11:38:05AM -0300, Alvaro Herrera wrote:
>>>> > On 2019-Sep-30, Joe Conway wrote:
>>>> >
>>>> > > I am not sure I will get to this today. I assume it is ok for me to 
>>>> > > move
>>>> > > it forward e.g. next weekend, or is that not in line with commitfest 
>>>> > > rules?
>>>> >
>>>> > You can commit whatever patch whenever you feel like it.  I will
>>>> > probably move this patch to the next commitfest before that, but you can
>>>> > mark it committed there as soon as you commit it.
>>>>
>>>> One month later, nothing has happened here.  Joe, are you planning to
>>>> look at this patch?
>>>>
>>>> The last patch I found does not apply properly, so please provide a
>>>> rebase.  I am switching the patch as waiting on author.
>>> 
>>> Michael,
>>> 
>>> I was able to apply the latest patches in the thread (9/25/19) on top
>>> of master. I have attached them for convenience.
>> 
>> Yes, I will look when I am able. Hopefully this weekend, almost
>> certainly before the end of this commitfest.
> 
> I tested this successfully on Rhinoceros, both with and without
> "db_table: { truncate }" loaded in the policy. Updated patches attached
> here with some editorialization. If there are no objections I will
> commit/push both in about a day or two.


...and I managed to drop the new files from the sepgsql patch. Complete
version attached.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Add object TRUNCATE hook

All operations with acl permissions checks should have a corresponding hook
so that, for example, mandatory access control (MAC) may be enforced by an
extension. The command TRUNCATE is missing this hook, so add it. Patch by
Yuli Khodorkovskiy with some editorialization by me. Based on the discussion
not back-patched. A separate patch will exercise the hook in the sepgsql
extension.

Author: Yuli Khodorkovskiy
Reviewed-by: Joe Conway
Discussion: https://postgr.es/m/CAFL5wJcomybj1Xdw7qWmPJRpGuFukKgNrDb6uVBaCMgYS9dkaA%40mail.gmail.com

---
diff --git a/src/backend/catalog/objectaccess.c b/src/backend/catalog/objectaccess.c
index b1619b8..d51d01f 100644
*** a/src/backend/catalog/objectaccess.c
--- b/src/backend/catalog/objectaccess.c
***
*** 11,16 
--- 11,17 
  #include "postgres.h"
  
  #include "catalog/objectaccess.h"
+ #include "catalog/pg_class.h"
  #include "catalog/pg_namespace.h"
  #include "catalog/pg_proc.h"
  
*** RunObjectDropHook(Oid classId, Oid objec
*** 65,70 
--- 66,87 
  }
  
  /*
+  * RunObjectTruncateHook
+  *
+  * It is the entrypoint of OAT_TRUNCATE event
+  */
+ void
+ RunObjectTruncateHook(Oid objectId)
+ {
+ 	/* caller should check, but just in case... */
+ 	Assert(object_access_hook != NULL);
+ 
+ 	(*object_access_hook) (OAT_TRUNCATE,
+ 		   RelationRelationId, objectId, 0,
+ 		   NULL);
+ }
+ 
+ /*
   * RunObjectPostAlterHook
   *
   * It is entrypoint of OAT_POST_ALTER event
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index 45aae59..5440eb9 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
*** truncate_check_rel(Oid relid, Form_pg_cl
*** 1937,1942 
--- 1937,1944 
  (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
   errmsg("permission denied: \"%s\" is a system catalog",
  		relname)));
+ 
+ 	InvokeObjectTruncateHook(relid);
  }
  
  /*
diff --git a/src/include/catalog/objectaccess.h b/src/include/catalog/objectaccess.h
index 0170f1f..9824adf 100644
*** a/src/include/catalog/objectaccess.h
--- b/src/include/catalog/objectaccess.h
***
*** 37,42 
--- 37,46 
   * creation or altering, because OAT_POST_CREATE or OAT_POST_ALTER are
   * sufficient for extensions to track these kind of checks.
   *
+  * OAT_TRUNCATE should be invoked just before truncation of objects. This
+  * event is equivalent to truncate permission on a relation under the
+  * default access control mechanism.
+  *
   * Other types may be added in the future.
   */
  typedef enum ObjectAccessType
*** typedef enum ObjectAccessType
*** 45,51 
  	OAT_DROP,
  	OAT_POST_ALTER,
  	OAT_NAMESPACE_SEARCH,
! 	OAT_FUNCTION_EXECUTE
  } ObjectAccessType;
  
  /*
--- 49,56 
  	OAT_DROP,
  	OAT_POST_ALTER,

Re: add a MAC check for TRUNCATE

[Joe Conway]

On 11/22/19 3:07 AM, Michael Paquier wrote:
> On Wed, Nov 20, 2019 at 02:30:12PM -0500, Joe Conway wrote:
>> I tested this successfully on Rhinoceros, both with and without
>> "db_table: { truncate }" loaded in the policy. Updated patches attached
>> here with some editorialization. If there are no objections I will
>> commit/push both in about a day or two.
> 
> Thanks for the update, Joe.  I have switched the patch as ready for
> committer, with your name as committer of the entry to reflect this
> status.

Pushed.

Thanks,

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: string literal continuations in C

[Joe Conway]

On 12/23/19 2:51 PM, Alvaro Herrera wrote:
> Per a recent thread, these patches remove string literals split with
> \-escaped newlines.  The first is for the message "materialize mode
> required, but it is not allowed in this context" where it's more
> prevalent, and we keep perpetuating it; the second is for other
> messages, whose bulk is in dblink and tablefunc.  I think the split is
> pointless and therefore propose to push both together as a single
> commit, but maybe somebody would like me to leave those contrib modules
> alone.

I take it since I was explicitly CC'd that the contrib comment was aimed
at me? I likely copied the convention from somewhere else in the
backend, but I don't care either way if you want to change them. However
I guess we should coordinate since I've been berated regarding error
codes and will likely go change at least two of them in tablefunc soon
(not likely before Thursday though)...

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: RFC: seccomp-bpf support

[Joe Conway]

On 1/6/20 8:37 PM, Tomas Vondra wrote:
> Hi,
> 
> This patch is currently in "needs review" state, but the last message is
> from August 29, and my understanding is that there have been a couple of
> objections / disagreements about the architecture, difficulties with
> producing the set of syscalls, and not providing any built-in policy.
> 
> I don't think we're any closer to resolve those disagreements since
> August, so I think we should make some decision about this patch,
> instead of just moving it from one CF to the next one. The "needs
> review" status seems not reflecting the situation.
> 
> Are there any plans to post a new version of the patch with a different
> design, or something like that? If not, I propose we mark it either as
> rejected or returned with feedback (and maybe get a new patch in the
> future).


I assumed it was rejected.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [PATCH v20] GSSAPI encryption support

[Joe Conway]

On 4/2/19 6:18 PM, Stephen Frost wrote:
> Greetings,
> 
> On Tue, Apr 2, 2019 at 18:10 Peter Eisentraut
>  > wrote:
> 
> On 2019-02-23 17:27, Stephen Frost wrote:
> >> About pg_hba.conf: The "hostgss" keyword seems a bit confusing. 
> It only
> >> applies to encrypted gss-using connections, not all of them.  Maybe
> >> "hostgssenc" or "hostgsswrap"?
> > Not quite sure what you mean here, but 'hostgss' seems to be quite
> well
> > in-line with what we do for SSL...  as in, we have 'hostssl', we don't
> > say 'hostsslenc'.  I feel like I'm just not understanding what you
> mean
> > by "not all of them".
> 
> Reading the latest patch, I think this is still a bit confusing.
> Consider an entry like
> 
>     hostgss all             all             0.0.0.0/0
>                gss
> 
> The "hostgss" part means, the connection is GSS-*encrypted*.  The "gss"
> entry in the last column means use gss for *authentication*.  But didn't
> "hostgss" already imply that?  No.  I understand what's going on, but it
> seems quite confusing.  They both just say "gss"; you have to know a lot
> about the nuances of pg_hba.conf processing to get that.
> 
> If you have line like
> 
>     hostgss all             all             0.0.0.0/0
>                md5
> 
> it is not obvious that this means, if GSS-encrypted, use md5.  It could
> just as well mean, if GSS-authenticated, use md5.
> 
> The analogy with SSL is such that we use "hostssl" for connections using
> SSL encryption and "cert" for the authentication method.  So there we
> use two different words for two different aspects of SSL.
> 
> 
> I don’t view it as confusing, but I’ll change it to hostgssenc as was
> suggested earlier to address that concern.  It’s a bit wordy but if it
> helps reduce confusion then that’s a good thing.

Personally I don't find it as confusing as is either, and I find hostgss
to be a good analog of hostssl. On the other hand hostgssenc is long and
unintuitive. So +1 for leaving as is and -1 one for changing it IMHO.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: Should the docs have a warning about pg_stat_reset()?

[Joe Conway]

On 4/13/19 3:42 PM, Tomas Vondra wrote:
> If only we had a way to regularly snapshot the data from within the
> database, and then compute the deltas on that. If only we could insert
> data from one table into another one a then do some analysics on it,
> with like small windows moving over the data or something ...

Why not store deltas separately with their own delta reset command?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

TRACE_SORT defined by default

[Joe Conway]

I just noticed that TRACE_SORT is defined by default (since 2005
apparently). It seems odd since it is the only debugging code enabled by
default.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: TRACE_SORT defined by default

[Joe Conway]

On 4/24/19 5:10 PM, Peter Geoghegan wrote:
> On Wed, Apr 24, 2019 at 2:07 PM Joe Conway  wrote:
>> I just noticed that TRACE_SORT is defined by default (since 2005
>> apparently). It seems odd since it is the only debugging code enabled by
>> default.
> 
> I think that we should get rid of the #ifdef stuff, so that it isn't
> possible to disable the trace_sort instrumentation my commenting out
> the TRACE_SORT entry in pg_config_manual.h. I recall being opposed on
> this point by Robert Haas. Possibly because he just didn't want to
> deal with it at the time.


Has anyone ever (or recently) measured the impact on performance to have
this enabled? Is it that generically useful for debugging of production
instances of Postgres that we really want it always enabled despite the
performance impact?

Maybe the answer to both is "yes", but if so I would agree that we ought
to remove the define and ifdef's and just bake it in.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: How to install login_hook in Postgres 10.5

[Joe Conway]

On 5/13/19 8:32 PM, Michael Paquier wrote:
> On Mon, May 13, 2019 at 01:06:10PM -0700, legrand legrand wrote:
>> that finished commited
>> "pgsql: Add hooks for session start and session end"
>> https://www.postgresql.org/message-id/flat/575d6fa2-78d0-4456-8600-302fc35b2591%40dunslane.net#0819e315c6e44c49a36c69080cab644d
>> 
>> but was finally rollbacked because it didn't pass installcheck test
>> ...
>> 
>> Maybe you are able to patch your pg installation, 
>> it would be a solution of choice (there is a nice exemple 
>> of extension included)
> 
> You will need to patch Postgres to add this hook, and you could
> basically reuse the patch which has been committed once.  I don't
> think that it would be that much amount of work to get it working
> correctly on the test side to be honest, so we may be able to get
> something into v13 at this stage.  This is mainly a matter of
> resources though, and of folks willing to actually push for it.


I am interested in this, so if Andrew wants to create a buildfarm module
I will either add it to rhinoceros or stand up another buildfarm animal
for it. I am also happy to help push it for v13.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

stawidth inconsistency with all NULL columns

[Joe Conway]

Consider:

CREATE TABLE testwid
(
  txtnotnull text,
  txtnull text,
  int8notnull int8,
  int8null int8
);
INSERT INTO testwid
SELECT 'a' || g.i,
   NULL,
   g.i,
   NULL
FROM generate_series(1,1) AS g(i);
ANALYZE testwid;
SELECT attname, avg_width FROM pg_stats WHERE tablename = 'testwid';
   attname   | avg_width
-+---
 txtnotnull  | 5
 txtnull | 0
 int8notnull | 8
 int8null| 8
(4 rows)


I see in analyze.c
8<-
/* We can only compute average width if we found some non-null values.*/
if (nonnull_cnt > 0)

  [snip]

else if (null_cnt > 0)
{
/* We found only nulls; assume the column is entirely null */
stats->stats_valid = true;
stats->stanullfrac = 1.0;
if (is_varwidth)
stats->stawidth = 0;/* "unknown" */
else
stats->stawidth = stats->attrtype->typlen;
stats->stadistinct = 0.0;   /* "unknown" */
}
8<-

So apparently intentional, but seems gratuitously inconsistent. Could
this cause any actual inconsistent behaviors? In any case that first
comment does not reflect the code.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: stawidth inconsistency with all NULL columns

[Joe Conway]

On 5/21/19 3:55 PM, Tom Lane wrote:
> Joe Conway  writes:
>> else if (null_cnt > 0)
>> {
>> /* We found only nulls; assume the column is entirely null */
>> stats->stats_valid = true;
>> stats->stanullfrac = 1.0;
>> if (is_varwidth)
>> stats->stawidth = 0;/* "unknown" */
>> else
>> stats->stawidth = stats->attrtype->typlen;
>> stats->stadistinct = 0.0;   /* "unknown" */
>> }
>> 8<-
> 
>> So apparently intentional, but seems gratuitously inconsistent. Could
>> this cause any actual inconsistent behaviors? In any case that first
>> comment does not reflect the code.
> 
> Are you suggesting that we should set stawidth to zero even for a
> fixed-width datatype?  That seems pretty silly.  We know exactly what
> the value should be, and would be if we'd chanced to find even one
> non-null entry.

Well you could argue in similar fashion for variable width values -- if
we find even one of those, it will be at least 4 bytes. So why set those
to zero?

Not a big deal, but it struck me as odd when I was looking at the
current state of affairs.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: initdb recommendations

[Joe Conway]

On 5/23/19 10:30 PM, Stephen Frost wrote:
> Greetings,
> 
> * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> "Jonathan S. Katz"  writes:
>> > For now I have left in the password based method to be scram-sha-256 as
>> > I am optimistic about the support across client drivers[1] (and FWIW I
>> > have an implementation for crystal-pg ~60% done).
>> 
>> > However, this probably means we would need to set the default password
>> > encryption guc to "scram-sha-256" which we're not ready to do yet, so it
>> > may be moot to leave it in.
>> 
>> > So, thinking out loud about that, we should probably use "md5" and once
>> > we decide to make the encryption method "scram-sha-256" by default, then
>> > we update the recommendation?
>> 
>> Meh.  If we're going to break things, let's break them.  Set it to
>> scram by default and let people who need to cope with old clients
>> change the default.  I'm tired of explaining that MD5 isn't actually
>> insecure in our usage ...
> 
> +many.

many++

Are we doing this for pg12? In any case, I would think we better loudly
point out this change somewhere.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: initdb recommendations

[Joe Conway]

On 5/24/19 8:13 AM, Stephen Frost wrote:
> Greetings,
> 
> * Joe Conway (m...@joeconway.com) wrote:
>> On 5/23/19 10:30 PM, Stephen Frost wrote:
>> > * Tom Lane (t...@sss.pgh.pa.us) wrote:
>> >> "Jonathan S. Katz"  writes:
>> >> > For now I have left in the password based method to be scram-sha-256 as
>> >> > I am optimistic about the support across client drivers[1] (and FWIW I
>> >> > have an implementation for crystal-pg ~60% done).
>> >> 
>> >> > However, this probably means we would need to set the default password
>> >> > encryption guc to "scram-sha-256" which we're not ready to do yet, so it
>> >> > may be moot to leave it in.
>> >> 
>> >> > So, thinking out loud about that, we should probably use "md5" and once
>> >> > we decide to make the encryption method "scram-sha-256" by default, then
>> >> > we update the recommendation?
>> >> 
>> >> Meh.  If we're going to break things, let's break them.  Set it to
>> >> scram by default and let people who need to cope with old clients
>> >> change the default.  I'm tired of explaining that MD5 isn't actually
>> >> insecure in our usage ...
>> > 
>> > +many.
>> 
>> many++
>> 
>> Are we doing this for pg12? In any case, I would think we better loudly
>> point out this change somewhere.
> 
> Sure, we should point it out, but I don't know that it needs to be
> screamed from the rooftops considering the packagers have already been
> largely ignoring our defaults here anyway...

Yeah, I thought about that, but anyone not using those packages will be
in for a big surprise. Don't get me wrong, I wholeheartedly endorse the
change, but I predict many related questions on the lists, and anything
we can do to mitigate that should be done.

Joe
-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: Excessive memory usage in multi-statement queries w/ partitioning

[Joe Conway]

On 5/24/19 1:47 AM, Amit Langote wrote:
> On 2019/05/23 4:15, Andreas Seltenreich wrote:
>> …but when doing it on the parent relation, even 100 statements are
>> enough to exceed the limit:
>> 
>> ,
>> | $ psql -c "$(yes update t set c=c where c=6 \; | head -n 100)"
>> | FEHLER:  Speicher aufgebraucht
>> | DETAIL:  Failed on request of size 200 in memory context "MessageContext".
>> `
>> 
>> The memory context dump shows plausible values except for the MessageContext:
>> 
>> TopMemoryContext: 124336 total in 8 blocks; 18456 free (11 chunks); 105880 
>> used
>>   [...]
>>   MessageContext: 264241152 total in 42 blocks; 264 free (0 chunks); 
>> 264240888 used
>>   [...]
> 
> As David Rowley said, planning that query hundreds of times under a single
> MessageContext is not something that will end well on 11.3, because even a
> single instance takes up tons of memory that's only released when
> MessageContext is reset.
> 
>> Maybe some tactically placed pfrees or avoiding putting redundant stuff
>> into MessageContext can relax the situation?
> 
> I too have had similar thoughts on the matter.  If the planner had built
> all its subsidiary data structures in its own private context (or tree of
> contexts) which is reset once a plan for a given query is built and passed
> on, then there wouldn't be an issue of all of that subsidiary memory
> leaking into MessageContext.  However, the problem may really be that
> we're subjecting the planner to use cases that it wasn't perhaps designed
> to perform equally well under -- running it many times while handling the
> same message.  It is worsened by the fact that the query in question is
> something that ought to have been documented as not well supported by the
> planner; David has posted a documentation patch for that [1].  PG 12 has
> alleviated the situation to a large degree, so you won't see the OOM
> occurring for this query, but not for all queries unfortunately.


I admittedly haven't followed this thread too closely, but if having 100
partitions causes out of memory on pg11, that sounds like a massive
regression to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: initdb recommendations

[Joe Conway]

On 5/24/19 8:56 AM, Jonathan S. Katz wrote:
> On 5/24/19 8:33 AM, Stephen Frost wrote:
>> * Magnus Hagander (mag...@hagander.net) wrote:
>>> Making the default change away from trust in the source distro will affect
>>> few people.
>> 
>> Agreed.
> 
> +1

Fewer people, but likely disproportionately high representation on pgsql
lists. Anyway, nuff said -- I guess the future will tell one way or the
other.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: Excessive memory usage in multi-statement queries w/ partitioning

[Joe Conway]

On 5/24/19 9:33 AM, David Rowley wrote:
> On Sat, 25 May 2019 at 00:18, Joe Conway  wrote:
>> I admittedly haven't followed this thread too closely, but if having 100
>> partitions causes out of memory on pg11, that sounds like a massive
>> regression to me.
> 
> For it to have regressed it would have had to once have been better,
> but where was that mentioned?  The only thing I saw was
> non-partitioned tables compared to partitioned tables, but you can't
> really say it's a regression if you're comparing apples to oranges.


I have very successfully used multiple hundreds and even low thousands
of partitions without running out of memory under the older inheritance
based "partitioning", and declarative partitioning is supposed to be
(and we have advertised it to be) better, not worse, isn't it?

At least from my point of view if 100 partitions is unusable due to
memory leaks it is a regression. Perhaps not *technically* a regression
assuming it behaves this way in pg10 also, but I bet lots of users would
see it that way.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: Excessive memory usage in multi-statement queries w/ partitioning

[Joe Conway]

On 5/24/19 10:28 AM, Tom Lane wrote:
> Joe Conway  writes:
>> On 5/24/19 9:33 AM, David Rowley wrote:
>>> For it to have regressed it would have had to once have been better,
>>> but where was that mentioned?  The only thing I saw was
>>> non-partitioned tables compared to partitioned tables, but you can't
>>> really say it's a regression if you're comparing apples to oranges.
> 
>> I have very successfully used multiple hundreds and even low thousands
>> of partitions without running out of memory under the older inheritance
>> based "partitioning", and declarative partitioning is supposed to be
>> (and we have advertised it to be) better, not worse, isn't it?
> 
> Have you done the exact thing described in the test case?  I think
> that's going to be quite unpleasantly memory-intensive in any version.


Ok, fair point. Will test and report back.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: New committer: David Rowley

[Joe Conway]

On 5/30/19 11:43 AM, Andres Freund wrote:
> Hi,
> 
> On 2019-05-30 11:39:23 -0400, Magnus Hagander wrote:
>> For those of you that have not read the minutes from the developer meeting
>> ahead of pgcon (can be found at
>> https://wiki.postgresql.org/wiki/PgCon_2019_Developer_Meeting), we'd like
>> to announce here as well that David Rowley has joined the ranks of
>> PostgreSQL committers.
>>
>> Congratulations to David, may the buildfarm be gentle to him, and his first
>> revert far away!
> 
> Congrats!

+1

Congratulations David!

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: pg_config wrongly marked as not parallel safe?

[Joe Conway]

On 11/26/18 6:45 PM, Andres Freund wrote:
> Hi,
> 
> Triggered by the thread at [1] I looked for functions marked as
> immutable but not parallel safe.
> 
> postgres[19492][1]=# SELECT oid::regprocedure, provolatile, proparallel FROM 
> pg_proc WHERE provolatile = 'i' AND proparallel != 's';
> ┌─┬─┬─┐
> │ oid │ provolatile │ proparallel │
> ├─┼─┼─┤
> │ pg_config() │ i   │ r   │
> └─┴─┴─┘
> (1 row)
> 
> # pg_config
> { oid => '3400', descr => 'pg_config binary as a function',
>   proname => 'pg_config', prorows => '23', proretset => 't', proparallel => 
> 'r',
>   prorettype => 'record', proargtypes => '', proallargtypes => '{text,text}',
>   proargmodes => '{o,o}', proargnames => '{name,setting}',
>   prosrc => 'pg_config' },
> 
> so that function is marked as immutable but not parallel safe, without
> an explanation for that odd combination.
> 
> Now obviously I don't think it practially matters for pg_config(), but
> it seems unnecessarily confusing as a general matter.
> 
> I think we probably should fix this specific case, and then add a check
> to opr_sanity.sql or such.
> 
> As far as I can tell pg_config() was marked as such since its addition
> in [2]. Joe, I assume this wasn't intentional?

Not intentional. Though, sitting here chatting with Stephen about it, I
am now wondering if pg_config() should actually be marked immutable:

select * from pg_config() where name = 'VERSION';
  name   | setting
-+-
 VERSION | PostgreSQL 10.5
(1 row)

[...upgrade the postgres binaries...]

select * from pg_config() where name = 'VERSION';
  name   | setting
-+-
 VERSION | PostgreSQL 10.6
(1 row)

So the correct answer is probably to mark pg_config() stable, but it
still seems to be parallel safe to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: pg_config wrongly marked as not parallel safe?

[Joe Conway]

On 11/26/18 7:08 PM, Andres Freund wrote:
> On 2018-11-26 19:04:46 -0500, Joe Conway wrote:
>> Not intentional. Though, sitting here chatting with Stephen about it, I
>> am now wondering if pg_config() should actually be marked immutable:
>>
>> select * from pg_config() where name = 'VERSION';
>>   name   | setting
>> -+-
>>  VERSION | PostgreSQL 10.5
>> (1 row)
>>
>> [...upgrade the postgres binaries...]
>>
>> select * from pg_config() where name = 'VERSION';
>>   name   | setting
>> -+-
>>  VERSION | PostgreSQL 10.6
>> (1 row)
>>
>> So the correct answer is probably to mark pg_config() stable, but it
>> still seems to be parallel safe to me.
> 
> I don't think we should consider immutability to mean anything across
> major versions. What'd be helped by doing that? We'd have to rule out
> any behaviour change to any immutable function for that to make
> sense. Including making an immutable function not immutable anymore.

Umm, this is a minor version not major.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development

Re: pg_config wrongly marked as not parallel safe?

[Joe Conway]

On 11/30/18 3:30 AM, Kyotaro HORIGUCHI wrote:
> # And returning to the topic, I vote for pg_config should be "stable".

And on that note, Does this change does warrant backpatching, or should
be applied to master only?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

missing toast table for pg_policy

[Joe Conway]

Currently if you try to create a too large policy, it fails with:

  ERROR: row is too big: size X, maximum size 8160

An example for reproducing this is attached.

Looking at the issue, the problem seems to be missing toast table for
pg_policy. Also attached is a one line patch. It isn't clear to me
whether this is a candidate for backpatching.

Thoughts?

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index f6387ae143..10348bcbfe 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -49,6 +49,7 @@ extern void BootstrapToastTable(char *relName,
 DECLARE_TOAST(pg_attrdef, 2830, 2831);
 DECLARE_TOAST(pg_constraint, 2832, 2833);
 DECLARE_TOAST(pg_description, 2834, 2835);
+DECLARE_TOAST(pg_policy, 2579, 2580);
 DECLARE_TOAST(pg_proc, 2836, 2837);
 DECLARE_TOAST(pg_rewrite, 2838, 2839);
 DECLARE_TOAST(pg_seclabel, 3598, 3599);


large-policy-error.sql
Description: application/sql


signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Joe Conway]

On 02/16/2018 05:07 PM, Andres Freund wrote:
> Hi,
> 
> On 2018-02-16 16:56:15 -0500, Joe Conway wrote:
>> Looking at the issue, the problem seems to be missing toast table for
>> pg_policy. Also attached is a one line patch. It isn't clear to me
>> whether this is a candidate for backpatching.
> 
> Don't think it is - it'd not take effect on already initdb'ed clusters.

Yep, knew that, but...

> If problematic for < master users I think you'll have to restart cluster
> with allow_system_table_mods, manually create/drop toasted column. IIRC
> that should add a toast table even after dropping.

I wasn't sure if we would want to backpatch and put the manual procedure
steps into the release notes.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Joe Conway]

On 02/16/2018 05:24 PM, Tom Lane wrote:
> Joe Conway  writes:
>> On 02/16/2018 05:07 PM, Andres Freund wrote:
>>> If problematic for < master users I think you'll have to restart cluster
>>> with allow_system_table_mods, manually create/drop toasted column. IIRC
>>> that should add a toast table even after dropping.
> 
>> I wasn't sure if we would want to backpatch and put the manual procedure
>> steps into the release notes.
> 
> The example you give seems like pretty bad practice to me.  I don't think
> we should back-patch unless it's possible to trigger the problem with a
> more realistic policy expression.

Fair enough, but the origin of this was a real life-based complaint.

> (Also, one can always work around it by putting the complicated condition
> into a function, which would likely be a better idea anyway from a
> maintenance standpoint.)

Yes, exactly. I'm fine with not backpatching, just wanted to raise the
possibility. I will push later today to HEAD (with a catalog version bump).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Joe Conway]

On 02/17/2018 11:39 AM, Tom Lane wrote:
> Joe Conway  writes:
>> Yes, exactly. I'm fine with not backpatching, just wanted to raise the
>> possibility. I will push later today to HEAD (with a catalog version bump).
> 
> BTW, I was wondering if it'd be a good idea to try to forestall future
> oversights of this sort by adding a test query in, say, misc_sanity.sql.
> Something like
> 
> select relname, attname, atttypid::regtype
> from pg_class c join pg_attribute a on c.oid = attrelid
> where c.oid < 16384 and reltoastrelid=0 and relkind = 'r' and attstorage != 
> 'p'
> order by 1,2;
> 
> If you try that you'll see the list is quite long:



> I think in most of these cases we've consciously decided not to toast-ify,
> but maybe some of them need a second look.

Is there really a compelling reason to not just create toast tables for
all system catalogs as in the attached? Then we could just check for 0
rows in misc_sanity.sql.

For what its worth:

HEAD

# du -h --max-depth=1 $PGDATA
[...]
22M /usr/local/pgsql-head/data/base
[...]
584K/usr/local/pgsql-head/data/global
[...]
38M /usr/local/pgsql-head/data

time make check
real0m16.295s
user0m3.597s
sys 0m1.465s


with patch

# du -h --max-depth=1 $PGDATA
[...]
23M /usr/local/pgsql-head/data/base
[...]
632K/usr/local/pgsql-head/data/global
[...]
39M /usr/local/pgsql-head/data

time make check
real0m16.462s
user0m3.521s
sys 0m1.531s

select relname, attname, atttypid::regtype
from pg_class c join pg_attribute a on c.oid = attrelid
where c.oid < 16384 and reltoastrelid=0 and relkind = 'r' and attstorage
!= 'p'
order by 1,2;
 relname | attname | atttypid
-+-+--
(0 rows)


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 809749add9..813b3b87cc 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -258,7 +258,19 @@ IsSharedRelation(Oid relationId)
 		relationId == PgDbRoleSettingToastTable ||
 		relationId == PgDbRoleSettingToastIndex ||
 		relationId == PgShseclabelToastTable ||
-		relationId == PgShseclabelToastIndex)
+		relationId == PgShseclabelToastIndex ||
+		relationId == PgAuthidToastTable ||
+		relationId == PgAuthidToastIndex ||
+		relationId == PgDatabaseToastTable ||
+		relationId == PgDatabaseToastIndex ||
+		relationId == PgPlTemplateToastTable ||
+		relationId == PgPlTemplateToastIndex ||
+		relationId == PgReplicationOriginToastTable ||
+		relationId == PgReplicationOriginToastIndex ||
+		relationId == PgSubscriptionToastTable ||
+		relationId == PgSubscriptionToastIndex ||
+		relationId == PgTablespaceToastTable ||
+		relationId == PgTablespaceToastIndex)
 		return true;
 	return false;
 }
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index f6387ae143..3ef3990ea3 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -46,25 +46,64 @@ extern void BootstrapToastTable(char *relName,
  */
 
 /* normal catalogs */
+DECLARE_TOAST(pg_aggregate, 4139, 4140);
 DECLARE_TOAST(pg_attrdef, 2830, 2831);
+DECLARE_TOAST(pg_attribute, 4141, 4142);
+DECLARE_TOAST(pg_class, 4143, 4144);
+DECLARE_TOAST(pg_collation, 4145, 4146);
 DECLARE_TOAST(pg_constraint, 2832, 2833);
+DECLARE_TOAST(pg_default_acl, 4147, 4148);
 DECLARE_TOAST(pg_description, 2834, 2835);
+DECLARE_TOAST(pg_event_trigger, 4149, 4150);
+DECLARE_TOAST(pg_extension, 4151, 4152);
+DECLARE_TOAST(pg_foreign_data_wrapper, 4153, 4154);
+DECLARE_TOAST(pg_foreign_server, 4155, 4156);
+DECLARE_TOAST(pg_foreign_table, 4157, 4158);
+DECLARE_TOAST(pg_index, 4159, 4160);
+DECLARE_TOAST(pg_init_privs, 4161, 4162);
+DECLARE_TOAST(pg_language, 4163, 4164);
+DECLARE_TOAST(pg_largeobject, 4165, 4166);
+DECLARE_TOAST(pg_largeobject_metadata, 4167, 4168);
+DECLARE_TOAST(pg_namespace, 4169, 4170);
+DECLARE_TOAST(pg_partitioned_table, 4171, 4172);
+DECLARE_TOAST(pg_policy, 4173, 4174);
 DECLARE_TOAST(pg_proc, 2836, 2837);
 DECLARE_TOAST(pg_rewrite, 2838, 2839);
 DECLARE_TOAST(pg_seclabel, 3598, 3599);
 DECLARE_TOAST(pg_statistic, 2840, 2841);
 DECLARE_TOAST(pg_statistic_ext, 3439, 3440);
 DECLARE_TOAST(pg_trigger, 2336, 2337);
+DECLARE_TOAST(pg_ts_dict, 4175, 4176);
+DECLARE_TOAST(pg_type, 4177, 4178);
+DECLARE_TOAST(pg_user_mapping, 4179, 4180);
 
 /* shared catalogs */
-DECLARE_TOAST(pg_shdescription, 2846, 2847);
-#define PgShdescriptionToastTable 2846
-#define PgShdescriptionToastIndex 2847
+DECLARE_TOAST(pg_authid, 4181, 4182);
+#define PgAuthidToastTable 4181
+#define PgAuthidToastIndex 4182
+DECLARE_TOAST(pg_database, 4183, 4184);	
+#define PgDatabaseToastTable 4183
+#de

Re: missing toast table for pg_policy

[Joe Conway]

On 02/18/2018 11:18 AM, Tom Lane wrote:
> Joe Conway  writes:
>> Is there really a compelling reason to not just create toast tables for
>> all system catalogs as in the attached?
> 
> What happens when you VACUUM FULL pg_class?  (The associated toast table
> would have to be nonempty for the test to prove much.)

I tried this:
create table foo (id int);
do $$declare i int; begin for i in 1..1000 loop execute 'create user u'
|| i; end loop; end;$$;
do $$declare i int; begin for i in 1..1000 loop execute 'grant all on
foo to u' || i; end loop; end;$$;
vacuum full pg_class;

Worked without issue FWIW.

> I'm fairly suspicious of toasting anything that the toast mechanism itself
> depends on, actually, and that would include at least pg_attribute and
> pg_index as well as pg_class.  Maybe we could get away with it because
> there would never be any actual recursion only potential recursion ...
> but it seems scary.

Well that is the other approach we could pursue -- instead of justifying
which system catalogs need toast tables we could create an exclusion
list of which ones should not have toast tables, with the current
candidates being pg_class, pg_attribute, and pg_index.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: SHA-2 functions

[Joe Conway]

On 02/19/2018 08:43 AM, Peter Eisentraut wrote:
> I also noticed while working on some SSL code that we have perfectly
> good SHA-2 functionality in the server already, but it has no test
> coverage outside the SCRAM tests.
> 
> So I suggest these patches that expose the new functions sha224(),
> sha256(), sha384(), sha512().  That allows us to make the SSL and SCRAM
> tests more robust, and it will allow them to be used in general purpose
> contexts over md5().

I didn't look closely at the patch itself, but +1 on the concept.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: missing toast table for pg_policy

[Joe Conway]

On 02/18/2018 01:33 PM, Joe Conway wrote:
> On 02/18/2018 11:18 AM, Tom Lane wrote:
>> I'm fairly suspicious of toasting anything that the toast mechanism itself
>> depends on, actually, and that would include at least pg_attribute and
>> pg_index as well as pg_class.  Maybe we could get away with it because
>> there would never be any actual recursion only potential recursion ...
>> but it seems scary.
> 
> Well that is the other approach we could pursue -- instead of justifying
> which system catalogs need toast tables we could create an exclusion
> list of which ones should not have toast tables, with the current
> candidates being pg_class, pg_attribute, and pg_index.

The attached does exactly this. Gives all system tables toast tables
except pg_class, pg_attribute, and pg_index, and includes cat version
bump and regression test in misc_sanity.

Any further discussion, comments, complaints?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/catalog/catalog.c b/src/backend/catalog/catalog.c
index 809749add9..813b3b87cc 100644
--- a/src/backend/catalog/catalog.c
+++ b/src/backend/catalog/catalog.c
@@ -258,7 +258,19 @@ IsSharedRelation(Oid relationId)
 		relationId == PgDbRoleSettingToastTable ||
 		relationId == PgDbRoleSettingToastIndex ||
 		relationId == PgShseclabelToastTable ||
-		relationId == PgShseclabelToastIndex)
+		relationId == PgShseclabelToastIndex ||
+		relationId == PgAuthidToastTable ||
+		relationId == PgAuthidToastIndex ||
+		relationId == PgDatabaseToastTable ||
+		relationId == PgDatabaseToastIndex ||
+		relationId == PgPlTemplateToastTable ||
+		relationId == PgPlTemplateToastIndex ||
+		relationId == PgReplicationOriginToastTable ||
+		relationId == PgReplicationOriginToastIndex ||
+		relationId == PgSubscriptionToastTable ||
+		relationId == PgSubscriptionToastIndex ||
+		relationId == PgTablespaceToastTable ||
+		relationId == PgTablespaceToastIndex)
 		return true;
 	return false;
 }
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 433d6db4f6..202072c3c7 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -53,6 +53,6 @@
  */
 
 /*			mmddN */
-#define CATALOG_VERSION_NO	201802061
+#define CATALOG_VERSION_NO	201802191
 
 #endif
diff --git a/src/include/catalog/toasting.h b/src/include/catalog/toasting.h
index f6387ae143..4586ac93b2 100644
--- a/src/include/catalog/toasting.h
+++ b/src/include/catalog/toasting.h
@@ -46,25 +46,61 @@ extern void BootstrapToastTable(char *relName,
  */
 
 /* normal catalogs */
+DECLARE_TOAST(pg_aggregate, 4139, 4140);
 DECLARE_TOAST(pg_attrdef, 2830, 2831);
+DECLARE_TOAST(pg_collation, 4141, 4142);
 DECLARE_TOAST(pg_constraint, 2832, 2833);
+DECLARE_TOAST(pg_default_acl, 4143, 4144);
 DECLARE_TOAST(pg_description, 2834, 2835);
+DECLARE_TOAST(pg_event_trigger, 4145, 4146);
+DECLARE_TOAST(pg_extension, 4147, 4148);
+DECLARE_TOAST(pg_foreign_data_wrapper, 4149, 4150);
+DECLARE_TOAST(pg_foreign_server, 4151, 4152);
+DECLARE_TOAST(pg_foreign_table, 4153, 4154);
+DECLARE_TOAST(pg_init_privs, 4155, 4156);
+DECLARE_TOAST(pg_language, 4157, 4158);
+DECLARE_TOAST(pg_largeobject, 4159, 4160);
+DECLARE_TOAST(pg_largeobject_metadata, 4161, 4162);
+DECLARE_TOAST(pg_namespace, 4163, 4164);
+DECLARE_TOAST(pg_partitioned_table, 4165, 4166);
+DECLARE_TOAST(pg_policy, 4167, 4168);
 DECLARE_TOAST(pg_proc, 2836, 2837);
 DECLARE_TOAST(pg_rewrite, 2838, 2839);
 DECLARE_TOAST(pg_seclabel, 3598, 3599);
 DECLARE_TOAST(pg_statistic, 2840, 2841);
 DECLARE_TOAST(pg_statistic_ext, 3439, 3440);
 DECLARE_TOAST(pg_trigger, 2336, 2337);
+DECLARE_TOAST(pg_ts_dict, 4169, 4170);
+DECLARE_TOAST(pg_type, 4171, 4172);
+DECLARE_TOAST(pg_user_mapping, 4173, 4174);
 
 /* shared catalogs */
-DECLARE_TOAST(pg_shdescription, 2846, 2847);
-#define PgShdescriptionToastTable 2846
-#define PgShdescriptionToastIndex 2847
+DECLARE_TOAST(pg_authid, 4175, 4176);
+#define PgAuthidToastTable 4175
+#define PgAuthidToastIndex 4176
+DECLARE_TOAST(pg_database, 4177, 4178);
+#define PgDatabaseToastTable 4177
+#define PgDatabaseToastIndex 4178
 DECLARE_TOAST(pg_db_role_setting, 2966, 2967);
 #define PgDbRoleSettingToastTable 2966
 #define PgDbRoleSettingToastIndex 2967
+DECLARE_TOAST(pg_pltemplate, 4179, 4180);
+#define PgPlTemplateToastTable 4179
+#define PgPlTemplateToastIndex 4180
+DECLARE_TOAST(pg_replication_origin, 4181, 4182);
+#define PgReplicationOriginToastTable 4181
+#define PgReplicationOriginToastIndex 4182
+DECLARE_TOAST(pg_shdescription, 2846, 2847);
+#define PgShdescriptionToastTable 2846
+#define PgShdescriptionToastIndex 2847
 DECLARE_TOAST(pg_shseclabel, 4060, 4061);
 #define PgShseclabelToastTable 4060
 #define PgShseclabelToastIndex 4061
+DECLARE_TOAST(pg_subscription, 4183, 4184);
+#define PgSubscriptionToastTable 4183
+#define PgSubscriptio

Re: public schema default ACL

[Joe Conway]

On 03/03/2018 01:56 AM, Noah Misch wrote:
> Commit 5770172 ("Document security implications of search_path and the public
> schema.") is largely a workaround for the fact that the boot_val of
> search_path contains "public" while template0 gets "GRANT CREATE, USAGE ON
> SCHEMA public TO PUBLIC".  It's like having world-writable /usr/bin.  The
> security team opted not to change that in released branches, but we thought to
> revisit it later.  I propose, for v11, switching to "GRANT USAGE ON SCHEMA
> public TO PUBLIC" (omit CREATE).  Concerns?

+1. Doing this, or even revoking everything for schema public from
PUBLIC, is already common enough and good practice.

> If we do that alone, databases reaching v11 via dump/reload or pg_upgrade will
> get the new default ACL if they had not changed the ACL of schema public.  If
> they had GRANTed or REVOKEd on schema public, pg_dump will recreate the
> resulting ACL.  This is the standard pg_dump behavior for ACLs on system
> objects.  I think that's okay for the public schema, too, and I like
> preserving that usual rule.  However, if we wanted to minimize upgrade-time
> surprises, we could make pg_dump include GRANT for schema public
> unconditionally.  That way, the default ACL change would apply to new
> databases only.  Does anyone want to argue for that?

What about a pg_dump option to do that and then a big note in the
release notes telling people why they might want to use it?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: postgres_fdw: perform UPDATE/DELETE .. RETURNING on a join directly

[Joe Conway]

On 03/05/2018 11:19 AM, Tom Lane wrote:
> Joe, I wonder if you could add "log_autovacuum_min_duration = 0" to
> rhinoceros' extra_config options, temporarily?  Correlating that log
> output with the log_statement output from the test proper would let
> us confirm or deny whether it's autovacuum.


Done just now. Do you want me to force a run?


Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: postgres_fdw: perform UPDATE/DELETE .. RETURNING on a join directly

[Joe Conway]

On 03/05/2018 02:07 PM, Tom Lane wrote:
> So you can revert the rhinoceros config change if you like --- thanks
> for making it so quickly!

Ok, reverted.

> Meanwhile, I'm back to wondering what could possibly have affected
> the planner's estimates, if pg_proc and pg_statistic didn't change.
> I confess bafflement ... but we've now eliminated the autovacuum-
> did-it theory entirely, so it's time to start looking someplace else.
> I wonder if something in the postgres_fdw remote join machinery
> is not as deterministic as it should be.

Do you want me to do anything manual locally on this VM?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: System username in pg_stat_activity

[Joe Conway]

On 1/10/24 08:59, Magnus Hagander wrote:

On Wed, Jan 10, 2024 at 2:56 PM Bertrand Drouvot

I think it depends what we want the new field to reflect. If it is the exact
same thing as the SYSTEM_USER then I think it has to be text (as the SYSTEM_USER
is made of "auth_method:identity"). Now if we want it to be "only" the identity
part of it, then the `name` datatype would be fine. I'd vote for the exact same
thing as the SYSTEM_USER (means auth_method:identity).


I definitely think it should be the same. If it's not exactly the
same, then it should be *two* columns, one with auth method and one
with the name.

And thinking more about it maybe that's cleaner, because that makes it
easier to do things like filter based on auth method?


+1 for the overall feature and +1 for two columns


> + 
> +  
> +   authname name
> +  
> +  
> +   The authentication method and identity (if any) that the user
> +   used to log in. It contains the same value as
> +returns in the backend.
> +  
> + 

I'm fine with auth_method:identity.

> +S.authname,

What about using system_user as the field name? (because if we keep
auth_method:identity it's not really the authname anyway).


I was worried system_user or sysuser would both be confusing with the
fact that we have usesysid -- which would reference a *different*
sys...



I think if it is exactly "system_user" it would be pretty clearly a 
match for SYSTEM_USER



--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: [17] CREATE SUBSCRIPTION ... SERVER

[Joe Conway]

On 1/12/24 20:17, Jeff Davis wrote:

On Fri, 2024-01-05 at 16:11 +0530, Ashutosh Bapat wrote:

I don't think we need to add a test for every FDW. E.g. adding a test
in file_fdw would be pointless. But postgres_fdw is special. The test
could further create a foreign table ftab_foo on subscriber
referencing foo on publisher and then compare the data from foo and
ftab_foo to make sure that the replication is happening. This will
serve as a good starting point for replicated tables setup in a
sharded cluster.



Attached updated patch set with added TAP test for postgres_fdw, which
uses a postgres_fdw server as the source for subscription connection
information.

I think 0004 needs a bit more work, so I'm leaving it off for now, but
I'll bring it back in the next patch set.


I took a quick scan through the patch. The only thing that jumped out at 
me was that it seems like it might make sense to use 
quote_literal_cstr() rather than defining your own appendEscapedValue() 
function?


--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: pgjdbc is not working with PKCS8 certificates with password

[Joe Conway]

On 2/7/24 06:42, just madhu wrote:

On further investigation,
/With certificate generated as below. JDBC connection is successful./
openssl pkcs8 -topk8 -inform PEM -in client.key -outform DER -out 
client.pk8  -passout pass:foobar / -v1 PBE-MD5-DES


But a connection from pgAdmin (connection failed: 
\SSLCerts\pk8_pass\client_pass_PBE.pk8": no start line) and psql(psql: 
error: could not load private key file "client_pass_PBE.pk8": 
unsupported) is failing


Is there a common way in which certificate with passwords can be 
created  for both libpq and jdbc ?



You may want to check with the pgjdbc project on github rather than (or 
in addition to?) here; see:


  https://github.com/pgjdbc/pgjdbc/issues

Joe

On Wed, Feb 7, 2024 at 3:17 PM just madhu <mailto:justvma...@gmail.com>> wrote:


Hi ,

postgresql-42.7.1.jar

Trying to use establish a connection using PKCS8 certificate created
with password.

/openssl pkcs8 -topk8 -inform PEM -in client.key -outform DER -out
client.pk8  -passout pass:foobar
/

I set the properties as below:
/.../
/sslProperties.setProperty("sslkey", "client.pk8");
sslProperties.setProperty("sslpassword","foobar");/
/.../
/Connection connection = DriverManager.getConnection(jdbcUrl,
sslProperties);
/
//
/This is failing with the error:/
/org.postgresql.util.PSQLException: SSL error: Connection reset
at org.postgresql.ssl.MakeSSL.convert(MakeSSL.java:43)
at

org.postgresql.core.v3.ConnectionFactoryImpl.enableSSL(ConnectionFactoryImpl.java:584)
at

org.postgresql.core.v3.ConnectionFactoryImpl.tryConnect(ConnectionFactoryImpl.java:168)
/
/.../

Regards,
Madhu



--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Non-superuser subscription owners

[Joe Conway]

On 2/22/23 14:12, Mark Dilger wrote:

On Feb 22, 2023, at 10:49 AM, Jeff Davis  wrote:
On Wed, 2023-02-22 at 09:27 -0800, Mark Dilger wrote:

Another option is to execute under the intersection of their
privileges, where both the definer and the invoker need the
privileges in order for the action to succeed.  That would be more
permissive than the proposed SECURITY NONE, while still preventing
either party from hijacking privileges of the other.


Interesting idea, I haven't heard of something like that being done
before. Is there some precedent for that or a use case where it's
helpful?

 > No current use case comes to mind, but I proposed it for event
triggers one or two development cycles ago, to allow for
non-superuser event trigger owners.  The problems associated with
allowing non-superusers to create and own event triggers were pretty
similar to the problems being discussed in this thread.



The intersection of privileges is used, for example, in multi-level 
security contexts where the intersection of the network-allowed levels 
and the subject allowed levels is used to bracket what can be accessed 
and how.


Other examples I found with a quick search:

https://docs.oracle.com/javase/8/docs/api/java/security/AccessController.html#doPrivileged-java.security.PrivilegedAction-java.security.AccessControlContext-

https://learn.microsoft.com/en-us/dotnet/api/system.security.permissions.dataprotectionpermission.intersect?view=dotnet-plat-ext-7.0


--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Improving inferred query column names

[Joe Conway]

On 2/22/23 23:03, Tom Lane wrote:

Andres Freund  writes:

We could just do something like printing __. So
something like avg(reltuples / relpages) would end up as
avg_reltuples_float48div_relpages.
Whether that's worth it, or whether column name lengths would be too painful,
IDK.


I think you'd soon be hitting NAMEDATALEN limits ...




Probably an unpalatable idea, but if we did something like 
md5('avg(reltuples / relpages)') for the column name, it would be 
(reasonably) unique and deterministic. Not pretty, but possibly useful 
in some cases.




--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Possibility to disable `ALTER SYSTEM`

[Joe Conway]

On 3/19/24 07:49, Andrew Dunstan wrote:



On Tue, Mar 19, 2024 at 5:26 AM Heikki Linnakangas <mailto:hlinn...@iki.fi>> wrote:


I want to remind everyone of this from Gabriele's first message that
started this thread:

 > At the moment, a possible workaround is that `ALTER SYSTEM` can
be blocked
 > by making the postgresql.auto.conf read only, but the returned
message is
 > misleading and that’s certainly bad user experience (which is very
 > important in a cloud native environment):
 >
 >
 > ```
 > postgres=# ALTER SYSTEM SET wal_level TO minimal;
 > ERROR:  could not open file "postgresql.auto.conf": Permission denied
 > ```

I think making the config file read-only is a fine solution. If you
don't want postgres to mess with the config files, forbid it with the
permission system.

Problems with pg_rewind, pg_basebackup were mentioned with that
approach. I think if you want the config files to be managed outside
PostgreSQL, by kubernetes, patroni or whatever, it would be good for
them to be read-only to the postgres user anyway, even if we had a
mechanism to disable ALTER SYSTEM. So it would be good to fix the
problems with those tools anyway.

The error message is not great, I agree with that. Can we improve it?
Maybe just add a HINT like this:

postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR:  could not open file "postgresql.auto.conf" for writing:
Permission denied
HINT:  Configuration might be managed outside PostgreSQL


Perhaps we could make that even better with a GUC though. I propose a
GUC called 'configuration_managed_externally = true / false". If you
set
it to true, we prevent ALTER SYSTEM and make the error message more
definitive:

postgres=# ALTER SYSTEM SET wal_level TO minimal;
ERROR:  configuration is managed externally

As a bonus, if that GUC is set, we could even check at server startup
that all the configuration files are not writable by the postgres user,
and print a warning or refuse to start up if they are.

(Another way to read this proposal is to rename the GUC that's been
discussed in this thread to 'configuration_managed_externally'. That
makes it look less like a security feature, and describes the intended
use case.)




I agree with pretty much all of this.



+1 me too.

--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com

Re: Popcount optimization using AVX512

[Joe Conway]

On 3/25/24 11:12, Tom Lane wrote:

"Amonson, Paul D"  writes:

I am re-posting the patches as CI for Mac failed (CI error not code/test 
error). The patches are the same as last time.


Just for a note --- the cfbot will re-test existing patches every
so often without needing a bump.  The current cycle period seems to
be about two days.



Just an FYI -- there seems to be an issue with all three of the macos 
cfbot runners (mine included). I spent time over the weekend working 
with Thomas Munro (added to CC list) trying different fixes to no avail. 
Help from macos CI wizards would be gratefully accepted...


--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com