Bug 491719 - Review Request: hmaccalc - Tools for computing and checking HMAC values for files
Review Request: hmaccalc - Tools for computing and checking HMAC values for f...
Status: CLOSED CURRENTRELEASE
Product: Red Hat Enterprise Linux 5
Classification: Red Hat
Component: Package Review (Show other bugs)
5.0
All Linux
medium Severity medium
: rc
: ---
Assigned To: Jarod Wilson
:
Depends On:
Blocks: 188271 FIPS-140-Tracker 467500 491724
  Show dependency treegraph
 
Reported: 2009-03-23 14:12 EDT by Nalin Dahyabhai
Modified: 2012-07-13 05:54 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2012-07-13 05:54:01 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Nalin Dahyabhai 2009-03-23 14:12:42 EDT
Spec URL: http://nalin.fedorapeople.org/hmaccalc.spec
SRPM URL: http://nalin.fedorapeople.org/hmaccalc-0.9-1.src.rpm
Description: 
The hmaccalc package contains tools which can calculate HMAC values for
files.  The names and interfaces are meant to mimic the sha*sum tools
provided by the coreutils package.
Though a patch exists to provide this as part of coreutils, it wouldn't be using a FIPS-approved library, so we'd still need something else.  This implementation uses NSS.
Comment 1 Jarod Wilson 2009-03-24 12:08:43 EDT
Initial pass through the spec...

1) According to http://fedoraproject.org/wiki/Licensing, the License: tag should be MIT instead of X11.

2) I see a commented out URL: tag... Given that it was written in-house for our own purposes, I guess there isn't really any upstream project to reference, but it would still be nice to have somewhere to point to for people to get more info on it. Worst-case, to get *something* in there, maybe point at et.redhat.com? I dunno...

3) the Summary: and %description both say just HMAC, might be good to go with full expansion of the acronym in %description for the sake of thoroughness.

4) non-executable and hidden bits in /usr/bin/ are frowned upon. It would make more sense to me for the binaries' hmacs to be stored in /usr/share/hmaccalc/ or similar, w/o the . prefix.

5) I take it those .hmac files aren't supposed to be human-readable... But is there any reason they can't be stored in a format that is, so that people can do manual inspection? Similarly, the output from the -S option can do Bad Things to ones console. Some simple binary to hex string conversion doesn't seem like it should incur too much overhead. Probably neither here nor there though, the man page does say "internal use, you shouldn't use it".

6) no version in changelog entry, the packaging guidelines now say you must have them (still amusing that they're called "guidelines" but enforced more like "rules"...)


rpmlint spew:

$ rpmlint hmaccalc-0.9-1.src.rpm
hmaccalc.src: W: no-version-in-last-changelog
hmaccalc.src: W: invalid-license X11
hmaccalc.src: W: no-url-tag
1 packages and 0 specfiles checked; 0 errors, 3 warnings.

All already covered above.


$ rpmlint hmaccalc-*.x86_64.rpm
hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha384hmac.hmac
hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha384hmac.hmac 0644
hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha256hmac.hmac
hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha256hmac.hmac 0644
hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha512hmac.hmac
hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha512hmac.hmac 0644
hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha1hmac.hmac
hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha1hmac.hmac 0644
hmaccalc.x86_64: W: no-version-in-last-changelog
hmaccalc.x86_64: W: invalid-license X11
hmaccalc.x86_64: W: no-url-tag
hmaccalc-debuginfo.x86_64: W: no-version-in-last-changelog
hmaccalc-debuginfo.x86_64: W: invalid-license X11
hmaccalc-debuginfo.x86_64: W: no-url-tag
2 packages and 0 specfiles checked; 0 errors, 14 warnings.

All already covered above as well.
Comment 2 Nalin Dahyabhai 2009-03-24 13:07:27 EDT
(In reply to comment #1)
> Initial pass through the spec...
> 
> 1) According to http://fedoraproject.org/wiki/Licensing, the License: tag
> should be MIT instead of X11.

Okay, will fix.

> 2) I see a commented out URL: tag... Given that it was written in-house for our
> own purposes, I guess there isn't really any upstream project to reference, but
> it would still be nice to have somewhere to point to for people to get more
> info on it. Worst-case, to get *something* in there, maybe point at
> et.redhat.com? I dunno...

It should be at https://fedorahosted.org/hmaccalc/ now, it'll be added.
 
> 3) the Summary: and %description both say just HMAC, might be good to go with
> full expansion of the acronym in %description for the sake of thoroughness.

Oh, good idea.
 
