[Rpm-maint] Plugin ponderings

[Panu Matilainen]

On 11/26/2012 07:26 PM, Reshetova, Elena wrote:
> Hi,
>
> I am attaching next version of the scriptlet-related patch. Next I will go
> back to current hooks and make a small patch to make sure they are called as
> we agreed.

Some issues with the patch still, comments + further ideas below:

In rpmScriptRun(), just grab the internal/extranal bit into a variable 
early so you dont need double calls differing only by the 
internal/external argument to the hooks. So the whole thing then becomes 
something like

---
     int script_type = SCRIPTLET_TYPE_EXTERNAL;

     if (rstreq(args[0], "<lua>")
         script_type = SCRIPTLET_TYPE_INTERNAL;

     rc = rpmpluginsCallScriptletPre(plugins, script->descr,
                                     script_type);

     if (rc != RPMRC_FAIL) {
         if (script_type == SCRIPTLET_TYPE_INTERNAL)
             rc = runLuaScript(...);
         else
             rc = runExtScript(...);
     }

     rc = rpmpluginsCallScriptletPost(plugins, script->descr,
                                      script_type, rc);
---

Except that here we get to the return code issues again: the rc returned 
from Post hook overrides whatever the earlier result might've been, eg 
if the scriptlet execution failed then the post-hook definetely 
shouldn't turn that into an ok. That flaw is present both in your patch 
and my sample code above. The other question is: if the scriptlet 
execution succeeded, is there ever a valid case for a plugin to turn 
that into a failure? For such a seemingly simple thing, this is 
surprisingly tricky to get right :)

BTW perhaps it would make sense to change the internal/external 
scriptlet type thing into a bitfield which tells the hooks how exactly 
the scriptlet will get executed. Basically an "external" script would be 
something like (RPMSCRIPT_FORK|RPMSCRIPT_EXEC) and "internal" scripts 
neither, until we add the ability to fork the Lua execution too, at 
which point Lua scripts get RPMSCRIPT_FORK (but no EXEC). Something like 
that should remove any ambiguosity from what "internal" and "external" 
script actually means.

Then there's a bunch of indentation issues, at least these:

- missing indentation level on all the new rpmpluginsCallScriptletFoo() 
for loops, eg:

     for (i = 0; i < plugins->count; i++) {
     name = plugins->names[i];
     RPMPLUGINS_SET_HOOK_FUNC(PLUGINHOOK_SCRIPTLET_PRE);
     if (hookFunc(s_name, type) == RPMRC_FAIL)
         rc = RPMRC_FAIL;
     }

..and here:

         if (xx == 0) {
         xx = execv(argv[0], argv);
         }

...and a few cases of soft tabs where hard tabs should be used. Hard vs 
soft tab mixup is mostly just a slight annoyance (due to diffs not 
aligning "perfectly") and not the end of the world, but the indentation 
levels really need to be right.

Another minor cosmetics issue I seem to have missed on previous rounds: 
the ampersand placement in K&R is next to the variable name, not the 
type. Eg "char* s_name" should be "char *s_name" instead (although a lot 
of rpm code has whitespace on both sides, thats ok too)

> Will do this tomorrow. Also some comments below:
>
>> There's eg RPMRC_NOTTRUSTED that could be (ab)used for certain types of
> security-related failures, but what if we have several plugins on a hook,
> one returning RPMRC_OK, another one RPMRC_FAILED and the third one
> RPMRC_NOTTRUSTED - which >one should the CallFooHook() function return?
>> One of the failures sure but...
>
> The security theory is very easy for this case: the most restrictive
> case/result is applied/returned. So, in this case I would return that we
> failed on policy and indicate it with " RPMRC_NOTTRUSTED ". In this way rpm
> can always enforce the strictest rule, for example if it wants to abort
> installation after receiving RPMRC_NOTTRUSTED.

In a purely security context that certainly makes sense, but in our 
case(s) the results tend to be mixups of potential security and 
miscellanous other errors. At least to me its not at all obvious which 
should "win" in various circumstances.

>> One possibility could be a bitfield type return code, where each plugin
> can raise relevant bits of fairly generic categories. There's no telling
> which plugin raised what bits, but at least it'd be possible to reflect more
> than one kind of failure per hook then. >But again, mostly just something to
> keep in mind / think about for now.
>
> This would be cool, especially if we get LSM stacking accepted upstream! Now
> there are active discussions on security mailing list on Casey's patches and
> I can imagine that in the some distant future we can have a number of
> simultaneous LSM working with rpm and a number of plugins that can use it.
> And for each plugin we can log what happens and make rpm to be more aware.
> But you are right: this is just future so far...

