[Rpm-maint] First attempt for the patch on extending the plugin interface for rpm

[Panu Matilainen]

On 10/08/2012 03:03 PM, Reshetova, Elena wrote:
> Hi,

Hi,

> Following up our recent discussion, I have composed a first patch in order
> not to talk about the abstract concepts only.
> I have taken the less intrusive approach and tried to integrate the new
> hooks and the notion of "security plugin"
>   into the code with least amount of changes for core rpm and for existing
> collection hooks.  This would also allow to leave the existing
> SELinux collection hooks as they are and only move the integrated SElinux
> functionality from rpm to the hooks.

Note that the existing hooks are by no means "sacred" for other than 
documenting the needs of the SELinux collection. AFAIK the collections 
aren't actually being used anywhere yet (its still marked experimental), 
and the interfaces are internal-only so if there's something that 
needs/wants to be changed, they can be changed.

>
> Please note that this patch is very rough and I would like to post it only
> for the purpose of asking if this approach is valid.
> There are a number of decisions that I had to make about this patch that are
> worth discussing:
>
> - The patch defines a new type of rpm plugin, called security plugin and add
> a rpmpluginsAddSecurityPlugin() function to initialize it.
> This is done in the similar style as rpmpluginsAddCollectionPlugin()
> function. The top level function that uses it is rpmteSetupSecurityPlugins()
> from transaction.c. The main difference from the collection plugin case is
> that this function can automatically discover the security plugins
> that should be loaded based on the %__security_plugins macro from macros.in
> that contains comma separated list of security plugins to be loaded.
> I don't know how viable this approach is, but it seemed for me the easiest
> way to make the plugins to be configured.

Macros the means through which rpm is generally configured, and that 
kind of approach should make it reasonably simple to allow selective 
enabling/disabling of them from cli (annoying but inevitable necessity). 
Might be more annoying for other API users - currently disabling SELinux 
is a simple matter of setting RPMTRANS_FLAG_NOCONTEXTS flag for the 
transaction, with plugins it would be something ... quite different.

>
> -Due to necessity of calling the plugins hooks with the plugin struct that
> is stored in ts, some rpm functions have to pass the rpmts struct.
> In addition since the ts->plugins is the actual needed parameter,
> ts_internal.h has to be exposed to couple of more .c file. I find it to be
> quite
> ugly, but could not figure out the better way with the current plugin
> definition. I was especially annoyed by the script functions that now have
> to pass ts, too.
> Any suggestion on this?

I think this was one of the prime reasons I had that look into the 
"security manager" object thingie: to avoid having to pass the 
transaction set to places where it really does not belong to.

OTOH it would seem to me (most of) the places could simply be passed a 
handle to the plugins instead of ts, as the ts is pretty much only used 
for ts->plugins.

> - Instead of making a loop for each plugin hook to be called for each
> plugin, I am iterating by the plugins and calling the hook for each plugin
> inside the rpmpluginsCallxxx() functions. IMO it makes the hook code
> scattered through the rpm cleaner, but this is just my opinion.

...and this is related to what was one of the issues the "security 
manager" thing ran into. Assuming there can be more than one present: 
how to meaningfully deal with errors? Returning an error if one or more 
of them failed is a simple answer, but whether that's sufficient for the 
callee(s) I dont know, I seem to recall some things wanting a better 
idea of what failed and how (but that might well be just from staring 
too much at the way things are currently done)

I haven't read through the patch too carefully, so just a few comments 
on things that I happened to notice (on top of the issues/questions 
already raised)

> +
> +        /* Run file updated hook for all plugins */
> +        rc = rpmpluginsCallFileUpdated(ts->plugins, fsm->path, fsm->buf,
> len);

Hmm. Looking at the MSM plugin (from 
git://github.com/ereshetova/rpm.git), this is primarily used for 
calculating yet another digest for the file being processed. This should 
be avoidable, the rpmio FD_t type supports calculating more than one on 
the digest on the fly, only the interfaces to use it are currently 
buried in rpmio/rpmio_internal.h (but not actually hidden from ABI).


>   		    rc = fsmSetSELabel(sehandle, dn, mode);
> -
> +                    if (!rc)
> +                        rc = rpmpluginsCallDirCreated(ts->plugins, dn,
> mode);

This is a pretty ugly special case hack :) Part of the reason for 
fsmSetSELabel() was to isolate the whole labeling thing into something 
fairly generic that could be as well in a plugin. So this should be more 
of a "post file/dir-creation hook" I think . Also access to such a thing 
(or a way to avoid the need there completely...) would be needed in 
rpmdbMoveDatabase() too.


>   	if (xx == 0) {
> -	    xx = execv(argv[0], argv);
> +                /* Run script exec hook for all plugins */
> +                if (rpmpluginsCallScriptExec(ts->plugins, argv) !=
> RPMRC_FAIL) {
> +		        xx = execv(argv[0], argv);
> +                }

Ah, this is one of the places where I remember pondering over with the 
"security manager" experiment: my idea was to bury the execv() call 
itself into the manager so it could be execv(), rpm_execcon() or 
whatever depending on the manager type/what's loaded, but that didn't 
fly at all :) This seems like a much more feasible approach, but the 
hook name wants clarification as the hook MUST NOT actually perform the 
exec, only setup any preliminaries it wants for the exec (eg 
setexeccon() in case of selinux)


> @@ -409,6 +411,9 @@ static void handleInstInstalledFile(const rpmts ts,
> rpmte p, rpmfi fi, int fx,
>   	if (rpmtsFilterFlags(ts) & RPMPROB_FILTER_REPLACEOLDFILES)
>   	    rConflicts = 0;
>
> +        /* run file conflict hook for all plugins */
> +        rpmpluginsCallFileConflict(ts->plugins, ts, p, fi, otherFi,
> otherHeader);
> +

This wont work in rpm >= 4.10 which no longer uses the rpmfi 
"self-iterator" here - random access to the file info sets is needed and 
it can even end up accessing the same file info set at two places 
simultaneously (its possible for a package to have file conflicts within 
itself via directory symlinks). So you'd have to actually pass indexes 
for both fi and otherFi, but then the indexed accessors to rpmfi aren't 
exported in the API currently and...

Overall I'm still not really comfortable of letting plugins mess around 
with such a core functionality of rpm (I do remember the previous 
discussion around this though)

	- Panu -