> 4) non-executable and hidden bits in /usr/bin/ are frowned upon. It would make
> more sense to me for the binaries' hmacs to be stored in /usr/share/hmaccalc/
> or similar, w/o the . prefix.

That might make it tricky to move around, for example, into an initrd.  So long as the checksum can be in the same directory as the binary itself (see nss for example), then I'm okay with changing the naming convention -- it's already a configure-time option.

> 5) I take it those .hmac files aren't supposed to be human-readable... But is
> there any reason they can't be stored in a format that is, so that people can
> do manual inspection? Similarly, the output from the -S option can do Bad
> Things to ones console. Some simple binary to hex string conversion doesn't
> seem like it should incur too much overhead. Probably neither here nor there
> though, the man page does say "internal use, you shouldn't use it".

No strong reason; I should probably just go ahead and do that.

> 6) no version in changelog entry, the packaging guidelines now say you must
> have them (still amusing that they're called "guidelines" but enforced more
> like "rules"...)

Yeah, that'd only include "initial package" at this point, but it's being populated now.

> rpmlint spew:
> 
> $ rpmlint hmaccalc-0.9-1.src.rpm
> hmaccalc.src: W: no-version-in-last-changelog
> hmaccalc.src: W: invalid-license X11
> hmaccalc.src: W: no-url-tag
> 1 packages and 0 specfiles checked; 0 errors, 3 warnings.
> 
> All already covered above. 
> 
> $ rpmlint hmaccalc-*.x86_64.rpm
> hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha384hmac.hmac
> hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha384hmac.hmac 0644
> hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha256hmac.hmac
> hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha256hmac.hmac 0644
> hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha512hmac.hmac
> hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha512hmac.hmac 0644
> hmaccalc.x86_64: W: hidden-file-or-dir /usr/bin/.sha1hmac.hmac
> hmaccalc.x86_64: W: non-executable-in-bin /usr/bin/.sha1hmac.hmac 0644
> hmaccalc.x86_64: W: no-version-in-last-changelog
> hmaccalc.x86_64: W: invalid-license X11
> hmaccalc.x86_64: W: no-url-tag
> hmaccalc-debuginfo.x86_64: W: no-version-in-last-changelog
> hmaccalc-debuginfo.x86_64: W: invalid-license X11
> hmaccalc-debuginfo.x86_64: W: no-url-tag
> 2 packages and 0 specfiles checked; 0 errors, 14 warnings.
> 
> All already covered above as well.  

Updated .spec file: http://git.fedorahosted.org/git/?p=hmaccalc.git;a=blob;f=hmaccalc.spec
Comment 3 Jarod Wilson 2009-03-24 14:11:46 EDT
(In reply to comment #2)
> > 1) According to http://fedoraproject.org/wiki/Licensing, the License: tag
> > should be MIT instead of X11.
> 
> Okay, will fix.
> 
> > 2) I see a commented out URL: tag... Given that it was written in-house for our
> > own purposes, I guess there isn't really any upstream project to reference, but
> > it would still be nice to have somewhere to point to for people to get more
> > info on it. Worst-case, to get *something* in there, maybe point at
> > et.redhat.com? I dunno...
> 
> It should be at https://fedorahosted.org/hmaccalc/ now, it'll be added.

Fixes for 1) and 2) are mentioned in the changelog of the spec in git, but the changes themselves aren't actually there. :)


> > 4) non-executable and hidden bits in /usr/bin/ are frowned upon. It would make
> > more sense to me for the binaries' hmacs to be stored in /usr/share/hmaccalc/
> > or similar, w/o the . prefix.
> 
> That might make it tricky to move around, for example, into an initrd.  So long
> as the checksum can be in the same directory as the binary itself (see nss for
> example), then I'm okay with changing the naming convention -- it's already a
> configure-time option.

Eh, we could have /usr/share/hmaccalc in the initrd too. Or my thought was that the checksums might first be looked for in the same dir as the binary, and if not found there, looked for in /usr/share/hmaccalc/, so for initrd purposes, they could go in the same dir as the binaries. Packages really shouldn't be dropping non-bin stuff in %_bindir though.


> > 5) I take it those .hmac files aren't supposed to be human-readable... But is
> > there any reason they can't be stored in a format that is, so that people can
> > do manual inspection? Similarly, the output from the -S option can do Bad
> > Things to ones console. Some simple binary to hex string conversion doesn't
> > seem like it should incur too much overhead. Probably neither here nor there
> > though, the man page does say "internal use, you shouldn't use it".
> 
> No strong reason; I should probably just go ahead and do that.

Cool, I like it.


