Horton
Menu

Thread

  1. My first patch! (to \df output)

    postgresql@thewickedtribe.net — 2012-10-27T15:30:11Z

    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 ;)

  2. Re: My first patch! (to \df output)

    Pavel Stehule <pavel.stehule@gmail.com> — 2012-10-27T15:45:54Z

    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 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@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

  3. Re: My first patch! (to \df output)

    Greg Sabino Mullane <greg@turnstep.com> — 2012-10-27T23:15:14Z

    -----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-----

  4. Re: My first patch! (to \df output)

    postgresql@thewickedtribe.net — 2012-11-09T19:53:20Z

    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 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.

  5. Re: My first patch! (to \df output)

    postgresql@thewickedtribe.net — 2012-11-09T20:22:06Z

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

  6. Re: My first patch! (to \df output)

    Robert Haas <robertmhaas@gmail.com> — 2012-11-14T22:38:53Z

    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

  7. Re: My first patch! (to \df output)

    Stephen Frost <sfrost@snowman.net> — 2012-12-29T18:56:09Z

    * 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

  8. Re: My first patch! (to \df output)

    Phil Sorber <phil@omniti.com> — 2013-01-18T23:51:30Z

    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.

  9. Re: My first patch! (to \df output)

    postgresql@thewickedtribe.net — 2013-01-19T15:54:57Z

    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@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.

  10. Re: My first patch! (to \df output)

    Phil Sorber <phil@omniti.com> — 2013-01-19T17:00:18Z

    On Jan 19, 2013 10:55 AM, "Jon Erdman" <postgresql@thewickedtribe.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.

  11. Re: My first patch! (to \df output)

    Stephen Frost <sfrost@snowman.net> — 2013-01-19T18:51:17Z

    * 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

  12. Re: My first patch! (to \df output)

    postgresql@thewickedtribe.net — 2013-01-23T05:10:46Z

    -----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-----

  13. Re: My first patch! (to \df output)

    Phil Sorber <phil@omniti.com> — 2013-01-23T05:17:39Z

    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-----

  14. Re: My first patch! (to \df output)

    postgresql@thewickedtribe.net — 2013-01-23T05:31:54Z

    -----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-----

  15. Re: My first patch! (to \df output)

    Phil Sorber <phil@omniti.com> — 2013-01-23T17:50:48Z

    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-----

  16. Re: My first patch! (to \df output)

    Craig Ringer <craig@2ndquadrant.com> — 2013-01-24T07:27:09Z

    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

  17. Re: My first patch! (to \df output)

    Phil Sorber <phil@omniti.com> — 2013-01-24T11:40:40Z

    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
>

  18. Re: My first patch! (to \df output)

    Heikki Linnakangas <hlinnakangas@vmware.com> — 2013-01-25T15:26:37Z

    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