[Rpm-maint] Plugin ponderings

[Panu Matilainen]

On 11/23/2012 03:08 PM, Reshetova, Elena wrote:
>
>> Like said in some earlier email, there's not much hope getting the details
> 100% right the first time. The best way of finding out what works and what
> doesn't is to try it out, so...
>> I started looking into creating a syslog plugin for rpm. One could argue
> that logging is fundamental enough to be in rpm core, but then having it in
> a plugin allows nicely for alternatives (such as *gasp* native systemd
> journal support) and is a nice and harmless way to test the plugin system.
> Lets just say I ran >into issues real fast. No fingerpointing implied - I
> didn't catch these on review either and some of the issues are direct
> consequences from my own suggestions, doh :)
>
> Yes, I have warned that I am a security person, nothing more :) So my head
> is full of security use cases which are unfortunately far from covering all
> potential use cases :( I think I will keep learning a lot about package
> management for a long while still!

Oh, I haven't been looking at this outside the SELinux aspect either. 
Which is far too narrow view into a generic plugin interface - getting a 
wider perspective was a part of the reason for playing around with 
something completely different.

>> For logging installs and erasures, one of the primary interests is
> PLUGINHOOK_PSM_POST. As it is now, the hook doesn't get called if the
> package failed to install, which is a case you'll certainly want to log:
>>
>>              /* Run post transaction element hook for all plugins */
>>              if (!rc) rc = rpmpluginsCallPsmPost(ts->plugins, te);
>
>> The other problem here is that the PSM_POST hook doesn't get passed the
> result code of the install (otoh it doesn't get called on failure to begin
> with so...). One could get the status from rpmteFailed(), but that element
> failure is set only after rpmpsmRun() returns so its not available here.
>> Similar issues are present in the TSM hooks: TSM_POST doesn't get called on
> some error situations (file conflicts etc) where TSM_PRE is called, and the
> transaction result code is not passed to plugins.
>
> Yes, I guess it would make sense to make them all symmetric, like we are now
> trying to get for scripts. And in this case of course return code is must.
>
>> Me thinks we seriously need to define a consistent convention for all
> plugin hooks and then scrutinize the existing and future hook paths for
> correctness in all possible situations.
> Fully agree, just it will be probably also work in progress constantly as
> getting hooks right :)
>
>> I'd think part of the convention needs to be that whenever PRE-hooks get
> called, the pairing POST hooks are guaranteed to get called too. Even if
> some of the PRE-hooks returned errors: some plugin(s) might return an error
> from its PRE-hook, but there could be other plugins which succeeded in their
> own >business and might have allocated resources they need to free up in
> POST.
> Makes sense.

Ok, lets try to stick this for now then. Almost certainly there will be 
further curveballs (things that dont fit into the nice and clean paired 
hooks idiom) to deal with sooner or later, but having some kind of 
baseline convention should help anyway.

>> Then there's the issue of return codes: maybe all the POST hooks should
> take an added 'rc' argument for the return code of the actual operation.
> I think this is good, we would just need to define for plugins what return
> code means. Wonder if we can do it generic enough after all without going to
> define a bunch of "InstallationSucededWithWarningsType1",
> "InstallationSucededWithWarningsType2", "InstallationFailedReasonA",
> "InstallationFailedReasonB",.... Like to tell that this hook will always get
> RPMRC_OK, if package is installed (somehow, even with warnings) and FAIL, if
> installation badly failed and package traces aren't present in filesystem.

Trying to enumerate all the possible failure causes and then 
interpreting them in the callers is indeed doomed to fail. It'll need to 
be far more generic than that, my point was simply that I dunno if rpmRC 
is sufficient - just something to think about.

>> That seems simple enough in principle, but there be dragons in the details:
>
>> For example rpmpsmRun() returns RPMRC_FAIL only if the package didn't get
> installed (or removed) at all. %pre and %preun scriptlets can prevent
> install/erase from taking place, %post and %postun cannot (as the operations
> cannot be rolled back) so RPMRC_OK is returned from
>> rpmpsmRun() even if they fail: basically the operation succeeded with
> warnings. This semantic needs to be taken into account with plugin hook
> results too, so rpmpluginsCallPsmPost() can't really cause RPMRC_FAIL to be
> returned from rpmpsmRun(). Or we need to revamp the whole damn return code
>> system (which might well be the sanest option really, especially since its
> just an internal API)
>> Similar quirks are present in rpmtsRun() return codes, but that's a more
> difficult case as its part of the public API (which everybody loves to hate
> because of the nutty and useless return codes :)
>> Another question is whether rpmRC codes are sufficient for the task.
>> It's used for a lot of things in rpm, but many of those uses are ugly
> abuses in reality.
>
> Return codes are really hard. Suppose we define some new ones for plugin
> purposes, so that hooks can return more sane codes and potentially indicate
> even type of failure. I would love to see some kind of security failure as
> return code in this case and treat it more seriously :)

Indeed. I could imagine wanting to differentiate between io errors and 
policy denials for example.

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

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.

> But then for hooks
> like rpmpluginsCallPsmPost() it doesn't matter anyway like you said: we
> can't revert installation at this point, so even if we return
> "EverythingIsReallyReallyBad", what can rpm do?

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.

> I have noticed this at the
> beginning when our initial patch was labelling the filesystem in
> rpmpluginsCallPsmPost(), which made me think what if labelling fails at that
> point? Plugin can return whatever, but it is too late to do anything about
> it.  Actually this problem keeps me occupied for FileClosed() hook: file has
> been already placed on filesystem at that time, we want to setup a nice
> security context on it, but lsetattr() fails, what are the next steps apart
> from panic? There is no way to scratch that file out of filesystem, so
> initial labelling has to be secure by default (not usable for that file
> probably, but secure).

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.

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 
labeling is done on the final destination so there's not much chance of 
undoing in case of failure.

> This brings to the fact that when file is written to
> the filesystem, it should be placed with some kind of "Isolated label" or
> isolated security context, meaning that we need to change rpm process
> security context to have this isolated context/label, place the file, change
> the label of rpm process back to rpm and then call the closed hook to label
> the file. At least two more hooks :( But from security point of view, setup
> will be safe: if labelling fails, then at least file is labelled with
> Isolated label and can't harm. However, again, returning failure or any
> other code that "OK" from FileClosed() hook doesn't make much sense.
> All of this just to say that I am not sure if new return codes will add so
> much value...

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

>> P.S. I certainly dont mean you need to do all this, I'll help combing the
> call- and error paths for correctness once we figure out what "correct"
> actually is.
> I am not against working with this at all! Just would like to understand
> also what is the best way about this...

Oh, my point was just that I'm certainly willing to help with the 
details on code level too. There are plenty of details to scratch head 
over on both the conceptual and code-detail level :)

	- Panu -