> > 6) no version in changelog entry, the packaging guidelines now say you must
> > have them (still amusing that they're called "guidelines" but enforced more
> > like "rules"...)
> 
> Yeah, that'd only include "initial package" at this point, but it's being
> populated now.

Much better, thanks much. Never hurts to have a version-release entry for the initial package too, so you know exactly when it was first packaged.
Comment 4 Nalin Dahyabhai 2009-03-24 14:34:49 EDT
(In reply to comment #3)
> Fixes for 1) and 2) are mentioned in the changelog of the spec in git, but the
> changes themselves aren't actually there. :)

Aargh, forgot to redo those when I switched to using the fedorahosted git repository as my 'origin'.  Fixed now.  This time, for sure!

> Eh, we could have /usr/share/hmaccalc in the initrd too.

One more hard-to-troubleshoot detail if it gets forgotten.  As an aside, /usr/share is not a good location because the contents of the self-check file are dependent on the contents of the binary, which are always going to be arch-dependent.

>                                                         Or my thought was that
> the checksums might first be looked for in the same dir as the binary, and if
> not found there, looked for in /usr/share/hmaccalc/, so for initrd purposes,
> they could go in the same dir as the binaries. Packages really shouldn't be
> dropping non-bin stuff in %_bindir though.

No execute bit, so I'm not overly concerned about it, but it doesn't match precedent, either.  Searching multiple directories for the file with the right name would be painful -- if the data can be in multiple places, it can conflict, and I really don't want to deal with that possibility.

Maybe %{_sysconfdir}/%{name} or something like that instead of /usr/share, to avoid littering /usr/bin.
Comment 5 Jarod Wilson 2009-03-24 17:18:15 EDT
(In reply to comment #4)
> (In reply to comment #3)
> > Fixes for 1) and 2) are mentioned in the changelog of the spec in git, but the
> > changes themselves aren't actually there. :)
> 
> Aargh, forgot to redo those when I switched to using the fedorahosted git
> repository as my 'origin'.  Fixed now.  This time, for sure!

Looks good. One more thing I neglected to point out... Since you've got a tarballs directory web-accessible, it'd be good to change the Sourc0: line to match the full download URL, i.e.:

Source0: https://fedorahosted.org/released/hmaccalc/%{name}-%{release}.tar.gz

> > Eh, we could have /usr/share/hmaccalc in the initrd too.
> 
> One more hard-to-troubleshoot detail if it gets forgotten.  As an aside,
> /usr/share is not a good location because the contents of the self-check file
> are dependent on the contents of the binary, which are always going to be
> arch-dependent.

Good point, hadn't considered that...


> >                                                         Or my thought was that
> > the checksums might first be looked for in the same dir as the binary, and if
> > not found there, looked for in /usr/share/hmaccalc/, so for initrd purposes,
> > they could go in the same dir as the binaries. Packages really shouldn't be
> > dropping non-bin stuff in %_bindir though.
> 
> No execute bit, so I'm not overly concerned about it, but it doesn't match
> precedent, either.  Searching multiple directories for the file with the right
> name would be painful -- if the data can be in multiple places, it can
> conflict, and I really don't want to deal with that possibility.
> 
> Maybe %{_sysconfdir}/%{name} or something like that instead of /usr/share, to
> avoid littering /usr/bin.  

%{_libdir}/%{name} also comes to mind as a possibility. Not sure which is more apropos, since its neither a library nor a config file (being in /etc, it suggests perhaps the user may edit it).
Comment 6 Nalin Dahyabhai 2009-03-24 19:04:42 EDT
(In reply to comment #5)
> Looks good. One more thing I neglected to point out... Since you've got a
> tarballs directory web-accessible, it'd be good to change the Sourc0: line to
> match the full download URL, i.e.:

Works for me, done.

> %{_libdir}/%{name} also comes to mind as a possibility. Not sure which is more
> apropos, since its neither a library nor a config file (being in /etc, it
> suggests perhaps the user may edit it).  

Well, libdir gets all sorts of arch-specific things thrown under it, so that's fine by me.  Changed.
Comment 7 Jarod Wilson 2009-03-25 16:53:34 EDT
Ugh. Figures. rpmlint now complains like so:

$ rpmlint -i hmaccalc-*0.9.3-1.fc10.x86_64.rpm
hmaccalc.x86_64: E: only-non-binary-in-usr-lib
There are only non binary files in /usr/lib so they should be in /usr/share.

2 packages and 0 specfiles checked; 1 errors, 0 warnings.

Since these really are arch-specific, I don't really see a problem just ignoring this complaint. Outside of that, my local 0.9.3-1 build looks to be in fine shape now. Just successfully completed a mock build of it as well.

Package APPROVED, lets get this puppy imported...

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