I was kinda planning to clean this up a bit and comment some things before submitting the review request, but I figured I could submit what I had and we can go from there. This is the exact package currently in use on https://openqa.fedoraproject.org/ .

As with os-autoinst the spec is based on the openSUSE one and I'd like to keep it similar as far as possible, to make it easy to sync back future changes from them.

One particular note, explaining this line:

%requires_eq    perl-Mojolicious-Plugin-Bootstrap3 perl-Mojolicious-Plugin-AssetPack

and this chunk:

# strange way to precompile assets :)
./script/initdb --init_database
./script/openqa version  -m production
cp -a public/packed %{buildroot}%{_datadir}/openqa/public/

openQA uses a web framework (Mojolicious) with an asset management plugin (AssetPack), and a plugin that basically just provides Bootstrap in an AssetPack-y way (Bootstrap3). The way this is kinda envisioned to work is that the app will 'compile' the needed assets on-demand - that is, bundle up all the CSS and JS bits it actually needs into single, minified files - and keep them around, tracking whether the source files have changed with checksums, and re-generating the minified and flattened copies when the source files change.

The SUSE guys decided to handle it a bit differently: they make it so the app has no write permissions to the directory where the app stores the assets when run normally, and pre-create the minified+flattened assets by running it as root during the package build.

The reason for doing this is to reduce the security exposure of the webapp, which seems like a pretty reasonable point to me. The tradeoff is that if the source files change, the webapp will notice and try to recompile the flattened assets, but it will fail because it can't write to the asset directory. This is why the package has that %requires_eq line. "%requires_eq foo" creates a dependency on the exact EVR of 'foo' that was installed at the time of the package build. So by having %requires_eq for Bootstrap3 and AssetPack, any time those packages (which provide the source assets) are updated, openQA's deps will break, cueing us to rebuild it (whereupon the assets included in the package will be the correct ones again).

If we were to just go ahead and let the app write to the asset dir we could simplify the spec file and avoid the need to rebuild the package any time Bootstrap3 or AssetPack changed, but I do think there's a valid point about potential security exposure by allowing the server to write files it will later serve out to clients for execution. So I'm kinda on the fence about this one and willing to hear reviewers' thoughts.

On this topic - I do want to build an SELinux policy for openQA, but a) I haven't had time to yet and b) I can't really do a proper one until the selinux-policy folks look at https://bugzilla.redhat.com/show_bug.cgi?id=1277312 . Right now the actual openQA webapp runs unconfined (as it's a separate process which a full-fat web server is intended to reverse proxy, as in the included sample Apache configuration).

CCing Zbyszek (as he's kindly reviewed several things for me, including os-autoinst, openQA's underlying test runner) and mgrepl for thoughts on SELinux.

I've taken a quick look-over of the package, and I've noticed a few quirks.

* The worker subpackage has "Requires(post): os-autoinst >= 4" and "Requires: os-autoinst < 5". This is rather strange, as usually semantic versioning enforcement is in the same class of Requires (be it BuildRequires, Requires, or Requires(*)). It's not usually mixed. Is there a good reason for this? The spec didn't indicate anything obvious that would require it.

* Why aren't we running the tests in the %check section? We delete a test, but then don't actually run any tests here.

* This is a bit of a style nitpick, but don't we usually use %{} format for user-defined macros too? I see "%openqa_services" and "%openqa_worker_services" instead of "%{openqa_services}" and "%{openqa_worker_services}"

* In the scriptlets, I don't see usage of macros for file paths that we use elsewhere. This has the potential to break things if the macros were redefined in the future. Please use them in the scriptlets. I believe they'll get evaluated before being written to the package, so it shouldn't be a problem.

* If at all possible, could you split out the httpd configuration to a separate subpackage? It might be possible in the future to get nginx or another webserver supported for using with openQA, and it'd be nice if it wasn't hardcoded from the get-go for httpd. You should be able to set up some kind of virtual Provide to be required or use Requires (which would eventually turn into a rich dependency) to handle this for ensuring openQA continued to work.

