Thread: My first patch! (to \df output)
Hello Hackers! So, currently the only way to see if a function is security definer or not is to directly query pg_proc. This is both irritating,and I think perhaps dangerous since security definer functions can be so powerful. I thought that rectifyingthat would make an excellent first patch, and I was bored today here in Prague since pgconf.eu is now over...sohere it is. :) This patch adds a column to the output of \df titled "Security" with values of "definer" or "invoker" based on the booleansecdef column from pg_proc. I've also included a small doc patch to match. This patch is against master from git.Comments welcome! I just realized I didn't address regression tests, so I guess this is not actually complete yet. I should have time for thatnext week after I get back to the states. I would also like to start discussion about perhaps adding a couple more things to \df+, specifically function executionpermissions (which are also exposed nowhere outside the catalog to my knowledge), and maybe search_path since that'srelated to secdef. Thoughts? This was actually kind of anti-climactic, since it only took about 5 minutes to make the change and get it working. Didn'treally feel the way I expected it to ;) -- Jon T Erdman Postgresql Zealot
Attachment
Hello 2012/10/27 Jon Erdman <postgresql@thewickedtribe.net>: > > Hello Hackers! > > So, currently the only way to see if a function is security definer or not is to directly query pg_proc. This is both irritating,and I think perhaps dangerous since security definer functions can be so powerful. I thought that rectifyingthat would make an excellent first patch, and I was bored today here in Prague since pgconf.eu is now over...sohere it is. :) > > This patch adds a column to the output of \df titled "Security" with values of "definer" or "invoker" based on the booleansecdef column from pg_proc. I've also included a small doc patch to match. This patch is against master from git.Comments welcome! > > I just realized I didn't address regression tests, so I guess this is not actually complete yet. I should have time forthat next week after I get back to the states. > > I would also like to start discussion about perhaps adding a couple more things to \df+, specifically function executionpermissions (which are also exposed nowhere outside the catalog to my knowledge), and maybe search_path since that'srelated to secdef. Thoughts? I prefer show this in \dt+ for column "Security" - and for other functionality maybe new statement. > > This was actually kind of anti-climactic, since it only took about 5 minutes to make the change and get it working. Didn'treally feel the way I expected it to ;) > :) yes, hacking is funny Regards Pavel > > > -- > Jon T Erdman > Postgresql Zealot > > > > > > > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers >
-----BEGIN PGP SIGNED MESSAGE----- Hash: RIPEMD160 > This was actually kind of anti-climactic, since it only > took about 5 minutes to make the change and get it > working. Didn't really feel the way I expected it to ;) Well, we can reject your patch and start bike-shedding it for the next four months, if that makes you feel better! :) Congrats! - -- Greg Sabino Mullane greg@turnstep.com PGP Key: 0x14964AC8 201210271914 http://biglumber.com/x/web?pk=2529DF6AB8F79407E94445B4BC9B906714964AC8 -----BEGIN PGP SIGNATURE----- iEYEAREDAAYFAlCMau4ACgkQvJuQZxSWSshdoQCg6eJ14LLcJrn04rN2/efO14iz swgAoPbBSv8PAre6qtVrRH3LL/iNQqeD =m/ns -----END PGP SIGNATURE-----
On Oct 27, 2012, at 10:45 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote:
> Hello
>
> 2012/10/27 Jon Erdman <postgresql@thewickedtribe.net>:
>>
>> Hello Hackers!
>>
>> So, currently the only way to see if a function is security definer or not is to directly query pg_proc. This is
bothirritating, and I think perhaps dangerous since security definer functions can be so powerful. I thought that
rectifyingthat would make an excellent first patch, and I was bored today here in Prague since pgconf.eu is now
over...sohere it is. :)
>>
>> This patch adds a column to the output of \df titled "Security" with values of "definer" or "invoker" based on the
booleansecdef column from pg_proc. I've also included a small doc patch to match. This patch is against master from
git.Comments welcome!
>>
>> I just realized I didn't address regression tests, so I guess this is not actually complete yet. I should have time
forthat next week after I get back to the states.
>>
>> I would also like to start discussion about perhaps adding a couple more things to \df+, specifically function
executionpermissions (which are also exposed nowhere outside the catalog to my knowledge), and maybe search_path since
that'srelated to secdef. Thoughts?
>
> I prefer show this in \dt+ for column "Security" - and for other
> functionality maybe new statement.
I'm assuming you meant "\df+", and I've changed it accordingly. With this change there is now nothing to change in the
regressiontests, so please consider this my formal and complete submission.
Is there anything else I need to do to get this considered?
Oh, in case anyone is interested, here's what the query now looks like and the new output:
jerdman=# \df+ public.akeys
********* QUERY **********
SELECT n.nspname as "Schema",
p.proname as "Name",
pg_catalog.pg_get_function_result(p.oid) as "Result data type",
pg_catalog.pg_get_function_arguments(p.oid) as "Argument data types",
CASE
WHEN p.proisagg THEN 'agg'
WHEN p.proiswindow THEN 'window'
WHEN p.prorettype = 'pg_catalog.trigger'::pg_catalog.regtype THEN 'trigger'
ELSE 'normal'
END as "Type",
CASE
WHEN prosecdef THEN 'definer'
ELSE 'invoker'
END AS "Security",
CASE
WHEN p.provolatile = 'i' THEN 'immutable'
WHEN p.provolatile = 's' THEN 'stable'
WHEN p.provolatile = 'v' THEN 'volatile'
END as "Volatility",
pg_catalog.pg_get_userbyid(p.proowner) as "Owner",
l.lanname as "Language",
p.prosrc as "Source code",
pg_catalog.obj_description(p.oid, 'pg_proc') as "Description"
FROM pg_catalog.pg_proc p
LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace
LEFT JOIN pg_catalog.pg_language l ON l.oid = p.prolang
WHERE p.proname ~ '^(akeys)$'
AND n.nspname ~ '^(public)$'
ORDER BY 1, 2, 4;
**************************
List of functions
Schema | Name | Result data type | Argument data types | Type | Security | Volatility | Owner | Language | Source
code | Description
--------+-------+------------------+---------------------+--------+----------+------------+---------+----------+--------------+-------------
public | akeys | text[] | hstore | normal | invoker | immutable | jerdman | c |
hstore_akeys|
(1 row)
--
Jon T Erdman
Postgresql Zealot
Attachment
Oops! Here it is in the proper diff format. I didn't have my env set up correctly :( -- Jon T Erdman Postgresql Zealot On Nov 9, 2012, at 1:53 PM, Jon Erdman <postgresql@thewickedtribe.net> wrote: > On Oct 27, 2012, at 10:45 AM, Pavel Stehule <pavel.stehule@gmail.com> wrote: > >> Hello >> >> 2012/10/27 Jon Erdman <postgresql@thewickedtribe.net>: >>> >>> Hello Hackers! >>> >>> So, currently the only way to see if a function is security definer or not is to directly query pg_proc. This is bothirritating, and I think perhaps dangerous since security definer functions can be so powerful. I thought that rectifyingthat would make an excellent first patch, and I was bored today here in Prague since pgconf.eu is now over...sohere it is. :) >>> >>> This patch adds a column to the output of \df titled "Security" with values of "definer" or "invoker" based on the booleansecdef column from pg_proc. I've also included a small doc patch to match. This patch is against master from git.Comments welcome! >>> >>> I just realized I didn't address regression tests, so I guess this is not actually complete yet. I should have time forthat next week after I get back to the states. >>> >>> I would also like to start discussion about perhaps adding a couple more things to \df+, specifically function executionpermissions (which are also exposed nowhere outside the catalog to my knowledge), and maybe search_path since that'srelated to secdef. Thoughts? >> >> I prefer show this in \dt+ for column "Security" - and for other >> functionality maybe new statement. > > I'm assuming you meant "\df+", and I've changed it accordingly. With this change there is now nothing to change in theregression tests, so please consider this my formal and complete submission. <describe.patch> > > Is there anything else I need to do to get this considered? > > Oh, in case anyone is interested, here's what the query now looks like and the new output: > > jerdman=# \df+ public.akeys > ********* QUERY ********** > SELECT n.nspname as "Schema", > p.proname as "Name", > pg_catalog.pg_get_function_result(p.oid) as "Result data type", > pg_catalog.pg_get_function_arguments(p.oid) as "Argument data types", > CASE > WHEN p.proisagg THEN 'agg' > WHEN p.proiswindow THEN 'window' > WHEN p.prorettype = 'pg_catalog.trigger'::pg_catalog.regtype THEN 'trigger' > ELSE 'normal' > END as "Type", > CASE > WHEN prosecdef THEN 'definer' > ELSE 'invoker' > END AS "Security", > CASE > WHEN p.provolatile = 'i' THEN 'immutable' > WHEN p.provolatile = 's' THEN 'stable' > WHEN p.provolatile = 'v' THEN 'volatile' > END as "Volatility", > pg_catalog.pg_get_userbyid(p.proowner) as "Owner", > l.lanname as "Language", > p.prosrc as "Source code", > pg_catalog.obj_description(p.oid, 'pg_proc') as "Description" > FROM pg_catalog.pg_proc p > LEFT JOIN pg_catalog.pg_namespace n ON n.oid = p.pronamespace > LEFT JOIN pg_catalog.pg_language l ON l.oid = p.prolang > WHERE p.proname ~ '^(akeys)$' > AND n.nspname ~ '^(public)$' > ORDER BY 1, 2, 4; > ************************** > > List of functions > Schema | Name | Result data type | Argument data types | Type | Security | Volatility | Owner | Language | Sourcecode | Description > --------+-------+------------------+---------------------+--------+----------+------------+---------+----------+--------------+------------- > public | akeys | text[] | hstore | normal | invoker | immutable | jerdman | c | hstore_akeys| > (1 row) > > -- > Jon T Erdman > Postgresql Zealot > >
Attachment
On Fri, Nov 9, 2012 at 3:22 PM, Jon Erdman <postgresql@thewickedtribe.net> wrote: > Oops! Here it is in the proper diff format. I didn't have my env set up correctly :( Better add it here so it doesn't get lost: https://commitfest.postgresql.org/action/commitfest_view/open -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
* Jon Erdman (postgresql@thewickedtribe.net) wrote:
> Oops! Here it is in the proper diff format. I didn't have my env set up correctly :(
No biggie, and to get the bike-shedding started, I don't really like the
column name or the values.. :) I feel like something clearer would be
"Runs_As" with "caller" or "owner".. Saying "Security" makes me think
of ACLs more than what user ID the function runs as, to be honest.
Looking at the actual patch itself, it looks like you have some
unecessary whitespace changes included..?
Thanks!
Stephen
On Sat, Dec 29, 2012 at 1:56 PM, Stephen Frost <sfrost@snowman.net> wrote: > * Jon Erdman (postgresql@thewickedtribe.net) wrote: >> Oops! Here it is in the proper diff format. I didn't have my env set up correctly :( > > No biggie, and to get the bike-shedding started, I don't really like the > column name or the values.. :) I feel like something clearer would be > "Runs_As" with "caller" or "owner".. Saying "Security" makes me think > of ACLs more than what user ID the function runs as, to be honest. > > Looking at the actual patch itself, it looks like you have some > unecessary whitespace changes included..? > > Thanks! > > Stephen Stephen, I think Jon's column name and values make a lot of sense. That being said, I do agree with your point of making it clearer for the person viewing the output, I just don't know if it would be confusing when they wanted to change it or were trying to understand how it related. Agree on the extra spaces in the docs. Jon, I think you inserted your changes improperly in the docs. The classifications apply to the type, not to security. Also, you need to use the %s place holder and the gettext_noop() call for your values as well as your column name. Compiles and tests ok. Results look as expected.
I did realize that since I moved it to + the doc should change, but I didn't address that. I'll get on it this weekend. As far as the column name and displayed values go, they're taken from the CREATE FUNCTION syntax, and were recommended byMagnus, Bruce, and Fetter, who were all sitting next to me day after pgconf.eu Prague. I personally have no strong feelingseither way, I just want to be able to see the info without having to directly query pg_proc. Whatever you all agreeon is fine by me. -- Jon T Erdman Chief Information Officer voice: (312) 285-6735 Progressive Practice, Inc. jon@progressivepractice.com P.O. Box 17288 www.progressivepractice.com Rochester, NY 14617 On Jan 18, 2013, at 5:51 PM, Phil Sorber <phil@omniti.com> wrote: > On Sat, Dec 29, 2012 at 1:56 PM, Stephen Frost <sfrost@snowman.net> wrote: >> * Jon Erdman (postgresql@thewickedtribe.net) wrote: >>> Oops! Here it is in the proper diff format. I didn't have my env set up correctly :( >> >> No biggie, and to get the bike-shedding started, I don't really like the >> column name or the values.. :) I feel like something clearer would be >> "Runs_As" with "caller" or "owner".. Saying "Security" makes me think >> of ACLs more than what user ID the function runs as, to be honest. >> >> Looking at the actual patch itself, it looks like you have some >> unecessary whitespace changes included..? >> >> Thanks! >> >> Stephen > > Stephen, I think Jon's column name and values make a lot of sense. > That being said, I do agree with your point of making it clearer for > the person viewing the output, I just don't know if it would be > confusing when they wanted to change it or were trying to understand > how it related. > > Agree on the extra spaces in the docs. > > Jon, I think you inserted your changes improperly in the docs. The > classifications apply to the type, not to security. > > Also, you need to use the %s place holder and the gettext_noop() call > for your values as well as your column name. > > Compiles and tests ok. Results look as expected.
<div dir="ltr"><p>On Jan 19, 2013 10:55 AM, "Jon Erdman" <<a href="mailto:postgresql@thewickedtribe.net" target="_blank">postgresql@thewickedtribe.net</a>>wrote:<br /> ><br /> ><br /> > I did realize that since I movedit to + the doc should change, but I didn't address that. I'll get on it this weekend.<br /> ><br /> > As faras the column name and displayed values go, they're taken from the CREATE FUNCTION syntax, and were recommended by Magnus,Bruce, and Fetter, who were all sitting next to me day after <a href="http://pgconf.eu" target="_blank">pgconf.eu</a>Prague. I personally have no strong feelings either way, I just want to be able to see the infowithout having to directly query pg_proc. Whatever you all agree on is fine by me.<p>Sounds like you have a +4/-1 onthe names then. I'd keep them as is.</div>
* Phil Sorber (phil@omniti.com) wrote:
> Stephen, I think Jon's column name and values make a lot of sense.
a'ight. I can't think of anything better.
Thanks,
Stephen
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Updated the patch in commitfest with the doc change, and added a
comment to explain the whitespace change (it was to clean up the sql
indentation). I've also attached the new patch here for reference.
- --
Jon T Erdman (aka StuckMojo)
PostgreSQL Zealot
On 01/20/2013 08:27 PM, Craig Ringer wrote:
> On 01/19/2013 11:54 PM, Jon Erdman wrote:
>> I did realize that since I moved it to + the doc should change,
>> but I didn't address that. I'll get on it this weekend.
> Held as waiting on author, then. Please update
> https://commitfest.postgresql.org/action/patch_view?id=1008 when
> you post a new revision.
>
> -- Craig Ringer http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Training & Services
>
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with undefined - http://www.enigmail.net/
iEYEARECAAYFAlD/cNYACgkQRAk1+p0GhSGwJQCfa+8SbL9cYHZkqfmlRlgqcXf9
qD4AnjSZwSXQmOMK8thSs6CdiDxQkJCJ
=H+km
-----END PGP SIGNATURE-----
Attachment
On Wed, Jan 23, 2013 at 12:10 AM, Jon Erdman <postgresql@thewickedtribe.net> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > > Updated the patch in commitfest with the doc change, and added a > comment to explain the whitespace change (it was to clean up the sql > indentation). I've also attached the new patch here for reference. Docs looks good. Spaces gone. Still need to replace 'definer' and 'invoker' with %s and add the corresponding gettext_noop() calls I think. > - -- > Jon T Erdman (aka StuckMojo) > PostgreSQL Zealot > > On 01/20/2013 08:27 PM, Craig Ringer wrote: >> On 01/19/2013 11:54 PM, Jon Erdman wrote: >>> I did realize that since I moved it to + the doc should change, >>> but I didn't address that. I'll get on it this weekend. >> Held as waiting on author, then. Please update >> https://commitfest.postgresql.org/action/patch_view?id=1008 when >> you post a new revision. >> >> -- Craig Ringer http://www.2ndQuadrant.com/ >> PostgreSQL Development, 24x7 Support, Training & Services >> > -----BEGIN PGP SIGNATURE----- > Comment: Using GnuPG with undefined - http://www.enigmail.net/ > > iEYEARECAAYFAlD/cNYACgkQRAk1+p0GhSGwJQCfa+8SbL9cYHZkqfmlRlgqcXf9 > qD4AnjSZwSXQmOMK8thSs6CdiDxQkJCJ > =H+km > -----END PGP SIGNATURE-----
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Done. Attached.
- --
Jon T Erdman (aka StuckMojo)
PostgreSQL Zealot
On 01/22/2013 11:17 PM, Phil Sorber wrote:
> On Wed, Jan 23, 2013 at 12:10 AM, Jon Erdman
> <postgresql@thewickedtribe.net> wrote:
>
> Updated the patch in commitfest with the doc change, and added a
> comment to explain the whitespace change (it was to clean up the
> sql indentation). I've also attached the new patch here for
> reference.
>
>> Docs looks good. Spaces gone.
>
>> Still need to replace 'definer' and 'invoker' with %s and add
>> the corresponding gettext_noop() calls I think.
>
-----BEGIN PGP SIGNATURE-----
Comment: Using GnuPG with undefined - http://www.enigmail.net/
iEYEARECAAYFAlD/dcoACgkQRAk1+p0GhSEKHQCZAW8UNqSjYxBgBvt2nuffrkrV
+9AAn1hChpY5Jg8G8T3XmlIb+3VUSEQ2
=3cFD
-----END PGP SIGNATURE-----
Attachment
On Wed, Jan 23, 2013 at 12:31 AM, Jon Erdman <postgresql@thewickedtribe.net> wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > > Done. Attached. > - -- > Jon T Erdman (aka StuckMojo) > PostgreSQL Zealot > > On 01/22/2013 11:17 PM, Phil Sorber wrote: >> On Wed, Jan 23, 2013 at 12:10 AM, Jon Erdman >> <postgresql@thewickedtribe.net> wrote: >> >> Updated the patch in commitfest with the doc change, and added a >> comment to explain the whitespace change (it was to clean up the >> sql indentation). I've also attached the new patch here for >> reference. >> >>> Docs looks good. Spaces gone. >> >>> Still need to replace 'definer' and 'invoker' with %s and add >>> the corresponding gettext_noop() calls I think. >> This looks good to me now. Compiles and works as described. One thing I would note for the future though, when updating a patch, add a version to the file name to distinguish it from older versions of the patch. > -----BEGIN PGP SIGNATURE----- > Comment: Using GnuPG with undefined - http://www.enigmail.net/ > > iEYEARECAAYFAlD/dcoACgkQRAk1+p0GhSEKHQCZAW8UNqSjYxBgBvt2nuffrkrV > +9AAn1hChpY5Jg8G8T3XmlIb+3VUSEQ2 > =3cFD > -----END PGP SIGNATURE-----
On 01/24/2013 01:50 AM, Phil Sorber wrote:<br /><span style="white-space: pre;">> This looks good to me now. Compilesand works as described.</span><br /> Ready to go?<br /><br /><a class="moz-txt-link-freetext" href="https://commitfest.postgresql.org/action/patch_view?id=1008">https://commitfest.postgresql.org/action/patch_view?id=1008</a><br /><br/> -- <br /> Craig Ringer <a class="moz-txt-link-freetext" href="http://www.2ndQuadrant.com/">http://www.2ndQuadrant.com/</a><br/> PostgreSQL Development, 24x7 Support, Training &Services<br /><br />
On Thu, Jan 24, 2013 at 2:27 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > On 01/24/2013 01:50 AM, Phil Sorber wrote: >> This looks good to me now. Compiles and works as described. > Ready to go? > > https://commitfest.postgresql.org/action/patch_view?id=1008 > I guess I wasn't ready to be so bold, but sure. :) I changed it to 'ready for committer'. > > -- > Craig Ringer http://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Training & Services >
On 23.01.2013 07:31, Jon Erdman wrote: > Done. Attached. Thanks, committed. On 29.12.2012 20:56, Stephen Frost wrote:> No biggie, and to get the bike-shedding started, I don't really like the> columnname or the values.. :) I feel like something clearer would be> "Runs_As" with "caller" or "owner".. Saying "Security"makes me think> of ACLs more than what user ID the function runs as, to be honest. I have to agree that calling the property "security definer/invoker" is a poor name in general. "security" is such on overloaded word that it could mean anything. "Run as" would make a lot more sense. But given that that's the nomenclature we have in the CREATE FUNCTION statement, the docs, prosecdef column name and everywhere, that's what we have to call it in \df+ too. - Heikki