Spec URL: https://github.com/odubaj/firejail/blob/master/firejail.spec SRPM URL: https://github.com/odubaj/firejail/blob/master/firejail-0.9.56-1.fc28.src.rpm Description: Firejail is a SUID sandbox program that reduces the risk of security breaches by restricting the running environment of untrusted applications using Linux namespaces. It includes a sandbox profile for Mozilla Firefox. Fedora Account System Username: odubaj
Next time please link the 'raw', not html or upload the files somewhere, e.g. to fedorapeople.org (once you get sponsored).
(In reply to Jaroslav Škarvada from comment #1) > Next time please link the 'raw', not html or upload the files somewhere, > e.g. to fedorapeople.org (once you get sponsored). I.e. 'wget URL' should download the file.
- please wordwrap comments to 80 columns at max - please use %make_build instead of 'make' - if possible do not use %attr for normal modes like 755, fix the modes of installed files instead - list all buildrequires like gcc, make, etc., it's wrong to make any assumptions about packages pre-installed in the buildroot - do not redefine %url macro, just use 'URL:' before the 'Source0:' Also note: if you need to define macros, please used '%global' instead of '%define'
Bugs in spec file fixed, but must use %attr. Without it I cannot build rpm package. Spec URL: https://github.com/odubaj/firejail/blob/master/firejail.spec SRPM URL: https://github.com/odubaj/firejail/blob/master/firejail-0.9.56-2.fc28.src.rpm Description: Firejail is a SUID sandbox program that reduces the risk of security breaches by restricting the running environment of untrusted applications using Linux namespaces. It includes a sandbox profile for Mozilla Firefox. Fedora Account System Username: odubaj
(In reply to Ondrej Dubaj from comment #4) > Bugs in spec file fixed, but must use %attr. Without it I cannot build rpm > package. > > Spec URL: https://github.com/odubaj/firejail/blob/master/firejail.spec > SRPM URL: > https://github.com/odubaj/firejail/blob/master/firejail-0.9.56-2.fc28.src.rpm > Description: Firejail is a SUID sandbox program that reduces the risk of > security > breaches by restricting the running environment of untrusted applications > using Linux namespaces. It includes a sandbox profile for Mozilla Firefox. > Fedora Account System Username: odubaj Next time please use link to raw, i.e.: https://raw.githubusercontent.com/odubaj/firejail/master/firejail.spec for the spec and similarlry for the srpm. The command: wget 'https://raw.githubusercontent.com/odubaj/firejail/master/firejail.spec' should return the file not HTML page as it do with the links you provided.
Please just use: %make_build not '%make_build %{?_smp_mflags -j3}' You can check what the macros finally expand to by using the 'rpm --eval MACRO', e.g. on my system: $ rpm --eval %make_build /usr/bin/make -O -j3 The %make_build is defined as '%{__make} %{_make_output_sync} %{?_smp_mflags}' which dynamically expands according to available cores. With the '%{?_smp_mflags -j3}' the last -j3 (everything after the space) is IMHO ignored.
(In reply to Ondrej Dubaj from comment #4) > Bugs in spec file fixed, but must use %attr. Without it I cannot build rpm > package. It works for me without %attr by just using: chmod 0755 %{buildroot}%{_bindir}/%{name} in the %install phase. But AFAIK firejail is SUID program, so if you remove SUID bit (either with the attr or chmod), it will not work. What build error you encountered? I built it OK, then: $ rpmls ./firejail-0.9.56-2.fc27.x86_64.rpm | grep /usr/bin/firejail -rwsr-xr-x /usr/bin/firejail Note the 's' bit, it's for SUID.
Thanks for packaging firejail! I have some comments concerning the spec file: - Your spec file does not mention a special license, which afaik means that MIT is going to be assumed. However, you seem to have taken the spec from the firejail repository, which means that it is GPLv2 licensed and changing the license to MIT is definitely problematic. - The flags to the configure step: > %configure --disable-userns --disable-contrib-install were (judging from the git log) added for compatibility reasons with RHEL/CentOS. The two disable flags could maybe be dropped for Fedora (not sure if installing contrib is useful though). - The %files section should not add man pages with hardcoded extensions, since it could be changed without further notice. So this: > %{_mandir}/man5/%{name}-login.5.gz > %{_mandir}/man5/%{name}-profile.5.gz > %{_mandir}/man5/%{name}-users.5.gz should become this: > %{_mandir}/man5/%{name}-login.5.* > %{_mandir}/man5/%{name}-profile.5.* > %{_mandir}/man5/%{name}-users.5.* - Minor: you can get a tarball with a better name from github via this url: > Source0: %{URL}/archive/%{version}/%{name}-%{version}.tar.gz - I am not completely sure about the rules when a package should own a directory, but I think it might be necessary for the spec to explicitly own the following: > %dir %{_libdir}/%{name} > %dir %{_datadir}/doc/%{name} > %dir %{_datarootdir}/bash-completion/completions (really not sure about the last one, there was a discussion in the previous packaging request of firejail: https://bugzilla.redhat.com/show_bug.cgi?id=1301286) - Firejail has a testsuite, it can be run with make test (there is also a more "destructive" test suite, which messes with the OS' settings, wouldn't advise to use that one). It requires at least expect to be installed. It seems to test some basic capabilities of firejail and also some applications and their behavior (like firefox, thunderbird, dig, telnet, etc.). I have however not taken an in depth look at it, for instance I am not really sure if it returns a non-zero value on test failure. I am also not sure whether it makes sense to test all the applications requiring X to be running (probably not). Nevertheless, running at least some basic tests would be desirable.
(In reply to dan.cermak from comment #8) Thanks for comments. > Thanks for packaging firejail! > > I have some comments concerning the spec file: > > - Your spec file does not mention a special license, which afaik means that > MIT is going to be assumed. However, you seem to have taken the spec from > the firejail repository, which means that it is GPLv2 licensed and changing > the license to MIT is definitely problematic. > This is good point. I think this could be resolved by asking firejail authors whether we could reuse their SPEC file under MIT. If not there should be explicit comment stating that the spec file is licensed under GPLv2+. > - The flags to the configure step: > > %configure --disable-userns --disable-contrib-install > were (judging from the git log) added for compatibility reasons with > RHEL/CentOS. The two disable flags could maybe be dropped for Fedora (not > sure if installing contrib is useful though). > No idea about it. I would also try to drop it. > - The %files section should not add man pages with hardcoded extensions, > since it could be changed without further notice. So this: > > %{_mandir}/man5/%{name}-login.5.gz > > %{_mandir}/man5/%{name}-profile.5.gz > > %{_mandir}/man5/%{name}-users.5.gz > should become this: > > %{_mandir}/man5/%{name}-login.5.* > > %{_mandir}/man5/%{name}-profile.5.* > > %{_mandir}/man5/%{name}-users.5.* > Valid: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages > - Minor: you can get a tarball with a better name from github via this url: > > Source0: %{URL}/archive/%{version}/%{name}-%{version}.tar.gz > Good point. > - I am not completely sure about the rules when a package should own a > directory, but I think it might be necessary for the spec to explicitly own > the following: > > %dir %{_libdir}/%{name} Not needed, it's already owned, check rpmls. > > %dir %{_datadir}/doc/%{name} owned by %doc macro > > %dir %{_datarootdir}/bash-completion/completions > (really not sure about the last one, there was a discussion in the previous > packaging request of firejail: > https://bugzilla.redhat.com/show_bug.cgi?id=1301286) > IMHO no, because it's already owned by filesystem: https://fedoraproject.org/wiki/Packaging:Guidelines#File_and_Directory_Ownership But IMHO the path should be fixed to /etc/bash_completion.d > - Firejail has a testsuite, it can be run with make test (there is also a > more "destructive" test suite, which messes with the OS' settings, wouldn't > advise to use that one). It requires at least expect to be installed. It > seems to test some basic capabilities of firejail and also some applications > and their behavior (like firefox, thunderbird, dig, telnet, etc.). I have > however not taken an in depth look at it, for instance I am not really sure > if it returns a non-zero value on test failure. I am also not sure whether > it makes sense to test all the applications requiring X to be running > (probably not). Nevertheless, running at least some basic tests would be > desirable. Personally I probably wouldn't run the tests in the form there are available now, because they seems like integration tests to me (I didn't check thoroughly). They have a lot of dependencies, which are in fact build time dependencies, but it could slow down or even break the build. Also it seems there are bundled binaries used in the tests you shouldn't definitely run in the build time, e.g. test/filters/memwrexe. Well, there is source for it, but I cannot find recipe to build it. But of course this could be reported upstream, e.g. request to provide some minimal test target.
Thank you for your comments. (In reply to Jaroslav Škarvada from comment #9) > (In reply to dan.cermak from comment #8) > Thanks for comments. > > > Thanks for packaging firejail! > > > > I have some comments concerning the spec file: > > > > - Your spec file does not mention a special license, which afaik means that > > MIT is going to be assumed. However, you seem to have taken the spec from > > the firejail repository, which means that it is GPLv2 licensed and changing > > the license to MIT is definitely problematic. > > > This is good point. I think this could be resolved by asking firejail > authors whether we could reuse their SPEC file under MIT. If not there > should be explicit comment stating that the spec file is licensed under > GPLv2+. > Is it then OK just to leave a comment? Or it should be better to ask upstream? > > > - The flags to the configure step: > > > %configure --disable-userns --disable-contrib-install > > were (judging from the git log) added for compatibility reasons with > > RHEL/CentOS. The two disable flags could maybe be dropped for Fedora (not > > sure if installing contrib is useful though). > > > No idea about it. I would also try to drop it. > > > - The %files section should not add man pages with hardcoded extensions, > > since it could be changed without further notice. So this: > > > %{_mandir}/man5/%{name}-login.5.gz > > > %{_mandir}/man5/%{name}-profile.5.gz > > > %{_mandir}/man5/%{name}-users.5.gz > > should become this: > > > %{_mandir}/man5/%{name}-login.5.* > > > %{_mandir}/man5/%{name}-profile.5.* > > > %{_mandir}/man5/%{name}-users.5.* > > > Valid: https://fedoraproject.org/wiki/Packaging:Guidelines#Manpages > > > - Minor: you can get a tarball with a better name from github via this url: > > > Source0: %{URL}/archive/%{version}/%{name}-%{version}.tar.gz > > > Good point. > > > - I am not completely sure about the rules when a package should own a > > directory, but I think it might be necessary for the spec to explicitly own > > the following: > > > %dir %{_libdir}/%{name} > Not needed, it's already owned, check rpmls. > > > > %dir %{_datadir}/doc/%{name} > owned by %doc macro > > > > %dir %{_datarootdir}/bash-completion/completions > > (really not sure about the last one, there was a discussion in the previous > > packaging request of firejail: > > https://bugzilla.redhat.com/show_bug.cgi?id=1301286) > > > IMHO no, because it's already owned by filesystem: > https://fedoraproject.org/wiki/Packaging: > Guidelines#File_and_Directory_Ownership > > But IMHO the path should be fixed to /etc/bash_completion.d I probbaly do not understand, how could I package something that does not exist in my buildroot? > > > - Firejail has a testsuite, it can be run with make test (there is also a > > more "destructive" test suite, which messes with the OS' settings, wouldn't > > advise to use that one). It requires at least expect to be installed. It > > seems to test some basic capabilities of firejail and also some applications > > and their behavior (like firefox, thunderbird, dig, telnet, etc.). I have > > however not taken an in depth look at it, for instance I am not really sure > > if it returns a non-zero value on test failure. I am also not sure whether > > it makes sense to test all the applications requiring X to be running > > (probably not). Nevertheless, running at least some basic tests would be > > desirable. > > Personally I probably wouldn't run the tests in the form there are available > now, because they seems like integration tests to me (I didn't check > thoroughly). They have a lot of dependencies, which are in fact build time > dependencies, but it could slow down or even break the build. Also it seems > there are bundled binaries used in the tests you shouldn't definitely run in > the build time, e.g. test/filters/memwrexe. Well, there is source for it, > but I cannot find recipe to build it. But of course this could be reported > upstream, e.g. request to provide some minimal test target.
(In reply to Ondrej Dubaj from comment #10) > Is it then OK just to leave a comment? Or it should be better to ask > upstream? > AFAIK (feel free to correct me if I am wrong) Fedora specs could be licensed under any Fedora compatible license. So probably OK to just leave there comment about the license. But if you don't want to pollute spec with such comments, you can ask upstream. Email to the maintainer or ticket @ github should work. I don't see any reason why they shouldn't approve it. > I probbaly do not understand, how could I package something that does not > exist in my buildroot? You need to call 'move' in the %install phase to correct the path after the %make_install.
(In reply to Jaroslav Škarvada from comment #11) > (In reply to Ondrej Dubaj from comment #10) > > Is it then OK just to leave a comment? Or it should be better to ask > > upstream? > > > AFAIK (feel free to correct me if I am wrong) Fedora specs could be licensed > under any Fedora compatible license. So probably OK to just leave there > comment about the license. But if you don't want to pollute spec with such > comments, you can ask upstream. Email to the maintainer or ticket @ github > should work. I don't see any reason why they shouldn't approve it. > > > I probbaly do not understand, how could I package something that does not > > exist in my buildroot? > > You need to call 'move' in the %install phase to correct the path after the > %make_install. mv OLDPATH NEWPAH
Thank you for your help. Posting modified spec file and srpm according to your comments: Spec URL: https://raw.githubusercontent.com/odubaj/firejail/master/firejail.spec SRPM URL: https://raw.githubusercontent.com/odubaj/firejail/master/firejail-0.9.56-3.fc28.src.rpm Mail to upstream according to license is sent, waiting for approval.
(In reply to Ondrej Dubaj from comment #13) > Thank you for your help. > > Posting modified spec file and srpm according to your comments: > > Spec URL: > https://raw.githubusercontent.com/odubaj/firejail/master/firejail.spec > SRPM URL: > https://raw.githubusercontent.com/odubaj/firejail/master/firejail-0.9.56-3. > fc28.src.rpm > > Mail to upstream according to license is sent, waiting for approval. %config(noreplace) /etc/bash-completion.d/completions/* I think this is wrong. IMHO the completion scripts should go directly under /etc/bash-completion.d/ Could you check whether the completion is working? chmod 0755 %{buildroot}%{_bindir}/%{name} You are removing the SUID bit, I think it will not work correctly. This line should be removed. Which problem you have with the SUID? Could you post your build error? It builds OK for me.
It builds for me also. But using rpmlint I get these errors: $ rpmlint RPMS/x86_64/firejail-0.9.56-3.fc28.x86_64.rpm firejail.x86_64: E: setuid-binary /usr/bin/firejail root 4755 firejail.x86_64: E: non-standard-executable-perm /usr/bin/firejail 4755 I am not exactly sure if it will be better to remove the suid bit or to ignore these errors.
(In reply to Ondrej Dubaj from comment #15) > It builds for me also. But using rpmlint I get these errors: > > $ rpmlint RPMS/x86_64/firejail-0.9.56-3.fc28.x86_64.rpm > firejail.x86_64: E: setuid-binary /usr/bin/firejail root 4755 > firejail.x86_64: E: non-standard-executable-perm /usr/bin/firejail 4755 > > I am not exactly sure if it will be better to remove the suid bit or to > ignore these errors. This is false positive (in this case).
(In reply to Jaroslav Škarvada from comment #16) > (In reply to Ondrej Dubaj from comment #15) > > It builds for me also. But using rpmlint I get these errors: > > > > $ rpmlint RPMS/x86_64/firejail-0.9.56-3.fc28.x86_64.rpm > > firejail.x86_64: E: setuid-binary /usr/bin/firejail root 4755 > > firejail.x86_64: E: non-standard-executable-perm /usr/bin/firejail 4755 > > > > I am not exactly sure if it will be better to remove the suid bit or to > > ignore these errors. > > This is false positive (in this case). While firejail itself would like to be setuid root, that could be a security problem. See for instance this discussion on the SUSE Bugzilla: https://bugzilla.suse.com/show_bug.cgi?id=1059013 . They have decided to drop the suid root and instead create a Firejail group, to which each user must add themselves (see SUSE's spec: https://build.opensuse.org/package/view_file/Virtualization/firejail/firejail.spec?expand=1). Maybe we could consider that option, too?
(In reply to Ondrej Dubaj from comment #13) > > Mail to upstream according to license is sent, waiting for approval. There would be one advantage with keeping the license at GPLv2: upstream could easier reuse this spec file (albeit MIT licensed shouldn't be a problem).
(In reply to dan.cermak from comment #18) > (In reply to Ondrej Dubaj from comment #13) > > > > Mail to upstream according to license is sent, waiting for approval. > > There would be one advantage with keeping the license at GPLv2: upstream > could easier reuse this spec file (albeit MIT licensed shouldn't be a > problem). MIT is more flexible - that's why I see it to be better in this case. Upstream could take the final spec under MIT and re-release it under GPLv2+ without any legal problem.
(In reply to dan.cermak from comment #17) > (In reply to Jaroslav Škarvada from comment #16) > > (In reply to Ondrej Dubaj from comment #15) > > > It builds for me also. But using rpmlint I get these errors: > > > > > > $ rpmlint RPMS/x86_64/firejail-0.9.56-3.fc28.x86_64.rpm > > > firejail.x86_64: E: setuid-binary /usr/bin/firejail root 4755 > > > firejail.x86_64: E: non-standard-executable-perm /usr/bin/firejail 4755 > > > > > > I am not exactly sure if it will be better to remove the suid bit or to > > > ignore these errors. > > > > This is false positive (in this case). > > While firejail itself would like to be setuid root, that could be a security > problem. See for instance this discussion on the SUSE Bugzilla: > https://bugzilla.suse.com/show_bug.cgi?id=1059013 . They have decided to > drop the suid root and instead create a Firejail group, to which each user > must add themselves (see SUSE's spec: > https://build.opensuse.org/package/view_file/Virtualization/firejail/ > firejail.spec?expand=1). > > Maybe we could consider that option, too? I will let it on the package maintainer. From my point of view: - solution with special group is more clean and could be safer - with SUID, the build is already hardened and I think the possible attack vector is low - it would require finding some security bug in the firejail application to exploit. - IIRC in the past FESCO kept list of SUID packages, and every new SUID package required explicit approval, this is no more thus we could also go with the SUID. - one more thing to note there is also the 'upstream first' philosophy in Fedora, so if we go with the special group, we should try to get it to the upstream not to diverge from it.
Thank you for your comments. I have not modified the SUID bit and still waiting for approval about the MIT licence. Currently distributed under GLPv2+. Posting modified spec file and srpm: Spec URL: https://raw.githubusercontent.com/odubaj/firejail/master/firejail.spec SRPM URL: https://raw.githubusercontent.com/odubaj/firejail/master/firejail-0.9.56-4.fc28.src.rpm
Thanks, LGTM.
> mv %{buildroot}%{_datarootdir}/bash-completion/completions/ %{buildroot}/etc/bash_completion.d/ This is actually wrong. The location of the bash completions was correct. If you look at the bash-completion package, it provides this path.
> %config(noreplace) /etc/bash_completion.d/* Bash completions are not config files. They should never be marked as such. That's why they were moved to /usr/share/bash-completion/completions.
(In reply to Neal Gompa from comment #24) > > %config(noreplace) /etc/bash_completion.d/* > > Bash completions are not config files. They should never be marked as such. > That's why they were moved to /usr/share/bash-completion/completions. Thanks, for info, Ondrej could you fix the spec, to install under /usr/share/bash-completion/completion and not mark it as config? Thanks.
Got it and fixed. Posting modified spec file and srpm: Spec URL: https://raw.githubusercontent.com/odubaj/firejail/master/firejail.spec SRPM URL: https://raw.githubusercontent.com/odubaj/firejail/master/firejail-0.9.56-5.fc28.src.rpm
(In reply to Ondrej Dubaj from comment #26) > Got it and fixed. > > Posting modified spec file and srpm: > > Spec URL: > https://raw.githubusercontent.com/odubaj/firejail/master/firejail.spec > SRPM URL: > https://raw.githubusercontent.com/odubaj/firejail/master/firejail-0.9.56-5. > fc28.src.rpm Thanks, it's already approved, you can proceed with further steps.
(fedscm-admin): The Pagure repository was created at https://src.fedoraproject.org/rpms/firejail
This bug can be closed, right?
Yes, sorry for not closing it sooner