1. Good point on the Requires(post), I'm not entirely sure why it's there; I don't see why os-autoinst would be required for %post, unless it's somehow needed for database creation (I don't see why). I'll ask.

2. The tests have some deps that are not yet packaged. The test deletion comes from the openSUSE spec, where they do run the tests (see the note about staying in sync with upstream where possible).

3. I don't think it's 100% consistent, but yeah, I prefer %{} style. I'll have to check if it makes the diff to upstream uglier, but I think it might be OK to change.

4. Hmm, yep, especially the one which uses the macro in one line and hard codes the location in the next :) I think I agree, I will change that. Thanks!

5. Yeah, I was thinking of that too. It doesn't really need to be a subpackage, but it wouldn't hurt anything, and at least probably we shouldn't hard require Apache. I'm not very familiar with nginx config so would need someone to contribute it, but it'd be great to have a subpackage.

Here's 4.3-7, with all of Neal's comments addressed, and a couple of other changes:

- package review improvements:
- * no need for worker to Requires(post) os-autoinst
- * explain why tests are currently disabled
- * fix a few macro invocations to use curly braces
- * use directory macros where appropriate in scriptlets
- * split apache configuration into a subpackage

I forgot to put it in the changelog, but I also made the summaries and descriptions of the packages better, and dropped the superfluous Group tags (we're not really targeting EPEL with this ATM).

I'm trying something new for my reviews too - keeping the old specs around and providing diffs for easy comparison. The latest spec will always be symlinked as 'openqa.spec', but you can look at a specific spec as 'openqa.spec.version-release', so:

https://www.happyassassin.net/reviews/openqa/openqa.spec (links to https://www.happyassassin.net/reviews/openqa/openqa.spec.4.3-7 )
https://www.happyassassin.net/reviews/openqa/openqa-4.3-7.fc23.src.rpm
https://www.happyassassin.net/reviews/openqa/4.3-6_4.3-7.diff (diff from 4.3-6 to 4.3-7)

jdulaney's scratch build of openqa-4.3-7.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12894011

What is %{_prefix}/lib/systemd/system-generators used for?  I see it defined, and that's it.

Also, I note that if I get reverse-depcheck finished, new builds of any packages required by %requires_eq will wind up failing that check.  I'd rather see this done another way, tbh.  I admit, I'm not quite sure what that way might could be, but I also agree it's a bit of a security concern to have the OpenQA server do it.  If the updated versions of these packages get installed, does OpenQA break immediately (besides the detection logic, that is).  Would it be possible to disable automatic checks and instead run the check and recompile as a cron job?

I'm not actually sure what happens if the assets become invalidated and the re-generation fails - it may just use the underlying files and keep working (with a logged warning or so), or it may result in the app crashing. I haven't seen it. I'll try and test it out.

The general idea would be that we'd include an openqa build in the update any time those packages got updated. I would expect they won't be updated often, especially in stable releases.

Having the asset generation done outside of the openQA server process is kind of an interesting idea, but I'm not totally sure. A cron job seems kinda hacky. In an ideal world it'd somehow happen automagically when one of the relevant packages updated, but I can't see a sensible way to make that happen...

As for system-generators - there's a file in there, /usr/lib/systemd/system-generators/systemd-openqa-generator . The install process installs it. I'm not sure what it's for, but...something? :)

Adam, the URLs all return HTTP 403 Forbidden.

goddamnit, selinux...OK, fixed (it was only the spec actually).

%{?systemd_requires} is forbidden by the guidelines. I don't think we gain anything by that rule, but it's on the books.

What about parallel build?

I think user/group creation scriptlets should be suffixed with "|| :".
They should not be fatal to installation.
Also emitting message from %post is a bit unusual.

Strictly speaking, the generator is wrong, because generators cannot rely on /var being mounted. It will not operate correctly if someone has a system with separate /var partitions.

It seems a bit strange to use both the tmpfiles mechanism and explicit creation of files in a script (the log file). I think it would be cleaner to use a tmpfile also for the log file.

%config(noreplace) %attr(-,geekotest,root) %{_sysconfdir}/openqa/openqa.ini
%config(noreplace) %attr(-,geekotest,root) %{_sysconfdir}/openqa/database.ini
Why are those owned by the user? Can they be updated dynamically at runtime?

I don't understand this part:
%defattr(-,geekotest,root)
# attention: never package subdirectories owned by a user other
# than root as that opens a security hole!
%dir %{_localstatedir}/lib/openqa/db
Doesn't this do just that: create a directory owned by geekotest?

Worker package should not own
%{_unitdir}
or %{_prefix}/lib/systemd/system-generators, just the contents.

Similarly for the apache dirs, they are owned by httpd-filesystem.

"%{?systemd_requires} is forbidden by the guidelines. I don't think we gain anything by that rule, but it's on the books."

Thanks, I'll...er...do something about that?

"What about parallel build?"

The compile step is pretty short anyhow, so I guess I never thought about it. I can see.

"I think user/group creation scriptlets should be suffixed with "|| :".
They should not be fatal to installation."

Well, there are samples on the policy page for those, and they don't have "|| :". https://fedoraproject.org/wiki/Packaging:UsersAndGroups

"Also emitting message from %post is a bit unusual."

Yeah, it's kinda unusual for Fedora I guess, but it seemed useful so I kept it. Of course it should at least be in the httpd subpackage now :) Maybe I can add a README or something instead, I'll see what I feel.

"Strictly speaking, the generator is wrong, because generators cannot rely on /var being mounted. It will not operate correctly if someone has a system with separate /var partitions."

Is there a fix or change you can recommend? Honestly this just came from the SUSE spec, I don't even know what it does.

"It seems a bit strange to use both the tmpfiles mechanism and explicit creation of files in a script (the log file). I think it would be cleaner to use a tmpfile also for the log file."

Another thing straight from SUSE - IIRC, one bit is rather older than the other. I'll look at it.

"Doesn't this do just that: create a directory owned by geekotest?"

Y'know, I've always kinda wondered about that too. My *guess* is that this is actually some kind of note added by a SUSE watchdog, either manual or some kinda automated script - i.e. it was meant as a warning to the packagers, it's not something the packagers themselves added. I'll ask the openSUSE folks about it.

[directory ownership] - yep, you're right, will fix. I even explicitly added the httpd-filesystem dep then forgot to remove the directory ownership.

Thanks for the notes! Expect a new build tomorrow.

(In reply to awilliam from comment #12)
> "%{?systemd_requires} is forbidden by the guidelines. I don't think we gain
> anything by that rule, but it's on the books."
> 
> Thanks, I'll...er...do something about that?
Expand it. See https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd.

> "What about parallel build?"
> 
> The compile step is pretty short anyhow, so I guess I never thought about
> it. I can see.
> 
> "I think user/group creation scriptlets should be suffixed with "|| :".
> They should not be fatal to installation."
> 
> Well, there are samples on the policy page for those, and they don't have
> "|| :". https://fedoraproject.org/wiki/Packaging:UsersAndGroups

That's true, but I think that those guidelines are from old times. Nowadays we try not to fail in the scriptlets for any reason if at all possible. For this package, if the scriptlets fail, nothing terrible happens. In fact tmpfiles will report the failure nicely in the normal logs. 

> "Also emitting message from %post is a bit unusual."
> 
> Yeah, it's kinda unusual for Fedora I guess, but it seemed useful so I kept
> it. Of course it should at least be in the httpd subpackage now :) Maybe I
> can add a README or something instead, I'll see what I feel.

Yeah, that's why I said "unusual". Unusual is not wrong. I guess that if it sticks out too much and annoys people, somebody will file a bug report.

> "Strictly speaking, the generator is wrong, because generators cannot rely
> on /var being mounted. It will not operate correctly if someone has a system
> with separate /var partitions."
> 
> Is there a fix or change you can recommend? Honestly this just came from the
> SUSE spec, I don't even know what it does.

It adds openqa-worker@.service instance for every item in /var/lib/openqa/pool/[0-9]* to the openqa-worker.target. The annoying thing is that things will break in a confusing way if /var is separate: after initial installation things will be fine, and then after a reboot the units will not be generated as expected. But maybe that's not that much of a problem, after all this package will not be installed by too many random people.

There's an easy workaround in case /var is separate: just run 'systemctl add-wants openqa-worker.target openqa-worker@<x>.service'. (Pfff, it seems that this doesn't work for instances. Oh, systemd! If think it might be fixed by Jan Synacek's PR that is awaiting review. But the symlink can always be added by hand.)

So the %systemd_requires thing is only in a draft page so far as I can see - https://fedoraproject.org/wiki/User:Zbyszek/SystemdLinksDraft - but I'm fine with expanding it.

On the generator: I think maybe it's OK, because it's more a convenience thing than a critical bit of functionality - I actually didn't know about that target at all, I just run a 'for i in 1 .. 8' or whatever loop over the individual worker services =) I guess nothing too ugly happens if the generator does fail, there's just a log message and the target doesn't work right? I think we can live with that, especially if the alternative is just 'no generator for anyone' - unless there's a better way to achieve the same goal? For context, there's really no rules about the numbers of workers, you can run as many on a system as your hardware can cope with, and add them (and remove them) at any time.

I guess you've convinced me on the user creation...openQA *really needs* those user accounts to exist, but I suppose someone might want to run their accounts entirely out of FreeIPA or something. So I can change that, sure. It might be a good idea to submit a revision request for the guidelines?

"%config(noreplace) %attr(-,geekotest,root) %{_sysconfdir}/openqa/openqa.ini
%config(noreplace) %attr(-,geekotest,root) %{_sysconfdir}/openqa/database.ini
Why are those owned by the user? Can they be updated dynamically at runtime?"

ah, so, I remember! This is not necessary for openqa.ini , I can fix that.

The reason for database.ini is that it may contain sensitive information (specifically, the database password, depending on how you're doing database auth); we need only the openqa server to be able to read it. There's really no need for it to be writeable by the server, so perhaps we could make it 0440 or 0400 - IIRC root ignores perms and can write to anything that's not immutable, though I'm not sure if it's 'bad practice' to set a file you expect root to write to be read-only. We could do 0460, I guess, but that seems odd.

The perms are set by the upstream install script (we can always change them, but since we have a good relationship with upstream it would really make sense to change it there instead of having a downstream change).

https://fedoraproject.org/wiki/Packaging:ScriptletSnippets#Systemd says
"%systemd_requires macro must not be used".

If the generators fails, it will not log anything. Actually that's another error in the generator script. See http://www.freedesktop.org/software/systemd/man/systemd.generator.html#Notes.

For the permissions, (0640,root,geekotest) would seem more natural then. For example cups has a bunch of files /etc/cups that follow this pattern, /etc/ssmtp. It's nice because you can provide read permissions to other users by adding them to the group.

Using (0460,geekotest,root) doesn't achieve the purpose of preventing writes, because the owner of the file has permission to chmod +x. I don't know how security sensitive the file is, but if you're going to implement a lock down of the file, I think it should be done properly, not least because bad patterns tend to get copied and end up in more important places. I wouldn't worry about the upstream permissions: if you have an %attr() already, also setting the mode doesn't change much.

So I've put up a 4.3-9 with several changes:

https://www.happyassassin.net/reviews/openqa/openqa.spec
https://www.happyassassin.net/reviews/openqa/openqa-4.3-9.fc23.src.rpm

I haven't actually *tested* it yet, so, have fun :) I'll do that tomorrow. I aimed to tighten up ownerships/permissions a bit and explain the ones that can't be changed, or look odd, along with all the changes noted in the spec:

# The logfile mess is all gone, that's just working around AppArmor it seems
# systemd_requires is expanded
# build is 'parallel', but we also note that make does nothing at all ATM
# the change to user creation scriptlets is done
# the post-install message is moved to httpd subpackage post for now
# openqa.ini and database.ini ownership/permissions are changed
# unnecessary directory ownerships are dropped

Note, if you want to test this stuff, using the ansible plays from infra will make it a lot easier:

https://infrastructure.fedoraproject.org/cgit/ansible.git/tree/roles/openqa

the required variables are all documented in the comments, you can just set up a simple stub local playbook and define them plus the necessary hosts there and link in the roles directory from a checkout of the infra repo.

We need to test both an upgrade of an existing install and a fresh one from scratch to make sure none of these changes busted anything...

I still don't entirely grok that 'subdirectory ownership' thing, but I asked on #yum, and mls said:

<mls> 06:09:12> adamw: I guess this is about subdirectories which include other files/directories also packaged in rpm
<mls> 06:10:26> I think the security issue is that the non-root user can modify the directory while rpm messes (as root) with the directory contents
 06:10:48> e.g. creates symlinks and the like.

Ondrej (upstream) said he didn't add that comment and doesn't know why it's there, so that might be the best we'll get. It's not a script, I don't think, as Googling it returns no other locations of the same text.

I don't really see a way around geekotest owning these directories, it needs to be able to create files in them. I did change it so that geekotest does not own /share and /factory, only the leaf directories. I believe that should be OK (it doesn't need write access to /share or /factory directly, I don't believe).

I'll note that the Wordpress package has something very similar to what this now has:

%dir %attr(0775,apache,ftp) %{wp_content}/plugins
%dir %attr(0775,apache,ftp) %{wp_content}/themes
%dir %attr(0775,apache,ftp) %{wp_content}/upgrade
%dir %attr(0775,apache,ftp) %{wp_content}/uploads

so either it's not really a problem or every Fedora wordpress instance in the world is vulnerable to it :)

adamwill's scratch build of openqa-4.3-9.fc23.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12916515

adamwill's scratch build of openqa-4.3-9.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12916576

so sadly, just ditching the log file stuff doesn't work, because geekotest of course can't create /var/log/openqa (as it doesn't have the necessary permissions on /var/log). d'oh. So effectively we have the same issue openSUSE has.

I see a few options:

1) put the dodges back in the spec
2) use tmpfiles
3) just create /var/log/openqa owned by geekotest in %install and package it
4) log to /var/log/openqa/openqa
5) log to the goddamn system log

