Exempt src.rpm packages from file signature business

[dmnks]

Don't add file signatures to source packages and if they happen to be present (i.e. built with an older rpm version), ignore them when unpacking (by not running plugins at all for source packages).

AC (taken from original bug):

• IMA signature don't make any sense on src.rpm files - so Fedora shouldn't be signing them, and rpm shouldn't let them
• Even if the assertion above fails and you somehow end up with an src.rpm with IMA signatures on it, rpm should not barf up on it

[dmnks]

Should be fixed now, based on the above discussion.

Note that I'm not a huge fan of the additional NULL checks in all the plugin hook functions (first commit in the series), it just makes it more noisy, and we could just initialize an empty rpmPlugins struct with rpmpluginsNew() in fsmPlugins() instead of returning NULL. But then, it's probably safer to have the NULL checks in place.

[dmnks]

Argh, what a strange CI failure, looking now...

[dmnks]

A silly memory leak in my code! Fixing now...

[dmnks]

... aaand fixed.

[pmatilai]

Note that I'm not a huge fan of the additional NULL checks in all the plugin hook functions (first commit in the series), it just makes it more noisy, and we could just initialize an empty rpmPlugins struct with rpmpluginsNew() in fsmPlugins() instead of returning NULL. But then, it's probably safer to have the NULL checks in place.

Hmm. I thought we wouldn't need any new NULL checks because I thought that's what happens with --noplugins anyway, but apparently --noplugins ends up initializing an empty rpmPlugins struct, and just falling through with that.

Adding NULL checks isn't the end of the world, just a little annoying indeed.

One observation from a closer look is that we also have various per-te plugin hooks in psm, and those shouldn't be called on an src.rpm either. I don't think they get called currently ... yup, because the src.rpm unpack sneaks its way around it. It's weird that you can add src.rpm's to a transaction but then not do anything with them there. Except okay, check dependencies, that is why it's permitted.

So with all that, perhaps the easiest way is probably to temporarily replace ts->plugins with an empty set during rpmInstallSourcePackage(). It's a bit dirty of course, a cleaner way would be passing plugins through psm to fsm, but that requires changing a whole lot of stuff on the way and we prefer a small backportable patch for this.

Another possibility might be checking for src.rpm centrally from RPMPLUGINS_SET_HOOK_FUNC, we could pass a te/NULL to it and let it "return" NULL hookFunc for any src.rpm.

Hmm 🤔

[dmnks]

Hmm. I thought we wouldn't need any new NULL checks because I thought that's what happens with --noplugins anyway, but apparently --noplugins ends up initializing an empty rpmPlugins struct, and just falling through with that.

Oh, I didn't realize that. That seems like a good precedent for doing just that, instead of messing around with NULL checks just to accommodate for this very use case.

One observation from a closer look is that we also have various per-te plugin hooks in psm, and those shouldn't be called on an src.rpm either. I don't think they get called currently ... yup, because the src.rpm unpack sneaks its way around it. It's weird that you can add src.rpm's to a transaction but then not do anything with them there. Except okay, check dependencies, that is why it's permitted.

Yeah, I don't think any non-fsm hooks get called for src.rpms. Only binary packages are added to the transaction set in rpmInstall() so the code paths eventually separate for binary and source packages (rpmcliTransaction() vs. rpmInstallSource()). The tsm and psm hooks only fire when going through rpmcliTransaction() (rpmtsRun() for the former, rpmpsmRun() for the latter).

Running with --debug confirms this, too (no "calling hook" messages for anything else than init when installing a src.rpm).

So with all that, perhaps the easiest way is probably to temporarily replace ts->plugins with an empty set during rpmInstallSourcePackage().

Why would that be needed, though, assuming the above is true?

Another possibility might be checking for src.rpm centrally from RPMPLUGINS_SET_HOOK_FUNC, we could pass a te/NULL to it and let it "return" NULL hookFunc for any src.rpm.

Right, that would indeed be another way, I'll exercise that as an option 😄

[pmatilai]

Why would that be needed, though, assuming the above is true?

Hmm? rpmInstallSourcePackage() is the route through which any source rpm is "installed" and ends up calling the fsm hooks. So if you swap the ts->plugins for an empty set during that, it should achieve this thing we're trying to do here 😅 But, it does feel kinda dirty.

[dmnks]

Hmm? rpmInstallSourcePackage() is the route through which any source rpm is "installed" and ends up calling the fsm hooks. So if you swap the ts->plugins for an empty set during that, it should achieve this thing we're trying to do here 😅 But, it does feel kinda dirty.

Sure, I was just wondering why you'd suggested that as a solution, instead of what this PR currently does? That said, I agree it would be nicer perhaps 😄

[dmnks]

To clarify a bit:

If we were to go without the NULL checks, we could just return the empty plugins struct in fsmPlugins() instead of returning NULL there. Your suggestion moves that higher in the stack which might be nicer but I don't see any technical reason to prefer one over the other, unless I'm missing something 🤔

[pmatilai]

The issue with fsmPlugins() returning an empty set is that you need to free it too someplace. The one from rpmts gets freed along with the ts itself and cannot be freed from the places where fsmPlugins() is called, whereas the other case has to. Possible, but perhaps a bit murky.

...and hence the suggestion wrt rpmInstallSourcePackage(): creating + freeing it is easily contained. Just ugly otherwise 😅

[dmnks]

Oh, now it makes sense, thanks 😄 Indeed, I haven't thought about freeing it but of course...

[pmatilai]

But yup, handling it in RPMPLUGINS_SET_HOOK_FUNC seems kinda attractive too because it then covers all the plugin hooks, regardless of what gets called just now vs in the future etc. And doesn't have the "hold your nose" factor 😄

[dmnks]

Here's a variant that's a blend of all the above options we've discussed. This one makes use of RPMTRANS_FLAG_NOPLUGINS (i.e. same as --noplugins) to turn off the hooks during src.rpm unpacking. Note that this also includes the init/cleanup hooks, which is a nice side effect.

This way, we don't need to pass around a te argument to every hook function (this would, annoyingly, also impact a couple of the static fsm functions, e.g. ensureDir()) and just making the function signatures longer as a result. After all, we know that we're processing source packages before calling the hooks via rpmInstallSource() so one could argue that it's more natural to handle it at that level (by disabling the plugins), as opposed to checking for rpmteIsSource() within the hooks themselves.

It's also cleaner than swapping out the plugin list with an empty one temporarily (this would have to be done from within rpmInstallSourcePackage(), i.e. once per every src.rpm, since ts->plugins may only be initialized for the first time in that function). Not impossible, just ugly, as you noted 😄

Lastly, as it often happens, one discovers tasty bugs while looking in dusty corners, which is exactly what happened here: It turns out the combination of binary and source packages (e.g. rpm -i foo.rpm bar.src.rpm) never really worked properly, due to a missed rpmtsEmpty() call, so that's also fixed in this PR 😄

Let me know if you like this approach or not. If not, the other workable alternative is passing around the te argument. I have that variant in a topic branch, too, just didn't like it as much, due to the reasons mentioned.

[pmatilai]

Using NOPLUGINS seems fine, but it needs to be set inside rpmInstallSourcePackage() as that's the lowest level public API we have for that.

[dmnks]

Ah right, haven't considered that at all. Sigh 😄

[dmnks]

Let's go with the empty-list variant after all. Since we need to do the disabling as low as in rpmInstallSourcePackage(), the flag would be of no use anyway. This way we actually avoid having to touch anything besides that function, isn't that neat? 🥳

[pmatilai]

I'd call this "origplugins" or something like that to make its purpose clearer.

[dmnks]

Indeed, fixed now.