Bug 991314 - Review Request: libscrypt - Library that implements the secure password hashing function "scrypt"
Review Request: libscrypt - Library that implements the secure password hashi...
Status: CLOSED ERRATA
Product: Fedora
Classification: Fedora
Component: Package Review (Show other bugs)
rawhide
All Linux
unspecified Severity medium
: ---
: ---
Assigned To: Michael Schwendt
Fedora Extras Quality Assurance
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2013-08-02 02:35 EDT by Joshua Small
Modified: 2013-08-20 20:00 EDT (History)
4 users (show)

See Also:
Fixed In Version: libscrypt-1.13-1.fc19
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2013-08-20 19:59:21 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---
bugs.michael: fedora‑review+
limburgher: fedora‑cvs+


Attachments (Terms of Use)

  None (edit)
Description Joshua Small 2013-08-02 02:35:19 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.
Comment 1 Christopher Meng 2013-08-02 03:56:48 EDT
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 {} ';'
Comment 2 Christopher Meng 2013-08-02 04:00:23 EDT
Oh, forgot a link for static library:

http://fedoraproject.org/wiki/Packaging:Guidelines#Packaging_Static_Libraries
Comment 3 Joshua Small 2013-08-02 06:11:22 EDT
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.
Comment 4 Michael Schwendt 2013-08-02 09:51:12 EDT
* 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.
Comment 5 Michael Schwendt 2013-08-02 10:40:13 EDT
> $ rpmls -p libscrypt-1.12-1.fc19.x86_64.rpm 
> -rw-r--r--  /usr/lib64/libscrypt.so.0

chmod 0755
Comment 6 Joshua Small 2013-08-03 00:06:23 EDT
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
Comment 7 Joshua Small 2013-08-03 01:50:27 EDT
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=
Comment 8 Christopher Meng 2013-08-03 02:11:53 EDT
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 ;)
Comment 9 Michael Schwendt 2013-08-03 02:46:21 EDT
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
Comment 10 Christopher Meng 2013-08-03 03:19:57 EDT
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
Comment 11 Joshua Small 2013-08-03 04:08:13 EDT
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
Comment 12 Joshua Small 2013-08-03 07:26:15 EDT
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.
Comment 13 Joshua Small 2013-08-06 22:14:41 EDT
Hi,

I have performed another review:

https://bugzilla.redhat.com/show_bug.cgi?id=993636
Comment 14 Christopher Meng 2013-08-06 22:39:21 EDT
I think he is good enough now.

What about you?
Comment 15 Joshua Small 2013-08-06 22:40:32 EDT
Also: https://bugzilla.redhat.com/show_bug.cgi?id=993456
Comment 16 Michael Schwendt 2013-08-07 12:29:39 EDT
No blocker left here. The update from comment 11 fixes the packaging issues.

APPROVED
Comment 17 Joshua Small 2013-08-07 19:51:50 EDT
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:
Comment 18 Christopher Meng 2013-08-07 20:00:16 EDT
Hey....You mustn't change the fedora-review flag to + by yourself...

Please let Michael do this.

And no f20 branch available now.
Comment 19 Joshua Small 2013-08-07 20:15:55 EDT
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?
Comment 20 Christopher Meng 2013-08-07 20:21:29 EDT
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.
Comment 21 Michael Schwendt 2013-08-08 06:39:12 EDT
New Package SCM Request
=======================
Package Name: libscrypt
Short Description: Library that implements the secure password hashing function "scrypt"
Owners: technion
Branches: f18 f19
InitialCC:
Comment 22 Gwyn Ciesla 2013-08-08 08:22:01 EDT
Git done (by process-git-requests).
Comment 23 Fedora Update System 2013-08-11 19:39:27 EDT
libscrypt-1.13-1.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/libscrypt-1.13-1.fc18
Comment 24 Fedora Update System 2013-08-11 19:39:38 EDT
libscrypt-1.13-1.fc19 has been submitted as an update for Fedora 19.
https://admin.fedoraproject.org/updates/libscrypt-1.13-1.fc19
Comment 25 Fedora Update System 2013-08-12 13:58:32 EDT
libscrypt-1.13-1.fc18 has been pushed to the Fedora 18 testing repository.
Comment 26 Fedora Update System 2013-08-20 19:59:21 EDT
libscrypt-1.13-1.fc18 has been pushed to the Fedora 18 stable repository.
Comment 27 Fedora Update System 2013-08-20 20:00:07 EDT
libscrypt-1.13-1.fc19 has been pushed to the Fedora 19 stable repository.

Note You need to log in before you can comment on or make changes to this bug.