Bug 916627

One thing I notice is this:
> export LDFLAGS='-Wl,--as-needed'
Doesn't this trash the default RPM_LD_FLAGS? I think you want:
export LDFLAGS='%{?__global_ldflags} -Wl,--as-needed'
instead.

(And I assume you'll reset Release to 1 with the next version upgrade. I realize we're stuck with the 19xx nonsense for now because the upgrade path needs to be kept.)

Could you push systemd file upstream, as well as the logrotate file ?

And adding a comment saying "remove this obsolete in fedora for this version of fedora" would be nice so we can clean them one day.

and %{_datadir}/tor/ seems to not be owned by the rpm.

I would also comment why torsocks is needed ( I guess that's for torify )

Thanks for the re-reviews so far Kevin and Michael. I knew it was a good idea.

Kevin wrote:
>One thing I notice is this:
>> export LDFLAGS='-Wl,--as-needed'

This is a remnant of the previous package that I forgot to remove. I don't really see a compelling reason for it, and upstream don't do it in their package, so I've removed it until someone convinces me otherwise.


Michael wrote:
> Could you push systemd file upstream, as well as the logrotate file ?

Logrotate file is already upstream. The version in this package is slightly modified (as we run with different user, and reload with systemd instead of initscripts). I've added a comment about this.

I've opened a ticket upstream about including the systemd file.


Michael wrote:
> And adding a comment saying "remove this obsolete in fedora for
> this version of fedora"

Done.


Michael wrote:
> and %{_datadir}/tor/ seems to not be owned by the rpm.

Good catch, this was my mistake. Fixed.


Michael wrote:
> I would also comment why torsocks is needed ( I guess that's
> for torify )

Yes it's for torify. Comment added.


Spec: http://jamielinux.fedorapeople.org/fix-tor/tor-1927.spec
SRPM: http://jamielinux.fedorapeople.org/fix-tor/SRPMS/tor-0.2.3.25-1927.fc18.src.rpm

Kevin wrote:
> (And I assume you'll reset Release to 1 with the next
> version upgrade. I realize we're stuck with the 19xx
> nonsense for now because the upgrade path needs to be
> kept.)

Yes that is correct. I fully intend to amend this when appropriate.

Jamie: "This is a remnant of the previous package that I forgot to remove. I don't really see a compelling reason for it"

--as-needed is intended to reduce overlinking, IIRC. I don't think a package maintainer adding it to a single downstream spec by fiat is a good idea, though; it should either be something upstream does, or some kind of project-wide policy. So I'd support removing it. I believe it's highly unlikely that removing it could make things _less_ likely to work, though in theory it might mean the compiled code links against more stuff than it strictly has to. (My knowledge on this area is somewhat out of date and the flag may be irrelevant these days).

Aside from that, FWIW, I eyeballed the spec and it looks good to me. Thanks for your work on this.

Thanks very much Adam for contributing to the re-review. My understanding of --as-needed is pretty rudimentary, but my take on it is pretty similar to yours. I'm glad you support removing it. And thanks for looking through the spec. The more eyes the better, as I'm just human after all and I've made bad mistakes before...


We've made a couple more commits since last time.

Spec: http://jamielinux.fedorapeople.org/fix-tor/tor-1929.spec
SRPM: http://jamielinux.fedorapeople.org/fix-tor/SRPMS/tor-0.2.3.25-1929.fc18.src.rpm

* Sat Mar 02 2013 Jamie Nguyen <jamielinux> 0.2.3.25-1929
- add "Log notice syslog" back to tor.defaults-torrc as recommended by
  upstream: https://bugzilla.redhat.com/show_bug.cgi?id=532373#c19

* Fri Mar 01 2013 Jamie Nguyen <jamielinux> 0.2.3.25-1928
- increase LimitNOFILE in tor.service from 4096 to 32768, as advised by
  upstream: https://trac.torproject.org/projects/tor/ticket/8368#comment:4


misc also mentioned some systemd hardening that might be done (eg, chroot or restricting access to certain directories, restricting access to /dev etc.) so we'll be tackling that after the first update has been pushed to the repository.

tor-0.2.3.25-1802.fc18 has been submitted as an update for Fedora 18.
https://admin.fedoraproject.org/updates/tor-0.2.3.25-1802.fc18

Package tor-0.2.3.25-1802.fc18:
* should fix your issue,
* was pushed to the Fedora 18 testing repository,
* should be available at your local mirror within two days.
Update it with:
# su -c 'yum update --enablerepo=updates-testing tor-0.2.3.25-1802.fc18'
as soon as you are able to.
Please go to the following url:
https://admin.fedoraproject.org/updates/FEDORA-2013-3434/tor-0.2.3.25-1802.fc18
then log in and leave karma (feedback).

I did a review, but since I'm co-maintainer, if we do an official review, someone else should do it to.

- I changed the URL to use https - I removed the unused lastver/verinfo files

- I'm confused why /var/log/tor is group-readable but /var/lib/tor is
  not. I assume we either trust the group, or not. I'd say enable group
  read for /var/lib/tor. That way a user can add themselves to the group
  and look at what /  how tor is doing without needing root or sudo to the
  toranon user. Or change /var/log/tor to be user/root only.
  (waiting on answer of upstream)

- I assume once a new release happens by upstream, we are getting rid of
  the insane four digit versioning?  
  (Jamie confirmed)

- /usr/bin/torify should never be needed, since we Require: torsocks ?
  Can we just not install it? Would a user call this directly?
  (if not, should it not go into /usr/lib/tor/ or libexec?)
  (Jamie clarified users might call it and expect it to be there, so ok)

Package Review
==============

Key:
[x] = Pass
[!] = Fail
[-] = Not applicable
[?] = Not evaluated
[ ] = Manual review needed


Issues:
=======
[-]: Package do not use a name that already exist
     Note: A package already exist with this name, please check
     https://admin.fedoraproject.org/pkgdb/acls/name/tor
See: https://fedoraproject.org/wiki/Packaging/NamingGuidelines#Conflicting_Package_Names
[!]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 1433600 bytes in 10 files.
See: http://fedoraproject.org/wiki/Packaging/Guidelines#PackageDocumentation

pwouters: docs are changelog/release notes. A bit large, but seems silly to package separately.
          Should we not package it up?

===== MUST items =====

C/C++:
[x]: Header files in -devel subpackage, if present.
[x]: Package does not contain any libtool archives (.la)
[x]: Package does not contain kernel modules.
[x]: Package contains no static executables.
[x]: Rpath absent or only used for internal libs.

Generic:
[x]: Package is licensed with an open-source compatible license and meets
     other legal requirements as defined in the legal section of Packaging
     Guidelines.
[x]: Package successfully compiles and builds into binary rpms on at least one
     supported primary architecture.
[x]: %build honors applicable compiler flags or justifies otherwise.
[x]: All build dependencies are listed in BuildRequires, except for any that
     are listed in the exceptions section of Packaging Guidelines.
[x]: Package contains no bundled libraries.
[x]: Changelog in prescribed format.
[x]: Package does not run rm -rf %{buildroot} (or $RPM_BUILD_ROOT) at the
     beginning of %install.
[x]: Sources contain only permissible code or content.
[x]: %config files are marked noreplace or the reason is justified.
[x]: Each %files section contains %defattr if rpm < 4.4
[x]: Macros in Summary, %description expandable at SRPM build time.
[-]: Package contains desktop file if it is a GUI application.
[-]: Development files must be in a -devel package
[x]: Package requires other packages for directories it uses.
[x]: Package uses nothing in %doc for runtime.
[x]: Package is not known to require ExcludeArch.
[x]: Package does not contain duplicates in %files.
[x]: Permissions on files are set properly.
[x]: Package complies to the Packaging Guidelines
[x]: Spec file lacks Packager, Vendor, PreReq tags.
[x]: If (and only if) the source package includes the text of the license(s)
     in its own file, then that file, containing the text of the license(s)
     for the package is included in %doc.
[x]: License field in the package spec file matches the actual license.
     Note: Checking patched sources after %prep for licenses. Licenses found:
     "BSD (3 clause)", "*No copyright* Beerware", "GPL (v2 or later)",
     "Unknown or generated". 4 files have unknown license. Detailed output of
     licensecheck in /home/paul/fedora/tor/review-tor/licensecheck.txt

pwouters: those are fine

[x]: Package consistently uses macro is (instead of hard-coded directory
     names).
[x]: Package is named using only allowed ASCII characters.
[x]: Package is named according to the Package Naming Guidelines.
[x]: No %config files under /usr.
[x]: Package does not generate any conflict.
     Note: Package contains no Conflicts: tag(s)
[-]: Package do not use a name that already exist
[x]: Package obeys FHS, except libexecdir and /usr/target.
[x]: If the package is a rename of another package, proper Obsoletes and
     Provides are present.
[x]: Package must own all directories that it creates.
[x]: Package does not own files or directories owned by other packages.
[x]: Package installs properly.
[x]: Package is not relocatable.
[x]: Requires correct, justified where necessary.
[x]: CheckResultdir
[x]: Rpmlint is run on all rpms the build produces.
     Note: There are rpmlint messages (see attachment).
[x]: Sources used to build the package match the upstream source, as provided
     in the spec URL.
[x]: Spec file is legible and written in American English.
[x]: Spec file name must match the spec package %{name}, in the format
     %{name}.spec.
[x]: Package contains systemd file(s) if in need.
[x]: File names are valid UTF-8.
[x]: Useful -debuginfo package or justification otherwise.
[!]: Large documentation must go in a -doc subpackage.
     Note: Documentation size is 1433600 bytes in 10 files.
pwouters: OK, its release notes and changelogs.
[x]: Packages must not store files under /srv, /opt or /usr/local
===== SHOULD items =====

Generic:
[x]: Reviewer should test that the package builds in mock.
[x]: Buildroot is not present
[x]: Package has no %clean section with rm -rf %{buildroot} (or
     $RPM_BUILD_ROOT)
[-]: If the source package does not include license text(s) as a separate file
     from upstream, the packager SHOULD query upstream to include it.
[x]: Dist tag is present.
[x]: No file requires outside of /etc, /bin, /sbin, /usr/bin, /usr/sbin.
[-]: Final provides and requires are sane (rpm -q --provides and rpm -q
     --requires).
[x]: Package functions as described.
[x]: Latest version is packaged.
[x]: Package does not include license text files separate from upstream.
[x]: The placement of pkgconfig(.pc) files are correct.
[x]: Scriptlets must be sane, if used.
[x]: SourceX tarball generation or download is documented.
[x]: SourceX / PatchY prefixed with %{name}.
[x]: SourceX is a working URL.
[-]: Description and summary sections in the package spec file contains
     translations for supported Non-English languages, if available.
[x]: Package should compile and build into binary rpms on all supported
     architectures.
[-]: %check is present and all tests pass.
[x]: Packages should try to preserve timestamps of original installed files.
[x]: Spec use %global instead of %define.

===== EXTRA items =====
Generic:
[x]: Rpmlint is run on all installed packages.
     Note: There are rpmlint messages (see attachment).
[x]: Large data in /usr/share should live in a noarch subpackage if package is
     arched.
     Note: Arch-ed rpms have a total of 5877760 bytes in /usr/share 5877760
     tor-0.2.3.25-1929.fc18.x86_64.rpm

pwouters: is ok, caused by large changelog/release notes files.

Rpmlint
-------
Checking: tor-0.2.3.25-1929.fc18.x86_64.rpm
          tor-debuginfo-0.2.3.25-1929.fc18.x86_64.rpm
          tor-0.2.3.25-1929.fc18.src.rpm
tor.x86_64: W: spelling-error Summary(en_US) Anonymizing -> Anatomizing, Canonizing, Anodizing
tor.x86_64: W: only-non-binary-in-usr-lib
tor.x86_64: W: non-standard-uid /var/log/tor toranon
tor.x86_64: W: non-standard-gid /var/log/tor toranon
tor.x86_64: E: non-standard-dir-perm /var/log/tor 0750L
tor.x86_64: W: non-standard-uid /var/lib/tor toranon
tor.x86_64: W: non-standard-gid /var/lib/tor toranon
tor.x86_64: E: non-standard-dir-perm /var/lib/tor 0700L

pwouters: I do think /var/lib/tor should be 0750

tor.src: W: spelling-error Summary(en_US) Anonymizing -> Anatomizing, Canonizing, Anodizing
3 packages and 0 specfiles checked; 2 errors, 7 warnings.

Rpmlint (installed packages)
----------------------------
# rpmlint tor tor-debuginfo
tor.x86_64: W: spelling-error Summary(en_US) Anonymizing -> Anatomizing, Canonizing, Anodizing
tor.x86_64: W: only-non-binary-in-usr-lib
tor.x86_64: W: non-standard-uid /var/log/tor toranon
tor.x86_64: W: non-standard-gid /var/log/tor toranon
tor.x86_64: E: non-standard-dir-perm /var/log/tor 0750L
tor.x86_64: W: non-standard-uid /var/lib/tor toranon
tor.x86_64: W: non-standard-gid /var/lib/tor toranon
tor.x86_64: E: non-standard-dir-perm /var/lib/tor 0700L
2 packages and 0 specfiles checked; 2 errors, 6 warnings.
# echo 'rpmlint-done:'



Requires
--------
tor-0.2.3.25-1929.fc18.x86_64.rpm (rpmlib, GLIBC filtered):

    /bin/sh
    config(tor) = 0.2.3.25-1929.fc18
    libc.so.6()(64bit)
    libcrypto.so.10()(64bit)
    libcrypto.so.10(OPENSSL_1.0.1)(64bit)
    libcrypto.so.10(libcrypto.so.10)(64bit)
    libdl.so.2()(64bit)
    libevent-2.0.so.5()(64bit)
    libm.so.6()(64bit)
    libpthread.so.0()(64bit)
    librt.so.1()(64bit)
    libssl.so.10()(64bit)
    libssl.so.10(libssl.so.10)(64bit)
    libz.so.1()(64bit)
    rtld(GNU_HASH)
    shadow-utils
    systemd
    torsocks

tor-debuginfo-0.2.3.25-1929.fc18.x86_64.rpm (rpmlib, GLIBC filtered):



Provides
--------
tor-0.2.3.25-1929.fc18.x86_64.rpm:

    config(tor) = 0.2.3.25-1929.fc18
    tor = 0.2.3.25-1929.fc18
    tor(x86-64) = 0.2.3.25-1929.fc18
    tor-core = 0:0.2.3.25-1929.fc18
    tor-systemd = 0:0.2.3.25-1929.fc18
    torify = 0:0.2.3.25-1929.fc18

tor-debuginfo-0.2.3.25-1929.fc18.x86_64.rpm:

    tor-debuginfo = 0.2.3.25-1929.fc18
    tor-debuginfo(x86-64) = 0.2.3.25-1929.fc18

MD5-sum check
-------------
Using local file /home/paul/fedora/tor/tor.logrotate as upstream
file:///home/paul/fedora/tor/tor.logrotate :
  CHECKSUM(SHA256) this package     : 0d45f7a0fecc1fa0b11438e82cbaeaece766c5d72ae26aed36d4558d5ac3a650
  CHECKSUM(SHA256) upstream package : 0d45f7a0fecc1fa0b11438e82cbaeaece766c5d72ae26aed36d4558d5ac3a650
Using local file /home/paul/fedora/tor/tor.defaults-torrc as upstream
file:///home/paul/fedora/tor/tor.defaults-torrc :
  CHECKSUM(SHA256) this package     : bec4638642961432a978e1e0d5cf10a26b4dca076a2e6f55a5c4ae5830643eb2
  CHECKSUM(SHA256) upstream package : bec4638642961432a978e1e0d5cf10a26b4dca076a2e6f55a5c4ae5830643eb2
https://www.torproject.org/dist/tor-0.2.3.25.tar.gz :
  CHECKSUM(SHA256) this package     : bb2d6f1136f33e11d37e6e34184143bf191e59501613daf33ae3d6f78f3176a0
  CHECKSUM(SHA256) upstream package : bb2d6f1136f33e11d37e6e34184143bf191e59501613daf33ae3d6f78f3176a0
https://www.torproject.org/dist/tor-0.2.3.25.tar.gz.asc :
  CHECKSUM(SHA256) this package     : e48fb60547eca4c9b40e2d43802f8ddd9d17502432428122607cad7174f4327c
  CHECKSUM(SHA256) upstream package : e48fb60547eca4c9b40e2d43802f8ddd9d17502432428122607cad7174f4327c
Using local file /home/paul/fedora/tor/tor.systemd.service as upstream
file:///home/paul/fedora/tor/tor.systemd.service :
  CHECKSUM(SHA256) this package     : 9251be4fd8b325656cc70f501e69f9ab94637f8d1cfeae3f2efac7388cd4e5ff
  CHECKSUM(SHA256) upstream package : 9251be4fd8b325656cc70f501e69f9ab94637f8d1cfeae3f2efac7388cd4e5ff


Generated by fedora-review 0.3.1 (b71abc1) last change: 2012-10-16
Buildroot used: fedora-18-x86_64
Command line :/usr/bin/fedora-review -rn tor-0.2.3.25-1929.fc19.src.rpm

Thanks for the re-review Paul!

> I'm confused why /var/log/tor is group-readable but /var/lib/tor is
> not. I assume we either trust the group, or not. I'd say enable group
> read for /var/lib/tor. That way a user can add themselves to the group
> and look at what /  how tor is doing without needing root or sudo to the
> toranon user. Or change /var/log/tor to be user/root only.
> (waiting on answer of upstream)

I changed /var/lib/tor to 0750 to match /var/log/tor, but then at runtime tor "fixes" the permissions of /var/lib/tor back to 0700. So instead now both /var/log/tor and /var/lib/tor are 0700.

I think at this stage I'm quite happy with the amount of re-review that has been done for this package with at least 5 sets of eyes having looking through it. Thanks everyone! If there are any other comments/criticisms then please do speak up.

tor-0.2.3.25-1802.fc18 has been pushed to the Fedora 18 stable repository.  If problems still persist, please make note of it in this bug report.