Bug 798071

Summary: Review Request: fedora-arm-installer - Writes binary image files to any specified block device
Product: [Fedora] Fedora Reporter: Jon Chiappetta <jonc_mailbox>
Component: Package ReviewAssignee: Rex Dieter <rdieter>
Status: CLOSED CURRENTRELEASE QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: medium Docs Contact:
Priority: unspecified    
Version: rawhideCC: ctyler.fedora, herrold, mail, misc, notting, package-review, pingou, rdieter
Target Milestone: ---Flags: rdieter: fedora‑review+
limburgher: fedora‑cvs+
Target Release: ---   
Hardware: All   
OS: Linux   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2013-01-24 17:03:02 EST Type: ---
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---

Description Jon Chiappetta 2012-02-27 19:43:30 EST
Spec URL: http://fossjon.fedorapeople.org/fedora-arm-installer.spec
SRPM URL: http://fossjon.fedorapeople.org/fedora-arm-installer-1.0.0-1.fc16.src.rpm
Description: 
Allows one to first select a source image (local or remote). The image must be
a binary file containing: [MBR + Partitions + File Systems + Data]. A 
destination block device should then be selected for final installation.

This is my first package and need a sponsor. I work for Chris Tyler who may be able to help me out and have worked with Paul Whalen who has done this before.

Thanks for your time,
Jon Chiappetta
Comment 1 Michael Scherer 2012-03-17 19:14:17 EDT
I cannot sponsor you, but I can help to do review of the package ( not sure if I need to be a sponsor to do a formal review, so let's do a informal one for now, and I will check the wiki later )

1) echo "#%PAM-1.0" >> pam/%{name}
echo "auth		include		config-util" >> pam/%{name}

is ugly, you should IMHO use heredocument :
cat > pam/%{name} <<EOF
#%PAM-1.0" >> pam/%{name}
auth		include		config-util
account		include		config-util
session		include		config-util
EOF

2) 
%clean
rm -rf $RPM_BUILD_ROOT

this is uneeded cf https://fedoraproject.org/wiki/Packaging/Guidelines#.25clean
( as it was the same for every spec file on earth, it was set as default and should be removed, no need to keep useless code )