I kinda like 5 because it saves me from all sorts of concerns, but it'd require an upstream change. Ditto 4 (could be done with a trivial patch but I really don't want to patch downstream if we can possibly avoid it), and if I'm gonna change upstream I'd rather go 5. 3 is simple but means the logs disappear if the package is removed, I guess. 2 is simple but tmpfiles claims to be for 'temporary and volatile files', a log file doesn't really seem to fit the bill (unless we consider it 'volatile'?), do you really think this is an appropriate use? 1 is kinda hinky but at least we know it works.

I'm going to sleep, night. :) let me know what you think.

I think I prefer 4 over 5;  it can make it easier to scrape logs as necessary.  For 3, how often do you plan to uninstall openqa?  It's sort of like setting up a web server and then uninstalling apache.  2 is not a good idea; tmpfiles are not meant to be persistent and contain no guarantee that they would be.  1 also just does not seem to be a good plan.

(In reply to awilliam from comment #20)
> so sadly, just ditching the log file stuff doesn't work, because geekotest
> of course can't create /var/log/openqa (as it doesn't have the necessary
> permissions on /var/log). d'oh. So effectively we have the same issue
> openSUSE has.

It's not a bug, it's a feature :-) Daemon users with full access to the
directory they are logging to hit logrotate badly in the past:
http://openwall.com/lists/oss-security/2011/03/04/16
The safe choice is to have the directory owned by root, so the daemon can only
access it's log file. Therefore one has to create an empty file when installing
the package. If %post+%ghost is undesirable that could be also
achieved with %install and %verify(not md5 size mtime) I guess.

