[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 awill...@redhat.com changed: What|Removed |Added Status|ASSIGNED|CLOSED Resolution|--- |RAWHIDE Last Closed||2016-02-16 16:23:04 --- Comment #57 from awill...@redhat.com --- 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! -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #56 from Jon Ciesla --- Package request has been approved: https://admin.fedoraproject.org/pkgdb/package/openqa -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #55 from awill...@redhat.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 John Dulaney changed: What|Removed |Added Flags|fedora-review? |fedora-review+ --- Comment #54 from John Dulaney --- Ayep, I'm going to go ahead and put my Stamp of Approval on. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #53 from Zbigniew Jędrzejewski-Szmek --- I think the package looks good. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #52 from awill...@redhat.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #51 from Upstream Release Monitoring --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #50 from awill...@redhat.com --- The .test.X scratch builds are me testing a complex patch I'm working on upstream, not directly related to package review. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #49 from Upstream Release Monitoring --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #48 from awill...@redhat.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #47 from awill...@redhat.com --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #46 from Upstream Release Monitoring --- adamwill's scratch build of openqa-4.3-12.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12956321 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #45 from Zbigniew Jędrzejewski-Szmek --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #44 from Zbigniew Jędrzejewski-Szmek --- (In reply to awill...@redhat.com 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #43 from John Dulaney --- 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? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #42 from Upstream Release Monitoring --- adamwill's scratch build of openqa-4.3-12.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12955213 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #41 from awill...@redhat.com --- 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 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #40 from Upstream Release Monitoring --- adamwill's scratch build of openqa-4.3-11.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12954670 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #39 from awill...@redhat.com --- 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! -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #38 from Upstream Release Monitoring --- adamwill's scratch build of openqa-4.3-11.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12953562 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #37 from awill...@redhat.com --- 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... -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #36 from awill...@redhat.com --- 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? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #35 from awill...@redhat.com ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #34 from awill...@redhat.com --- 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? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #33 from awill...@redhat.com --- 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'). -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #32 from awill...@redhat.com --- 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... -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #31 from awill...@redhat.com --- "/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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #30 from Zbigniew Jędrzejewski-Szmek --- Created attachment 1123306 --> https://bugzilla.redhat.com/attachment.cgi?id=1123306&action=edit 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #29 from awill...@redhat.com --- Forgot to mention - openqa logging is pretty light by default (info level). debug level is very chatty, but it's not the default. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #28 from awill...@redhat.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #27 from Upstream Release Monitoring --- adamwill's scratch build of openqa-4.3-10.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12934037 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #26 from Zbigniew Jędrzejewski-Szmek ---
(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 awill...@redhat.com from comment #17)
> I still don't entirely grok that 'subdirectory ownership' thing, but I asked
> on #yum, and mls said:
>
> 06:09:12> adamw: I guess this is about subdirectories which include
> other files/directories also packaged in rpm
> 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 ;)
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #25 from awill...@redhat.com --- 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... -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #24 from John Dulaney --- 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? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #23 from Ludwig Nussel --- (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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 Ludwig Nussel changed: What|Removed |Added CC||ludwig.nus...@suse.de --- Comment #22 from Ludwig Nussel --- (In reply to awill...@redhat.com 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #21 from John Dulaney --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #20 from awill...@redhat.com --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #19 from Upstream Release Monitoring --- adamwill's scratch build of openqa-4.3-9.fc23.src.rpm for f23 completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12916576 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #18 from Upstream Release Monitoring --- adamwill's scratch build of openqa-4.3-9.fc23.src.rpm for f23 failed http://koji.fedoraproject.org/koji/taskinfo?taskID=12916515 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #17 from awill...@redhat.com ---
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:
06:09:12> adamw: I guess this is about subdirectories which include other
files/directories also packaged in rpm
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 :)
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 Zbigniew Jędrzejewski-Szmek changed: What|Removed |Added Assignee|nob...@fedoraproject.org|jdula...@fedoraproject.org --- Comment #16 from Zbigniew Jędrzejewski-Szmek --- 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. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #15 from awill...@redhat.com ---
"%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).
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #14 from awill...@redhat.com --- 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? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #13 from Zbigniew Jędrzejewski-Szmek ---
(In reply to awill...@redhat.com 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@.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.)
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #12 from awill...@redhat.com ---
"%{?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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #9 from Neal Gompa --- Adam, the URLs all return HTTP 403 Forbidden. -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #10 from awill...@redhat.com --- goddamnit, selinux...OK, fixed (it was only the spec actually). -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #8 from awill...@redhat.com --- 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? :) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #7 from awill...@redhat.com --- 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... -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #11 from Zbigniew Jędrzejewski-Szmek ---
%{?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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #6 from John Dulaney ---
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?
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #5 from Upstream Release Monitoring --- jdulaney's scratch build of openqa-4.3-7.fc23.src.rpm for rawhide completed http://koji.fedoraproject.org/koji/taskinfo?taskID=12894011 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 --- Comment #4 from awill...@redhat.com --- 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) -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 John Dulaney changed: What|Removed |Added Status|NEW |ASSIGNED CC||jdula...@fedoraproject.org Flags||fedora-review? -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
--- Comment #3 from awill...@redhat.com ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
Neal Gompa changed:
What|Removed |Added
CC||ngomp...@gmail.com
--- Comment #2 from Neal Gompa ---
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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882 awill...@redhat.com changed: What|Removed |Added See Also||https://bugzilla.redhat.com ||/show_bug.cgi?id=1277312 -- You are receiving this mail because: You are on the CC list for the bug. You are always notified about changes to this product and component ___ package-review mailing list package-review@lists.fedoraproject.org https://admin.fedoraproject.org/mailman/listinfo/package-review
[Bug 1304882] Review Request: openqa - OS-level automated test framework and web UI
https://bugzilla.redhat.com/show_bug.cgi?id=1304882
awill...@redhat.com changed:
What|Removed |Added
CC||mgr...@redhat.com,
||zbys...@in.waw.pl
--- Comment #1 from awill...@redhat.com ---
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_eqperl-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.
--
You are receiving this mail because:
You are on the CC list for the bug.
You are always notified about changes to this product and component
___
package-review mailing list
package-review@lists.fedoraproject.org
https://admin.fedoraproject.org/mailman/listinfo/package-review
