Re: [Rpm-maint] FSM hooks for rpm plugin
> Should we do the stat passing then for fsmCommit hooks? I am attaching > the current state of my fsmCommit hooks just to show the place where I > was thinking to add them. I haven't checked the tabs and other stuff, > so this is just to give idea. I was thinking that instead of passing > mode_t to the both hooks (in this patch it still does that), the whole > stat stuct can be passed and this would give at least these hooks > enough info. What do you think? Or if is too bad/assymetric to have mode_t > in other hooks and stat here? >I think we better leave the stat struct out of the picture for now. Its not >just that the information in the stat struct would be more than a bit fuzzy, >but also its not really sufficient either. For example a %config management >plugin would need to know >whether a file is a %config or not, and that's >currently not available to plugins without jumping through a lot of hoops. >What we'd really need is being able to pass rpmfi objects to the hooks (in >addition to some other things), but that requires largish >changes in the rpm >internals... But I've talked about that long enough maybe its time to >actually do it. It shouldn't be a particularly *difficult* change, just >fairly large and tedious one. Ok, I will then stick with mode_t for now and then we can get a proper rpmfi object later. >As for the preliminary patch, yes that's what I was thinking of as well >- in particular, calling the pre-commit hook before fsmBackup() so in case a >backup is needed, the pre-commit hook can grab the contents before its moved >out of the way. There are some open questions here too >however: if there's a failure, the post-commit hook doesn't really know >whether it was the backup that failed or the actual commit. True, but question is does it need to differentiate? If backup has failed, commit can't be successful, isn't it? And then there's the special case of directory replacing something else, in which case the backup is (and needs to be) >taken in rpmPackageFilesInstall() already. And then there's the whole erasure business to deal with... >Just wondering if there actually should be *yet another* hook for backups, >which would allow avoiding the ambiguity in fsmCommit() and make plugins >aware of all the scenarios in which backups are taken. So many hooks, sigh... >dunno. Hm... I am not against new hooks, but they are staring to multiply quite fast... I will try to think more about it... In general I think we should not introduce the hooks unless good use case is present, otherwise we will lose track of them and create interfaces that noone else is using. So, the question is does a backup operation is smth plugin should be aware and understand or it is now fully internal to rpm operation. At least we're >not in danger of running out of "supported hooks" bits anymore :) I changed the plugin initialization >+ hook-calling system fairly radically over the weekend as discussed. Nice :) I will take a look on it after I am back from travelling in 1.5 weeks and then will be also able to continue on the patch! Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/28/2013 01:16 PM, Reshetova, Elena wrote: On 03/27/2013 02:34 PM, Reshetova, Elena wrote: After far too much pondering... I went ahead and added the prepare hook + some related bits and pieces. And actually ripped out SELinux + support >from rpm core while at it, replaced by a simple SELinux plugin. Wohoo. Looks cool :) Hope it works ;) The basics seem to be working fine :) The plugin configuration mechanism probably wants a bit more thinking + work though: for some things you'd want to be able to enable a plugin by merely installing the relevant (sub)package. For example I would want the SELinux plugin to get enabled whenever its present, without having to hunt where __transaction_plugins is defined and override it, which is annoying and error-prone. In other words, I think there should be a drop-in directory for the plugin configuration where the plugin sub-packages can drop their default configuration as separate files, including whether they should be enabled by default or not. There was something else in this direction too ... but I can't remember it >right now. Yes, this is what I was wondering also. In our case I have the msm plugin in a separate rpm binary and our build engineers would like to have an easy way to switch it on and off, when building the image. And the most easiest way for them is to just include/not include the rpm plugin package. So, I think having this configurable separately would for sure make them happy and as you said probably would be much more robust than defining this in macros.in file. Right, agreed then. I think I'll leave the commit-hooks to you though :) Ok, I think I will be able to send you a version for review today, but I have got one question. I was under impression that we at some point agreed to pass to hooks the whole stat structure as opposite for just mode_t. This would allow plugins to make checks on things like st_nlink and other useful info about the file. Do I remember this wrongly? No, you are right, but I chickened out :) See http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=354c00ba7558e2dd78dd6f5906d3ba3e4c41e74a http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=3ae4e4087e48e0bbfbafe5c9948bdcbc5fe1c63e The trouble starts with the special case of fsmMkdirs(): there's a struct stat handy, but the directories only get created if the stat() fails, in which case the struct stat contents are undefined. Sure, it'd be possible to hack up a struct stat that resembles a directory for that case, but that rang a "proceed with >caution" alarm bell in my head. If, or rather when, we need to fake up stuff the semantics get fuzzy real quick. For example the st_nlink thing: the adjusted count is currently actually only available in fsmCommit() so different hooks would see different values for the same file. Etc. I still actually think we'll eventually want to pass the whole stat struct (or roughly equivalent amount of data in some other means) to plugins, being able to sanely do so requires further hacking of the FSM. Should we do the stat passing then for fsmCommit hooks? I am attaching the current state of my fsmCommit hooks just to show the place where I was thinking to add them. I haven't checked the tabs and other stuff, so this is just to give idea. I was thinking that instead of passing mode_t to the both hooks (in this patch it still does that), the whole stat stuct can be passed and this would give at least these hooks enough info. What do you think? Or if is too bad/assymetric to have mode_t in other hooks and stat here? I think we better leave the stat struct out of the picture for now. Its not just that the information in the stat struct would be more than a bit fuzzy, but also its not really sufficient either. For example a %config management plugin would need to know whether a file is a %config or not, and that's currently not available to plugins without jumping through a lot of hoops. What we'd really need is being able to pass rpmfi objects to the hooks (in addition to some other things), but that requires largish changes in the rpm internals... But I've talked about that long enough maybe its time to actually do it. It shouldn't be a particularly *difficult* change, just fairly large and tedious one. As for the preliminary patch, yes that's what I was thinking of as well - in particular, calling the pre-commit hook before fsmBackup() so in case a backup is needed, the pre-commit hook can grab the contents before its moved out of the way. There are some open questions here too however: if there's a failure, the post-commit hook doesn't really know whether it was the backup that failed or the actual commit. And then there's the special case of directory replacing something else, in which case the backup is (and needs to be) taken in rpmPackageFilesInstall() already. And then there's the whole erasure business to deal with... Just wondering if there actually should be *yet another* hook fo
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/27/2013 02:34 PM, Reshetova, Elena wrote: > >>> After far too much pondering... I went ahead and added the prepare >>> hook >>> + some related bits and pieces. And actually ripped out SELinux >>> + support >>>from rpm core while at it, replaced by a simple SELinux plugin. Wohoo. >> >> Looks cool :) Hope it works ;) >The basics seem to be working fine :) >The plugin configuration mechanism probably wants a bit more thinking + work >though: for some things you'd want to be able to enable a plugin by merely >installing the relevant (sub)package. For example I would want the SELinux >plugin to get enabled whenever its present, without having to hunt where > >__transaction_plugins is defined and override it, which is annoying and >error-prone. >In other words, I think there should be a drop-in directory for the plugin >configuration where the plugin sub-packages can drop their default >configuration as separate files, including whether they should be enabled by >default or not. There was something else in this direction too ... but I >can't remember it >right now. Yes, this is what I was wondering also. In our case I have the msm plugin in a separate rpm binary and our build engineers would like to have an easy way to switch it on and off, when building the image. And the most easiest way for them is to just include/not include the rpm plugin package. So, I think having this configurable separately would for sure make them happy and as you said probably would be much more robust than defining this in macros.in file. >> I think I'll leave the commit-hooks to you though :) > > Ok, I think I will be able to send you a version for review today, but > I have got one question. I was under impression that we at some point > agreed to pass to hooks the whole stat structure as opposite for just > mode_t. This would allow plugins to make checks on things like > st_nlink and other useful info about the file. Do I remember this wrongly? >No, you are right, but I chickened out :) See >http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=354c00ba7558e2dd78dd6f5906d3ba3e4c41e74a >http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=3ae4e4087e48e0bbfbafe5c9948bdcbc5fe1c63e >The trouble starts with the special case of fsmMkdirs(): there's a struct >stat handy, but the directories only get created if the stat() fails, in >which case the struct stat contents are undefined. Sure, it'd be possible to >hack up a struct stat that resembles a directory for that case, but that rang >a "proceed with >caution" alarm bell in my head. If, or rather when, we need >to fake up stuff the semantics get fuzzy real quick. For example the st_nlink >thing: the adjusted count is currently actually only available in fsmCommit() >so different hooks would see different values for the same file. Etc. >I still actually think we'll eventually want to pass the whole stat struct >(or roughly equivalent amount of data in some other means) to plugins, being >able to sanely do so requires further hacking of the FSM. Should we do the stat passing then for fsmCommit hooks? I am attaching the current state of my fsmCommit hooks just to show the place where I was thinking to add them. I haven't checked the tabs and other stuff, so this is just to give idea. I was thinking that instead of passing mode_t to the both hooks (in this patch it still does that), the whole stat stuct can be passed and this would give at least these hooks enough info. What do you think? Or if is too bad/assymetric to have mode_t in other hooks and stat here? Best Regards, Elena 0001-Adding-FSM_COMMIT_PRE-and-FSM_COMMIT_POST-plugin-hoo.patch Description: Binary data smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/27/2013 02:34 PM, Reshetova, Elena wrote: After far too much pondering... I went ahead and added the prepare hook + some related bits and pieces. And actually ripped out SELinux support from rpm core while at it, replaced by a simple SELinux plugin. Wohoo. Looks cool :) Hope it works ;) The basics seem to be working fine :) The plugin configuration mechanism probably wants a bit more thinking + work though: for some things you'd want to be able to enable a plugin by merely installing the relevant (sub)package. For example I would want the SELinux plugin to get enabled whenever its present, without having to hunt where __transaction_plugins is defined and override it, which is annoying and error-prone. In other words, I think there should be a drop-in directory for the plugin configuration where the plugin sub-packages can drop their default configuration as separate files, including whether they should be enabled by default or not. There was something else in this direction too ... but I can't remember it right now. I think I'll leave the commit-hooks to you though :) Ok, I think I will be able to send you a version for review today, but I have got one question. I was under impression that we at some point agreed to pass to hooks the whole stat structure as opposite for just mode_t. This would allow plugins to make checks on things like st_nlink and other useful info about the file. Do I remember this wrongly? No, you are right, but I chickened out :) See http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=354c00ba7558e2dd78dd6f5906d3ba3e4c41e74a http://rpm.org/gitweb?p=rpm.git;a=commitdiff;h=3ae4e4087e48e0bbfbafe5c9948bdcbc5fe1c63e The trouble starts with the special case of fsmMkdirs(): there's a struct stat handy, but the directories only get created if the stat() fails, in which case the struct stat contents are undefined. Sure, it'd be possible to hack up a struct stat that resembles a directory for that case, but that rang a "proceed with caution" alarm bell in my head. If, or rather when, we need to fake up stuff the semantics get fuzzy real quick. For example the st_nlink thing: the adjusted count is currently actually only available in fsmCommit() so different hooks would see different values for the same file. Etc. I still actually think we'll eventually want to pass the whole stat struct (or roughly equivalent amount of data in some other means) to plugins, being able to sanely do so requires further hacking of the FSM. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>After far too much pondering... I went ahead and added the prepare hook >+ some related bits and pieces. And actually ripped out SELinux support >from rpm core while at it, replaced by a simple SELinux plugin. Wohoo. Looks cool :) Hope it works ;) >I think I'll leave the commit-hooks to you though :) Ok, I think I will be able to send you a version for review today, but I have got one question. I was under impression that we at some point agreed to pass to hooks the whole stat structure as opposite for just mode_t. This would allow plugins to make checks on things like st_nlink and other useful info about the file. Do I remember this wrongly? Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/19/2013 03:49 PM, Reshetova, Elena wrote: Hm... I guess you are right: even without changing fsm, such hook can be introduced now and started to be used right away. But do you want to have one hook (for install and erase) or two hooks (pre- and post-process)? I think having pre- hook isn’t very beneficial, but post-unpack is the needed one. >Maybe we can call it smth like FilePrepare or so to indicate that file is tmp location and hook is called to setup the necessary attributes and check the file? Hmm, I kinda like that: FilePrepare is generic enough that it can be used for both installs (ie post create) and erasures (pre remove). Dunno if its *the* most descriptive name possible but really good names are annoyingly hard to come by with :) It's good enough for now in any case. And yeah, having separate pre/post hooks is probably an overkill for this purpose. BTW I further hacked fsm yesterday to move the metadata setting out of fsmCommit() into the main rpmPackageFilesInstall() loop. It doesn't make much of a difference technically yet, but I think it makes it a bit easier to think about it as a separate step from commit when it actually is :) Yes, I think this is a good idea: committing the file and setting its metadata are indeed two different steps. The only thing that I can't find so far a usage for pre commit hook for future: it would be kind of called on the same context (file is unpacked in tpm dir) and the future metadata/content screening hook One idea can be that for security needs, plugins can actually use pre- and post hooks to verify that permissions were preserved (and set) correctly and abort if they see some mismatch. But maybe this is too paranoid again :) That's perhaps slightly paranoid, yes :) But then its not my job to say what the plugins should be used... for some purpose having yet another layer of verifying might be reasonable. One use-case I see for the pre- and post-commit hooks is a plugin keeping track of config file contents: in pre-commit it would stage the change of content (think of "git apply"), and in post-commit it would either commit or abort (think of "git commit" or >"git reset --hard") depending on whether fsm commit succeeded or not. Yes, I think this might be a better use case than a paranoid plugin :) So, then we have 3 hooks: pre- and post- commit and some kind of filePrepare after unpack. I think this should be smth we will be able to leave with for quite a while even when fsm is changed. Yup, those three hooks are something I think we can reasonably expect to stay semantically the same no matter how much things change underneath. That's perhaps been one of the bigger problems in introducing the file hooks: trying to wedge hooks with sane semantics onto the twisted beast that fsm was (and of course still is, if to slightly lesser degree) ... well, fsm fights back with vengeance every step of the way :) OK, so how should we proceed on this? Will you commit the hooks or should I try to do a patch and send it for review? After far too much pondering... I went ahead and added the prepare hook + some related bits and pieces. And actually ripped out SELinux support from rpm core while at it, replaced by a simple SELinux plugin. Wohoo. I think I'll leave the commit-hooks to you though :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
> > Hm... I guess you are right: even without changing fsm, such hook can be > introduced now and started to be used right away. But do you want to have one > hook (for install and erase) or two hooks (pre- and post-process)? I think > having pre- hook isn’t very beneficial, but post-unpack is the needed one. > >Maybe we can call it smth like FilePrepare or so to indicate that file is > tmp location and hook is called to setup the necessary attributes and check > the file? >Hmm, I kinda like that: FilePrepare is generic enough that it can be used for >both installs (ie post create) and erasures (pre remove). Dunno if its *the* >most descriptive name possible but really good names are annoyingly hard to >come by with :) It's good enough for now in any case. >And yeah, having separate pre/post hooks is probably an overkill for this >purpose. >BTW I further hacked fsm yesterday to move the metadata setting out of >fsmCommit() into the main rpmPackageFilesInstall() loop. It doesn't make much >of a difference technically yet, but I think it makes it a bit easier to think >about it as a separate step from commit when it actually is :) Yes, I think this is a good idea: committing the file and setting its metadata are indeed two different steps. > >> The only thing that I can't find so far a usage for pre commit hook >> for future: it would be kind of called on the same context (file is >> unpacked in tpm dir) and the future metadata/content screening >> hook One idea can be that for security needs, plugins can >> actually use pre- and post hooks to verify that permissions were >> preserved (and >> set) correctly and abort if they see some mismatch. But maybe this is >> too paranoid again :) > >> That's perhaps slightly paranoid, yes :) But then its not my job to say what >> the plugins should be used... for some purpose having yet another layer of >> verifying might be reasonable. > >> One use-case I see for the pre- and post-commit hooks is a plugin keeping >> track of config file contents: in pre-commit it would stage the change of >> content (think of "git apply"), and in post-commit it would either commit or >> abort (think of "git commit" or >"git reset --hard") depending on whether >> fsm commit succeeded or not. > > Yes, I think this might be a better use case than a paranoid plugin :) So, > then we have 3 hooks: pre- and post- commit and some kind of filePrepare > after unpack. I think this should be smth we will be able to leave with for > quite a while even when fsm is changed. >Yup, those three hooks are something I think we can reasonably expect to stay >semantically the same no matter how much things change underneath. >That's perhaps been one of the bigger problems in introducing the file >hooks: trying to wedge hooks with sane semantics onto the twisted beast that >fsm was (and of course still is, if to slightly lesser degree) ... >well, fsm fights back with vengeance every step of the way :) OK, so how should we proceed on this? Will you commit the hooks or should I try to do a patch and send it for review? Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/15/2013 12:45 PM, Reshetova, Elena wrote: On 03/14/2013 03:01 PM, Reshetova, Elena wrote: Sure, I'm not suggesting delaying everything until I someday get around to fixing it, just that we could try thinking ahead for that model to hopefully avoid having to change the plugin interfaces later. I pushed a bunch of fsm changes yesterday, the two >more interesting ones that we already talked about being: 1) Reflect the hardlink count in st_nlink so the real files vs hardlinks can be easily detected 2) Set permissions before committing to the rename to final destination. With 2) in place, we might be able to model the hooks in a way that doesn't require changing later. The question (again) just is, what the hooks should actually be. I think we'd want those pre- and post-commit hooks in any case: for example a %config versioning system plugin would want to know whether a file is being replaced and if it actually succeeded. The pre-commit hook could of course be used for setting >additional permissions, content checking etc as well, but in the alleged new model of unpack + set permissions on all files first and only then commit, I think one would want to abort the whole thing as early as possible. Not that it matters all that much if we really are able to undo the whole thing. So I guess we'll just go with the pre- and post-commit hooks for now to be able to move forward with this. At least no-one can say this hasn't been thoroughly discussed :) I just went through your yesterday's changes. I think it now slowly falls together nicely. I think it is right that we need pre and post fsm hooks because even if we were able to unpack everything and successfully set all permissions on all files in tmp location, it isn't a guarantee that committing the whole thing to the final location would be successful. It is always possible that preserving security labels might fail or anything else might happen. And when you change a fsm model to new one, we can just add a new hook that would be called after each file is unpacked to tpm location: this would be primary hook for setting additional metadata on file and good time to scan the content of a file, too (so that we can revert the whole thing and delete a file if malware is found in it). Yup, and its this part I'm still pondering about: should we just add that post-unpack hook (or whatever its called) already and go with that for SELinux and all from the start, because that's what they really want. That's kinda what the setmetadata hook >idea, but perhaps a more generic name would be appropriate now that it wouldn't be limited to that. Maybe something like file pre- and post-process, which can cover metadata, malware scanning and whatnot, both for install and erase (which needs the >perhaps otherwise unnecessary pre-hook) Hm... I guess you are right: even without changing fsm, such hook can be introduced now and started to be used right away. But do you want to have one hook (for install and erase) or two hooks (pre- and post-process)? I think having pre- hook isn’t very beneficial, but post-unpack is the needed one. Maybe we can call it smth like FilePrepare or so to indicate that file is tmp location and hook is called to setup the necessary attributes and check the file? Hmm, I kinda like that: FilePrepare is generic enough that it can be used for both installs (ie post create) and erasures (pre remove). Dunno if its *the* most descriptive name possible but really good names are annoyingly hard to come by with :) It's good enough for now in any case. And yeah, having separate pre/post hooks is probably an overkill for this purpose. BTW I further hacked fsm yesterday to move the metadata setting out of fsmCommit() into the main rpmPackageFilesInstall() loop. It doesn't make much of a difference technically yet, but I think it makes it a bit easier to think about it as a separate step from commit when it actually is :) The only thing that I can't find so far a usage for pre commit hook for future: it would be kind of called on the same context (file is unpacked in tpm dir) and the future metadata/content screening hook One idea can be that for security needs, plugins can actually use pre- and post hooks to verify that permissions were preserved (and set) correctly and abort if they see some mismatch. But maybe this is too paranoid again :) That's perhaps slightly paranoid, yes :) But then its not my job to say what the plugins should be used... for some purpose having yet another layer of verifying might be reasonable. One use-case I see for the pre- and post-commit hooks is a plugin keeping track of config file contents: in pre-commit it would stage the change of content (think of "git apply"), and in post-commit it would either commit or abort (think of "git commit" or >"git reset --hard") depending on whether fsm commit succeeded or not. Yes, I think this might be a better use case than a paran
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/14/2013 03:01 PM, Reshetova, Elena wrote: >> Sure, I'm not suggesting delaying everything until I someday get >> around to fixing it, just that we could try thinking ahead for that >> model to hopefully avoid having to change the plugin interfaces >> later. I pushed a bunch of fsm changes yesterday, the two >more >> interesting ones that we already talked about being: > >> 1) Reflect the hardlink count in st_nlink so the real files vs >> hardlinks can be easily detected > >> 2) Set permissions before committing to the rename to final destination. > >> With 2) in place, we might be able to model the hooks in a way that >> doesn't require changing later. The question (again) just is, what >> the hooks should actually be. > >> I think we'd want those pre- and post-commit hooks in any case: for >> example a %config versioning system plugin would want to know whether >> a file is being replaced and if it actually succeeded. The pre-commit >> hook could of course be used for setting >additional permissions, >> content checking etc as well, but in the alleged new model of unpack >> + set permissions on all files first and only then commit, I think >> one would want to abort the whole thing as early as possible. > >> Not that it matters all that much if we really are able to undo the >> whole thing. So I guess we'll just go with the pre- and post-commit >> hooks for now to be able to move forward with this. At least no-one >> can say this hasn't been thoroughly discussed :) > > I just went through your yesterday's changes. I think it now slowly > falls together nicely. I think it is right that we need pre and post > fsm hooks because even if we were able to unpack everything and > successfully set all permissions on all files in tmp location, it > isn't a guarantee that committing the whole thing to the final > location would be successful. It is always possible that preserving > security labels might fail or anything else might happen. And when you > change a fsm model to new one, we can just add a new hook that would > be called after each file is unpacked to tpm location: this would be > primary hook for setting additional metadata on file and good time to > scan the content of a file, too (so that we can revert the whole thing and > delete a file if malware is found in it). >Yup, and its this part I'm still pondering about: should we just add that >post-unpack hook (or whatever its called) already and go with that for SELinux >and all from the start, because that's what they really want. That's kinda >what the setmetadata hook >idea, but perhaps a more generic name would be >appropriate now that it wouldn't be limited to that. Maybe something like file >pre- and post-process, which can cover metadata, malware scanning and whatnot, >both for install and erase (which needs the >perhaps otherwise unnecessary >pre-hook) Hm... I guess you are right: even without changing fsm, such hook can be introduced now and started to be used right away. But do you want to have one hook (for install and erase) or two hooks (pre- and post-process)? I think having pre- hook isn’t very beneficial, but post-unpack is the needed one. Maybe we can call it smth like FilePrepare or so to indicate that file is tmp location and hook is called to setup the necessary attributes and check the file? > The only thing that I can't find so far a usage for pre commit hook > for future: it would be kind of called on the same context (file is > unpacked in tpm dir) and the future metadata/content screening > hook One idea can be that for security needs, plugins can actually > use pre- and post hooks to verify that permissions were preserved (and > set) correctly and abort if they see some mismatch. But maybe this is > too paranoid again :) >That's perhaps slightly paranoid, yes :) But then its not my job to say what >the plugins should be used... for some purpose having yet another layer of >verifying might be reasonable. >One use-case I see for the pre- and post-commit hooks is a plugin keeping >track of config file contents: in pre-commit it would stage the change of >content (think of "git apply"), and in post-commit it would either commit or >abort (think of "git commit" or >"git reset --hard") depending on whether fsm >commit succeeded or not. Yes, I think this might be a better use case than a paranoid plugin :) So, then we have 3 hooks: pre- and post- commit and some kind of filePrepare after unpack. I think this should be smth we will be able to leave with for quite a while even when fsm is changed. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/14/2013 03:01 PM, Reshetova, Elena wrote: Sure, I'm not suggesting delaying everything until I someday get around to fixing it, just that we could try thinking ahead for that model to hopefully avoid having to change the plugin interfaces later. I pushed a bunch of fsm changes yesterday, the two >more interesting ones that we already talked about being: 1) Reflect the hardlink count in st_nlink so the real files vs hardlinks can be easily detected 2) Set permissions before committing to the rename to final destination. With 2) in place, we might be able to model the hooks in a way that doesn't require changing later. The question (again) just is, what the hooks should actually be. I think we'd want those pre- and post-commit hooks in any case: for example a %config versioning system plugin would want to know whether a file is being replaced and if it actually succeeded. The pre-commit hook could of course be used for setting >additional permissions, content checking etc as well, but in the alleged new model of unpack + set permissions on all files first and only then commit, I think one would want to abort the whole thing as early as possible. Not that it matters all that much if we really are able to undo the whole thing. So I guess we'll just go with the pre- and post-commit hooks for now to be able to move forward with this. At least no-one can say this hasn't been thoroughly discussed :) I just went through your yesterday's changes. I think it now slowly falls together nicely. I think it is right that we need pre and post fsm hooks because even if we were able to unpack everything and successfully set all permissions on all files in tmp location, it isn't a guarantee that committing the whole thing to the final location would be successful. It is always possible that preserving security labels might fail or anything else might happen. And when you change a fsm model to new one, we can just add a new hook that would be called after each file is unpacked to tpm location: this would be primary hook for setting additional metadata on file and good time to scan the content of a file, too (so that we can revert the whole thing and delete a file if malware is found in it). Yup, and its this part I'm still pondering about: should we just add that post-unpack hook (or whatever its called) already and go with that for SELinux and all from the start, because that's what they really want. That's kinda what the setmetadata hook idea, but perhaps a more generic name would be appropriate now that it wouldn't be limited to that. Maybe something like file pre- and post-process, which can cover metadata, malware scanning and whatnot, both for install and erase (which needs the perhaps otherwise unnecessary pre-hook) The only thing that I can't find so far a usage for pre commit hook for future: it would be kind of called on the same context (file is unpacked in tpm dir) and the future metadata/content screening hook One idea can be that for security needs, plugins can actually use pre- and post hooks to verify that permissions were preserved (and set) correctly and abort if they see some mismatch. But maybe this is too paranoid again :) That's perhaps slightly paranoid, yes :) But then its not my job to say what the plugins should be used... for some purpose having yet another layer of verifying might be reasonable. One use-case I see for the pre- and post-commit hooks is a plugin keeping track of config file contents: in pre-commit it would stage the change of content (think of "git apply"), and in post-commit it would either commit or abort (think of "git commit" or "git reset --hard") depending on whether fsm commit succeeded or not. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>Sure, I'm not suggesting delaying everything until I someday get around to >fixing it, just that we could try thinking ahead for that model to hopefully >avoid having to change the plugin interfaces later. I pushed a bunch of fsm >changes yesterday, the two >more interesting ones that we already talked >about being: >1) Reflect the hardlink count in st_nlink so the real files vs hardlinks can >be easily detected >2) Set permissions before committing to the rename to final destination. >With 2) in place, we might be able to model the hooks in a way that doesn't >require changing later. The question (again) just is, what the hooks should >actually be. >I think we'd want those pre- and post-commit hooks in any case: for example a >%config versioning system plugin would want to know whether a file is being >replaced and if it actually succeeded. The pre-commit hook could of course be >used for setting >additional permissions, content checking etc as well, but >in the alleged new model of unpack + set permissions on all files first and >only then commit, I think one would want to abort the whole thing as early as >possible. >Not that it matters all that much if we really are able to undo the whole >thing. So I guess we'll just go with the pre- and post-commit hooks for now >to be able to move forward with this. At least no-one can say this hasn't >been thoroughly discussed :) I just went through your yesterday's changes. I think it now slowly falls together nicely. I think it is right that we need pre and post fsm hooks because even if we were able to unpack everything and successfully set all permissions on all files in tmp location, it isn't a guarantee that committing the whole thing to the final location would be successful. It is always possible that preserving security labels might fail or anything else might happen. And when you change a fsm model to new one, we can just add a new hook that would be called after each file is unpacked to tpm location: this would be primary hook for setting additional metadata on file and good time to scan the content of a file, too (so that we can revert the whole thing and delete a file if malware is found in it). The only thing that I can't find so far a usage for pre commit hook for future: it would be kind of called on the same context (file is unpacked in tpm dir) and the future metadata/content screening hook One idea can be that for security needs, plugins can actually use pre- and post hooks to verify that permissions were preserved (and set) correctly and abort if they see some mismatch. But maybe this is too paranoid again :) Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/13/2013 01:08 PM, Reshetova, Elena wrote: Do you want to do the changes? I can also try to do it tomorrow if they aren't objections. I probably should merge (at least some of) the "study" and link count patches first, as those change the landscape quite a bit. I'll try to do that as soon as the caffeine kicks in for good. Sure, I will wait for changes. On a somewhat related note, I'm pondering about changing fsm to do staged removals too, ie rename before actually removing. It doesn't make much difference as things are now, but I've also started seriously thinking about changing the fsm to the model we discussed earlier where unpacking and setting >permissions etc is first done for all files, and only if that succeeds completely we actually commit to renaming them all to the final target, and undo the whole lot if anything in unpacking failed. I think this would be the safest way not only from security, but also from correctness and also makes installation more robust in case of sudden power cuts and etc. Indeed. The way rpm currently behaves on failure is just plain embarrassing. ...which of course would actually fundamentally change the landscape again: if commit is changed to consist only of renaming a file, then "commit" hooks would no longer the right place to do security labeling etc. Argh! :) In that model we'd be back to the "set metadata" hook, or actually two of them to preserve the possibility of doing something after rpm did its own business. >And in that model, both pre and post metadata hooks should get the temp and final path as separate arguments. Yeah, but I guess maybe we can first finish with the current system and check that it works for whatever test cases we have (I can start using new hooks in msm plugin) and then change it when you move rpm to a new fsm model. I think this would be a big change for fsm, so won't be possible to do it fast anyway. Sure, I'm not suggesting delaying everything until I someday get around to fixing it, just that we could try thinking ahead for that model to hopefully avoid having to change the plugin interfaces later. I pushed a bunch of fsm changes yesterday, the two more interesting ones that we already talked about being: 1) Reflect the hardlink count in st_nlink so the real files vs hardlinks can be easily detected 2) Set permissions before committing to the rename to final destination. With 2) in place, we might be able to model the hooks in a way that doesn't require changing later. The question (again) just is, what the hooks should actually be. I think we'd want those pre- and post-commit hooks in any case: for example a %config versioning system plugin would want to know whether a file is being replaced and if it actually succeeded. The pre-commit hook could of course be used for setting additional permissions, content checking etc as well, but in the alleged new model of unpack + set permissions on all files first and only then commit, I think one would want to abort the whole thing as early as possible. Not that it matters all that much if we really are able to undo the whole thing. So I guess we'll just go with the pre- and post-commit hooks for now to be able to move forward with this. At least no-one can say this hasn't been thoroughly discussed :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>> Do you want to do the changes? I can also try to do it tomorrow if >> they aren't objections. >I probably should merge (at least some of) the "study" and link count patches >first, as those change the landscape quite a bit. I'll try to do that as soon >as the caffeine kicks in for good. Sure, I will wait for changes. >On a somewhat related note, I'm pondering about changing fsm to do staged >removals too, ie rename before actually removing. It doesn't make much >difference as things are now, but I've also started seriously thinking about >changing the fsm to the model we discussed earlier where unpacking and >setting >permissions etc is first done for all files, and only if that >succeeds completely we actually commit to renaming them all to the final >target, and undo the whole lot if anything in unpacking failed. I think this would be the safest way not only from security, but also from correctness and also makes installation more robust in case of sudden power cuts and etc. >...which of course would actually fundamentally change the landscape >again: if commit is changed to consist only of renaming a file, then "commit" >hooks would no longer the right place to do security labeling etc. Argh! :) >In that model we'd be back to the "set metadata" hook, or actually two of >them to preserve the possibility of doing something after rpm did its own >business. >And in that model, both pre and post metadata hooks should get the >temp and final path as separate arguments. Yeah, but I guess maybe we can first finish with the current system and check that it works for whatever test cases we have (I can start using new hooks in msm plugin) and then change it when you move rpm to a new fsm model. I think this would be a big change for fsm, so won't be possible to do it fast anyway. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/12/2013 03:15 PM, Reshetova, Elena wrote: Setting the permissions before moving would fix that and also avoid replacing a previous file at all in case we fail to in one of the metadata steps. For the stock metadata the actual path makes no difference, but for security labels you'd want the final path though (to avoid having to figure out and strip the >temp extension from the file), so it'd require passing two paths to the pre-commit hook: current and final. Maybe it is the fact that I had to wake up 3am today to come back to Helsinki, but I don't understand why do we need to know the final path for security labels labelling? I don't think file is labelled based on its destination: it is more like based on what is inside package, manifest, device security policies and etc. It's needed for things where the label does not come from the package but system policy, exactly because we lay the disk on temporary name first. At least with SELinux the label is (automatically) set at file/directory creation time based on its path, and rename() does not automatically relabel it. And >because rpm creates the files with temporary names, the initial labels are "always" wrong and we need to manually adjust them for the final path. Ok, I guess you are right, assigning label by path is also a valid way, and there can be restrictions on this. Of course it would be possible to just leave such things for post-commit where we already have the final path, that would be exactly the same as what currently happens. It just means missing the opportunity to get it right early, eliminating one window of live-update breakage (and an opportunity to bail >before committing the file at all on errors). No, I think we agreed already that I would be better to do it before commit, so we can do it right from beginning :) So, should we then conclude on having just two more hooks (pre and post commit) in addition to already existing hooks? I guess your previous exercise on it can be a base and then we can add additional parameters, like path and etc. I guess that'd be the conclusion. At least until we find the next wrinkle in the theory :) Do you want to do the changes? I can also try to do it tomorrow if they aren't objections. I probably should merge (at least some of) the "study" and link count patches first, as those change the landscape quite a bit. I'll try to do that as soon as the caffeine kicks in for good. On a somewhat related note, I'm pondering about changing fsm to do staged removals too, ie rename before actually removing. It doesn't make much difference as things are now, but I've also started seriously thinking about changing the fsm to the model we discussed earlier where unpacking and setting permissions etc is first done for all files, and only if that succeeds completely we actually commit to renaming them all to the final target, and undo the whole lot if anything in unpacking failed. ...which of course would actually fundamentally change the landscape again: if commit is changed to consist only of renaming a file, then "commit" hooks would no longer the right place to do security labeling etc. Argh! :) In that model we'd be back to the "set metadata" hook, or actually two of them to preserve the possibility of doing something after rpm did its own business. And in that model, both pre and post metadata hooks should get the temp and final path as separate arguments. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>> Setting the permissions before moving would fix that and also avoid >> replacing a previous file at all in case we fail to in one of the >> metadata steps. For the stock metadata the actual path makes no >> difference, but for security labels you'd want the final path though >> (to avoid having to figure out and strip the >temp extension from the >> file), so it'd require passing two paths to the pre-commit hook: current >> and final. > > Maybe it is the fact that I had to wake up 3am today to come back to > Helsinki, but I don't understand why do we need to know the final path > for security labels labelling? I don't think file is labelled based on > its destination: it is more like based on what is inside package, > manifest, device security policies and etc. >It's needed for things where the label does not come from the package but >system policy, exactly because we lay the disk on temporary name first. At >least with SELinux the label is (automatically) set at file/directory >creation time based on its path, and rename() does not automatically relabel >it. And >because rpm creates the files with temporary names, the initial >labels are "always" wrong and we need to manually adjust them for the final >path. Ok, I guess you are right, assigning label by path is also a valid way, and there can be restrictions on this. >Of course it would be possible to just leave such things for post-commit >where we already have the final path, that would be exactly the same as what >currently happens. It just means missing the opportunity to get it right >early, eliminating one window of live-update breakage (and an opportunity to >bail >before committing the file at all on errors). No, I think we agreed already that I would be better to do it before commit, so we can do it right from beginning :) So, should we then conclude on having just two more hooks (pre and post commit) in addition to already existing hooks? I guess your previous exercise on it can be a base and then we can add additional parameters, like path and etc. Do you want to do the changes? I can also try to do it tomorrow if they aren't objections. Best Regards, Elena smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/11/2013 02:14 PM, Reshetova, Elena wrote: Yup, rpm pretty much has to trust its plugins. OTOH... this made me think of another related issue: it would actually be better to set the permissions etc before moving the file to its final location. Currently we first move the file and then start setting permissions, which means executables and all will for a >short period of time have totally incorrect permissions, labels and all. So if you happen to execute that binary during that period, who knows what will happen: it could fail to execute at all, execute with wrong capabilities / labels etc. Yes, this would be the safest way of doing it. But it isn't that bad in the current scenario: if your security settings are proper (like labels of rpm itself and etc.), noone would be able to even access the tmp files before the proper labelling is in place. But agree: doing it right from beginning is even better and removes possibility of bad setup. Yup, its probably more of a usability / update robustness issue than security: in the current way of things, files that should be accessible and executable are not so for the small period of time it takes getting from rename() to chown(), chmod() etc. So for example if 'ls' executed precisely at the wrong moment during upgrade of that package, you can get permission denied for the executable. Setting permissions on the temporary name would avoid that unnecessary temporary breakage, but then live-updates can never be truly fixed. Setting the permissions before moving would fix that and also avoid replacing a previous file at all in case we fail to in one of the metadata steps. For the stock metadata the actual path makes no difference, but for security labels you'd want the final path though (to avoid having to figure out and strip the >temp extension from the file), so it'd require passing two paths to the pre-commit hook: current and final. Maybe it is the fact that I had to wake up 3am today to come back to Helsinki, but I don't understand why do we need to know the final path for security labels labelling? I don't think file is labelled based on its destination: it is more like based on what is inside package, manifest, device security policies and etc. It's needed for things where the label does not come from the package but system policy, exactly because we lay the disk on temporary name first. At least with SELinux the label is (automatically) set at file/directory creation time based on its path, and rename() does not automatically relabel it. And because rpm creates the files with temporary names, the initial labels are "always" wrong and we need to manually adjust them for the final path. Of course it would be possible to just leave such things for post-commit where we already have the final path, that would be exactly the same as what currently happens. It just means missing the opportunity to get it right early, eliminating one window of live-update breakage (and an opportunity to bail before committing the file at all on errors). - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>Yup, rpm pretty much has to trust its plugins. OTOH... this made me think of >another related issue: it would actually be better to set the permissions etc >before moving the file to its final location. Currently we first move the >file and then start setting permissions, which means executables and all will >for a >short period of time have totally incorrect permissions, labels and >all. So if you happen to execute that binary during that period, who knows >what will happen: it could fail to execute at all, execute with wrong >capabilities / labels etc. Yes, this would be the safest way of doing it. But it isn't that bad in the current scenario: if your security settings are proper (like labels of rpm itself and etc.), noone would be able to even access the tmp files before the proper labelling is in place. But agree: doing it right from beginning is even better and removes possibility of bad setup. >Setting the permissions before moving would fix that and also avoid replacing >a previous file at all in case we fail to in one of the metadata steps. For >the stock metadata the actual path makes no difference, but for security >labels you'd want the final path though (to avoid having to figure out and >strip the >temp extension from the file), so it'd require passing two paths >to the pre-commit hook: current and final. Maybe it is the fact that I had to wake up 3am today to come back to Helsinki, but I don't understand why do we need to know the final path for security labels labelling? I don't think file is labelled based on its destination: it is more like based on what is inside package, manifest, device security policies and etc. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>A file is a hard-link if (S_ISREG(st->st_mode) && st->st_nlink > 1) is true. >When erasing, we get this info from filesystem so that remains accurate (the >last one would be seen as the "real" file). On installation the stat struct >of a file is made up by rpm, so we can pass whatever we want in there. >Currently >st_nlink for hardlinks equals the total number of links a file >will have, but we can easily change that to the number of *current* links so >that it better matches reality. Ie the first one will have st_nlink == 1 so >it will be seen as the real file, the rest will st_nlink++ each. See attached >patch for a quick >implementation of this. Oh, I guess I just wanted to say that after file is installed there is no way to determine where is the initial file and where is the hard link, but indeed in rpm case for installation, it can be indicated (as your patch does) by rpm, so I guess it is all fine, then :) Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/07/2013 08:05 PM, Panu Matilainen wrote: On 03/07/2013 07:55 PM, Panu Matilainen wrote: On 03/07/2013 04:27 PM, Reshetova, Elena wrote: If instead of hook 2) we have a post fsm commit hook that with proper parameter, I think it can be serving the same purpose. The only thing is that setting permissions before rpm does it isn't really possible in this case, because before fsmcommit() file isn't in final location yet. But again, I guess rpm would always trust its own plugins by a nature of a system setup, so we can assume that plugins won't try to change permissions that would break rpm query or they would break their own system in that case. Yup, rpm pretty much has to trust its plugins. OTOH... this made me think of another related issue: it would actually be better to set the permissions etc before moving the file to its final location. Currently we first move the file and then start setting permissions, which means executables and all will for a short period of time have totally incorrect permissions, labels and all. So if you happen to execute that binary during that period, who knows what will happen: it could fail to execute at all, execute with wrong capabilities / labels etc. Setting the permissions before moving would fix that and also avoid replacing a previous file at all in case we fail to in one of the metadata steps. For the stock metadata the actual path makes no difference, but for security labels you'd want the final path though (to avoid having to figure out and strip the temp extension from the file), so it'd require passing two paths to the pre-commit hook: current and final. ...or we could open an fd to the temporary file and pass that along with the final pathname. At least all the standard metadata + file capabilities + selinux labels are settable via file descriptor as well as by path so for those the fd approach wouldn't be a problem. ...except that doesn't work for symlinks. So scratch that idea. We need to construct both paths in commit anyway so there's no added cost or trouble, just needs changing fsmCommit() a little bit. If the number of arguments starts growing too long, one option could be stuffing them into a struct whose pointer we hand to the hooks. Actually, taking that a little bit further, we could hand an opaque "file info" handle to the hooks and force the hooks to go through an API to get the data, which would allow changing implementation details and adding more data without affecting compatibility. And yes I'm getting ahead of the schedule here, just thinking out loud :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/07/2013 07:55 PM, Panu Matilainen wrote: On 03/07/2013 04:27 PM, Reshetova, Elena wrote: If instead of hook 2) we have a post fsm commit hook that with proper parameter, I think it can be serving the same purpose. The only thing is that setting permissions before rpm does it isn't really possible in this case, because before fsmcommit() file isn't in final location yet. But again, I guess rpm would always trust its own plugins by a nature of a system setup, so we can assume that plugins won't try to change permissions that would break rpm query or they would break their own system in that case. Yup, rpm pretty much has to trust its plugins. OTOH... this made me think of another related issue: it would actually be better to set the permissions etc before moving the file to its final location. Currently we first move the file and then start setting permissions, which means executables and all will for a short period of time have totally incorrect permissions, labels and all. So if you happen to execute that binary during that period, who knows what will happen: it could fail to execute at all, execute with wrong capabilities / labels etc. Setting the permissions before moving would fix that and also avoid replacing a previous file at all in case we fail to in one of the metadata steps. For the stock metadata the actual path makes no difference, but for security labels you'd want the final path though (to avoid having to figure out and strip the temp extension from the file), so it'd require passing two paths to the pre-commit hook: current and final. ...or we could open an fd to the temporary file and pass that along with the final pathname. At least all the standard metadata + file capabilities + selinux labels are settable via file descriptor as well as by path so for those the fd approach wouldn't be a problem. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/07/2013 04:27 PM, Reshetova, Elena wrote: If we now see a proper use case: like pre-commit can be used for content checking or even checksum calculation (I would still probably prefer to calculate it when it is read from archive: feels a bit safer that way, while I understand logically that if file is in tmp location and properly protected, nothing should happen to it anyway). Yup, if you get it directly from the "source" then nothing can be tampering with it, and certainly for at least checksum calculation is going to be more efficient if you can do it on the fly (but then rpm can do multiple checksums already, what's missing is exporting that functionality in a sane way). OTOH if >something can tamper with the file that we're creating then it can do nasty stuff anyway, so its perhaps a bit academic. Yes, I agree, I guess I went too paranoid on this, which I guess happens if you work related to security :) Yeah, occupational hazards. Each job has their own :) Another concrete use-case could be a plugin to put %config files into a version control of some kind. While this too would mostly be interested in actually created/erased files, it might actually want to know about %ghosts and such to be able to snapshot >changes made by admin that might've occurred outside of the package. Guess this needs further thought, or better yet, actually trying to do it, but this might be a use-case for the current hooks as they are now, getting called for all files of a package >whether actually touched or not. This almost drives us to have this set of hooks: 1) pre/post where they are now 2) metadata hook (both install and remove) 3) pre fsm commit hook for checking on a file content () Indeed needs more thinking Naah, we just need more hooks! :P Until we get so many of them that we can't manage them anymore and they are going to live on their own ;) Seriously though, I've started thinking of that very same set of hooks so we're probably not too far off-target here. But now I find myself wondering whether 2) is actually necessary afterall: if we have pre- (and perhaps post too) commit hook, and can pass in some additional information as file action flag >bits... we could have something like FA_SETMETA bit for the entries where its appropriate (ie non-hardlinks I guess), and if there's a post-commit hook as well, the plugins can decide whether to set permissions before or after rpm. If instead of hook 2) we have a post fsm commit hook that with proper parameter, I think it can be serving the same purpose. The only thing is that setting permissions before rpm does it isn't really possible in this case, because before fsmcommit() file isn't in final location yet. But again, I guess rpm would always trust its own plugins by a nature of a system setup, so we can assume that plugins won't try to change permissions that would break rpm query or they would break their own system in that case. Yup, rpm pretty much has to trust its plugins. OTOH... this made me think of another related issue: it would actually be better to set the permissions etc before moving the file to its final location. Currently we first move the file and then start setting permissions, which means executables and all will for a short period of time have totally incorrect permissions, labels and all. So if you happen to execute that binary during that period, who knows what will happen: it could fail to execute at all, execute with wrong capabilities / labels etc. Setting the permissions before moving would fix that and also avoid replacing a previous file at all in case we fail to in one of the metadata steps. For the stock metadata the actual path makes no difference, but for security labels you'd want the final path though (to avoid having to figure out and strip the temp extension from the file), so it'd require passing two paths to the pre-commit hook: current and final. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/07/2013 03:59 PM, Reshetova, Elena wrote:
On 03/06/2013 05:01 PM, Reshetova, Elena wrote:
What's missing is that another call to the hook is needed in
fsmMkdirs() for the unowned directories, and there we should perhaps pass
in the "unowned"
aspect somehow. Having a separate argument for that seems like an
overkill though... maybe we could pass that piece of info in the
action argument instead. One possibility could be adding a new
action, eg FA_CREATEUNOWNED that could be used for unowned
directories (and files, but there aren't any currently). Or we could
define the action as a partial bitfield: leave the existing actions
alone but reserve the "upper" byte for special bits, such as
"unknown". I had some other use-case for turning it into (partial) bitfield
but can't remember what it was right now.
I think bitfield would be better in this case: if one start introduce
actions FA_CREATEUNOWNED, then why not have FA_CREATELINK and etc. I
will add hook calling for mkdirs, but I think it is better that you do
a change of actions in a separate commit.
Sure, and I agree bitfield seems like the better option as it'll allow
cramming a whole lot more information in there. There's a whole lot of
redundancy in the current actions (all the skip cases for example) and some
of the values are totally unused as well.
As the file actions aren't really exported in the API in a way that somebody
could actually be using them, we might be able to get away with just
redefining the whole thing as a bitfield and add the old symbols as defines
or'ed together from the bits. But I'll need to think about it some more, in
any case such >a total conversion is not required in order to add a handful
of bits right now.
Ok, I think if you can add an "unowned" bit to it for a start, this can be a
good beginning and then even the existing pre/post hooks can get one less
argument, which is great.
Sure, will do.
Continue on bit field idea, it would be great for plugins to get the
basic info about the file acted upon from action also: so , if we are
adding unowned bit, should the basic bits indicating hard or sym link
be also supplied in action? Like action could point that it is
creation of symlink or removal of hard link. Is it too bad idea?
Hmm, but we already pass the mode (and planning to pass the whole stat
struct) around, so you can tell whether its a symlink, directory or something
else. Hardlinks are the more special case but you can tell them apart from
others by st_nlinks from the stat struct. Except that you dont necessarily
know which one is "real" file without extra tracking... unless st_nlinks is 1
for real >files (including the first
hardlink) and more for the actual links. I'm not sure if that's the case even
already, but should be possible to arrange at least.
I am quite sure you can't tell it from stat struct which is real file and
which is hard link: the value for st_nlinks is stored in inode and not in
dentry in my understanding, that's why it isn't really easy for plugin to
detect the hard links, so indicator would be a plus...
A file is a hard-link if (S_ISREG(st->st_mode) && st->st_nlink > 1) is
true. When erasing, we get this info from filesystem so that remains
accurate (the last one would be seen as the "real" file). On
installation the stat struct of a file is made up by rpm, so we can pass
whatever we want in there. Currently st_nlink for hardlinks equals the
total number of links a file will have, but we can easily change that to
the number of *current* links so that it better matches reality. Ie the
first one will have st_nlink == 1 so it will be seen as the real file,
the rest will st_nlink++ each. See attached patch for a quick
implementation of this.
- Panu -
diff --git a/lib/fsm.c b/lib/fsm.c
index f28010c..5bd5bfa 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -1052,7 +1052,7 @@ static int fsmMakeLinks(FSM_t fsm, hardLink_t li)
return ec;
}
-static int fsmCommit(FSM_t fsm, int ix, int setmeta);
+static int fsmCommit(FSM_t fsm, int ix, const struct stat * st);
/** \ingroup payload
* Commit hard linked file set atomically.
@@ -1065,8 +1065,7 @@ static int fsmCommitLinks(FSM_t fsm)
const char * nsuffix = fsm->nsuffix;
struct stat * st = &fsm->sb;
int rc = 0;
-int setmeta = 1;
-nlink_t i;
+nlink_t i, link_count = 1;
hardLink_t li;
fsm->path = NULL;
@@ -1081,10 +1080,10 @@ static int fsmCommitLinks(FSM_t fsm)
if (li->filex[i] < 0) continue;
rc = fsmMapPath(fsm, li->filex[i]);
if (!XFA_SKIPPING(fsm->action)) {
- rc = fsmCommit(fsm, li->filex[i], setmeta);
- /* only the first created link needs permissions etc to be set */
+ st->st_nlink = link_count;
+ rc = fsmCommit(fsm, li->filex[i], st);
if (!rc)
- setmeta = 0;
+ link_count++;
}
fsm->path = _free(fsm->path);
li->filex[i] = -1;
@@ -1542,10 +1541,9 @@ static int fsmBackup(FSM_t fsm)
return rc;
}
-static int fsmCommit(F
Re: [Rpm-maint] FSM hooks for rpm plugin
> If we now see a proper use case: like pre-commit can be used for > content checking or even checksum calculation (I would still probably > prefer to calculate it when it is read from archive: feels a bit safer > that way, while I understand logically that if file is in tmp location > and properly protected, nothing should happen to it anyway). >Yup, if you get it directly from the "source" then nothing can be tampering >with it, and certainly for at least checksum calculation is going to be more >efficient if you can do it on the fly (but then rpm can do multiple checksums >already, what's missing is exporting that functionality in a sane way). OTOH >if >something can tamper with the file that we're creating then it can do >nasty stuff anyway, so its perhaps a bit academic. Yes, I agree, I guess I went too paranoid on this, which I guess happens if you work related to security :) >> Another concrete use-case could be a plugin to put %config files into a >> version control of some kind. While this too would mostly be interested in >> actually created/erased files, it might actually want to know about %ghosts >> and such to be able to snapshot >changes made by admin that might've >> occurred outside of the package. Guess this needs further thought, or >> better yet, actually trying to do it, but this might be a use-case for the >> current hooks as they are now, getting called for all files of a package >> >whether actually touched or not. > > This almost drives us to have this set of hooks: > > 1) pre/post where they are now > 2) metadata hook (both install and remove) > 3) pre fsm commit hook for checking on a file content () > > Indeed needs more thinking >Naah, we just need more hooks! :P Until we get so many of them that we can't manage them anymore and they are going to live on their own ;) >Seriously though, I've started thinking of that very same set of hooks so >we're probably not too far off-target here. But now I find myself wondering >whether 2) is actually necessary afterall: if we have pre- (and perhaps post >too) commit hook, and can pass in some additional information as file action >flag >bits... we could have something like FA_SETMETA bit for the entries >where its appropriate (ie non-hardlinks I guess), and if there's a >post-commit hook as well, the plugins can decide whether to set permissions >before or after rpm. If instead of hook 2) we have a post fsm commit hook that with proper parameter, I think it can be serving the same purpose. The only thing is that setting permissions before rpm does it isn't really possible in this case, because before fsmcommit() file isn't in final location yet. But again, I guess rpm would always trust its own plugins by a nature of a system setup, so we can assume that plugins won't try to change permissions that would break rpm query or they would break their own system in that case. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/06/2013 05:01 PM, Reshetova, Elena wrote: >> What's missing is that another call to the hook is needed in >> fsmMkdirs() for the unowned directories, and there we should perhaps pass >> in the "unowned" >> aspect somehow. Having a separate argument for that seems like an >> overkill though... maybe we could pass that piece of info in the >> action argument instead. One possibility could be adding a new >> action, eg FA_CREATEUNOWNED that could be used for unowned >> directories (and files, but there aren't any currently). Or we could >> define the action as a partial bitfield: leave the existing actions >> alone but reserve the "upper" byte for special bits, such as >> "unknown". I had some other use-case for turning it into (partial) bitfield >> but can't remember what it was right now. > > I think bitfield would be better in this case: if one start introduce > actions FA_CREATEUNOWNED, then why not have FA_CREATELINK and etc. I > will add hook calling for mkdirs, but I think it is better that you do > a change of actions in a separate commit. >Sure, and I agree bitfield seems like the better option as it'll allow >cramming a whole lot more information in there. There's a whole lot of >redundancy in the current actions (all the skip cases for example) and some >of the values are totally unused as well. >As the file actions aren't really exported in the API in a way that somebody >could actually be using them, we might be able to get away with just >redefining the whole thing as a bitfield and add the old symbols as defines >or'ed together from the bits. But I'll need to think about it some more, in >any case such >a total conversion is not required in order to add a handful >of bits right now. Ok, I think if you can add an "unowned" bit to it for a start, this can be a good beginning and then even the existing pre/post hooks can get one less argument, which is great. > Continue on bit field idea, it would be great for plugins to get the > basic info about the file acted upon from action also: so , if we are > adding unowned bit, should the basic bits indicating hard or sym link > be also supplied in action? Like action could point that it is > creation of symlink or removal of hard link. Is it too bad idea? >Hmm, but we already pass the mode (and planning to pass the whole stat >struct) around, so you can tell whether its a symlink, directory or something >else. Hardlinks are the more special case but you can tell them apart from >others by st_nlinks from the stat struct. Except that you dont necessarily >know which one is "real" file without extra tracking... unless st_nlinks is 1 >for real >files (including the first >hardlink) and more for the actual links. I'm not sure if that's the case even >already, but should be possible to arrange at least. I am quite sure you can't tell it from stat struct which is real file and which is hard link: the value for st_nlinks is stored in inode and not in dentry in my understanding, that's why it isn't really easy for plugin to detect the hard links, so indicator would be a plus... Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/06/2013 12:34 PM, Reshetova, Elena wrote: I see basically two options: either feeding the file bit by bit to a plugin as we read it, or using a pre-commit hook where the whole thing is unpacked onto disk but before moving to final position. The latter option seems far easier in many ways, both for the >plugin and the rpm-side. OTOH there *could* be uses for hooks that get fed the files chunk-by-chunk, such as to allow custom checksums and the like. Checksums can of course be calculated from the final file in pre-commit too, but then you need to re-read the entire thing. Yeah, in my second email I also started to lean towards the second option. So, it looks like your pre/post commit hooks study might be useful even more. You mentioned before that smth about it didn't feel right, do you have smth in mind or? That something was mostly about trying to fit the symmetric hooks through asymmetric use-hole, but that's not all: I still have this nagging feeling we're missing some "obvious" way of doing this all in a simpler way. Or something to that direction... exposure to the fsm tends to mess up any clear thinking :) If we now see a proper use case: like pre-commit can be used for content checking or even checksum calculation (I would still probably prefer to calculate it when it is read from archive: feels a bit safer that way, while I understand logically that if file is in tmp location and properly protected, nothing should happen to it anyway). Yup, if you get it directly from the "source" then nothing can be tampering with it, and certainly for at least checksum calculation is going to be more efficient if you can do it on the fly (but then rpm can do multiple checksums already, what's missing is exporting that functionality in a sane way). OTOH if something can tamper with the file that we're creating then it can do nasty stuff anyway, so its perhaps a bit academic. Another concrete use-case could be a plugin to put %config files into a version control of some kind. While this too would mostly be interested in actually created/erased files, it might actually want to know about %ghosts and such to be able to snapshot >changes made by admin that might've occurred outside of the package. Guess this needs further thought, or better yet, actually trying to do it, but this might be a use-case for the current hooks as they are now, getting called for all files of a package >whether actually touched or not. This almost drives us to have this set of hooks: 1) pre/post where they are now 2) metadata hook (both install and remove) 3) pre fsm commit hook for checking on a file content () Indeed needs more thinking Naah, we just need more hooks! :P Seriously though, I've started thinking of that very same set of hooks so we're probably not too far off-target here. But now I find myself wondering whether 2) is actually necessary afterall: if we have pre- (and perhaps post too) commit hook, and can pass in some additional information as file action flag bits... we could have something like FA_SETMETA bit for the entries where its appropriate (ie non-hardlinks I guess), and if there's a post-commit hook as well, the plugins can decide whether to set permissions before or after rpm. Yes, needs more thinking :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/06/2013 05:01 PM, Reshetova, Elena wrote: What's missing is that another call to the hook is needed in fsmMkdirs() for the unowned directories, and there we should perhaps pass in the "unowned" aspect somehow. Having a separate argument for that seems like an overkill though... maybe we could pass that piece of info in the action argument instead. One possibility could be adding a new action, eg FA_CREATEUNOWNED that could be used for unowned directories (and files, but there aren't any currently). Or we could define the action as a partial bitfield: leave the existing actions alone but reserve the "upper" byte for special bits, such as "unknown". I had some other use-case for turning it into (partial) bitfield but can't remember what it was right now. I think bitfield would be better in this case: if one start introduce actions FA_CREATEUNOWNED, then why not have FA_CREATELINK and etc. I will add hook calling for mkdirs, but I think it is better that you do a change of actions in a separate commit. Sure, and I agree bitfield seems like the better option as it'll allow cramming a whole lot more information in there. There's a whole lot of redundancy in the current actions (all the skip cases for example) and some of the values are totally unused as well. As the file actions aren't really exported in the API in a way that somebody could actually be using them, we might be able to get away with just redefining the whole thing as a bitfield and add the old symbols as defines or'ed together from the bits. But I'll need to think about it some more, in any case such a total conversion is not required in order to add a handful of bits right now. Continue on bit field idea, it would be great for plugins to get the basic info about the file acted upon from action also: so , if we are adding unowned bit, should the basic bits indicating hard or sym link be also supplied in action? Like action could point that it is creation of symlink or removal of hard link. Is it too bad idea? Hmm, but we already pass the mode (and planning to pass the whole stat struct) around, so you can tell whether its a symlink, directory or something else. Hardlinks are the more special case but you can tell them apart from others by st_nlinks from the stat struct. Except that you dont necessarily know which one is "real" file without extra tracking... unless st_nlinks is 1 for real files (including the first hardlink) and more for the actual links. I'm not sure if that's the case even already, but should be possible to arrange at least. Let me put it this way: I'm not opposed to adding a special flag for hardlinks if it turns out its actually needed, but I'm not really convinced we need it. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>Another issue I just realized is that with this patch, the metadata hook call
>is not under the setmeta condition so it will get called always, ie in cases
>like hardlinks when it specifically should *not* be called :)
>I think the fsm-part should be something like this instead:
@@ -1569,6 +1569,11 @@ static int fsmCommit(FSM_t fsm, int ix, int setmeta)
}
if (setmeta) {
+ /* Run fsm metadata hook for all plugins */
+ if (!rc) {
+ rc = rpmpluginsCallFsmFileMetadata(fsm->plugins,
+ fsm->path, st, fsm->action);
+ }
/* Set file security context (if enabled) */
if (!rc && !getuid()) {
rc = fsmSetSELabel(fsm->sehandle, fsm->path, fsm->sb.st_mode);
Ups, indeed, will fix!
smime.p7s
Description: S/MIME cryptographic signature
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>What's missing is that another call to the hook is needed in fsmMkdirs() for >the unowned directories, and there we should perhaps pass in the "unowned" >aspect somehow. Having a separate argument for that seems like an overkill >though... maybe we could pass that piece of info in the action argument >instead. One possibility could be adding a new action, eg FA_CREATEUNOWNED >that could be used for unowned directories (and files, but there aren't any >currently). Or we could define the action as a partial bitfield: leave the >existing actions alone but reserve the "upper" byte for special bits, such as >"unknown". I had some other use-case for turning it into (partial) bitfield >but can't remember what it was right now. I think bitfield would be better in this case: if one start introduce actions FA_CREATEUNOWNED, then why not have FA_CREATELINK and etc. I will add hook calling for mkdirs, but I think it is better that you do a change of actions in a separate commit. Continue on bit field idea, it would be great for plugins to get the basic info about the file acted upon from action also: so , if we are adding unowned bit, should the basic bits indicating hard or sym link be also supplied in action? Like action could point that it is creation of symlink or removal of hard link. Is it too bad idea? Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/06/2013 03:59 PM, Panu Matilainen wrote:
What's missing is that another call to the hook is needed in fsmMkdirs()
for the unowned directories, and there we should perhaps pass in the
"unowned" aspect somehow. Having a separate argument for that seems like
an overkill though... maybe we could pass that piece of info in the
action argument instead. One possibility could be adding a new action,
eg FA_CREATEUNOWNED that could be used for unowned directories (and
files, but there aren't any currently). Or we could define the action as
a partial bitfield: leave the existing actions alone but reserve the
"upper" byte for special bits, such as "unknown". I had some other
use-case for turning it into (partial) bitfield but can't remember what
it was right now.
Another issue I just realized is that with this patch, the metadata hook
call is not under the setmeta condition so it will get called always, ie
in cases like hardlinks when it specifically should *not* be called :)
I think the fsm-part should be something like this instead:
@@ -1569,6 +1569,11 @@ static int fsmCommit(FSM_t fsm, int ix, int setmeta)
}
if (setmeta) {
+ /* Run fsm metadata hook for all plugins */
+ if (!rc) {
+ rc = rpmpluginsCallFsmFileMetadata(fsm->plugins,
+ fsm->path, st, fsm->action);
+ }
/* Set file security context (if enabled) */
if (!rc && !getuid()) {
rc = fsmSetSELabel(fsm->sehandle, fsm->path, fsm->sb.st_mode);
- Panu -
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/06/2013 01:14 PM, Reshetova, Elena wrote: Hi, I am attaching the fast write-up of a metadata hook. While it is dead simple, there are actually two things that made me think: Heh, seems nothing is as simple as it initially seems in this area. - Should it be called before or after setting the standard metadata, such as owner, caps and etc.? I chose to do it before because I don't think that there is a need for a plugin to mess up with standard metadata that can be transferred by rpm itself, but this is arguable. One might think that in some cases security plugin wants to enforce a stricter policy for example on file capabilities (example: package might not be enough trusted to get CAP_MAC_ADMIN), but then it becomes that plugin and rpm are setting two different things So, might be better to reject installation of such package then instead installing it without the right cap. Good question, coming up with arguments for both cases is not particularly hard. OTOH if plugins are allowed to mess with the standard metadata, rpm -V is going to show failures for changed permissions/ownership etc. Of course we could also do both, but... :) I think calling the hook before rpm sets the metadata is the simpler and saner approach at least for starters. - For the hook arguments, I think it is worth to give whole stat struct to the plugin as we discussed and I decided to keep action, too. Especially when we add this hook on removal, it would be very useful. Agreed. I don't think any other arguments are needed (at least not to SELinux and msm), but maybe I missed smth. What's missing is that another call to the hook is needed in fsmMkdirs() for the unowned directories, and there we should perhaps pass in the "unowned" aspect somehow. Having a separate argument for that seems like an overkill though... maybe we could pass that piece of info in the action argument instead. One possibility could be adding a new action, eg FA_CREATEUNOWNED that could be used for unowned directories (and files, but there aren't any currently). Or we could define the action as a partial bitfield: leave the existing actions alone but reserve the "upper" byte for special bits, such as "unknown". I had some other use-case for turning it into (partial) bitfield but can't remember what it was right now. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
Hi, I am attaching the fast write-up of a metadata hook. While it is dead simple, there are actually two things that made me think: - Should it be called before or after setting the standard metadata, such as owner, caps and etc.? I chose to do it before because I don't think that there is a need for a plugin to mess up with standard metadata that can be transferred by rpm itself, but this is arguable. One might think that in some cases security plugin wants to enforce a stricter policy for example on file capabilities (example: package might not be enough trusted to get CAP_MAC_ADMIN), but then it becomes that plugin and rpm are setting two different things So, might be better to reject installation of such package then instead installing it without the right cap. - For the hook arguments, I think it is worth to give whole stat struct to the plugin as we discussed and I decided to keep action, too. Especially when we add this hook on removal, it would be very useful. I don't think any other arguments are needed (at least not to SELinux and msm), but maybe I missed smth. Best Regards, Elena. 0001-New-FSM-plugin-hook-PLUGINHOOK_FSM_FILE_METADATA_FUN.patch Description: Binary data smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/06/2013 09:42 AM, Reshetova, Elena wrote:
>> I have been thinking about it now and I think having a hook for setting file
>> meta data is a good idea in any case (even if we decide to keep pre/post
>> hooks for some other purpose). It shows much clearer the purpose of the hook
>> and it can be placed nicely >exactly where metadata is set (and together
>> with your latest commit it will be setting things right for hardlinks and
>> etc.).
>> Another thing is if we still need some kind of pre/post hooks for
>> files separately... I was trying to think of use cases beyond just a
>> logging plugin that you were referring before. One more concrete use
>> case that I now need to look to is using plugin >interface for having
>> a package virus scanner. The idea would be that plugin would scan
>> the selected content from the package (native executables, maybe some
>> scripts) and if any malicious pattern is detected, then do smth about
>> it (preferably don't >install the content at all to avoid it to be
>> started even unintentially). The difficulty here is that plugin can't
>> scan the code by itself (especially on mobile device) since it
>> doesn’t have a knowledge to do so, so it would need to pass the
>> content of a file in >chunks to the actual scanning engine on the
>> platform and get result: malware detected or not. Ideally if malware
>> is detected, as a last layer of defence it would be very good not to
>> call fsmcommit on that file and abort the i
nstallation with error to avoid file >to be placed in real filesystem, but I
guess it would be even better if the scan can happen even earlier that we don't
need to abort the package installation in a such nasty way. If this can be done
earlier, then I guess there is no need to pre/post hooks, but >if not, they
might be very much needed together with the update hook that we have already
dismissed (it was used only for checksum calculation).
>
> Actually while thinking more about it, I think the cleanest way is
> toprocess the file when it has been unpacked to tpm location, but
> hasn't been committed yet. The file is already on filesystem, but
> since it is in tpm location and without proper attributes, it is quite
> limited on what it can do. This would make a need of one more hook
> somewhere just before fsmCommit happens. I would put it just after
> this piece of code in fsm.c to make sure it is called only for real
> files and not dirs, symlinks and etc.:
> 1721 if (rc == CPIOERR_ENOENT) {
> 1722 rc = expandRegular(fsm, psm, archive, nodigest);
>
> I think this hook can be called smth like FsmFileCheck() or smth
> likethis and it can be used for content checking or any other checks
> on a file before it is transferred to a final location.
> Do you think it make sense?
>I think the more generic option would be a pre-commit hook somewhat similar to
>what my "study" patch does, where it only gets called for files that are
>actually acted on but where the plugin just needs to filter out directories,
>links etc that its not >interested in. Although there's still the question of
>what to do with hardlinks in these cases:
>the plugin should be at least notified about it somehow.
But if we keep pre-post hooks as they are and have this hook in addition, then
maybe we can provide to a plugin a way to filter hardlinks also in pre-hook?
This can serve as notification of hardlinks existence, and the pre-fsmcommit
hook can be the actual hook for the acted files? Or is it too complicated?
Best Regards,
Elena.
smime.p7s
Description: S/MIME cryptographic signature
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>> Perhaps the problem is the hooks are too generic for their own good now. >> One possibility (that's perhaps more clear with the other cleanup >> work in the patch) is to have a "set additional file metadata" hook, >> as >> *that* is what our current use-cases (SELinux and MSSF) want to do. >> Which should actually be as simple as adding something like this after all >> the fsmChown(), fsmChmod() etc calls: > >> if (!rc) >> rc = rpmpluginsCallFsmFileMeta(fsm->plugins, fsm->path, ...) > >> And actually that brings it right next to where fsmSetSELabel() is currently >> getting called. I've a feeling a symmetric pre/post-hook pair doesn't >> actually make sense for this particular purpose, it only complicates things >> unnecessarily. > >> Thoughts? > > I have been thinking about it now and I think having a hook for > setting file meta data is a good idea in any case (even if we decide > to keep pre/post hooks for some other purpose). It shows much clearer > the purpose of the hook and it can be placed nicely exactly where > metadata is set (and together with your latest commit it will be > setting things right for hardlinks and etc.). >Ok, I think that's settled then: one specific hook for setting the file >metadata and metadata only. Want to do the honors? :) I can add it as well if >you're busy with the other stuff. Both works, but I think I can do it today: I don't have that much currently on my day plate. >Ultimately it should be called for erasures too (obviously before actually >erasing), but to cover all the cases that might need some further changes to >the fsm to make it sane. I guess we could start with just doing the >post-install hook which is by far the >more important and interesting part and >see about the pre-removal later. Ok, always easier to do it step by step. > Another thing is if we still need some kind of pre/post hooks for > files separately... I was trying to think of use cases beyond just a > logging plugin that you were referring before. I do think we need some kind of pre/post hooks for files in any case, but where exactly is the big question :) > One more concrete use > case that I now need to look to is using plugin interface for having a > package virus scanner. The idea would be that plugin would scan the > selected content from the package (native executables, maybe some > scripts) and if any malicious pattern is detected, then do smth about > it (preferably don't install the content at all to avoid it to be > started even unintentially). The difficulty here is that plugin can't > scan the code by itself (especially on mobile device) since it doesn’t > have a knowledge to do so, so it would need to pass the content of a > file in chunks to the actual scanning engine on the platform and get result: > malware detected or not. Ideally if malware is detected, as a last > layer of defence it would be very good not to call fsmcommit on that > file and abort the installation with error to avoid file to be placed > in real filesystem, but I guess it would be even better if the scan > can happen even earlier that we don't need to abort the package > installation in a such nasty way. If this can be done earlier, then I > guess there is no need to pre/post hooks, but if not, they might be > very much needed together with the update hook that we have already > dismissed (it was used only for checksum calculation). >Right, that's a real-world use-case (unlike my "perhaps some logging might >want to do some foobar with them" mumbling :) Scanning early is not really >possible as the rpm API allows (by design) populating and starting the >transaction with headers only, so >that payload can be downloaded during the >transaction. So at least for the foreseeable future it'd be limited to >aborting a malware package from within fsm. >I see basically two options: either feeding the file bit by bit to a plugin as >we read it, or using a pre-commit hook where the whole thing is unpacked onto >disk but before moving to final position. The latter option seems far easier >in many ways, both for the >plugin and the rpm-side. OTOH there *could* be >uses for hooks that get fed the files chunk-by-chunk, such as to allow custom >checksums and the like. >Checksums can of course be calculated from the final file in pre-commit too, >but then you need to re-read the entire thing. Yeah, in my second email I also started to lean towards the second option. So, it looks like your pre/post commit hooks study might be useful even more. You mentioned before that smth about it didn't feel right, do you have smth in mind or? If we now see a proper use case: like pre-commit can be used for content checking or even checksum calculation (I would still probably prefer to calculate it when it is read from archive: feels a bit safer that way, while I understand logically that if file is in tmp location and properly protected, nothing should happen
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/06/2013 09:42 AM, Reshetova, Elena wrote:
I have been thinking about it now and I think having a hook for setting file meta
data is a good idea in any case (even if we decide to keep pre/post hooks for some
other purpose). It shows much clearer the purpose of the hook and it can be placed
nicely >exactly where metadata is set (and together with your latest commit it
will be setting things right for hardlinks and etc.).
Another thing is if we still need some kind of pre/post hooks for files separately... I
was trying to think of use cases beyond just a logging plugin that you were referring
before. One more concrete use case that I now need to look to is using plugin
>interface for having a package virus scanner. The idea would be that plugin would
scan the selected content from the package (native executables, maybe some scripts) and
if any malicious pattern is detected, then do smth about it (preferably don't
>install the content at all to avoid it to be started even unintentially). The
difficulty here is that plugin can't scan the code by itself (especially on mobile
device) since it doesn’t have a knowledge to do so, so it would need to pass the content
of a file in >chunks to the actual scanning engine on the platform and get result:
malware detected or not. Ideally if malware is detected, as a last layer of defence it
would be very good not to call fsmcommit on that file and abort the i
nstallation with error to avoid file >to be placed in real filesystem, but I guess
it would be even better if the scan can happen even earlier that we don't need to
abort the package installation in a such nasty way. If this can be done earlier, then
I guess there is no need to pre/post hooks, but >if not, they might be very much
needed together with the update hook that we have already dismissed (it was used only
for checksum calculation).
Actually while thinking more about it, I think the cleanest way is
toprocess the file when it has been unpacked to tpm location, but hasn't
been committed yet. The file is already on filesystem, but since it is
in tpm location and without proper attributes, it is quite limited on
what it can do. This would make a need of one more hook somewhere just
before fsmCommit happens. I would put it just after this piece of code
in fsm.c to make sure it is called only for real files and not dirs,
symlinks and etc.:
1721 if (rc == CPIOERR_ENOENT) {
1722 rc = expandRegular(fsm, psm, archive, nodigest);
I think this hook can be called smth like FsmFileCheck() or smth
likethis and it can be used for content checking or any other checks on a
file before it is transferred to a final location.
Do you think it make sense?
I think the more generic option would be a pre-commit hook somewhat
similar to what my "study" patch does, where it only gets called for
files that are actually acted on but where the plugin just needs to
filter out directories, links etc that its not interested in. Although
there's still the question of what to do with hardlinks in these cases:
the plugin should be at least notified about it somehow.
- Panu -
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 03/04/2013 10:56 AM, Reshetova, Elena wrote: Looking at this, I just realized that rpm is currently doing chmod(), chown() and all for each hardlink it creates, which just doesn't make sense because ... by the very definition of a hardlink, it doesn't. Probably worth fixing regardless of what we end up doing with hooks, eg additional "setmeta" argument to fsmCommit() whether it should just create or set the additional metadata as well, and have pre/post-commit hooks get that so plugins get notified of all file creations but also can avoid redundant setting of labels etc. FWIW, this part is now pushed to git master, ie for hardlink sets the metadata (permissions etc) is only set once. I think this part looks good and clear, but indeed doesn't help with the below part. I'm going to poke around with this a bit to see what would make most sense, now that I have sufficient selinux plugin code to test it with. Like said, I'm starting to have second thoughts on the skipped files, so I'll probably look at changing the existing hooks to the "commit model" rather than add more: for actually created (and removed) files, the hook semantics would be rather obvious. With skipped (and some delayed and whatnot) files it gets far more convoluted. If it turns out the plugins *really* need the skipped file info as well, we can always add (back) more hooks later on :) Attached is one "study" into this direction, ie hooks are only called for files that are actually acted upon. This is a total diff of multiple commits with some semi-unrelated changes, so its more useful to look at the resulting code more than the diff itself. I'm not going to push this stuff until further discussion + thought, if at all: the more I look and think about this stuff, something about it all just doesn't feel quite right :) It doesn’t look anything bad to me, but you are right below that we are now more like trying to fit these hooks somewhere to keep symmetry without even being sure we need it. Indeed. On the positive side, that "study" made me realize some bugs and other murky stuff in the code, so it wasn't all for nothing :) Perhaps the problem is the hooks are too generic for their own good now. One possibility (that's perhaps more clear with the other cleanup work in the patch) is to have a "set additional file metadata" hook, as *that* is what our current use-cases (SELinux and MSSF) want to do. Which should actually be as simple as adding something like this after all the fsmChown(), fsmChmod() etc calls: if (!rc) rc = rpmpluginsCallFsmFileMeta(fsm->plugins, fsm->path, ...) And actually that brings it right next to where fsmSetSELabel() is currently getting called. I've a feeling a symmetric pre/post-hook pair doesn't actually make sense for this particular purpose, it only complicates things unnecessarily. Thoughts? I have been thinking about it now and I think having a hook for setting file meta data is a good idea in any case (even if we decide to keep pre/post hooks for some other purpose). It shows much clearer the purpose of the hook and it can be placed nicely exactly where metadata is set (and together with your latest commit it will be setting things right for hardlinks and etc.). Ok, I think that's settled then: one specific hook for setting the file metadata and metadata only. Want to do the honors? :) I can add it as well if you're busy with the other stuff. Ultimately it should be called for erasures too (obviously before actually erasing), but to cover all the cases that might need some further changes to the fsm to make it sane. I guess we could start with just doing the post-install hook which is by far the more important and interesting part and see about the pre-removal later. Another thing is if we still need some kind of pre/post hooks for files separately... I was trying to think of use cases beyond just a logging plugin that you were referring before. I do think we need some kind of pre/post hooks for files in any case, but where exactly is the big question :) One more concrete use case that I now need to look to is using plugin interface for having a package virus scanner. The idea would be that plugin would scan the selected content from the package (native executables, maybe some scripts) and if any malicious pattern is detected, then do smth about it (preferably don't install the content at all to avoid it to be started even unintentially). The difficulty here is that plugin can't scan the code by itself (especially on mobile device) since it doesn’t have a knowledge to do so, so it would need to pass the content of a file in chunks to the actual scanning engine on the platform and get result: malware detected or not. Ideally if malware is detected, as a last layer of defence it would be very good not to call fsmcommit on that file and abort the installation with error to avoid file to be placed in real filesystem, but I guess it would
Re: [Rpm-maint] FSM hooks for rpm plugin
>I have been thinking about it now and I think having a hook for setting file
>meta data is a good idea in any case (even if we decide to keep pre/post hooks
>for some other purpose). It shows much clearer the purpose of the hook and it
>can be placed nicely >exactly where metadata is set (and together with your
>latest commit it will be setting things right for hardlinks and etc.).
>Another thing is if we still need some kind of pre/post hooks for files
>separately... I was trying to think of use cases beyond just a logging plugin
>that you were referring before. One more concrete use case that I now need to
>look to is using plugin >interface for having a package virus scanner. The
>idea would be that plugin would scan the selected content from the package
>(native executables, maybe some scripts) and if any malicious pattern is
>detected, then do smth about it (preferably don't >install the content at all
>to avoid it to be started even unintentially). The difficulty here is that
>plugin can't scan the code by itself (especially on mobile device) since it
>doesn’t have a knowledge to do so, so it would need to pass the content of a
>file in >chunks to the actual scanning engine on the platform and get result:
>malware detected or not. Ideally if malware is detected, as a last layer of
>defence it would be very good not to call fsmcommit on that file and abort the
>installation with error to avoid file >to be placed in real filesystem, but I
>guess it would be even better if the scan can happen even earlier that we
>don't need to abort the package installation in a such nasty way. If this can
>be done earlier, then I guess there is no need to pre/post hooks, but >if not,
>they might be very much needed together with the update hook that we have
>already dismissed (it was used only for checksum calculation).
Actually while thinking more about it, I think the cleanest way is to process
the file when it has been unpacked to tpm location, but hasn't been committed
yet. The file is already on filesystem, but since it is in tpm location and
without proper attributes, it is quite limited on what it can do. This would
make a need of one more hook somewhere just before fsmCommit happens. I would
put it just after this piece of code in fsm.c to make sure it is called only
for real files and not dirs, symlinks and etc.:
1721 if (rc == CPIOERR_ENOENT) {
1722 rc = expandRegular(fsm, psm, archive, nodigest);
I think this hook can be called smth like FsmFileCheck() or smth like this and
it can be used for content checking or any other checks on a file before it is
transferred to a final location.
Do you think it make sense?
Best Regards,
Elena.
smime.p7s
Description: S/MIME cryptographic signature
___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
> Looking at this, I just realized that rpm is currently doing chmod(), > chown() and all for each hardlink it creates, which just doesn't make > sense because ... by the very definition of a hardlink, it doesn't. > Probably worth fixing regardless of what we end up doing with hooks, > eg additional "setmeta" argument to fsmCommit() whether it should just > create or set the additional metadata as well, and have > pre/post-commit hooks get that so plugins get notified of all file > creations but also can avoid redundant setting of labels etc. >FWIW, this part is now pushed to git master, ie for hardlink sets the metadata >(permissions etc) is only set once. I think this part looks good and clear, but indeed doesn't help with the below part. > I'm going to poke around with this a bit to see what would make most > sense, now that I have sufficient selinux plugin code to test it with. > Like said, I'm starting to have second thoughts on the skipped files, > so I'll probably look at changing the existing hooks to the "commit model" > rather than add more: for actually created (and removed) files, the > hook semantics would be rather obvious. With skipped (and some delayed > and > whatnot) files it gets far more convoluted. If it turns out the > plugins > *really* need the skipped file info as well, we can always add (back) > more hooks later on :) >Attached is one "study" into this direction, ie hooks are only called for >files that are actually acted upon. This is a total diff of multiple commits >with some semi-unrelated changes, so its more useful to look at the resulting >code more than the diff itself. >I'm not going to push this stuff until further discussion + thought, if at >all: the more I look and think about this stuff, something about it all just >doesn't feel quite right :) It doesn’t look anything bad to me, but you are right below that we are now more like trying to fit these hooks somewhere to keep symmetry without even being sure we need it. >Perhaps the problem is the hooks are too generic for their own good now. >One possibility (that's perhaps more clear with the other cleanup work in the >patch) is to have a "set additional file metadata" hook, as >*that* is what our current use-cases (SELinux and MSSF) want to do. >Which should actually be as simple as adding something like this after all the >fsmChown(), fsmChmod() etc calls: > if (!rc) > rc = rpmpluginsCallFsmFileMeta(fsm->plugins, fsm->path, ...) >And actually that brings it right next to where fsmSetSELabel() is currently >getting called. I've a feeling a symmetric pre/post-hook pair doesn't actually >make sense for this particular purpose, it only complicates things >unnecessarily. >Thoughts? I have been thinking about it now and I think having a hook for setting file meta data is a good idea in any case (even if we decide to keep pre/post hooks for some other purpose). It shows much clearer the purpose of the hook and it can be placed nicely exactly where metadata is set (and together with your latest commit it will be setting things right for hardlinks and etc.). Another thing is if we still need some kind of pre/post hooks for files separately... I was trying to think of use cases beyond just a logging plugin that you were referring before. One more concrete use case that I now need to look to is using plugin interface for having a package virus scanner. The idea would be that plugin would scan the selected content from the package (native executables, maybe some scripts) and if any malicious pattern is detected, then do smth about it (preferably don't install the content at all to avoid it to be started even unintentially). The difficulty here is that plugin can't scan the code by itself (especially on mobile device) since it doesn’t have a knowledge to do so, so it would need to pass the content of a file in chunks to the actual scanning engine on the platform and get result: malware detected or not. Ideally if malware is detected, as a last layer of defence it would be very good not to call fsmcommit on that file and abort the installation with error to avoid file to be placed in real filesystem, but I guess it would be even better if the scan can happen even earlier that we don't need to abort the package installation in a such nasty way. If this can be done earlier, then I guess there is no need to pre/post hooks, but if not, they might be very much needed together with the update hook that we have already dismissed (it was used only for checksum calculation). What do you think? Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 02/27/2013 11:14 AM, Panu Matilainen wrote:
On 02/27/2013 08:18 AM, Reshetova, Elena wrote:
Right, no surprises: there is an issue with hard links :)
Oh, this is smth I should have actually remembered, but forgot :( I
saw the
I forgot the specifics of hardlinks too, no worries :)
issue a while back on our system setup, but since from security labelling
hard links aren't interesting (security context should be set on a file
itself, not a hard link), I simply didn't call hooks for hard links.
But I
agree that it might be important to know that they will be created and
I guess
in this case we would need to know when and where it goes. Actually
not from
labelling point of view, but from general security, a tightly configured
system might have restrictions on where files/dirs/links are created. For
example, it might not allow creating hard links in certain paths (in
order for
system to stay well arranged or for example in order to avoid untrusted
packages creating hard links in sensitive dirs). The thing here is
that such a
check should be probably performed before we start to run FSM and
unpacking
the archive (for the same reason as conflicts).
Yup, restrictions would need to be enforced early on, FSM is too late.
So, then if we consider that
labelling and policy enforcement aren't valid use cases here, we only
have
informational use case left: plugin wants to know the links and their
location
as for any other files. Should we then create separate hook/call same
hook
(with modified parameters) actually at the moment when links are created
(later on)?
Hmm. Good question... arranging pre/post-commit in fsmCommit() hooks
should be trivial. Although by now, I'm actually starting to wonder
whether it really makes sense to call any hooks for skipped files
afterall. The "informational use cases" could just go and look at the
package file list and their states from psm pre/post-hooks.
Looking at this, I just realized that rpm is currently doing chmod(),
chown() and all for each hardlink it creates, which just doesn't make
sense because ... by the very definition of a hardlink, it doesn't.
Probably worth fixing regardless of what we end up doing with hooks, eg
additional "setmeta" argument to fsmCommit() whether it should just
create or set the additional metadata as well, and have pre/post-commit
hooks get that so plugins get notified of all file creations but also
can avoid redundant setting of labels etc.
FWIW, this part is now pushed to git master, ie for hardlink sets the
metadata (permissions etc) is only set once.
I'm going to poke around with this a bit to see what would make most
sense, now that I have sufficient selinux plugin code to test it with.
Like said, I'm starting to have second thoughts on the skipped files, so
I'll probably look at changing the existing hooks to the "commit model"
rather than add more: for actually created (and removed) files, the hook
semantics would be rather obvious. With skipped (and some delayed and
whatnot) files it gets far more convoluted. If it turns out the plugins
*really* need the skipped file info as well, we can always add (back)
more hooks later on :)
Attached is one "study" into this direction, ie hooks are only called
for files that are actually acted upon. This is a total diff of multiple
commits with some semi-unrelated changes, so its more useful to look at
the resulting code more than the diff itself.
I'm not going to push this stuff until further discussion + thought, if
at all: the more I look and think about this stuff, something about it
all just doesn't feel quite right :)
Perhaps the problem is the hooks are too generic for their own good now.
One possibility (that's perhaps more clear with the other cleanup work
in the patch) is to have a "set additional file metadata" hook, as
*that* is what our current use-cases (SELinux and MSSF) want to do.
Which should actually be as simple as adding something like this after
all the fsmChown(), fsmChmod() etc calls:
if (!rc)
rc = rpmpluginsCallFsmFileMeta(fsm->plugins, fsm->path, ...)
And actually that brings it right next to where fsmSetSELabel() is
currently getting called. I've a feeling a symmetric pre/post-hook pair
doesn't actually make sense for this particular purpose, it only
complicates things unnecessarily.
Thoughts?
- Panu -
diff --git a/lib/fsm.c b/lib/fsm.c
index f28010c..aebb885 100644
--- a/lib/fsm.c
+++ b/lib/fsm.c
@@ -463,7 +463,7 @@ static int fsmMapPath(FSM_t fsm, int i)
*/
static int saveHardLink(FSM_t fsm, hardLink_t * linkSet)
{
-struct stat * st = &fsm->sb;
+const struct stat * st = &fsm->sb;
int rc = 0;
int ix = -1;
int j;
@@ -1052,18 +1052,17 @@ static int fsmMakeLinks(FSM_t fsm, hardLink_t li)
return ec;
}
-static int fsmCommit(FSM_t fsm, int ix, int setmeta);
+static int fsmCommitCreate(FSM_t fsm, int ix, const struct stat *st, int setmeta);
/** \ingroup payload
* Commit hard l
Re: [Rpm-maint] FSM hooks for rpm plugin
On 02/27/2013 08:18 AM, Reshetova, Elena wrote: Right, no surprises: there is an issue with hard links :) Oh, this is smth I should have actually remembered, but forgot :( I saw the I forgot the specifics of hardlinks too, no worries :) issue a while back on our system setup, but since from security labelling hard links aren't interesting (security context should be set on a file itself, not a hard link), I simply didn't call hooks for hard links. But I agree that it might be important to know that they will be created and I guess in this case we would need to know when and where it goes. Actually not from labelling point of view, but from general security, a tightly configured system might have restrictions on where files/dirs/links are created. For example, it might not allow creating hard links in certain paths (in order for system to stay well arranged or for example in order to avoid untrusted packages creating hard links in sensitive dirs). The thing here is that such a check should be probably performed before we start to run FSM and unpacking the archive (for the same reason as conflicts). Yup, restrictions would need to be enforced early on, FSM is too late. So, then if we consider that labelling and policy enforcement aren't valid use cases here, we only have informational use case left: plugin wants to know the links and their location as for any other files. Should we then create separate hook/call same hook (with modified parameters) actually at the moment when links are created (later on)? Hmm. Good question... arranging pre/post-commit in fsmCommit() hooks should be trivial. Although by now, I'm actually starting to wonder whether it really makes sense to call any hooks for skipped files afterall. The "informational use cases" could just go and look at the package file list and their states from psm pre/post-hooks. Looking at this, I just realized that rpm is currently doing chmod(), chown() and all for each hardlink it creates, which just doesn't make sense because ... by the very definition of a hardlink, it doesn't. Probably worth fixing regardless of what we end up doing with hooks, eg additional "setmeta" argument to fsmCommit() whether it should just create or set the additional metadata as well, and have pre/post-commit hooks get that so plugins get notified of all file creations but also can avoid redundant setting of labels etc. I'm going to poke around with this a bit to see what would make most sense, now that I have sufficient selinux plugin code to test it with. Like said, I'm starting to have second thoughts on the skipped files, so I'll probably look at changing the existing hooks to the "commit model" rather than add more: for actually created (and removed) files, the hook semantics would be rather obvious. With skipped (and some delayed and whatnot) files it gets far more convoluted. If it turns out the plugins *really* need the skipped file info as well, we can always add (back) more hooks later on :) Regardless of how exactly the hardlinks are handled, perhaps we should pass a pointer to the entire stat struct instead of just st_mode to the hooks, that'd at least allow plugins to know they're dealing with hardlinks (and various other possibly useful >information). I guess this is true, should be easy to change. A different issue (much easier and one that we already discussed iirc) is that I think we must check the return code from rpmpluginsCallFsmFilePost() and allow it to fail, otherwise its not possible to preserve the current behavior where eg failure to set SELinux context (or other similar security thing) causes package installation to fail. I am always very supportive for giving more power to the plugin :) I think that it is possible to setup a system that failure to set a security context for files from the package might not result in security compromise, but then it might make package unusable and the effort required to do this is much bigger (we would need to play with security context of running rpm, which isn't that great). So, yes, I guess failing in this case is the easiest way. Nod. Also since rpm itself will abort the install if setting metadata fails, it seems only fair that plugins have the same power. The good news is that other than the two above issues, the plugin API seems to work quite nicely for SELinux. These are really good news! I was expecting more problems actually in beginning. Heh. I wasn't actually expecting much problems because the selinux stuff is pretty simple and straightforward, especially compared to the MSSF stuff. But its always good to see theory and practise mostly agreeing :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>Right, no surprises: there is an issue with hard links :) Oh, this is smth I should have actually remembered, but forgot :( I saw the issue a while back on our system setup, but since from security labelling hard links aren't interesting (security context should be set on a file itself, not a hard link), I simply didn't call hooks for hard links. But I agree that it might be important to know that they will be created and I guess in this case we would need to know when and where it goes. Actually not from labelling point of view, but from general security, a tightly configured system might have restrictions on where files/dirs/links are created. For example, it might not allow creating hard links in certain paths (in order for system to stay well arranged or for example in order to avoid untrusted packages creating hard links in sensitive dirs). The thing here is that such a check should be probably performed before we start to run FSM and unpacking the archive (for the same reason as conflicts). So, then if we consider that labelling and policy enforcement aren't valid use cases here, we only have informational use case left: plugin wants to know the links and their location as for any other files. Should we then create separate hook/call same hook (with modified parameters) actually at the moment when links are created (later on)? >Regardless of how exactly the hardlinks are handled, perhaps we should pass a >pointer to the entire stat struct instead of just st_mode to the hooks, >that'd at least allow plugins to know they're dealing with hardlinks (and >various other possibly useful >information). I guess this is true, should be easy to change. >A different issue (much easier and one that we already discussed iirc) is >that I think we must check the return code from >rpmpluginsCallFsmFilePost() and allow it to fail, otherwise its not possible >to preserve the current behavior where eg failure to set SELinux context (or >other similar security thing) causes package installation to fail. I am always very supportive for giving more power to the plugin :) I think that it is possible to setup a system that failure to set a security context for files from the package might not result in security compromise, but then it might make package unusable and the effort required to do this is much bigger (we would need to play with security context of running rpm, which isn't that great). So, yes, I guess failing in this case is the easiest way. >The good news is that other than the two above issues, the plugin API seems >to work quite nicely for SELinux. These are really good news! I was expecting more problems actually in beginning. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 02/22/2013 11:25 AM, Panu Matilainen wrote: On 02/20/2013 11:01 AM, Reshetova, Elena wrote: I think it looks much better now and integrating hooks to it is a pleasure. I am attaching the new version. Hope I didn't miss any strange case, but it looked very easy now after your change! I'm actually going to be mildly surprised if there aren't any strange cases we've missed wrt hard links or such :) Anyway, the patch looks as obviously-correct as it gets within fsm. Applied, thanks for the patch! Right, no surprises: there is an issue with hard links :) The problem is that the links are created in one go when we come to the final link of a set, but the hooks are getting called for all the links and will likely fail because the path passed to the post-hook doesn't exist despite fsm->action being FA_CREATE. How to deal with it is another question though: it could be worked around by passing eg FA_SKIP to the hooks for these files, but then plugins might want to know these files *are* actually going to be created, only later. We could introduce FA_DELAYED or such to signal the condition but then the plugins still wont know when exactly they are going to be created. And then because hardlinks are quite special, many plugins would probably want to differentiate them from other cases (eg for selinux, it makes no sense to repeatedly set a security context when a hardlink set can only have one context). OTOH some other plugins might want to know about them. Dunno... needs more thought (and a less tired mind to think with :) Regardless of how exactly the hardlinks are handled, perhaps we should pass a pointer to the entire stat struct instead of just st_mode to the hooks, that'd at least allow plugins to know they're dealing with hardlinks (and various other possibly useful information). A different issue (much easier and one that we already discussed iirc) is that I think we must check the return code from rpmpluginsCallFsmFilePost() and allow it to fail, otherwise its not possible to preserve the current behavior where eg failure to set SELinux context (or other similar security thing) causes package installation to fail. The good news is that other than the two above issues, the plugin API seems to work quite nicely for SELinux. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 02/22/2013 11:51 AM, Reshetova, Elena wrote: I'm actually going to be mildly surprised if there aren't any strange cases we've missed wrt hard links or such :) Anyway, the patch looks as obviously-correct as it gets within fsm. Applied, thanks for the patch! Yeah, but I guess we will hopefully find it out soon, when we will be using the hooks :) Also with this patch in place, I think all the required bits and pieces for moving the entire selinux support into a plugin are there now. Wohoo :) This is actually my next question: how should we proceed? There are still some hooks (like handling conflicts and policy enforcement on signatures) that I would like to be present in rpm (I think other systems like SELinux might benefit from them too), and also we were thinking about rearranging the plugin initialization and etc. Or should we first try to move the SELinux to plugin to verify that the basic set of hooks works for it? My plan is to get the SELinux plugin done ASAP, just to see how things work out in practise. On paper the current hook set should be easily sufficient to for the job, but you never *really* know until you try it :) Dont worry about the SELinux stuff, I'll take care of that. So feel free to start looking into the remaining hooks and plugin initialization rework - they shouldn't affect the SELinux plugin much as all the initialization it needs can happen (and does in my prototype plugin) from TSM_PRE. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
>I'm actually going to be mildly surprised if there aren't any strange cases >we've missed wrt hard links or such :) Anyway, the patch looks as >obviously-correct as it gets within fsm. Applied, thanks for the patch! Yeah, but I guess we will hopefully find it out soon, when we will be using the hooks :) >Also with this patch in place, I think all the required bits and pieces for >moving the entire selinux support into a plugin are there now. Wohoo :) This is actually my next question: how should we proceed? There are still some hooks (like handling conflicts and policy enforcement on signatures) that I would like to be present in rpm (I think other systems like SELinux might benefit from them too), and also we were thinking about rearranging the plugin initialization and etc. Or should we first try to move the SELinux to plugin to verify that the basic set of hooks works for it? Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 02/20/2013 11:01 AM, Reshetova, Elena wrote: Hi, Hi, sorry about the delay... the recent patch-flood on rpm-maint caught me by surprise :) Patch flood is always good, total silence is much worse :) Heh, yup :) I've cleaned it up somewhat now, for example the early return was just plain wrong as it would've leaked resources all over the place. But then it also was a case that could never be reached at all... The code still looks suspicious in many places and wants further inspection and sanitizing but achieving symmetrical behavior for the hooks might actually be possible now. At least its *closer* to that target if not there yet :) I think it looks much better now and integrating hooks to it is a pleasure. I am attaching the new version. Hope I didn't miss any strange case, but it looked very easy now after your change! I'm actually going to be mildly surprised if there aren't any strange cases we've missed wrt hard links or such :) Anyway, the patch looks as obviously-correct as it gets within fsm. Applied, thanks for the patch! Also with this patch in place, I think all the required bits and pieces for moving the entire selinux support into a plugin are there now. Wohoo :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
Hi, >Hi, sorry about the delay... the recent patch-flood on rpm-maint caught me by >surprise :) Patch flood is always good, total silence is much worse :) >I've cleaned it up somewhat now, for example the early return was just plain >wrong as it would've leaked resources all over the place. But then it also >was a case that could never be reached at all... >The code still looks suspicious in many places and wants further inspection >and sanitizing but achieving symmetrical behavior for the hooks might >actually be possible now. At least its *closer* to that target if not there >yet :) I think it looks much better now and integrating hooks to it is a pleasure. I am attaching the new version. Hope I didn't miss any strange case, but it looked very easy now after your change! Best Regards, Elena. 0001-Adding-FSM-file-hooks.patch Description: Binary data smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 02/05/2013 12:59 PM, Reshetova, Elena wrote: Hi, Hi, sorry about the delay... the recent patch-flood on rpm-maint caught me by surprise :) Here is the new version attached. The reason I didn't attach it in the last email was because I couldn't rearrange the " this particular jungle " in any reasonable way and was still wondering what to do with it :) But I will wait for you now to brush it first! I am leaving this peace untouched from previous version, but other places are adjusted as discussed. Yup, the jungle and the beasts within need to be tamed first :) I've cleaned it up somewhat now, for example the early return was just plain wrong as it would've leaked resources all over the place. But then it also was a case that could never be reached at all... The code still looks suspicious in many places and wants further inspection and sanitizing but achieving symmetrical behavior for the hooks might actually be possible now. At least its *closer* to that target if not there yet :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
Hi, Here is the new version attached. The reason I didn't attach it in the last email was because I couldn't rearrange the " this particular jungle " in any reasonable way and was still wondering what to do with it :) But I will wait for you now to brush it first! I am leaving this peace untouched from previous version, but other places are adjusted as discussed. Best Regards, Elena -Original Message- From: Panu Matilainen [mailto:pmati...@laiskiainen.org] Sent: Monday, February 04, 2013 3:42 PM To: Reshetova, Elena Cc: rpm-maint@lists.rpm.org Subject: Re: [Rpm-maint] FSM hooks for rpm plugin On 02/04/2013 02:18 PM, Reshetova, Elena wrote: > Hi, > > >> I've started to wonder about the hook names though: "open" and "close" >> seem out of place especially here (and to lesser degree elsewhere). >> Perhaps these hooks should simply follow the pre/post pattern even in >> the names, ie FILE_PRE and FILE_POST. I think it'd be more >> descriptive wrt what they actually do, and leave open and close free >> for possible later use for the cases where a kind of open+close >> (files from payload >> stream) is actually occurring. > > Sure, this is an easy change and I just did it. But the one below has > made me a headache :( > >> This gets quite ugly indeed, better refactor the surrounding code >> (which is ugly in itself) to allow for nice clean pairing. Oh and >> btw, this version of the patch introduces a new asymmetry case: if >> the open-hook fails, close-hook wont get called. It'll need something >> like open-hook just flagging postpone >>> without terminating the loop, and adjusting the rest of the code to >>> deal >> with it. I'll have a look at this, how hard can it be ;) > > I have written and rewritten now this part and still it looks ugly or > even worse :( Flagging postpone is actually easy as well as not > breaking out of loop in the beginning, but the early return is a small > nightmare. If you introduce a variable tracking this, smth like > "hardbreak" and setting it at that point, then you have a number of > stupid if() causes, which make the whole constructions completely > unreadable in addition to handling postpone and rc status (I have to > walk thought this with pen and big white sheet to track the cases). > Other option is "goto xyz" to bypass this but I am not sure this is any > better, but might be easier to read. What do you think is better? I meant that I'll have a look at streamlining this particular jungle first in a way that (hopefully) makes adding the hooks easy or at least easier. There are all sorts of suspicious spots in there... > I have also fixed the other two issues you pointed in the other email. Ok, good... but the patch seems to be missing from your mail :) - Panu - 0001-Adding-FSM-file-hooks.patch Description: Binary data smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 02/04/2013 02:18 PM, Reshetova, Elena wrote: Hi, I've started to wonder about the hook names though: "open" and "close" seem out of place especially here (and to lesser degree elsewhere). Perhaps these hooks should simply follow the pre/post pattern even in the names, ie FILE_PRE and FILE_POST. I think it'd be more descriptive wrt what they actually do, and leave open and close free for possible later use for the cases where a kind of open+close (files from payload stream) is actually occurring. Sure, this is an easy change and I just did it. But the one below has made me a headache :( This gets quite ugly indeed, better refactor the surrounding code (which is ugly in itself) to allow for nice clean pairing. Oh and btw, this version of the patch introduces a new asymmetry case: if the open-hook fails, close-hook wont get called. It'll need something like open-hook just flagging postpone without terminating the loop, and adjusting the rest of the code to deal with it. I'll have a look at this, how hard can it be ;) I have written and rewritten now this part and still it looks ugly or even worse :( Flagging postpone is actually easy as well as not breaking out of loop in the beginning, but the early return is a small nightmare. If you introduce a variable tracking this, smth like "hardbreak" and setting it at that point, then you have a number of stupid if() causes, which make the whole constructions completely unreadable in addition to handling postpone and rc status (I have to walk thought this with pen and big white sheet to track the cases). Other option is "goto xyz" to bypass this but I am not sure this is any better, but might be easier to read. What do you think is better? I meant that I'll have a look at streamlining this particular jungle first in a way that (hopefully) makes adding the hooks easy or at least easier. There are all sorts of suspicious spots in there... I have also fixed the other two issues you pointed in the other email. Ok, good... but the patch seems to be missing from your mail :) - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
Hi, >I've started to wonder about the hook names though: "open" and "close" >seem out of place especially here (and to lesser degree elsewhere). >Perhaps these hooks should simply follow the pre/post pattern even in the >names, ie FILE_PRE and FILE_POST. I think it'd be more descriptive wrt what >they actually do, and leave open and close free for possible later use for >the cases where a kind of open+close (files from payload >stream) is actually occurring. Sure, this is an easy change and I just did it. But the one below has made me a headache :( >This gets quite ugly indeed, better refactor the surrounding code (which is >ugly in itself) to allow for nice clean pairing. Oh and btw, this version of >the patch introduces a new asymmetry case: if the open-hook fails, close-hook >wont get called. It'll need something like open-hook just flagging postpone > >without terminating the loop, and adjusting the rest of the code to deal >with it. I'll have a look at this, how hard can it be ;) I have written and rewritten now this part and still it looks ugly or even worse :( Flagging postpone is actually easy as well as not breaking out of loop in the beginning, but the early return is a small nightmare. If you introduce a variable tracking this, smth like "hardbreak" and setting it at that point, then you have a number of stupid if() causes, which make the whole constructions completely unreadable in addition to handling postpone and rc status (I have to walk thought this with pen and big white sheet to track the cases). Other option is "goto xyz" to bypass this but I am not sure this is any better, but might be easier to read. What do you think is better? I have also fixed the other two issues you pointed in the other email. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 01/28/2013 02:47 PM, Reshetova, Elena wrote: Hi, I am attaching the next version of the patch with modifications explained inline below. The patch looks deceptively simple :) In principle things look ok to me, the issues I see have mostly to do with the symmetry part: In fsmMkdirs(), the FileOpen() hook is not called, only rpmpluginsCallFsmFileClose(). If we promise symmetry, we need to stick to it here too. Looks like just an oversight and trivial to fix. Yes, fixed now: I was looking to install/remove mostly and forgot about symmetry on this one. BTW, just spotted two other (new) issues regarding fsmMkdirs() in this version of the patch: passing fsm->action to fsmMkdirs() and from there to plugin hooks is not right, its probably FA_UNKNOWN at that state. Considering what fsmMkdirs() does, the right thing to do is passing an explicit FA_CREATE to the hooks. The other issue is a different kind of symmetry issue: + + /* Run fsm file open hook for all plugins */ + rc = rpmpluginsCallFsmFileOpen(plugins, dn, mode, DIR_TYPE_UNOWNED, action); + if (rc) + break; + rc = fsmMkdir(dn, mode); + + /* Run fsm file closed hook for all plugins */ + rpmpluginsCallFsmFileClose(plugins, dn, mode, DIR_TYPE_UNOWNED, action, rc); If the FileOpen() hook returns an error, this will bail out of the loop before FileClose() gets called. I'd think it'd be sufficient to do just rc = rpmpluginsCallFsmFileOpen(...); if (!rc) rc = fsmMkdir(dn, mode); rpmpluginsCallFsmFileClose(...); ...as it'll break out of the loop on non-zero rc in the next step anyway. - Panu - - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 01/28/2013 02:47 PM, Reshetova, Elena wrote: Hi, I am attaching the next version of the patch with modifications explained inline below. The patch looks deceptively simple :) In principle things look ok to me, the issues I see have mostly to do with the symmetry part: In fsmMkdirs(), the FileOpen() hook is not called, only rpmpluginsCallFsmFileClose(). If we promise symmetry, we need to stick to it here too. Looks like just an oversight and trivial to fix. Yes, fixed now: I was looking to install/remove mostly and forgot about symmetry on this one. Ok. I've started to wonder about the hook names though: "open" and "close" seem out of place especially here (and to lesser degree elsewhere). Perhaps these hooks should simply follow the pre/post pattern even in the names, ie FILE_PRE and FILE_POST. I think it'd be more descriptive wrt what they actually do, and leave open and close free for possible later use for the cases where a kind of open+close (files from payload stream) is actually occurring. rpmPackageFilesRemove() looks like symmetry is indeed guaranteed, but the problem here is that the hooks will get called whether the file/directory is actually removed or not, and the hook does has no way of knowing it: passing FSM_PKGERASE (or FSM_PKGINSTALL on install) is not sufficient. Either we need to pass fsm->action (those FA_CREATE, FA_ERASE, FA_SKIP etc things) to the hooks, or alternatively only call the hooks when an actual action is taken. Or even both: a plugin might want to know if eg a backup is taken. Not sure which option is the best here: from plugin POV it'd certainly be simpler to not have to worry about files that rpm has decided to skip entirely (there shouldn't be need to label etc such things). But then there might be use-cases where plugins would want to know the fate of each file, including >skipped. It's not exactly hard to do "if (XFA_SKIPPED(action))" in plugins that dont care about skipped files either. I think it would make sense to call a plugin anyway but supply the action. It is indeed easy for plugin to filter actions it wants to do smth about and it is hard to see all potential use cases. So, I am passing the action now as a parameter and then it is up to plugin to react on this. Yup, makes sense. It does expose some fairly murky internals (notably action on RPMFILE_GHOST items not always matching what is actually done to them, possibly something else as well) to plugins, but the problem here is the murky internals, not the information being passed to plugins. So we'll need a bit of cleaning in the internals but that's for the better anyway. And then there's the PITA also known as rpmPackageFilesInstall(). There are at least two cases where FileClose() hook could be skipped despite FileOpen() getting called (early "return" before expandRegular() and a "break" in the post-processing to rmdir/unlink failed file. At least the latter should be reasonably easy to refactor, the early return case .. guess I need to actually figure out what on earth its about in the first place :) Hm... I guess both can be fixed by manually calling fileclosed() hook before return or break (like in this version of the patcj), but it doesn't look that clean and nice. :( The verify() function before early return is hard to read for me, it seems to do million things and even called a number of times during installation. Another option would be to call fileOpen() hook later, but then one can't stop cleanly in emergency cases. This gets quite ugly indeed, better refactor the surrounding code (which is ugly in itself) to allow for nice clean pairing. Oh and btw, this version of the patch introduces a new asymmetry case: if the open-hook fails, close-hook wont get called. It'll need something like open-hook just flagging postpone without terminating the loop, and adjusting the rest of the code to deal with it. I'll have a look at this, how hard can it be ;) And like said, the same question whether to call the hooks for skipped files is true here as well, but probably somewhat trickier as there are more details and cases to handle. Would passing the action to all hooks help to it? Yup, passing the action like done in this patch version makes sense for installs too. I think the plugins should be allowed to cause an abort here, just like rpm itself can, in case of fatal unexpected failure. Unexpected is the key really: what I complained about was the MSM plugin *planning* to fail here on unresolved file conflicts, which is something that should be handled before anything gets installed or removed. Yes, I agree and I think the only reason before was the signature check hook wasn't called early enough, but this is one of the next todo items to do it properly :) Yup. - Panu - ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
Hi, I am attaching the next version of the patch with modifications explained inline below. >The patch looks deceptively simple :) In principle things look ok to me, the issues I see have mostly to do with the symmetry part: >In fsmMkdirs(), the FileOpen() hook is not called, only rpmpluginsCallFsmFileClose(). If we promise symmetry, we need to stick to it here too. Looks like just an oversight and trivial to fix. Yes, fixed now: I was looking to install/remove mostly and forgot about symmetry on this one. >rpmPackageFilesRemove() looks like symmetry is indeed guaranteed, but the problem here is that the hooks will get called whether the file/directory is actually removed or not, and the hook does has no way of knowing it: passing FSM_PKGERASE (or FSM_PKGINSTALL on install) is not sufficient. Either we >need to pass fsm->action (those FA_CREATE, FA_ERASE, FA_SKIP etc things) to the hooks, or alternatively only call the hooks when an actual action is taken. Or even both: a plugin might want to know if eg a backup is taken. >Not sure which option is the best here: from plugin POV it'd certainly be simpler to not have to worry about files that rpm has decided to skip entirely (there shouldn't be need to label etc such things). But then there might be use-cases where plugins would want to know the fate of each file, including >skipped. It's not exactly hard to do "if (XFA_SKIPPED(action))" in plugins that dont care about skipped files either. I think it would make sense to call a plugin anyway but supply the action. It is indeed easy for plugin to filter actions it wants to do smth about and it is hard to see all potential use cases. So, I am passing the action now as a parameter and then it is up to plugin to react on this. >And then there's the PITA also known as rpmPackageFilesInstall(). There are at least two cases where FileClose() hook could be skipped despite >FileOpen() getting called (early "return" before expandRegular() and a "break" in the post-processing to rmdir/unlink failed file. At least the latter should be reasonably easy to refactor, the early return case .. >guess I need to actually figure out what on earth its about in the first place :) Hm... I guess both can be fixed by manually calling fileclosed() hook before return or break (like in this version of the patcj), but it doesn't look that clean and nice. :( The verify() function before early return is hard to read for me, it seems to do million things and even called a number of times during installation. Another option would be to call fileOpen() hook later, but then one can't stop cleanly in emergency cases. >And like said, the same question whether to call the hooks for skipped files is true here as well, but probably somewhat trickier as there are more details and cases to handle. Would passing the action to all hooks help to it? >I think the plugins should be allowed to cause an abort here, just like >rpm itself can, in case of fatal unexpected failure. Unexpected is the >key really: what I complained about was the MSM plugin *planning* to >fail here on unresolved file conflicts, which is something that should >be handled before anything gets installed or removed. Yes, I agree and I think the only reason before was the signature check hook wasn't called early enough, but this is one of the next todo items to do it properly :) Best Regards, Elena. 0001-Adding-FSM-file-hooks.patch Description: Binary data smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 01/22/2013 04:36 PM, Reshetova, Elena wrote: Hi, Hi, Long time again since I replied :( Unfortunately had to resolve a number of other issues and wanted to attach smth already to this mail as opposite to just "reply". No worries. I have started from FSM hooks as you indicated and I am including the initial version of patch for review based on our discussion. I have two hooks: fileOpen and fileClose and call them separately for install and erase. I had to make a number of choices while writing this patch, let's see if they were good ones :) Some details: - I tried to keep the logic of other hooks: if pre_hook is called, post_hook is also called with the result of the operation. However, it is a bit trickier in fsm case. For that purpose, I moved the fileclose hook in installation out of fsmCommit() that we can nicely pass the result to the hook. I also think it looks better from symmetry point of view, but it does now perfom labelling of a file (if it happens inside of a plugin) not exactly at the same place where Selinux currently does it. The exact place of current selinux labeling doesn't really matter, that's not an issue. The patch looks deceptively simple :) In principle things look ok to me, the issues I see have mostly to do with the symmetry part: In fsmMkdirs(), the FileOpen() hook is not called, only rpmpluginsCallFsmFileClose(). If we promise symmetry, we need to stick to it here too. Looks like just an oversight and trivial to fix. rpmPackageFilesRemove() looks like symmetry is indeed guaranteed, but the problem here is that the hooks will get called whether the file/directory is actually removed or not, and the hook does has no way of knowing it: passing FSM_PKGERASE (or FSM_PKGINSTALL on install) is not sufficient. Either we need to pass fsm->action (those FA_CREATE, FA_ERASE, FA_SKIP etc things) to the hooks, or alternatively only call the hooks when an actual action is taken. Or even both: a plugin might want to know if eg a backup is taken. Not sure which option is the best here: from plugin POV it'd certainly be simpler to not have to worry about files that rpm has decided to skip entirely (there shouldn't be need to label etc such things). But then there might be use-cases where plugins would want to know the fate of each file, including skipped. It's not exactly hard to do "if (XFA_SKIPPED(action))" in plugins that dont care about skipped files either. And then there's the PITA also known as rpmPackageFilesInstall(). There are at least two cases where FileClose() hook could be skipped despite FileOpen() getting called (early "return" before expandRegular() and a "break" in the post-processing to rmdir/unlink failed file. At least the latter should be reasonably easy to refactor, the early return case .. guess I need to actually figure out what on earth its about in the first place :) And like said, the same question whether to call the hooks for skipped files is true here as well, but probably somewhat trickier as there are more details and cases to handle. - I also made it that result from fileclose hook is ignored currently for the same reason as for post_tsm and post_psm hooks: what can rpm do after file has been committed even if plugin is unhappy? Yup. This is fine with me. -The tricky part is what to do with the result code of fileOpen hook. In principle, this can be the place to abort installation/erasure of a concrete file in case smth really terrible happened (can't even think what can happen). Normally plugins should not abort anything on this hook (as we discussed) and if they do, then smth is wrong in plugin. On the other hand, rpm itself is physically able to abort at that point and even does it in cases for example if smth wrong with the archive unpacking. So, I am not really sure what to do with the return code in this case. I think the plugins should be allowed to cause an abort here, just like rpm itself can, in case of fatal unexpected failure. Unexpected is the key really: what I complained about was the MSM plugin *planning* to fail here on unresolved file conflicts, which is something that should be handled before anything gets installed or removed. Unfortunately there's no way to enforce this kind of semantic in the API we'll just have to settle for documenting the intention I guess. - I was also thinking that it is probably not worth making it initially more complicated and adding additional hooks, like for handling the temporal files, because they can't really help fully with the security part: we might succeed setting whatever label on tpm file, but fail a second after on real file, or not succeed setting a label even on tmp file. I guess these hooks can be added on demand or simply later if the strong need comes. Fine with me, better get the basics working first and if it turns out that's not enough, then we can think what to do about it. - Panu - __
Re: [Rpm-maint] FSM hooks for rpm plugin
Hi, Long time again since I replied :( Unfortunately had to resolve a number of other issues and wanted to attach smth already to this mail as opposite to just "reply". I have started from FSM hooks as you indicated and I am including the initial version of patch for review based on our discussion. I have two hooks: fileOpen and fileClose and call them separately for install and erase. I had to make a number of choices while writing this patch, let's see if they were good ones :) Some details: - I tried to keep the logic of other hooks: if pre_hook is called, post_hook is also called with the result of the operation. However, it is a bit trickier in fsm case. For that purpose, I moved the fileclose hook in installation out of fsmCommit() that we can nicely pass the result to the hook. I also think it looks better from symmetry point of view, but it does now perfom labelling of a file (if it happens inside of a plugin) not exactly at the same place where Selinux currently does it. - I also made it that result from fileclose hook is ignored currently for the same reason as for post_tsm and post_psm hooks: what can rpm do after file has been committed even if plugin is unhappy? -The tricky part is what to do with the result code of fileOpen hook. In principle, this can be the place to abort installation/erasure of a concrete file in case smth really terrible happened (can't even think what can happen). Normally plugins should not abort anything on this hook (as we discussed) and if they do, then smth is wrong in plugin. On the other hand, rpm itself is physically able to abort at that point and even does it in cases for example if smth wrong with the archive unpacking. So, I am not really sure what to do with the return code in this case. - I was also thinking that it is probably not worth making it initially more complicated and adding additional hooks, like for handling the temporal files, because they can't really help fully with the security part: we might succeed setting whatever label on tpm file, but fail a second after on real file, or not succeed setting a label even on tmp file. I guess these hooks can be added on demand or simply later if the strong need comes. Best Regards, Elena. 0001-Adding-FSM-file-hooks.patch Description: Binary data smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] FSM hooks for rpm plugin
On 01/09/2013 08:19 PM, Reshetova, Elena wrote: Hmm, do you mean FSM_INIT hook is the "pre" hook and FSM_COMMIT the "post" hook, or that both INIT and COMMIT have their own separate pre- and post-hooks? Sorry for being unclear, I just meant that for FSM it is better to have two hooks before file is put to the filesystem and after as opposite to one hook. Ah, yup. Hooks both before and after create/remove is not only better but necessary I think. If the former, its not symmetric as fsmCommit() does not get called on removed files, and there are almost certainly other (at least error) paths where fsmCommit() wont get called. Of course we can make the removal-path call FSM_COMMIT hook despite not being in fsmCommit(), but ... perhaps the hook names should follow the pre/post convention then, eg FSM_FILE_PRE and FSM_FILE_POST. In either case, I think file removals should be covered by the hooks too: you might want to strip security-related labels or such before actually removing, eg to prevent "leaks" through hardlinks (see removeSBITS() in fsm.c). I didn't think of removal cases originally, this is really a good point that we should take them into account, too. The safe choice won't be to strip the security-related label in this case, because in hardlink case this might open a hole in a system. For example, if my package has file "sensitive_data" and plugin labelled it "Sensitive" upon installation. During run-time only processes that have access to "Sensitive" can read/write this file. But suppose attacker would make a hard link to the file and then somehow initiate a package uninstall. Plugin will remove the file (the first path to it), strip the label and then attacker can access the file via hardlink because file isn't protected with the correct label anymore. Moreover in MAC systems you can't just remove a label, since everything has to have a label, you can only change label. So, I guess the correct way in this case would be to change the label to some special "Isolated" label that no one has any rights to read/write/execute. Then even if hardlink is left, it is unusable after package uninstallation. For MAC labels and the like, yes. For some other purposes (eg if we were to move the posix file capabilities handling into a plugin) you'd want to strip. While the immediate prime motivations for this stuff revolve around MAC systems, lets try to keep other possibilities in mind :) Perhaps the hooks should take an additional argument to indicate the operation - create/remove at least, but depending on other things skip might be needed as well. OK, so then I will make two hooks FSM_FILE_PRE and FSM_FILE_POST that would be always called for any file (both install and remove). I will pass the info about the operation type to the hook together with path and mode. I guess FSM_FILE_PRE can be called just after FsmInit(), but what would be the correct place for FSM_FILE_POST? Seems like I would have to call it twice based on operation performed either in commit() or unlink(). Hmm. It might actually be better to have both install and erase case individually call the hooks where appropriate for better control over symmetry and such: for example in erase case, fsmInit() can and often will get called without it ever unlinking anything. The install case is likely to be quite tricky to get just right because there are all sorts of failure paths and whatnot, but fsmCommit() is probably where the post hook needs to be. Actually makes me wonder whether it should be the place for pre-hook as well. There are all sorts of questions and issues here, depending what exactly we want. For the current in-rpm selinux implementation, having both PRE and POST hooks called from fsmCommit() would be sufficient I think. But if you want to use temporary labels while the files are getting unpacked... you might actually need yet another set of similar hooks, eg FSM_UNPACK_PRE|POST that get called when the temporary unpacking suffix is in use, and additional FSM_FILE_PRE|POST calls when the file is getting moved into its final place in fsmCommit(). Yup, path and mode is minimalism to the extreme :) Being able to sanely pass rpmfi objects would be nice but it requires bunch of other changes to happen first. Yes, I guess other arguments would have to wait for changes to come. BTW it should be possible to pass rpmfi objects if we want even now. It just needs a bit of wrapping to deal with the iterator index madness to ensure the hooks get called with correct indexes and save + restore the former values on entry/exit to the hooks. IIRC there are some funny little quirks related to how rpmfiSetFX() and friends "work", so perhaps its better to concentrate on the "simple arguments only" version first. Rpm checks signatures when reading packages/headers, so unless signature checking is disabled, the headers that were fed to rpmtsAddInstall/EraseElement() are already signature checked
Re: [Rpm-maint] FSM hooks for rpm plugin
>Hmm, do you mean FSM_INIT hook is the "pre" hook and FSM_COMMIT the "post" hook, or that both INIT and COMMIT have their own separate pre- and post-hooks? Sorry for being unclear, I just meant that for FSM it is better to have two hooks before file is put to the filesystem and after as opposite to one hook. >If the former, its not symmetric as fsmCommit() does not get called on removed files, and there are almost certainly other (at least error) paths where fsmCommit() wont get called. Of course we can make the removal-path call FSM_COMMIT hook despite not being in fsmCommit(), but ... perhaps the >hook names should follow the pre/post convention then, eg FSM_FILE_PRE and FSM_FILE_POST. >In either case, I think file removals should be covered by the hooks >too: you might want to strip security-related labels or such before actually removing, eg to prevent "leaks" through hardlinks (see >removeSBITS() in fsm.c). I didn't think of removal cases originally, this is really a good point that we should take them into account, too. The safe choice won't be to strip the security-related label in this case, because in hardlink case this might open a hole in a system. For example, if my package has file "sensitive_data" and plugin labelled it "Sensitive" upon installation. During run-time only processes that have access to "Sensitive" can read/write this file. But suppose attacker would make a hard link to the file and then somehow initiate a package uninstall. Plugin will remove the file (the first path to it), strip the label and then attacker can access the file via hardlink because file isn't protected with the correct label anymore. Moreover in MAC systems you can't just remove a label, since everything has to have a label, you can only change label. So, I guess the correct way in this case would be to change the label to some special "Isolated" label that no one has any rights to read/write/execute. Then even if hardlink is left, it is unusable after package uninstallation. >Perhaps the hooks should take an additional argument to indicate the operation - create/remove at least, but depending on other things skip might be needed as well. OK, so then I will make two hooks FSM_FILE_PRE and FSM_FILE_POST that would be always called for any file (both install and remove). I will pass the info about the operation type to the hook together with path and mode. I guess FSM_FILE_PRE can be called just after FsmInit(), but what would be the correct place for FSM_FILE_POST? Seems like I would have to call it twice based on operation performed either in commit() or unlink(). >Yup, path and mode is minimalism to the extreme :) Being able to sanely >pass rpmfi objects would be nice but it requires bunch of other changes >to happen first. Yes, I guess other arguments would have to wait for changes to come. >Rpm checks signatures when reading packages/headers, so unless signature >checking is disabled, the headers that were fed to >rpmtsAddInstall/EraseElement() are already signature checked. With >caveats. Eg. while rpm generally assumes the re-opened package within >transaction is the same thing as initially added to the transaction, >technially the callback can hand us something else. In which case things >are likely to go not very well :) And then there's the fact that rpm >doesn't have a mechanism to actually enforce signed packages only, etc. This is smth I didn't know about rpm: never looked into that part before: was thinking that signature is checked per package one by one. >But this reminds me... >The root issue with MSM delaying conflict checking until its basically >too late was insufficient information available during rpmtsPrepare(): >at that point rpm has long since ditched the header object, which you'd >need to get the MSM-specific data to decide if something should be >allowed or not. And the next time the header is available is indeed only >inside the psm/fsm stage deep inside already running transaction. >I think we need to have additional hook(s) in the >rpmtsAddInstall/EraseElement() area where the full header is available >for the cases where additional data is needed by plugins. There's just a >slight problem: these hooks would be called long before the plugins are >currently even initialized, so the plugin initialization would have to >change quite significantly. Yes, I guess if we go this path, then plugin initialization would have to happen in rpmtsCreate(). Currently there is only memory for pointer allocated, but I guess nothing prevents us from doing the loading of plugins here too. What do you think? I can move the loading here from rpmtsRun() and also then add hooks in addinstall and EraseElement. So, these hooks will be called per each package in transaction and before even transaction starts, and then from header h I can pass the needed info to the hook, which would make the plugin to be aware of sw source earlier than now. So, then the conflict hook is
Re: [Rpm-maint] FSM hooks for rpm plugin
On 01/08/2013 02:26 PM, Reshetova, Elena wrote: Hi Panu, Hi! I finally got back to file hooks and tried to look into this with a fresh head. Fresh head often helps. I'm afraid my head is a bit too fresh now, as in "details blissfully forgotten" :) I'll need to go back and re-read our previous discussions on the topic, but some preliminary comments... I think given our previous discussion, I would propose to have only two symmetrical hooks inside FSM for plugins. I would also make rpm to ignore any return code from them now, since there isn't much that we can really do even if they return some failure. FSM_INIT (const char* path, mode_t mode) Called after fsm.Init() has finished, can be used by plugins to get pre-warned that this file will be now installed to filesystem. Currently in msm plugin this hook is used very wrongly in a sense that it attempts to stop the file writing. I am looking forward to change this, but first I would need to resolve the conflict hook problem (see below). However, when the need to do this ugly functionality in this hook goes away, I think plugins might still be able to benefit from the hook or at least for symmetry looks (we do have pre and post hooks for ts and te). FSM_COMMIT (const char* path, mode_t mode) - Called inside fsm.commit(), can be used by plugins to perform file labelling Hmm, do you mean FSM_INIT hook is the "pre" hook and FSM_COMMIT the "post" hook, or that both INIT and COMMIT have their own separate pre- and post-hooks? If the former, its not symmetric as fsmCommit() does not get called on removed files, and there are almost certainly other (at least error) paths where fsmCommit() wont get called. Of course we can make the removal-path call FSM_COMMIT hook despite not being in fsmCommit(), but ... perhaps the hook names should follow the pre/post convention then, eg FSM_FILE_PRE and FSM_FILE_POST. In either case, I think file removals should be covered by the hooks too: you might want to strip security-related labels or such before actually removing, eg to prevent "leaks" through hardlinks (see removeSBITS() in fsm.c). Perhaps the hooks should take an additional argument to indicate the operation - create/remove at least, but depending on other things skip might be needed as well. - in the future it would be nice to pass to this hook also fidigest and digestalgo that plugins also can access the digest of the file that got written to fs and do additional labelling (like signing the file based on digest for IMA or smth like this). Yup, path and mode is minimalism to the extreme :) Being able to sanely pass rpmfi objects would be nice but it requires bunch of other changes to happen first. In addition to these two FSM hooks, we do need a file conflict hook in order to be able to prevent packages rewriting each other files when rpm is run with --replacefiles mode. Previously you mentioned that when the hook is called, signatures of all packages have been already verified, so in principle it should not be a problem to access at this point the info about who signed this particular package that brings this file. However, I can't understand how could it already be verified, if the hook is called inside rpmtsPrepare(). Rpm checks signatures when reading packages/headers, so unless signature checking is disabled, the headers that were fed to rpmtsAddInstall/EraseElement() are already signature checked. With caveats. Eg. while rpm generally assumes the re-opened package within transaction is the same thing as initially added to the transaction, technially the callback can hand us something else. In which case things are likely to go not very well :) And then there's the fact that rpm doesn't have a mechanism to actually enforce signed packages only, etc. But this reminds me... The root issue with MSM delaying conflict checking until its basically too late was insufficient information available during rpmtsPrepare(): at that point rpm has long since ditched the header object, which you'd need to get the MSM-specific data to decide if something should be allowed or not. And the next time the header is available is indeed only inside the psm/fsm stage deep inside already running transaction. I think we need to have additional hook(s) in the rpmtsAddInstall/EraseElement() area where the full header is available for the cases where additional data is needed by plugins. There's just a slight problem: these hooks would be called long before the plugins are currently even initialized, so the plugin initialization would have to change quite significantly. You mentioned that with future changes and introduction of an object we might be able to get needed parameters passed to the hook, but I think I don't understand how it will be working while looking to the code. Could you please explain a bit on this? If you're referring to the unified package object absraction I think I mentioned at some poin
[Rpm-maint] FSM hooks for rpm plugin
Hi Panu, I finally got back to file hooks and tried to look into this with a fresh head. I think given our previous discussion, I would propose to have only two symmetrical hooks inside FSM for plugins. I would also make rpm to ignore any return code from them now, since there isn't much that we can really do even if they return some failure. FSM_INIT (const char* path, mode_t mode) Called after fsm.Init() has finished, can be used by plugins to get pre-warned that this file will be now installed to filesystem. Currently in msm plugin this hook is used very wrongly in a sense that it attempts to stop the file writing. I am looking forward to change this, but first I would need to resolve the conflict hook problem (see below). However, when the need to do this ugly functionality in this hook goes away, I think plugins might still be able to benefit from the hook or at least for symmetry looks (we do have pre and post hooks for ts and te). FSM_COMMIT (const char* path, mode_t mode) - Called inside fsm.commit(), can be used by plugins to perform file labelling - in the future it would be nice to pass to this hook also fidigest and digestalgo that plugins also can access the digest of the file that got written to fs and do additional labelling (like signing the file based on digest for IMA or smth like this). In addition to these two FSM hooks, we do need a file conflict hook in order to be able to prevent packages rewriting each other files when rpm is run with --replacefiles mode. Previously you mentioned that when the hook is called, signatures of all packages have been already verified, so in principle it should not be a problem to access at this point the info about who signed this particular package that brings this file. However, I can't understand how could it already be verified, if the hook is called inside rpmtsPrepare(). You mentioned that with future changes and introduction of an object we might be able to get needed parameters passed to the hook, but I think I don't understand how it will be working while looking to the code. Could you please explain a bit on this? For FSM hooks, if there are no objections, I can send you a patch tomorrow for a review. Best Regards, Elena. smime.p7s Description: S/MIME cryptographic signature ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