If you prefer logging to the journal ie stdout feel free let your openQA package do that instead.

(In reply to Zbigniew Jędrzejewski-Szmek from comment #13)
> > "Strictly speaking, the generator is wrong, because generators cannot rely
> > on /var being mounted. It will not operate correctly if someone has a system
> > with separate /var partitions."
> > 
> > Is there a fix or change you can recommend? Honestly this just came from the
> > SUSE spec, I don't even know what it does.
> 
> It adds openqa-worker@.service instance for every item in
> /var/lib/openqa/pool/[0-9]* to the openqa-worker.target. The annoying thing
> is that things will break in a confusing way if /var is separate: after
> initial installation things will be fine, and then after a reboot the units
> will not be generated as expected. But maybe that's not that much of a
> problem, after all this package will not be installed by too many random
> people.

The generator was an attempt to make things more convenient, so one
doesn't have to configure the number of workers explicitly. It's true that it
doesn't work in all deployment scenarios (we use tmpfs for the pool for example
where it also doesn't work). If it didn't achieve that goal we may just as well
drop it.

I note that the httpd rpm install /var/log/httpd (the folder), but no log files are populated until the httpd server is started.  At that point, it is populated.  However, uninstalling httpd leaves the log files in place.


That said, why would you be uninstalling openqa once you had it on a system?

The apache case is a bit different because IIRC Apache starts as root and creates missing log files as root before it drops privs; /var/log/httpd is owned by root. The reason not to create /var/log/openqa owned by geekotest is given by Ludwig in #c22.

I like to try and 'do things right' as far as reasonably possible and not second-guess the user/sysadmin; I don't think it's gonna be super common to remove the package, but I still want to handle it as correctly as possible. I'm still technically on vacation, though, so I haven't had enough time to sit down and work through all the possibilities yet...

(In reply to Ludwig Nussel from comment #22)
> If you prefer logging to the journal ie stdout feel free let your openQA
> package do that instead.

That would seem like the best option, unless openqa generates huge amounts of logs. How much logs does openqa generate? logrotate is annoying, the journal is much nicer to use.

(In reply to awilliam from comment #17)
> I still don't entirely grok that 'subdirectory ownership' thing, but I asked
> on #yum, and mls said:
> 
> <mls> 06:09:12> adamw: I guess this is about subdirectories which include
> other files/directories also packaged in rpm
> <mls> 06:10:26> I think the security issue is that the non-root user can
> modify the directory while rpm messes (as root) with the directory contents
>  06:10:48> e.g. creates symlinks and the like.

Oh, OK, I think I get it now. Let's say that we have user-owned /var/a in %files, and then /var/a/b/file in %files. The user can rename /var/a/b to /var/a/b.old, create /var/a/b, and e.g. symlink /var/a/b/file → /etc/passwd. When rpm updates /var/a/file during package upgrade it will trash /etc/passwd. Similar considerations would hold for a subdirectory inside a user-owned directory. At least it allows the user to cause rpm to write files to arbitrary directories in the filesystem. I'm not sure if it's possible to carry out the attack with just one level of nesting. Maybe, it probably depends on the order in which rpm does operations and whether it uses O_EXCL.

Anyway, I think that the last version is OK, only leaf directories are owned by geekotest.

> I'll note that the Wordpress package has something very similar to what this
> now has:
> 
> %dir %attr(0775,apache,ftp) %{wp_content}/plugins
> %dir %attr(0775,apache,ftp) %{wp_content}/themes
> %dir %attr(0775,apache,ftp) %{wp_content}/upgrade
> %dir %attr(0775,apache,ftp) %{wp_content}/uploads
> 
> so either it's not really a problem or every Fedora wordpress instance in
> the world is vulnerable to it :)

In this snippet there is no nesting, so it's not relevant. But I wouldn't use wordpress to prove security anyway ;)

adamwill's scratch build of openqa-4.3-10.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12934037

So we can achieve fairly simple logging to the journal simply by setting the log 'file' setting to nothing at all in config:

file =

or making it undefined it in the code block that defines the 'defaults'. this causes Mojo to log to stderr, and the journal picks that up. The log message levels aren't picked up by journald when you do it this way, but it's good enough for now I think. A sysadmin can still easily log to file by setting `file` in openqa.ini. I've written a simple patch to change the default in this way and sent it upstream for discussion, and done a 4.3-10 build with the change:

https://github.com/os-autoinst/openQA/pull/541
https://www.happyassassin.net/reviews/openqa/openqa.spec
https://www.happyassassin.net/reviews/openqa/openqa-4.3-10.fc23.src.rpm
https://www.happyassassin.net/reviews/openqa/4.3-9_4.3-10.diff
https://koji.fedoraproject.org/koji/taskinfo?taskID=12934037

I wasn't citing Wordpress the project, but wordpress the package ;) It's one of our older and more heavily used webapp packages, so I tend to consider it a reasonable example spec. Yeah, I believe that avoiding nesting should avoid the attack vector.

Forgot to mention - openqa logging is pretty light by default (info level). debug level is very chatty, but it's not the default.

Created attachment 1123306 [details]
patch to use triggers to generate assets

/var/lib/openqa/share/factory/iso is not %ghosted, on purpose?

I played around with the spec file, to see if I could get rid of two iffy points:
1. Use of predicatable dir in /tmp (I know one could argue that this is only done on a build system, but it was still very ugly.)
2. Dependency on fixed version of perl-Mojolicious-Plugin-Bootstrap3, perl-Mojolicious-Plugin-AssetPack.

It seems that it can be done with a simple trigger script. Please see attached patch.

During installation an error is sometimes displayed [http://paste.fedoraproject.org/321450/52265321/]. I don't understand what is going on here. But it seems to have no effect on the generated assets.

"/var/lib/openqa/share/factory/iso is not %ghosted, on purpose?" Yeah, I kinda go back and forward on that one. It's somewhat different because the upstream Makefile actually installs it and it's more or less mandatory (you can run all your tests from disk images, I *guess*, but it'd be a very odd use of openQA). The other asset types are more optional (a simple config might not have any of them).

I like the trigger idea, I guess, Ludwig, WDYT? I guess I'd only question whether it'll work once we write an SELinux policy for openQA (which I'm planning to do).

I'll look into the errors.

Those errors look like a case where OPENQA_CONFIG isn't set (if it is set, Schema.pm doesn't look for ../etc/openqa/database.ini). I can't see why that would happen in your script.

It's *possible* they're actually coming from %post (where it calls initdb / upgradedb) and aren't caused by your patch at all, but by the change in ownership of `database.ini` to root.geekotest instead of geekotest.root . I just tested out a theory about that and it didn't pan out, but I'll keep digging...

Yeah, that's what it is. This fails when database.ini is root.geekotest 0640:

/usr/share/openqa/script/initdb --user geekotest --init_database

I think it's because of the way initdb drops privileges (the way it handles '--user geekotest').

So, uh, yeah, this is the problem, but it's rather odd:

http://fpaste.org/321675/52705111/

why does that work as geekotest but not as root?! Some subtlety of setuid / setgid that I don't understand?

If I modify the test script to throw an open() in there:

#!/bin/perl

use Config::IniFiles;
use POSIX qw/setuid setgid/;

setuid 995;
setgid 994;

open( my $fh, '<', '/etc/openqa/database.ini' );
while ( my $line = <$fh> ) {
    print $line;
}
close $fh;

my %ini;
tie %ini, 'Config::IniFiles', ( -file => "/etc/openqa/database.ini" );

print "We have $ini{production}{dsn}.\n" if $ini{production}{dsn};

then the open() doesn't complain, but when run as root, it prints nothing. When run as geekotest, it prints the contents of the file. Again, when it hits the 'tie' line, it crashes when run as root, works when run as geekotest.

OK, so I worked this out, but it's a bit...icky. It's indeed caused by database.ini being not owned by geekotest.

initdb has a `--user` argument used to run as a particular user. It switches UID to that user before reading the config file. It doesn't switch GID to that user's GID. So if the file is owned by root.geekotest and initdb is run with `--user geekotest`, it winds up not being able to read the file.

We can 'fix' that by making initdb switch to the specified user's GID, but unfortunately that just causes another problem. If it's operating in SQLite mode, initdb then attempts to change uid and gid *again* later: it finds out the ownership of the directory where the sqlite file will be written, and tries to switch to that UID and GID before writing the file, in order to make the file have the same ownership.

In the current case the directory is owned by geekotest.root - so we wind up with a process that has already change GID to geekotest's GID trying to change it back to root's GID. Which isn't allowed. And we blow up again.

An 'obvious' fix for this is to continue only switching UID (not GID) based on --user, and do it *after* the config file is read. I tested that, it works. Drawbacks:

* A bit more code is now run as root
* This is still kind of ugly bad code, I hate it all

Another fix, I guess, would be to have /var/lib/openqa/db owned by geekotest.geekotest , which would happen to work OK with the code as it exists, I think. I dunno if that's a good fix. Thoughts?

There's an irony here: if we change initdb to accommodate database.ini *not* being owned by geekotest, we might actually *create* an attack vector in the case where it *is* owned by geekotest. The problem with database.ini being owned by geekotest is that, in theory, an attacker who could get the webUI to execute arbitrary code could mess with it, right? So, what could an attacker do with that?

Well, try to have openQA mess with *other* databases, I guess. Here's one attack: they could fiddle database.ini to point to some other database, and then when initdb ran, it might mess with it.

As things stand, this wouldn't work. The attacker can't call initdb with elevated privileges themselves, but they can booby-trap database.ini and wait for the package scripts to run it as root. However, the script currently drops privileges before it tries to connect to the database, so this attack isn't going to work. Our attacker is foiled!

If we change initdb so it connects to the configured database as root *then* drops privileges, we might actually *enable* this attack. I don't know for sure - it might be the case that subsequent operations on the database after privileges are dropped would fail even if the schema instance was created before privileges were dropped, I'm not sure how that works. But it at least seems *more* possible.

oh security, how I hate you...

adamwill's scratch build of openqa-4.3-11.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12953562

Note while I work on this stuff: the trigger approach has one drawback, which is that it requires a regular dependency on rubygem(sass) - as the generation is now done on the target system and not in the build process, the system needs /usr/bin/sass available, as the asset generation process uses it. So we pull in ruby as a dep of openQA, what fun!

adamwill's scratch build of openqa-4.3-11.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12954670

Well, here's some crap that might be better, I guess?

https://www.happyassassin.net/reviews/openqa/openqa.spec
https://www.happyassassin.net/reviews/openqa/openqa-4.3-11.fc23.src.rpm
https://www.happyassassin.net/reviews/openqa/4.3-10_4.3-11.diff
https://koji.fedoraproject.org/koji/taskinfo?taskID=12954978

Changes - backport my PR for the sqlite DB uid/gid issues, use zbigniew's trigger stuff for asset generation, stop owning specific asset type dirs again (we really don't need to, it's always been up to the admin to create / mount them with appropriate privs).

https://i.imgur.com/naNop30.jpg

adamwill's scratch build of openqa-4.3-12.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12955213

Just out of curiosity, what happens if you leave the db directory owned by geekotest.root and then set facls to allow the geekotest user write access?

(In reply to awilliam from comment #36)
> Another fix, I guess, would be to have /var/lib/openqa/db owned by
> geekotest.geekotest , which would happen to work OK with the code as it
> exists, I think. I dunno if that's a good fix. Thoughts?

That sounds like an OK solution.

I think the asset generation script should remove /usr/share/openqa/public/sass/.sass-cache/ after it's done. That directory has mode 0700 which is annoying.

adamwill's scratch build of openqa-4.3-12.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12956321

On the UID/GID issue: upstream merged my patch to stop initdb attempting to switch uids/gids twice, which I think is a decent fix. It means the db file winds up owned by geekotest.geekotest (not geekotest.root) for us, but I don't think that causes any problems. John, I really don't think we need one more factor to confuse us further at this point :P

I tweaked your asset generation script slightly and submitted it upstream:

https://github.com/os-autoinst/openQA/pull/547

the way I set it up, it *should* be usable both for generate-assets-with-trigger and generate-assets-during-build, and if we keep the code in upstream we don't have to have all distros duplicate it or reinvent it or whatever.

Removing .sass-cache seems fine (though in my tests it's been 0755 not 0700), added that.

https://www.happyassassin.net/reviews/openqa/openqa.spec
https://www.happyassassin.net/reviews/openqa/openqa-4.3-12.fc23.src.rpm
https://www.happyassassin.net/reviews/openqa/4.3-11_4.3-12.diff
https://koji.fedoraproject.org/koji/taskinfo?taskID=12956417

- quiet the trigger script down a bit
- clean up sass cache in the trigger script
- more customizable trigger script for upstream submission
- setgid in upgradedb as well as initdb

A small note on the trigger stuff: it's not clean on upgrade from the old system - the bit of RPM that says "remove this directory that's no longer packaged" runs after the %triggerin script, so when you upgrade from an older package to one which uses the trigger, you lose the packed assets and openQA won't run.

I'm not sure this is worth fixing, as there will be very few affected systems (the 'official' instances, mine at happyassassin.net, and any that any intrepid folks playing with the packages so far have set up) and the manual fix is trivial: just run `/usr/share/openqa/script/generate-packed-assets` and restart the services.

adamwill's scratch build of openqa-4.3-12.test.1.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12957108

The .test.X scratch builds are me testing a complex patch I'm working on upstream, not directly related to package review.

adamwill's scratch build of openqa-4.3-12.test.3.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12957773

John: Zbigniew: where do we stand with this? Do you have outstanding concerns with 4.3-12? For the record, https://openqa.stg.fedoraproject.org has been using 4.3-12 for the last few days and seems fine.

I think the package looks good.

Ayep, I'm going to go ahead and put my Stamp of Approval on.

Thanks very much for the review, both of you!

I want to write an SELinux policy but I don't think I'll block the package for that.

Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/openqa

Package imported, Rawhide build done. I'm cleaning up a few more bits at present, but we can close the ticket. Thanks for the review!