Re: 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 rectifying that would make an excellent first patch, and I was bored today here in Prague since pgconf.eu is now over...so here it is. :)

This patch adds a column to the output of \df titled "Security" with values of "definer" or "invoker" based on the boolean secdef 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 that 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 execution permissions (which are also exposed nowhere outside the catalog to my knowledge), and maybe search_path since that's related 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't really feel the way I expected it to ;)

Hello

2012/10/27 Jon Erdman <postgresql(at)thewickedtribe(dot)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 rectifying that would make an excellent first patch, and I was bored today here in Prague since pgconf.eu is now over...so here it is. :)
>
> This patch adds a column to the output of \df titled "Security" with values of "definer" or "invoker" based on the boolean secdef 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 that 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 execution permissions (which are also exposed nowhere outside the catalog to my knowledge), and maybe search_path since that's related 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't really 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(at)postgresql(dot)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(at)turnstep(dot)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(dot)stehule(at)gmail(dot)com> wrote:

> Hello
>
> 2012/10/27 Jon Erdman <postgresql(at)thewickedtribe(dot)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 rectifying that would make an excellent first patch, and I was bored today here in Prague since pgconf.eu is now over...so here it is. :)
>>
>> This patch adds a column to the output of \df titled "Security" with values of "definer" or "invoker" based on the boolean secdef 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 that 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 execution permissions (which are also exposed nowhere outside the catalog to my knowledge), and maybe search_path since that's related 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 regression tests, so please consider this my formal and complete submission.

Oops! Here it is in the proper diff format. I didn't have my env set up correctly :(

On Fri, Nov 9, 2012 at 3:22 PM, Jon Erdman
<postgresql(at)thewickedtribe(dot)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(at)thewickedtribe(dot)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(at)snowman(dot)net> wrote:
> * Jon Erdman (postgresql(at)thewickedtribe(dot)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 by Magnus, Bruce, and Fetter, who were all sitting next to me day after pgconf.eu Prague. I personally have no strong feelings either way, I just want to be able to see the info without having to directly query pg_proc. Whatever you all agree on is fine by me.
--
Jon T Erdman

Chief Information Officer voice: (312) 285-6735
Progressive Practice, Inc. jon(at)progressivepractice(dot)com
P.O. Box 17288 www.progressivepractice.com
Rochester, NY 14617

On Jan 18, 2013, at 5:51 PM, Phil Sorber <phil(at)omniti(dot)com> wrote:

> On Sat, Dec 29, 2012 at 1:56 PM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
>> * Jon Erdman (postgresql(at)thewickedtribe(dot)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.

On Jan 19, 2013 10:55 AM, "Jon Erdman" <postgresql(at)thewickedtribe(dot)net>
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.
>
> As far as 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 pgconf.eu Prague. I personally
have no strong feelings either way, I just want to be able to see the info
without having to directly query pg_proc. Whatever you all agree on is fine
by me.

Sounds like you have a +4/-1 on the names then. I'd keep them as is.

* Phil Sorber (phil(at)omniti(dot)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-----

On Wed, Jan 23, 2013 at 12:10 AM, Jon Erdman
<postgresql(at)thewickedtribe(dot)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(at)thewickedtribe(dot)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-----

On Wed, Jan 23, 2013 at 12:31 AM, Jon Erdman
<postgresql(at)thewickedtribe(dot)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(at)thewickedtribe(dot)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:
> This looks good to me now. Compiles and works as described.
Ready to go?

https://commitfest.postgresql.org/action/patch_view?id=1008

--
Craig Ringer http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Jan 24, 2013 at 2:27 AM, Craig Ringer <craig(at)2ndquadrant(dot)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
> 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.

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