Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
Okay, very nearly good to go, I was just about to hit merge before realizing that the commit message is not at all the same as the PR description. Please explain the use-case in the commit message: basically the PR description plus a brief explanation of the issues involved and why this plugin is needed. The history of that needs to be recorded in the commit message, not bugzilla/GH discussions. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475#issuecomment-769058115___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka pushed 1 commit. e34dbab3dcff2035ca8c0ba7a80e3454b1d429dc Added fapolicyd plugin -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475/files/9b36cd27e0442c3e8dd2eb391bed7aab237b7e90..e34dbab3dcff2035ca8c0ba7a80e3454b1d429dc ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka pushed 1 commit. 9b36cd27e0442c3e8dd2eb391bed7aab237b7e90 Added fapolicyd plugin -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475/files/83b75dbe12a8d778089c01e0fd9f3e08fc0fe02e..9b36cd27e0442c3e8dd2eb391bed7aab237b7e90 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka pushed 1 commit. 83b75dbe12a8d778089c01e0fd9f3e08fc0fe02e Added fapolicyd plugin -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475/files/46bae1b13ba12e9d40f70f378c6846a881047c53..83b75dbe12a8d778089c01e0fd9f3e08fc0fe02e ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai commented on this pull request.
> +(void) close(fapolicyd_state.fd);
+
+fapolicyd_state.fd = -1;
+}
+
+
+static rpmRC fapolicyd_psm_pre(rpmPlugin plugin, rpmte te)
+{
+if (fapolicyd_state.fd == -1)
+goto end;
+
+if (rpmteType(te) == TR_ADDED)
+fapolicyd_state.installed_package = 1;
+else
+fapolicyd_state.installed_package = 0;
+
Oh, actually: tracking this isn't really necessary, as fsm_file_prepare() will
only ever be called for packages that are being installed.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#pullrequestreview-577363917___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
Some minor stylistic nits, but other that it seems quite fine to me now. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475#issuecomment-768301915___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai commented on this pull request.
> +if (!S_ISFIFO(s.st_mode)) {
+rpmlog(RPMLOG_DEBUG, "File: %s exists but it is not a pipe!\n",
state->fifo_path);
+goto bad;
+}
+
+/* keep only file's permition bits */
+mode_t mode = s.st_mode & ~S_IFMT;
+
+/* we require pipe to have 0660 permission */
+if (mode != 0660) {
+
+rpmlog(RPMLOG_ERR, "File: %s has %o instead of 0660 \n",
+state->fifo_path,
+mode );
+
+goto bad;
Using empty lines for grouping and as visual aid is totally okay, but these two
seem redundant/to the contrary to me at least.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#pullrequestreview-577358310___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai commented on this pull request.
> +};
+
+static struct fapolicyd_data fapolicyd_state = {
+.fd = -1,
+.installed_package = -1,
+.changed_files = 0,
+.fifo_path = "/run/fapolicyd/fapolicyd.fifo",
+};
+
+static rpmRC open_fifo(struct fapolicyd_data* state)
+{
+int fd = -1;
+struct stat s;
+
+fd = open(state->fifo_path, O_RDWR);
+if(fd == -1) {
There's a missing space after the if.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#pullrequestreview-577356301___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai commented on this pull request.
> +
+static rpmRC open_fifo(struct fapolicyd_data* state)
+{
+int fd = -1;
+struct stat s;
+
+fd = open(state->fifo_path, O_RDWR);
+if(fd == -1) {
+rpmlog(RPMLOG_DEBUG, "Open: %s -> %s\n", state->fifo_path,
strerror(errno));
+goto bad;
+}
+
+if (stat(state->fifo_path, ) == -1) {
+rpmlog(RPMLOG_DEBUG, "Stat: %s -> %s\n", state->fifo_path,
strerror(errno));
+goto bad;
+} else {
This else-branch is redundant now with the gotos. Not wrong, but mildly
confusing when there's no need for it.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#pullrequestreview-577355121___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai commented on this pull request.
> +/* send a signal that transaction is over */
+(void) write_fifo(_state, "1\n");
+/* flush cache */
+(void) write_fifo(_state, "2\n");
+}
+
+ end:
+return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_scriptlet_pre(rpmPlugin plugin, const char *s_name,
+ int type)
+{
+if (fapolicyd_state.fd > 0 &&
+fapolicyd_state.changed_files > 0 &&
+fapolicyd_state.installed_package ) {
The if condition is on same indentation level as the following code, making it
hard to see where the condition stops and code starts. There are various ways
to deal with that, my favourite generally is moving (parts of) the condition
into a helper variable, with well-named variables this has the effect of making
the code self-documenting as well. For example:
```
int need_flush = (fapolicyd_state.changed_files > 0) &&
fapolicyd_state.installed_package;
if (fapolicyd_state.fd > 0 && need_flush) {
...
}
```
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#pullrequestreview-577351791___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka pushed 1 commit. 46bae1b13ba12e9d40f70f378c6846a881047c53 Added fapolicyd plugin -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475/files/abb063fee6ac36439ce1a1a0550261b2c5d7972b..46bae1b13ba12e9d40f70f378c6846a881047c53 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka pushed 1 commit. abb063fee6ac36439ce1a1a0550261b2c5d7972b Added fapolicyd plugin -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475/files/f64c8f5afc1f17e2e3f757a5cf12e700bebbad1d..abb063fee6ac36439ce1a1a0550261b2c5d7972b ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka commented on this pull request.
> +
+static rpmRC fapolicyd_tsm_post(rpmPlugin plugin, rpmts ts, int res)
+{
+if (fapolicyd_state.ready)
+write_fifo(_state, "1", 2);
+
+return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_scriptlet_pre(rpmPlugin plugin, const char *s_name,
+ int type)
+{
+if (fapolicyd_state.ready && fapolicyd_state.first_scriptlet) {
+// send signal to flush cache
+write_fifo(_state, "2", 2);
+fapolicyd_state.first_scriptlet = 0;
I rewrote the plugin with the "files changed" part and I resolved most of the
issues.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#discussion_r563879105___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka pushed 1 commit. f64c8f5afc1f17e2e3f757a5cf12e700bebbad1d Added fapolicyd plugin -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475/files/9944f7d475c4a45535feda7e56725fb1a2ac3b02..f64c8f5afc1f17e2e3f757a5cf12e700bebbad1d ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka pushed 1 commit. 9944f7d475c4a45535feda7e56725fb1a2ac3b02 Added fapolicyd plugin -- You are receiving this because you are subscribed to this thread. View it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475/files/83f42fb43d259972bd98f41dae1f2131e8a672a7..9944f7d475c4a45535feda7e56725fb1a2ac3b02 ___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai commented on this pull request.
> +
+static rpmRC fapolicyd_tsm_post(rpmPlugin plugin, rpmts ts, int res)
+{
+if (fapolicyd_state.ready)
+write_fifo(_state, "1", 2);
+
+return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_scriptlet_pre(rpmPlugin plugin, const char *s_name,
+ int type)
+{
+if (fapolicyd_state.ready && fapolicyd_state.first_scriptlet) {
+// send signal to flush cache
+write_fifo(_state, "2", 2);
+fapolicyd_state.first_scriptlet = 0;
Hum, I sense a misunderstanding here. There can be no such global hook, as rpm
interleaves file operations and scriptlets. Even on per-package basis there can
first be various pre-scriptlets, then the files get laid down, and then there
can be more scriptlets running. And note that for the pre-scriptlet stuff you
do *not* want cache flushes as no files have been changed yet, so tracking the
"first scriptlet" is not what you want to do at all.
AIUI, what you really need to be doing is set a global "files changed" flag on
every entry to fsm_file_prepare, and then from scriptlet_pre flush the cache
*and* reset the changed variable for the next package. With that it'll track
the real situation and eg packages without files will not cause unnecessary
cache flushes.
Of course it's also possible I'm misunderstanding something here...
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#discussion_r559373699___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka commented on this pull request.
> +
+static rpmRC fapolicyd_tsm_post(rpmPlugin plugin, rpmts ts, int res)
+{
+if (fapolicyd_state.ready)
+write_fifo(_state, "1", 2);
+
+return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_scriptlet_pre(rpmPlugin plugin, const char *s_name,
+ int type)
+{
+if (fapolicyd_state.ready && fapolicyd_state.first_scriptlet) {
+// send signal to flush cache
+write_fifo(_state, "2", 2);
+fapolicyd_state.first_scriptlet = 0;
We need a hook to be placed right after all files are installed on the disk and
before the execution of the first scriptlet.
After flushing of the object cache we can handle execution of the scriptlets
with integrity checking on.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#discussion_r558259162___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@radosroka commented on this pull request.
> +fd = open(state->fifo_path, O_WRONLY);
+if(fd == -1) {
+rpmlog(RPMLOG_DEBUG, "Open: %s -> %s\n", state->fifo_path,
strerror(errno));
+return RPMRC_OK;
+}
+
+if (stat(state->fifo_path, ) == -1) {
+rpmlog(RPMLOG_DEBUG, "Stat: %s -> %s\n", state->fifo_path,
strerror(errno));
+close(fd);
+return RPMRC_OK;
+} else {
+
+if (!S_ISFIFO(s.st_mode)) {
+rpmlog(RPMLOG_DEBUG, "File: %s exists but it is not a pipe!\n",
state->fifo_path);
+close(fd);
+return RPMRC_OK;
> Another thing, more of a question: AIUI fapolicyd can actually prevent
> upgrades from taking place, so if it's actually active on the system then
> shouldn't a failure to initialize actually be returned as a failure here -
> its far, far better to fail early than in the middle of transaction.
Well, I designed the plugin to not block RPM transaction at any circumstances.
When there is no pipe, assumption is that daemon is not running so the plugin
should skip execution of the methods.
--
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/rpm-software-management/rpm/pull/1475#discussion_r558252460___
Rpm-maint mailing list
Rpm-maint@lists.rpm.org
http://lists.rpm.org/mailman/listinfo/rpm-maint
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai requested changes on this pull request.
See above, bunch of mostly minor issues and some questions to address.
> +return RPMRC_OK;
+}
+
+if (stat(state->fifo_path, ) == -1) {
+rpmlog(RPMLOG_DEBUG, "Stat: %s -> %s\n", state->fifo_path,
strerror(errno));
+close(fd);
+return RPMRC_OK;
+} else {
+
+if (!S_ISFIFO(s.st_mode)) {
+rpmlog(RPMLOG_DEBUG, "File: %s exists but it is not a pipe!\n",
state->fifo_path);
+close(fd);
+return RPMRC_OK;
+}
+
+// we require pipe to have 0660 permissions
Rpm coding style is `/* ... */` comments, please adjust for this everywhere.
> +fd = open(state->fifo_path, O_WRONLY);
+if(fd == -1) {
+rpmlog(RPMLOG_DEBUG, "Open: %s -> %s\n", state->fifo_path,
strerror(errno));
+return RPMRC_OK;
+}
+
+if (stat(state->fifo_path, ) == -1) {
+rpmlog(RPMLOG_DEBUG, "Stat: %s -> %s\n", state->fifo_path,
strerror(errno));
+close(fd);
+return RPMRC_OK;
+} else {
+
+if (!S_ISFIFO(s.st_mode)) {
+rpmlog(RPMLOG_DEBUG, "File: %s exists but it is not a pipe!\n",
state->fifo_path);
+close(fd);
+return RPMRC_OK;
There are multiple paths closing the fd and returning from this function, this
is prone to introducing resource leaks sooner or later. Please refactor to use
a common cleanup path for these, typically "goto errxit" style is use within
rpm.
> +!(s.st_mode & S_IWOTH) &&
+!(s.st_mode & S_IXOTH) )) {
+
+rpmlog(RPMLOG_ERR, "File: %s has 0%d%d%d instead of 0660 \n",
+state->fifo_path,
+((s.st_mode & S_IRUSR) ? 4 : 0) +
+((s.st_mode & S_IWUSR) ? 2 : 0) +
+((s.st_mode & S_IXUSR) ? 1 : 0),
+
+((s.st_mode & S_IRGRP) ? 4 : 0) +
+((s.st_mode & S_IWGRP) ? 2 : 0) +
+((s.st_mode & S_IXGRP) ? 1 : 0),
+
+((s.st_mode & S_IROTH) ? 4 : 0) +
+((s.st_mode & S_IWOTH) ? 2 : 0) +
+((s.st_mode & S_IXOTH) ? 1 : 0) );
This seems rather excessive for what is just a diagnostic message. Just log the
numeric s.st_mode value as is...
> +return RPMRC_OK;
+}
+}
+
+state->fd = fd;
+state->ready = 1;
+return RPMRC_OK;
+}
+
+static rpmRC write_fifo(struct fapolicyd_data* state, const char * str, size_t
len)
+{
+ssize_t ret = write(state->fd, str, len);
+
+if (ret == -1) {
+rpmlog(RPMLOG_DEBUG, "Write: %s -> %s\n", state->fifo_path,
strerror(errno));
+}
write() can fail with -EINTR, in which case you should just try again.
> +
+/* Ignore skipped files and unowned directories */
+if (XFA_SKIPPING(action) || (op & FAF_UNOWNED)) {
+rpmlog(RPMLOG_DEBUG, "fapolicyd skipping early: path %s dest %s\n",
+ path, dest);
+return RPMRC_OK;
+}
+
+if (!S_ISREG(rpmfiFMode(fi))) {
+rpmlog(RPMLOG_DEBUG, "fapolicyd skipping non regular: path %s dest
%s\n",
+ path, dest);
+return RPMRC_OK;
+}
+
+if (!fapolicyd_state.ready)
+return RPMRC_OK;
Here too, multiple early returns. It doesn't allocate any resources now, but
if/when it will in the future a simple change becomes more painful than it
should because it needs to be handled in multiple places. Please use a `goto
exit` idiom to jump out instead.
> +return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_init(rpmPlugin plugin, rpmts ts)
+{
+if (rpmtsFlags(ts) & (RPMTRANS_FLAG_TEST))
+return RPMRC_OK;
+
+open_fifo(_state);
+return RPMRC_OK;
+}
+
+static void fapolicyd_cleanup(rpmPlugin plugin)
+{
+if (fapolicyd_state.ready)
+close(fapolicyd_state.fd);
Do conditionalize the close() on the fd itself, this ensures it gets closed no
matter what. Or just leave the condition away, this is just a cleanup whose
return we're not checking anyhow so no harm done calling close(-1).
> +
+static rpmRC fapolicyd_tsm_post(rpmPlugin plugin, rpmts ts, int res)
+{
+if (fapolicyd_state.ready)
+write_fifo(_state, "1", 2);
+
+return RPMRC_OK;
+}
+
+static rpmRC fapolicyd_scriptlet_pre(rpmPlugin plugin, const char *s_name,
+ int type)
+{
+if (fapolicyd_state.ready && fapolicyd_state.first_scriptlet) {
+// send signal to flush cache
+write_fifo(_state, "2", 2);
+fapolicyd_state.first_scriptlet = 0;
Hmm, doesn't this need to take place once per package, rather than once per
transaction? (I could be misunderstanding and/or misremembering the discussion
though)
> +fd = open(state->fifo_path, O_WRONLY);
+if(fd == -1) {
+rpmlog(RPMLOG_DEBUG, "Open: %s -> %s\n", state->fifo_path,
strerror(errno));
+return RPMRC_OK;
+}
+
+if (stat(state->fifo_path, ) == -1) {
+
Re: [Rpm-maint] [rpm-software-management/rpm] [WIP] Added fapolicyd plugin (#1475)
@pmatilai commented on this pull request. > @@ -845,6 +845,14 @@ > AC_CHECK_HEADERS([linux/fsverity.h],[FSVERITY_IOCTL="yes"]) ]) AM_CONDITIONAL(FSVERITY_IOCTL,[test "x$FSVERITY_IOCTL" = xyes]) +#= +# Check for fapolicyd support +AC_ARG_WITH(fapolicyd, +AS_HELP_STRING([--with-fapolicyd],[build with File Access Policy Daemon support]), `--with-fapolicyd` sounds like such a *daemon* will be built as a result. I dunno, I'd perhaps call this "fapolicy" (ie drop the trailing d) everywhere, or maybe there are better alternatives. Not that it's a showstopper or anything, names are hard. -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/rpm-software-management/rpm/pull/1475#pullrequestreview-567037505___ Rpm-maint mailing list Rpm-maint@lists.rpm.org http://lists.rpm.org/mailman/listinfo/rpm-maint