3) %{_datadir}/%{name}/data/*

this create unowned directory on installation that would not be removed on rpm removal or managed by rpm ( thus that could have weird permissions, etc )

so you should add either :
%dir %{_datadir}/%{name}
%dir %{_datadir}/%{name}/data

 or just :

%{_datadir}/%{name}/

the 2nd one will take everything below, the first one would requires to precisely list what is in the directory. People recommend to do this usually, so no files are added without maintainer knowing it. ( of course, some maintainer do prefer the contrary )

4)
install -pm 0644 pam/* $RPM_BUILD_ROOT%{_sysconfdir}/pam.d/ ; rm -rf pam

no need to clean the source in %install, this will be removed automatically by builder.

5) the tag Buildroot is not needed and should be removed

6) to finish, there is a security issue in your script, regarding log :

   def logfile(self, prefstri):
        return ("/tmp/faii." + prefstri + ".log")

    def binfile(self, prefstri):
        return ("/tmp/faii." + prefstri + ".bin")

From what I see, there is no verification performed on the file ( ie, does it exist before, is this a symlink to /etc/passwd ) and the program is run as root, so this permit to overwrite any file on the system.

For example, if I create a directory in /tmp ( since /tmp/ is world writable ) called /tmp/faii.c.log, this could likely block you from using the software. Worst, if I do a link to /etc/passwd ( ln -s /etc/passwd /tmp/faii.c.log ), I think this will make the system unable to boot ( or a link to /dev/sda, should be fun too, or to /proc/sysrq-trigger ). Even without running as root, I could remove one of your file ( like, a thesis ) without you noticing. 

To correct that, the usual way os to create a temporary directory, and operate in it. You also need to create it with proper permission and atomically.
See this module : http://docs.python.org/library/tempfile.html

And this article would be a good explanation of the problem 
http://www.linuxsecurity.com/content/view/115462/151/

I would also suggest to create a helper script running as root to do only the necessary ( ie : write to the sdcard ) and make the rest as normal user. 


7) as a side note, since you are the upstream, maybe you should directly ship the desktop in the upstream tarball. You should also take a look at using distribute, or setuptools for the distribution of the code. 

And a tarball named fedora-arm-installer-1.0.0.tar.gz would be better, as this is the name of the directory inside it, and permit to know the version of the code without opening it.
Comment 2 Jon Chiappetta 2012-03-17 22:58:02 EDT
Thank you Michael Scherer for the tips, I have made the changes you recommended plus some bug fixes observed by others online. I'm not sure if I can edit the original post so I am replying to myself here with some updated files. 

Spec URL: http://fossjon.fedorapeople.org/fedora-arm-installer.spec
SRPM URL: http://fossjon.fedorapeople.org/fedora-arm-installer-1.0.0-2.fc16.src.rpm

Thanks for taking the time to review my work,
Jon Chiappetta
Comment 3 Michael Scherer 2012-03-18 07:06:45 EDT
Indeed, you cannot edit the post. Think of it like a mailing list more than like a forum :)

1) I think, but I am not sure, that it would be better to use exec in the last part of the helper script :
export GNOME_DESKTOP_SESSION_ID=needed
%{_sbindir}/%{name}

this way, you have only one process in the process table, and not 2, this is cleaner.

2) if you use consolehelper, I think you should have a requires on usermode. Even if this is installed by default on graphical desktop ( being required by firstboot ), it can be removed.


3) the license should be present in a %doc :
https://fedoraproject.org/wiki/Packaging:LicensingGuidelines#License_Text

4) the requires is on python, I think you should tell if this is version 2 or 3.


I need to go, so I will add some remarks later ( once I have done enough research too )
Comment 4 Jon Chiappetta 2012-03-18 12:09:10 EDT
I added a usermode requires in the spec file, changed the exec in the helper shell script and added a new GPL license file to the doc files section. Regarding the Python version 2 requirement, I can't seem to find a package that matches anything with the name python2, python-2, python2-devel and so on so I'm not sure how to specify that. I put these changes in the release 2 of the package I posted above. 

Thanks again for your time,
Jon Chiappetta
Comment 5 Michael Scherer 2012-03-18 18:17:41 EDT
On f16, i see that python rpm provides python2 :

$ rpm -q --provides python
Distutils  
python(abi) = 2.7
python(abi) = 2.7
python-abi = 2.7
python-argparse = 2.7.2-5.2.fc16
python-ctypes = 1.0.1
python-hashlib = 20081120
python-sqlite = 2.3.2
python-uuid = 1.31
python2 = 2.7.2
python = 2.7.2-5.2.fc16
python(x86-64) = 2.7.2-5.2.fc16


And for the desktop file, the guideline requires you to either run desktop-file-install, or desktop-file-validate, to make sure the file is correct :

https://fedoraproject.org/wiki/Packaging/Guidelines#.desktop_file_creation
Comment 6 Jon Chiappetta 2012-03-18 20:01:58 EDT
Ahhh, my mistake, I was running the wrong command in trying to find the real pointer to python2. I added the desktop-file-install command along with it's suggested buildrequires line in the release 2 package again. I also did a mock rebuild for Fedora 16 x86_64 to make sure the package still built successfully :). I'm learning a lot about properly packaging software given this simple python script so this is fun so far!

Thanks,
Jon Chiappetta
Comment 7 Jon Chiappetta 2012-09-14 21:59:26 EDT
Just in case anyone is still interested in this project, the latest packages and code (SRPM={source,spec}) URLs can be found below (maintained by me). The only complaint so far is that this program mainly works on English based systems due to the fact that I am parsing through subprocess output (with regex) on both Linux and Windows so it is not multi-language as of yet but could be one day. 

SRPM && Binary ( http://fossjon.fedorapeople.org/packages/fedora-arm-installer/ )

Source Code ( https://github.com/fossjon/Seneca/tree/master/source/projects/fedora-arm-installer )

Thanks!
Comment 8 Fabian Affolter 2012-11-10 05:04:17 EST
Can you please provide a link to your spec file?
Comment 9 Rex Dieter 2012-12-05 15:41:09 EST
I can review (and echo Fabian's request to post link to .spec too), i'll get grumpy if you make me redownload the srpm for each iteration.  :)
Comment 10 Jon Chiappetta 2012-12-05 16:02:15 EST
Sorry, I had missed Fabian's comment post earlier. So the links to my latest work for this project can be found here: 

Spec file: http://fossjon.fedorapeople.org/source/projects/specs/fedora-arm-installer.spec
Source RPM: http://fossjon.fedorapeople.org/packages/fedora-arm-installer/fedora-arm-installer-1.0.2-3.fc18.src.rpm
noarch RPM: http://fossjon.fedorapeople.org/packages/fedora-arm-installer/fedora-arm-installer-1.0.2-3.fc18.noarch.rpm

I'm still new to this process so please forgive me if I make some basic mistakes!
Thanks again for your time,

Jon
Comment 11 Rex Dieter 2012-12-11 09:55:59 EST
A few initial comments:

1.  I see no URL: tag, and this
Source0: %{name}-%{version}.tar.gz
references no upstream.   Ideally, we'd have some URL: tag and fully-qualified downloadable (and verifiable) source for this package.

If this is something that doesn't yet have an upstream, consider using
http://fedorahosted.org/

2.  you seem to create a lot of "here" documents, using
cat > exe/%{name}-helper <<EOF
...
it's largely a matter of taste, but I personally strongly prefer and suggest you include these items as separate
SourceX: 
items instead, and potentially work to include them in the upstream tarball someday.

3. you can most likely omit
Requires:       python2
python dependency should get picked up by rpm automatically (and implicitly via PyQt4 dependency too).
Comment 12 Rex Dieter 2012-12-11 09:57:00 EST
Oh, I can't seem to find your FAS username mentioned here anywhere, please do, i'll need that to sponsor you,
https://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Create_a_Fedora_Account
Comment 13 Jon Chiappetta 2012-12-11 15:22:02 EST
So, my FAS username is "fossjon" (I thought this procedure would have tied that in somewhere but maybe I missed it). I updated the spec file and rpms based on your suggestion which helped clean it up a bit more and rpmlint doesn't report any errors on any of the files. 

Spec: http://fossjon.fedorapeople.org/source/projects/specs/fedora-arm-installer.spec
SRPM: http://fossjon.fedorapeople.org/packages/fedora-arm-installer/fedora-arm-installer-1.0.2-4.fc18.src.rpm
RPM: http://fossjon.fedorapeople.org/packages/fedora-arm-installer/fedora-arm-installer-1.0.2-4.fc18.noarch.rpm

Thanks.
Comment 14 Jon Chiappetta 2013-01-10 13:46:03 EST
Sorry for the minor update but I made a temporary folder with the current spec file so that when I reorganize my backup stuff I don't have to keep updating links. 

Spec: http://fossjon.fedorapeople.org/review/fedora-arm-installer.spec
SRPM: http://fossjon.fedorapeople.org/packages/fedora-arm-installer/fedora-arm-installer-1.0.2-4.fc18.src.rpm
RPM: http://fossjon.fedorapeople.org/packages/fedora-arm-installer/fedora-arm-installer-1.0.2-4.fc18.noarch.rpm
Comment 15 Rex Dieter 2013-01-13 17:54:08 EST
4.  One last small item, turns out
%{_datadir}/%{name}
dir is unowned, options to fix include changing
%{_datadir}/%{name}/data/
to
%{_datadir}/%{name}/
or add
dir %{_datadir}/%{name}/


That's not blocker-worthy, but please to fix prior to issuing any official builds.

$ rpmlint *.rpm
1 packages and 0 specfiles checked; 0 errors, 0 warnings.

sources: ok
$ md5sum *.gz
5eb298a79bbbd643f31f1eaad6cc2fe3  fedora-arm-installer-1.0.2.tar.gz

naming: ok

license: ok
(though code doesn't include any license headers, you/upstream probably should add some)


APPROVED (and sponsored)


continue on to next steps:
http://fedoraproject.org/wiki/Join_the_package_collection_maintainers#Add_Package_to_Source_Code_Management_.28SCM.29_system_and_Set_Owner
Comment 16 Jon Chiappetta 2013-01-14 16:08:50 EST
New Package SCM Request
=======================
Package Name: fedora-arm-installer
Short Description: Allows an average user to easily install and create bootable ARM software on SD card media. 
Owners: fossjon
Branches: f18
InitialCC:
Comment 17 Jon Ciesla 2013-01-15 06:57:12 EST
Git done (by process-git-requests).
Comment 18 Fedora Update System 2013-01-15 11:17:32 EST
fedora-arm-installer-1.0.2-5.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/fedora-arm-installer-1.0.2-5.fc18
Comment 19 Fedora Update System 2013-01-16 14:50:36 EST
fedora-arm-installer-1.0.2-5.fc18 has been pushed to the Fedora 18 testing repository.
Comment 20 Fedora Update System 2013-01-24 17:03:06 EST
fedora-arm-installer-1.0.2-5.fc18 has been pushed to the Fedora 18 stable repository.