Right, perhaps I should look into this a bit. There are numerous places 
within rpm that would benefit from bitfield error codes, for example the 
rpmtsRun() return code is absolutely useless as it is now.

>> We could just make the hook return "void", but perhaps its better to let
> the hooks return errors codes and let rpm filter out (and perhaps log a
>> warning) what it cant handle: its possible that some things it can't handle
> now can be handled at some point in future.
>
> Agree, I think it is good to give plugins possibility to complain, even if
> the complains aren't heard at the moment. I just meant that maybe no reason
> to invent new return codes just yet.

Yup, lets see how far we can get with the rpmRC codes.

>> Note that rpm always creates files (but not directories) with a temporary
> extension, and they're only rename()'d to final location (which might
> replace existing files on upgrades) after everything was successfully
> unpacked to the temporary locations. Rpm >doesn't currently do a proper job
> of cleaning up the cruft in case of failure, but there is a chance to undo
> at least some of the things before the fsm "commits"
>> the files to final destination.
>
> Oh, this is actually very good, but I am not sure this is full solution. I
> guess I need to study fsm machine more deeply. It is the hardest one for me
> to understand so far :(

Heh, it quite probably is the most intricate piece of code there is in 
rpm, even after several rounds of gradual cleanups over the years. Lets 
just say we have no shortage of <cough> colorful <cough> expansions for 
the FSM acronym...

> Let's imagine we install a package and while putting its files to temp.
> location security labelling fails on one of them (maybe even not the first
> file and this is the bad thing). Plugin returns failure on that file in file
> closed hook (which in this case would need to be moved before FSM commit).
> What would be the proper behaviour? IMO we need to revert the whole package
> installation unfortunately. Because if file is left on filesystem but not
> properly labelled, then it might not be usable, and this might also lead to
> app/service/daemon/whateverinstalledfrom package not usable too.  Do we want
> such package on the filesystem then?

Yup, failure on file creation (including setting its permissions, 
security labels etc) should fail the entire package. I dont remember the 
exact details of what currently happens in all the possible cases, but 
it should be possible to improve the behavior considerably by doing more 
things before the "commit" step, and implement proper cleanup in case of 
failure. OTOH at this point, the package can already have executed some 
scripts (eg %pretrans and %pre) and actions done by those can't be 
undone :-/

> But then the problem how do we remove
> the files that has already been commited or if this file happens to be a
> dir?

For "normal" cases, undoing should be reasonably doable. Directories and 
directory-symlinks are much much tougher cookies though.

>> At least in SELinux, rename() doesn't affect the security labeling so it'd
> be possible to set the final contexts on the temporary files and only if
> that succeeds for all files, commit the result. Currently the labelling is
> done on the final destination so there's not >much chance of undoing in case
> of failure.
>
> Hm, isn't rename just a sub-case of mv? I think  in this case, xattrs might
> not be preserved (for example for cp if you don't use --preserve=xattrs,
> then you might lose your xattrs, including security labels (applies for both
> SELinux and any LSM using the xattrs)). I would need to setup a small test
> to remember this. But there should be a way for sure to move the files from
> temporal location to final one while preserving all attributes, given that
> rpm has needed privileges (which it does).

It's a sub-case of mv, but rename() is a "pure" rename and requires 
oldpath and newpath to be on the same filesystem. Except for time stamp 
updates it touches no other metadata - ownership, permissions etc do not 
chance in rename(). mv on the other hand can cross mount-points, in 
which case its actually a copy (and requires special attention to 
permissions, xattrs and all).

> I wonder if rpm could first
> unpack all files and dirs to some temp location and then do one FSM commit
> (if everything went fine) to commit them all one by one. This would solve
> the problem of how to remove already installed files.

That's *sort of* what it already does, except that permissions etc are 
set as part of commit instead of creation time, and since permissions & 
labels etc can fail...

It should be possible to considerably improve the current behavior: the 
to-be-replaced files could be moved to another temporary name before 
replacing so we could try go back to previous contents if commit fails, 
and we could try limiting the amount of things that can fail in commit. 
And we should clean up temporary leftovers in case of failure anyway.
But here too, directories are much tougher cookies.

	- Panu -

>> Yup... although some of this could be perhaps dealt with by rearranging
> the way file permissions etc are laid down: currently they're all done on
> the final destination, but if the labels, capabilities, modes etc were
> staged in it would allow dealing with, well, >not all but at least more
> failure cases. Would likely require quite some changes to fsm, but that's
> not exactly a bad thing (we've been slowly rewriting the damn thing anyway
> :)
>
> Yes, exactly my though above :)
>
> Best Regards,
> Elena.
>