Red Hat Bugzilla – Bug 991314
Review Request: libscrypt - Library that implements the secure password hashing function "scrypt"
Last modified: 2013-08-20 20:00:07 EDT
Spec URL: http://www.lolware.net/libscrypt.spec SRPM URL: http://www.lolware.net/libscrypt-1.1a-1.fc19.src.rpm Description: This library implements the "scrypt" secure password hashing system originally described here: http://www.tarsnap.com/scrypt.html. With the increase of insecurely hashed passwords leaked, this library can assist developers in improving security. Fedora Account System Username: technion This is my first package review and I require a sponsor. I have a successful Koji build here: http://www.lolware.net/libscrypt-koji.txt I am the creator of this library. I believe in most cases - developers will elect to use the most easily accessible security libraries to them, and believe acceptance into Fedora would help this goal significantly. FreeBSD have recently picked this package into their ports tree.
Hi, I think you can add your name in Bugzilla ;) This is an informal review: 0. I think you can improve the summary of this library, not just "scrypt() library". 1. Version field has a "a", is it an Alpha release? If so, see: http://fedoraproject.org/wiki/Packaging:NamingGuidelines#Version_Tag As rpmlint said: libscrypt.i686: W: incoherent-version-in-changelog 1.1 ['1.1a-1.fc20', '1.1a-1'] 2. Remove rm -rf $RPM_BUILD_ROOT in %install section. 3. Change PREFIX=/usr to PREFIX=%{_prefix} 4. We don't recommend shipping static libraries, see: So please change find $RPM_BUILD_ROOT -name '*.la' -exec rm -f {} ';' to find $RPM_BUILD_ROOT -name '*.*a' -exec rm -f {} ';'
Oh, forgot a link for static library: http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
Hi Christopher, Many thanks for this review. Going through your points: Name: Done. 0: I had a paragraph here, and rpmlint told me to write something smaller :p I think I've come up with a happy medium now. 1: It's definitely not an alpha release. I made a one line Makefile change from v1.1 to help this packaging process and called it 1.1a - apparently a bad move. I've just tagged a newer version as 1.12 which should resolve this issue. 2. Done. This was put here by the wizard. 3. Done. Definitely an improvement. 4. Done. However, I don't think it should have been there in the first place, so the .a file is no longer installed by "make install". This has been documented on my github page in the changenotes. Ironically, this is a revert to an earlier configuration, however, a packager for another distribution originally requested this. Hopefully I've addressed everything in one go. rpmlint only warns about spelling: [fedora@ip-172-31-20-108 rpmbuild]$ rpmlint ./SPECS/libscrypt.spec ./SRPMS/libscrypt-1.12-1.fc19.src.rpm libscrypt.src: W: spelling-error Summary(en_US) scrypt -> crypt, crypts, script libscrypt.src: W: spelling-error %description -l en_US scrypt -> crypt, crypts, script 1 packages and 1 specfiles checked; 0 errors, 2 warnings. Koji appeared to run well: http://koji.fedoraproject.org/koji/taskinfo?taskID=5691246 5691246 build (f19, libscrypt-1.12-1.fc19.src.rpm) completed successfully I've updated my testing reference to demonstrate Fedora as a confirmed installation: http://www.lolware.net/libscrypttesting.txt The spec file has been updated at the original URL: http://www.lolware.net/libscrypt.spec And a new version appropriate SRPM is here: http://www.lolware.net/libscrypt-1.12-1.fc19.src.rpm Many thanks for the review, hopefully I've addressed any concerns.
* Run rpmlint (or rpmlint -I for more helpful output) on the src.rpm and all built rpms. Feel free to ignore obvious false positives in the report, but fix anything else. Preferably add a comment here about whether/when you think what rpmlint reports is correct or incorrect. > Summary: Linux scrypt shared library Imagine all packages added "Linux" at the beginning of the summary. That's really superfluous. Mentioning that it's a "shared" lib currently is also uninteresting, since Fedora focuses on shared libs. However, you could spend a few more words on summing up what's included in the package. You don't need to restrict yourself to just 2-3 words, e.g. the first sentence from the %description sounds good: Summary: Library that implements the secure password hashing function "scrypt" Or more generic: Summary: Password hash crypto library > %description > A shared library that implements the secure password hashing > function "scrypt". Not really a full sentence. Better: %description This is a library that implements the secure password hashing function "scrypt". * In the build.log: > + make -j5 > gcc -O2 -Wall -g -D_FORTIFY_SOURCE=2 -fstack-protector -fPIC -c -o > crypto_scrypt-nosse.o crypto_scrypt-nosse.c It doesn't use Fedora's compiler %{optflags} yet: https://fedoraproject.org/wiki/Packaging:Guidelines#Compiler_flags > -Wl,-soname,libscrypt.so.0 Something's wrong there. The package (your koji build) doesn't provide the SONAME yet: $ rpm -qp --provides libscrypt-1.12-1.fc19.x86_64.rpm libscrypt = 1.12-1.fc19 libscrypt(x86-64) = 1.12-1.fc19 That's a major blocker. The -devel package depends on it, but it's not provided. The -devel package would not even install. > $ rpmls -p libscrypt-debuginfo-1.12-1.fc19.x86_64.rpm > $ The -debuginfo package is empty and useless.
> $ rpmls -p libscrypt-1.12-1.fc19.x86_64.rpm > -rw-r--r-- /usr/lib64/libscrypt.so.0 chmod 0755
Again, thank you for the review. I've used the summary and description you've suggested in my updated spec file with this. I've also configured it to use the %{optflag} macro, which I've brought back to what I was trying to achieve with the original CLFAGS by adding %global _hardened_build 1. Based on the build output it appears to be using it. Regarding the permissions/chmod issue, I saw this more as an upstream problem (make install should have managed this) and fixed it there in v1.13. Note, I haven't actually tagged v1.13 at the official distribution yet. I really don't want to end up with 49 "stable releases" due to troubleshooting on the packaging. The commit that the SRPM was created from will get this tag the moment I hear we look ready to go. I'm not sure what to make of the SONAME issue. I fixed the issues above, then ran your commands and the soname seems to be there: [fedora@ip-172-31-20-108 SPECS]$ rpm -qp --provides /home/fedora/rpmbuild/RPMS/x86_64/libscrypt-1.13-1.fc19.x86_64.rpm libscrypt = 1.13-1.fc19 libscrypt(x86-64) = 1.13-1.fc19 libscrypt.so.0()(64bit) libscrypt.so.0(libscrypt)(64bit) I must admit I don't fully understand this issue so any advice here would be appreciated. Likewise, the debuginfo seems full of files now: [fedora@ip-172-31-20-108 SPECS]$ rpmls -p /home/fedora/rpmbuild/RPMS/x86_64/libscrypt-debuginfo-1.13-1.fc19.x86_64.rpm rpm: no arguments given for query drwxr-xr-x /usr/lib/debug drwxr-xr-x /usr/lib/debug/.build-id drwxr-xr-x /usr/lib/debug/.build-id/97 ... In evaluating rpmlint: [fedora@ip-172-31-20-108 rpmbuild]$ rpmlint --verbose ./SPECS/libscrypt.spec ./SRPMS/libscrypt-1.13-1.fc19.src.rpm ./RPMS/x86_64/* Numerous spelling warnings: Spelling is correct, scrypt just happens to look like "script" or "crypt" libscrypt.src: W: invalid-url Source0: https://github.com/technion/libscrypt/archive/v1.13.tar.gz HTTP Error 404: Not Found As above, the second it looks like we've resolved everything else I'll tag 1.13 and this error will go away. W: no-documentation I've made some changed to the spec file and believe this issue is resolved for the main rpm. -devel also claims to have "no documentation", but the README.md covers both elements of the RPM and such I don't consider this an issue. Yet another SRPM update: http://www.lolware.net/libscrypt-1.13-1.fc19.src.rpm Updated spec file is in original location. New koji build: http://koji.fedoraproject.org/koji/taskinfo?taskID=5699750
Additional information regarding testing of the current RPMs: [fedora@ip-172-31-20-108 x86_64]$ sudo rpm -iv libscrypt-1.13-1.fc19.x86_64.rpm Preparing packages... libscrypt-1.13-1.fc19.x86_64 [fedora@ip-172-31-20-108 x86_64]$ sudo rpm -iv libscrypt-devel-1.13-1.fc19.x86_64.rpm Preparing packages... libscrypt-devel-1.13-1.fc19.x86_64 [fedora@ip-172-31-20-108 x86_64]$ ls -l /usr/lib64/libscry* lrwxrwxrwx. 1 root root 14 Aug 3 05:24 /usr/lib64/libscrypt.so -> libscrypt.so.0 -rwxr-xr-x. 1 root root 27688 Aug 3 03:54 /usr/lib64/libscrypt.so.0 [fedora@ip-172-31-20-108 x86_64]$ ls -l /usr/include/libscrypt.h -rw-r--r--. 1 root root 2204 Aug 3 03:54 /usr/include/libscrypt.h main.c is the test hardness distributed with the source. [fedora@ip-172-31-20-108 ~]$ gcc -Wall main.c -lscrypt -o reference [fedora@ip-172-31-20-108 ~]$ ./reference ... TEST TWELVE: SUCCESSSFUL. Received the following from simple hash: $s1$0e0810$tiBOLHS6jXyAJFaFmp+rFQ==$WxXBZhDqk0BtrcEDrparUjESet5sw2VpSsENNyE1aQu6C2+Xnsljvu823PjcJZAZa3284UlMreC7BUTah1v1Zw=
I think no big problems here right now. But you still need a sponsor ;) http://fedoraproject.org/wiki/How_to_get_sponsored_into_the_packager_group I can't help ;)
Yes, fixing the library file permissions has also fixed the SONAME Provides. > main.c is the test hardness distributed with the source. > [fedora@ip-172-31-20-108 ~]$ gcc -Wall main.c -lscrypt -o reference > [fedora@ip-172-31-20-108 ~]$ ./reference Great! You've just found something to run in the spec file's %check section. For example: %check gcc -Wall main.c -L$(pwd) -lscrypt -o reference LD_LIBRARY_PATH=$(pwd) ./reference
I found that in Makefile there is a check section. So as Michael has said, you should add a %check section below %install section: %check make check
Thanks for that confirmation Michael. I've just updated the spec file as per Christopher's suggestion. Koji runs for a heck of a long time, because my reference test runs a very high complexity count hash. But it does work and the build log appears to reflect the test harness. http://koji.fedoraproject.org/koji/taskinfo?taskID=5707849 http://kojipkgs.fedoraproject.org//work/tasks/7855/5707855/build.log I've pushed v1.13 tag upstream so we can remove those warnings. Here's a fresh rpmlint to make sure I didn't introduce any new issues: [fedora@ip-172-31-20-108 rpmbuild]$ rpmlint --verbose ./SPECS/libscrypt.spec ./SRPMS/libscrypt-1.13-1.fc19.src.rpm ./RPMS/x86_64/* Again, ignoring the spelling false-positives, the only warning is "W: no-documentation" on the devel package only. Spec file updated: http://www.lolware.net/libscrypt.spec SRPM: http://www.lolware.net/libscrypt-1.13-1.fc19.src.rpm Verification for current versions: # sha1sum libscrypt.spec 9af9dc3964df05428e9989294cc2fdf5f885024a libscrypt.spec # sha1sum libscrypt-1.13-1.fc19.src.rpm ee192d04adc83a7296a6c74a842ebdc197f64f8c libscrypt-1.13-1.fc19.src.rpm
To assist with sponsoring, I've done two informal reviews: https://bugzilla.redhat.com/show_bug.cgi?id=910340 https://bugzilla.redhat.com/show_bug.cgi?id=991531 I'm be looking for more on an ongoing basis.
Hi, I have performed another review: https://bugzilla.redhat.com/show_bug.cgi?id=993636
I think he is good enough now. What about you?
Also: https://bugzilla.redhat.com/show_bug.cgi?id=993456
No blocker left here. The update from comment 11 fixes the packaging issues. APPROVED
New Package SCM Request ======================= Package Name: libscrypt Short Description: Library that implements the secure password hashing function "scrypt" Owners: technion Branches: f18 f19 f20 InitialCC:
Hey....You mustn't change the fedora-review flag to + by yourself... Please let Michael do this. And no f20 branch available now.
Michael already did set the fedora-review flag to + in comment 16. I accidentally removed it when I was trying to set fedora-cvs.. then immediately set it back. f20 lists as a koji build and runs: http://koji.fedoraproject.org/koji/taskinfo?taskID=5792320 If I'm supposed to remove it from here, should it be a new SCM request?
fedora-review flag only can be set by others but not "you". So you should wait for Michael to change it. Don't worry, you have 12 hours until today's window of SCM process.
New Package SCM Request ======================= Package Name: libscrypt Short Description: Library that implements the secure password hashing function "scrypt" Owners: technion Branches: f18 f19 InitialCC:
Git done (by process-git-requests).
libscrypt-1.13-1.fc18 has been submitted as an update for Fedora 18. https://admin.fedoraproject.org/updates/libscrypt-1.13-1.fc18
libscrypt-1.13-1.fc19 has been submitted as an update for Fedora 19. https://admin.fedoraproject.org/updates/libscrypt-1.13-1.fc19
libscrypt-1.13-1.fc18 has been pushed to the Fedora 18 testing repository.
libscrypt-1.13-1.fc18 has been pushed to the Fedora 18 stable repository.
libscrypt-1.13-1.fc19 has been pushed to the Fedora 19 stable repository.