Add on_trusted_init and on_untrusted_init to plperl UPDATED [PATCH]
This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.
Changes in this patch:
- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
SPI functions are not available when the code is run.
Errors are detected and reported as ereport(ERROR, ...)
Corresponding documentation.
- select_perl_context() state management improved
An error during interpreter initialization will leave
the state (interp_state etc) unchanged.
- The utf8fix code has been greatly simplified.
Tim.
Attachments:
plperl-userinit2.patchtext/x-patch; charset=us-asciiDownload+195-101
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.Changes in this patch:
- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSET
Im not a fan of the names (I think everyone gets trusted vs untrusted
confused). May I humbly suggest:
plperl.on_init
plperlu.on_init
plperl.both_on_init <- this one is the one that throws the scheme off :(
SPI functions are not available when the code is run.
Hrm, we might want to stick why in the docs or as a comment somewhere.
I think this was the main concern?
* We call a plperl function for the first time in a session, causing
plperl.so to be loaded. This happens in the context of a superuser
calling a non-superuser security definer function, or perhaps vice
versa. Whose permissions apply to whatever the on_load code tries
to do? (Hint: every answer is wrong.)
- select_perl_context() state management improved
An error during interpreter initialization will leave
the state (interp_state etc) unchanged.
This looked good.
- The utf8fix code has been greatly simplified.
Yeah to the point that it makes me wonder if the old code had some
reason to spin up the FunctionCall stuff. Do you happen to know?
Looks good to me otherwise.
The tests dont seem to pass :( this is from a make installcheck-world
test plperl_shared ... FAILED
...
test plperl_init ... FAILED
with:
SET plperl.on_trusted_init = '$_SHARED{on_init} = 42';
+ ERROR: unrecognized configuration parameter "plperl.on_trusted_init"
-- test the shared hashIf I throw a LOAD 'plperl'; at the top of those sql files it works...
The only quibble I have with the docs is:
+ If the code fails with an error it will abort the initialization and
+ propagate out to the calling query, causing the current transaction or
+ subtransaction to be aborted. Any changes within the perl won't be
+ undone. If the <literal>plperl</> language is used again the
+ initialization will be repeated.Instead of "Any changes within the perl won't be undone". Maybe
"Changes to the perl interpreter will not be undone" ?
On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.Changes in this patch:
- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSETIm not a fan of the names (I think everyone gets trusted vs untrusted
confused). May I humbly suggest:
plperl.on_init
plperlu.on_init
plperl.both_on_init <- this one is the one that throws the scheme off :(SPI functions are not available when the code is run.
Hrm, we might want to stick why in the docs or as a comment somewhere.
I think this was the main concern?* We call a plperl function for the first time in a session, causing
plperl.so to be loaded. This happens in the context of a superuser
calling a non-superuser security definer function, or perhaps vice
versa. Whose permissions apply to whatever the on_load code tries
to do? (Hint: every answer is wrong.)- select_perl_context() state management improved
An error during interpreter initialization will leave
the state (interp_state etc) unchanged.This looked good.
- The utf8fix code has been greatly simplified.
Yeah to the point that it makes me wonder if the old code had some
reason to spin up the FunctionCall stuff. Do you happen to know?
Looks good to me otherwise.The tests dont seem to pass :( this is from a make installcheck-world
test plperl_shared ... FAILED
...
test plperl_init ... FAILEDwith:
SET plperl.on_trusted_init = '$_SHARED{on_init} = 42';
+ ERROR: unrecognized configuration parameter "plperl.on_trusted_init"
-- test the shared hashIf I throw a LOAD 'plperl'; at the top of those sql files it works...
The only quibble I have with the docs is: + If the code fails with an error it will abort the initialization and + propagate out to the calling query, causing the current transaction or + subtransaction to be aborted. Any changes within the perl won't be + undone. If the <literal>plperl</> language is used again the + initialization will be repeated.Instead of "Any changes within the perl won't be undone". Maybe
"Changes to the perl interpreter will not be undone" ?
With all due respect.... yuck.
...Robert
On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.Changes in this patch:
- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSETIm not a fan of the names (I think everyone gets trusted vs untrusted
confused). May I humbly suggest:
plperl.on_init
plperlu.on_init
plperl.both_on_init <- this one is the one that throws the scheme off :(SPI functions are not available when the code is run.
Hrm, we might want to stick why in the docs or as a comment somewhere.
I think this was the main concern?* We call a plperl function for the first time in a session, causing
plperl.so to be loaded. This happens in the context of a superuser
calling a non-superuser security definer function, or perhaps vice
versa. Whose permissions apply to whatever the on_load code tries
to do? (Hint: every answer is wrong.)- select_perl_context() state management improved
An error during interpreter initialization will leave
the state (interp_state etc) unchanged.This looked good.
- The utf8fix code has been greatly simplified.
Yeah to the point that it makes me wonder if the old code had some
reason to spin up the FunctionCall stuff. Do you happen to know?
Looks good to me otherwise.The tests dont seem to pass :( this is from a make installcheck-world
test plperl_shared ... FAILED
...
test plperl_init ... FAILEDwith:
SET plperl.on_trusted_init = '$_SHARED{on_init} = 42';
+ ERROR: unrecognized configuration parameter "plperl.on_trusted_init"
-- test the shared hashIf I throw a LOAD 'plperl'; at the top of those sql files it works...
The only quibble I have with the docs is: + If the code fails with an error it will abort the initialization and + propagate out to the calling query, causing the current transaction or + subtransaction to be aborted. Any changes within the perl won't be + undone. If the <literal>plperl</> language is used again the + initialization will be repeated.Instead of "Any changes within the perl won't be undone". Maybe
"Changes to the perl interpreter will not be undone" ?With all due respect.... yuck.
OK, third time is the charm. Sigh. The "yuck" was in reference
specifically to the proposed GUC names.
I like the original ones better.
...Robert
Robert Haas escribi�:
On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.Changes in this patch:
- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
� �on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSETIm not a fan of the names (I think everyone gets trusted vs untrusted
confused). May I humbly suggest:
plperl.on_init
plperlu.on_init
plperl.both_on_init <- this one is the one that throws the scheme off :(
With all due respect.... yuck.
OK, third time is the charm. Sigh. The "yuck" was in reference
specifically to the proposed GUC names.I like the original ones better.
With all due respect, I think you should get more used to trimming the
message you're replying to, so that your reply makes more sense to the
readership at large. I fully realize that Gmail is not the best
platform for that :-(
--
Alvaro Herrera http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Tue, Feb 2, 2010 at 10:51 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Robert Haas escribió:
On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.Changes in this patch:
- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSETIm not a fan of the names (I think everyone gets trusted vs untrusted
confused). May I humbly suggest:
plperl.on_init
plperlu.on_init
plperl.both_on_init <- this one is the one that throws the scheme off :(With all due respect.... yuck.
OK, third time is the charm. Sigh. The "yuck" was in reference
specifically to the proposed GUC names.I like the original ones better.
With all due respect, I think you should get more used to trimming the
message you're replying to, so that your reply makes more sense to the
readership at large. I fully realize that Gmail is not the best
platform for that :-(
Well, actually, I did that. Except I replied only to Alex. And then
I hit send.
Then I immediately realized my mistake, and did it over, trimming it
wrong the second time. And then I hit send again.
Actually, I find Gmail to be pretty good for this - it's just, I
shouldn't try to answer my email when I'm exhausted and half-asleep.
So... I'm going to bed now.
...Robert
On Tue, Feb 2, 2010 at 20:46, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 2, 2010 at 10:46 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Feb 2, 2010 at 10:42 PM, Alex Hunsaker <badalex@gmail.com> wrote:
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
This is an update the fourth of the patches to be split out from the
former 'plperl feature patch 1'.Changes in this patch:
- Adds plperl.on_trusted_init and plperl.on_untrusted_init GUCs
on_trusted_init is PGC_USERSET, on_untrusted_init is PGC_SUSETIm not a fan of the names (I think everyone gets trusted vs untrusted
confused). May I humbly suggest:
plperl.on_init
plperlu.on_init
plperl.both_on_init <- this one is the one that throws the scheme off :(
With all due respect.... yuck.
heh, well I feel as reviewer its my job to solicit feed back from the
community. If I have to do it by suggesting gross names, so be it.
OK, third time is the charm. Sigh. The "yuck" was in reference
specifically to the proposed GUC names.
Yeah the both is gross. How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?
I like the original ones better.
I think they are OK.
Alex Hunsaker <badalex@gmail.com> writes:
Yeah the both is gross. How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?
I like the first two. The problem of selecting a good name for the
third one is easily solved: don't have it. What would it be except
a headache and a likely security problem?
regards, tom lane
On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
Yeah the both is gross. How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?I like the first two. The problem of selecting a good name for the
third one is easily solved: don't have it. What would it be except
a headache and a likely security problem?
Well its already in. (FYI its also PGC_SIGHUP) I included it here
because I wanted to make sure it made sense in context of the other
new plperl GUCs.
I *think* its there so people can:
-"use" the same modules in both
-profile both plperl and plperlu code easily (which is really the same point...)
The main difference between on_init and the other two is the later get
eval()ed in while the former is treated as "perl -e". (Did I get that
right Tim? I did not really look over the first patch). Im not sure
if there are different consequences for that style... But I would
venture its done that way so we can profile any perl interpreter
startup stuff we do in plperl.c or the src/pl/plperl/*.pl files.
So there might be a reason for it...
Alex Hunsaker <badalex@gmail.com> writes:
On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
Yeah the both is gross. How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?I like the first two. The problem of selecting a good name for the
third one is easily solved: don't have it. What would it be except
a headache and a likely security problem?
Well its already in.
Well *that's* easily fixed. I think it's a bad idea, because it's
unclear what you should put there and what the security implications
are. Two entirely separate init strings seems much easier to understand
and administer.
regards, tom lane
On Wednesday, February 3, 2010, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
Yeah the both is gross. How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?I like the first two. The problem of selecting a good name for the
third one is easily solved: don't have it. What would it be except
a headache and a likely security problem?Well its already in.
Well *that's* easily fixed. I think it's a bad idea, because it's
unclear what you should put there and what the security implications
are. Two entirely separate init strings seems much easier to understand
and administer.
+1.
It's a simple copy/paste in the config file if you want them the same
anyway, right?
/Magnus
--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/
On Tue, Feb 2, 2010 at 22:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
Yeah the both is gross. How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?
Well its already in.
Well *that's* easily fixed. I think it's a bad idea, because it's
unclear what you should put there and what the security implications
are.
I can't speak for its virtue, maybe Tim, Andrew?
Two entirely separate init strings seems much easier to understand
and administer.
I think people might quibble with you on that...
But I do agree that it seems redundant.
On Wed, Feb 3, 2010 at 00:46, Alex Hunsaker <badalex@gmail.com> wrote:
On Tue, Feb 2, 2010 at 22:50, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
On Tue, Feb 2, 2010 at 21:38, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Alex Hunsaker <badalex@gmail.com> writes:
Yeah the both is gross. How about:
plperl.on_plperl_init
plperl.on_plperlu_init
plperl.on_init ?Well its already in.
Well *that's* easily fixed. I think it's a bad idea, because it's
unclear what you should put there and what the security implications
are.I can't speak for its virtue, maybe Tim, Andrew?
Ahh I think i figured it out.
plperl.on_trusted_init runs *inside* of the safe. So you cant do
unsafe things like use this or that module. plperl.on_init runs on
init *outside* of the safe so you can use modules and what not. So now
I can use say Digest::SHA without tossing the baby out with the bath
water (just using plperlu). Gaping security whole? Maybe, no more so
than installing an insecure C/plperlu function as you have to edit
postgresql.conf to change it. Right?
Maybe we should have:
plperl.on_plperl_safe_init (runs inside of the safe, PGC_USERSET)
plperl.on_plperl_init (runs outside safe, PGC_SUSET)
plperl.on_plpleru_init (PGC_SUSET)
All of the above have no SPI/database access.
I think we can gt away with PGC_USERSET on safe_init as it wont allow
you to do anything "scary" like play with security definer functions
or redefine functions etc... There does seem to be the risk that I
may not have plperl GRANTed but I can make any plperl function
elog(ERROR) as long as they have not loaded plperl via a
plperl_safe_init. We can probably fix that if people think its a
valid dos/attack vector.
Comments?
Alex Hunsaker wrote:
Well its already in.
Well *that's* easily fixed. I think it's a bad idea, because it's
unclear what you should put there and what the security implications
are.I can't speak for its virtue, maybe Tim, Andrew?
plperl.on_perl_init runs when the library is loaded. That makes it
useful for preloading perl modules and similar tasks. The other two in
the patch under disccussion run when the relevant interpreter is first
used in the current session. That makes them appropriate for doing
things like loading specific initial settings (e.g. in the interpreter's
%_SHARED).
I'm not going to be pleased if, having had a substantial debate on the
patch that contained on_perl_init a week or so ago there are now
attempts to rip it out. As I commented when I committed it:
The final thing that persuaded me that no great damage would be done
by on_perl_init was the realization that we already have the ability
to do more or less the same thing anyway via standard Perl mechanisms,
and I'd be very surprised if enterprising Perl users hadn't made use
of it.
Regarding the naming of the params, I'm not keen to have more than one
custom_variable_class for plperl. Within that, maybe we can bikeshed the
names a bit. I don't have terribly strong feelings.
cheers
andrew
On Wed, Feb 3, 2010 at 06:41, Andrew Dunstan <andrew@dunslane.net> wrote:
Alex Hunsaker wrote:
Well its already in.
Well *that's* easily fixed. I think it's a bad idea, because it's
unclear what you should put there and what the security implications
are.I can't speak for its virtue, maybe Tim, Andrew?
Regarding the naming of the params, I'm not keen to have more than one
custom_variable_class for plperl. Within that, maybe we can bikeshed the
names a bit. I don't have terribly strong feelings.
Hey! I don't think were quite to that nasty B word yet :) I would
argue that treating plperl and plperlu as the same language just
because it shares the same code is a mistake. But I hate the idea of
two custom_variable_classes for plperl(u) as well. Which is why I
quickly switched to plperl.on_plperl(u)_init. Any thoughts on those?
Again maybe people think the original names are fine... *shrug*.
On Wed, Feb 3, 2010 at 9:51 AM, Alex Hunsaker <badalex@gmail.com> wrote:
Hey! I don't think were quite to that nasty B word yet :) I would
argue that treating plperl and plperlu as the same language just
because it shares the same code is a mistake. But I hate the idea of
two custom_variable_classes for plperl(u) as well. Which is why I
quickly switched to plperl.on_plperl(u)_init. Any thoughts on those?
Again maybe people think the original names are fine... *shrug*.
I like plperl.on_plperl(u)_init better than plperl.on_(un)trusted_init.
...Robert
Robert Haas <robertmhaas@gmail.com> writes:
On Wed, Feb 3, 2010 at 9:51 AM, Alex Hunsaker <badalex@gmail.com> wrote:
Hey! I don't think were quite to that nasty B word yet :) �I would
argue that treating plperl and plperlu as the same language just
because it shares the same code is a mistake. �But I hate the idea of
two custom_variable_classes for plperl(u) as well. �Which is why I
quickly switched to plperl.on_plperl(u)_init. �Any thoughts on those?
Again maybe people think the original names are fine... *shrug*.
I like plperl.on_plperl(u)_init better than plperl.on_(un)trusted_init.
I agree. But the question in my mind is the relationship between plperl
and plperlu. I agree with the upthread comment that it would be better
if the init strings for them were entirely separate. ISTM we have got
three categories here:
plperl init done outside Safe (must be SUSET)
plperl init done inside Safe (can be USERSET)
plperlu init (must be SUSET)
and there is no good reason to conflate the first and third, nor to
insist that one must be a subset of the other, which AFAICS is the
effect of the current design. So we need a naming scheme that takes
some account of this. Perhaps
plperl.plperl_init
plperl.plperl_safe_init
plperl.plperlu_init
?
regards, tom lane
On Tue, Feb 02, 2010 at 08:42:21PM -0700, Alex Hunsaker wrote:
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
� �SPI functions are not available when the code is run.
Hrm, we might want to stick why in the docs or as a comment somewhere.
I think this was the main concern?* We call a plperl function for the first time in a session, causing
plperl.so to be loaded. This happens in the context of a superuser
calling a non-superuser security definer function, or perhaps vice
versa. Whose permissions apply to whatever the on_load code tries
to do? (Hint: every answer is wrong.)
It's hard to convey the underlying issues in a sentence or two. I tried.
I'm not sure it's worth it. (Though if anyone thinks it is I'd be happy
to get some specific suggestions for the wording to use.)
- The utf8fix code has been greatly simplified.
Yeah to the point that it makes me wonder if the old code had some
reason to spin up the FunctionCall stuff. Do you happen to know?
Before my refactoring led me to add safe_eval(), FunctionCall was
'the natural way' to invoke code inside the Safe compartment.
The tests dont seem to pass :( this is from a make installcheck-world
+ ERROR: unrecognized configuration parameter "plperl.on_trusted_init"
If I throw a LOAD 'plperl'; at the top of those sql files it works...
Ah. That's be because I've got custom_variable_classes = 'plperl' in my
postgresql.conf. I'll add LOAD 'plperl'; to the top of those tests.
The only quibble I have with the docs is: + If the code fails with an error it will abort the initialization and + propagate out to the calling query, causing the current transaction or + subtransaction to be aborted. Any changes within the perl won't be + undone. If the <literal>plperl</> language is used again the + initialization will be repeated.Instead of "Any changes within the perl won't be undone". Maybe
"Changes to the perl interpreter will not be undone" ?
In an earlier patch I'd used the word interpreter quite often. When
polishing up the doc changes in response to Tom's feedback I opted to
avoid that word when describing on_*trusted_init. This looks like a case
where I removed 'interpreter' but didn't fixup the rest of the sentence.
I'd prefer to simplify the sentence further, so I've changed it to
"Any changes within perl won't be undone".
On Wed, Feb 03, 2010 at 01:06:03AM -0700, Alex Hunsaker wrote:
plperl.on_trusted_init runs *inside* of the safe. So you cant do
unsafe things like use this or that module.
Yes. It's effectively the same as having a DO '...' language plperl;
that's guaranteed to run before any other use of plperl.
plperl.on_init runs on
init *outside* of the safe so you can use modules and what not. So now
I can use say Digest::SHA without tossing the baby out with the bath
water (just using plperlu). Gaping security whole? Maybe, no more so
than installing an insecure C/plperlu function as you have to edit
postgresql.conf to change it. Right?
Right.
I'll emphasise the point that only plperlu code has access to anything
loaded by plperl.on_init (aka .on_perl_init). Currently plperl code doesn't.
There seemed to be some confusion upthread about how the GUCs work
together so I'll recap here (using the original GUC names):
When plperl.so is loaded (possibly in the postmaster due to
shared_preload_libraries) a perl interpreter is created using
whatever version of perl the server was configured with.
That perl is initialized as if it had been started using:
perl -e $(cat plc_perlboot.pl)
If on_perl_init is set then the the initialization is effectively:
perl -e $(cat plc_perlboot.pl) -e $on_perl_init
That perl interpreter is now in a virgin 'unspecialized' state.
From that state the interpreter may become either a plperl or plperlu
interpreter depending on whichever language is first used.
When an interpreter transitions from the initial state to become
specialized for plperlu for a particular user then
plperl.on_untrusted_init is eval'd.
When an interpreter transitions from the initial state to become
specialized for plperl then plc_safe_ok.pl is executed to create the
Safe compartment and then plperl.on_trusted_init is eval'd *inside* the
compartment.
So, if all GUCs were set then plperl code would run in a perl
initialized with on_perl_init + on_trusted_init, and plperlu code would
run in a perl initialized with on_perl_init + on_untrusted_init.
To add some context, I envisage plperl.on_perl_init being a stable value
that typically pre-loads a set of modules etc., while the on_*trusted_init
GUCs will typically be used for short-term debug/testing. Which is why
on_untrusted_init (SUSET) is still useful with on_perl_init (SUSET).
Maybe we should have:
plperl.on_plperl_safe_init (runs inside of the safe, PGC_USERSET)
plperl.on_plperl_init (runs outside safe, PGC_SUSET)
plperl.on_plpleru_init (PGC_SUSET)
Which, except for the names, is essentially what the patches implement.
If we're going to bikeshed the names, I'd suggest:
plperl.on_init - run on interpreter creation
plperl.on_plperl_init - run when then specialized for plperl
plperl.on_plperlu_init - run when then specialized for plperlu
as being the most natural fit with the user and DBA perspective.
Since Tom also expressed a preference for .on_plperl_init/.on_plperlu_init
and you also suggested .on_init earlier, I'll rework the patch with those
names, plus the docs and test fixes nted above.
There does seem to be the risk that I may not have plperl GRANTed but
I can make any plperl function elog(ERROR) as long as they have not
loaded plperl via a plperl_safe_init. We can probably fix that if
people think its a valid dos/attack vector.
Interesting thought. If this is a valid concern (as it seems to be)
then I could add some logic to check if the language has been GRANTed.
(I.e. ignore on_plperl_init if the user can't write plperl code, and
ignore on_plperlu_init if the user can't write plperlu code.)
Tim.
Import Notes
Reply to msg id not found: 34d269d41002030006k17c9db72pc72bc38d57219bb9@mail.gmail.com34d269d41002021942ve176781j78314693539a715e@mail.gmail.com | Resolved by subject fallback
On Wed, Feb 3, 2010 at 09:31, Tim Bunce <Tim.Bunce@pobox.com> wrote:
On Tue, Feb 02, 2010 at 08:42:21PM -0700, Alex Hunsaker wrote:
On Sat, Jan 30, 2010 at 08:49, Tim Bunce <Tim.Bunce@pobox.com> wrote:
SPI functions are not available when the code is run.
Hrm, we might want to stick why in the docs or as a comment somewhere.
I think this was the main concern?* We call a plperl function for the first time in a session, causing
plperl.so to be loaded. This happens in the context of a superuser
calling a non-superuser security definer function, or perhaps vice
versa. Whose permissions apply to whatever the on_load code tries
to do? (Hint: every answer is wrong.)It's hard to convey the underlying issues in a sentence or two. I tried.
I'm not sure it's worth it. (Though if anyone thinks it is I'd be happy
to get some specific suggestions for the wording to use.)
All I know is I thought hrm... Why cant you have SPI ? It seems useful
and I dont immediately see why its a bad idea. So I dug up the old
threads. Im just afraid say in a year or two we will forget why we
disallow it.
I was thinking something along the lines of:
diff --git a/src/pl/plperl/plperl.c b/src/pl/plperl/plperl.c
index 6f577f0..a19f1da 100644
--- a/src/pl/plperl/plperl.c
+++ b/src/pl/plperl/plperl.c
@@ -422,6 +422,12 @@ plperl_init_interp(void) PERL_SET_CONTEXT(plperl);
perl_construct(plperl);
+
+ /*
+ * Allow things like SPI to happen *after* the plperl.*init functions have run
+ * this avoids nasty problems with security definer functions
+ * ...maybe some mail link here or the whole quote from Tom?
+ */
perl_parse(plperl, plperl_init_shared_libs,
nargs, embedding, NULL);The only quibble I have with the docs is: + If the code fails with an error it will abort the initialization and + propagate out to the calling query, causing the current transaction or + subtransaction to be aborted. Any changes within the perl won't be + undone. If the <literal>plperl</> language is used again the + initialization will be repeated.
I'd prefer to simplify the sentence further, so I've changed it to
"Any changes within perl won't be undone".
Much better.
Maybe we should have:
plperl.on_plperl_safe_init (runs inside of the safe, PGC_USERSET)
plperl.on_plperl_init (runs outside safe, PGC_SUSET)
plperl.on_plpleru_init (PGC_SUSET)Which, except for the names, is essentially what the patches implement.
Well not quite as with the above there is still no global "on_init".
If we're going to bikeshed the names, I'd suggest:
plperl.on_init - run on interpreter creation
plperl.on_plperl_init - run when then specialized for plperl
plperl.on_plperlu_init - run when then specialized for plperlu
Hrm, I think I agree with Tom that we should not have a global
on_init. And instead of two separate GUCs (we still end up with 3
gucs total). Im still thinking through it...
Since Tom also expressed a preference for .on_plperl_init/.on_plperlu_init
Well I think Magnus and Robert did as well :)
and you also suggested .on_init earlier, I'll rework the patch with those
names, plus the docs and test fixes nted above.
OK
There does seem to be the risk that I may not have plperl GRANTed but
I can make any plperl function elog(ERROR) as long as they have not
loaded plperl via a plperl_safe_init. We can probably fix that if
people think its a valid dos/attack vector.Interesting thought. If this is a valid concern (as it seems to be)
then I could add some logic to check if the language has been GRANTed.
(I.e. ignore on_plperl_init if the user can't write plperl code, and
ignore on_plperlu_init if the user can't write plperlu code.)
Well Im not sure. As a user I can probably cause havok just by
passing interesting values to a function. It does seem inconsistent
that you cant create plperl functions but you can potentially modify
SHARED. In-fact if you have a security definer plperl function it
seems quite scary that they could change values in SHARED. But any
plperl function can do that anyway. (do we have/need documentation
that SHARED in a plperl security definer function is unsafe?) So I
dont think its *that* big of deal as long as the GRANT check is in
place.
Thoughts?
On Wed, Feb 3, 2010 at 10:18, Alex Hunsaker <badalex@gmail.com> wrote:
On Wed, Feb 3, 2010 at 09:31, Tim Bunce <Tim.Bunce@pobox.com> wrote:
On Tue, Feb 02, 2010 at 08:42:21PM -0700, Alex Hunsaker wrote:
If we're going to bikeshed the names, I'd suggest:
plperl.on_init - run on interpreter creation
plperl.on_plperl_init - run when then specialized for plperl
plperl.on_plperlu_init - run when then specialized for plperluHrm, I think I agree with Tom that we should not have a global
on_init. And instead of two separate GUCs (we still end up with 3
gucs total). Im still thinking through it...
Err
"And instead have two separate GUCs (3 in total)" not
"And Instead of two...".
