Bug 487059

I'll review it. See bz# 225769 for my "merge review".

New spec file and src rpm have been uploaded for review:

http://people.redhat.com/jdennis/spec-review/freeradius2.spec
http://people.redhat.com/jdennis/spec-review/freeradius2-2.1.7-5.el5.src.rpm

We want to support the latest upstream version which is 2.1.7 which is why the version changed. The 2.1.7 versions have been available for a while now in Fedora and as a tech preview for RHEL and CentOS. I know people have been using both the Fedora and RHEL versions of 2.1.7 without issue.

I have run rpmlint (the F-12 version) on the spec file and all the generated
rpms. It generates 135 warnings and errors, however I believe all of them are
invalid for this package. If you discount duplicate messages there are only 9
unique issues reported by rpmlint. I have provided an explanation for why each of
these issues should be discounted in my view.

W: no-documentation

These warnings occur only for sub-packages. This is normal and fine,
sub-packages don't have documentation

E: non-readable /etc/raddb/*
E: non-standard-dir-perm /etc/raddb

Files and directories are non-readable files under /etc/raddb.
This is correct for this package. Why? Because this is an authentication server,
by definition an authentication server's configuration is sensitive and should
not be world readable. Some of these files have passwords in them, some show how
to access LDAP and SQL backends to retrieve sensitive user and system
information, some show the authentication and authorization policies for the
system, some show how to locate authentication proxies, some show how
certificates were generated for use with the server, etc. While it's true there
are some files under /etc/raddb which do not contain any sensitive information
trying to accurately identify that small subset and allow them to be world
readable does not seem like a good exercise. Correctly identifying which are
non-sensitive might be error prone and for those handful of files which are
non-sensitive it would be only of the most marginal value for a
non-administrator to be able to read them without the context of all the other
files they sit next to.

W: file-not-utf8:

There are 3 files marked as not utf-8, all are documentation files, two are
actual rfc documents. All are perfectly readable in a text editor. I don't know
which character(s) triggered the warning, but I don't view these warning as
significant and worthy of eliminating.

E: non-standard-executable-perm /etc/raddb/certs/bootstrap 0750

This is correct, this script is run by the server the first time it starts up
thus it has to be executable by the daemon (it also should be executable by
root). This script creates a set of temporary initial certificates with short
validity periods so that the server will function correctly when an admin is
first testing the server installation. I am not aware of any security
implication with regard to these initial temporary certificates. The FreeRADIUS
documentation, support forums, etc. all direct people to expect this
behavior. Failure to properly create these initial certificates is a common
question on the support forums because various authentication mechanisms will
mysteriously fail without them causing people to think their installation is
broken (for Red Hat that would mean support calls). That's why this bootstrap
process was instigated by upstream, they got tired of answering the same
questions over and over again. My feeling is we should be supporting this for
all the same reasons.

E: incoherent-logrotate-file /etc/logrotate.d/radiusd
W: incoherent-init-script-name radiusd ('freeradius', 'freeradiusd')

These are fine, the package name is freeradius, but the daemon's name is
radiusd. 

E: non-standard-dir-perm /var/log/radius 0700
E: non-standard-dir-perm /var/log/radius/radacct 0700

This is correct, the log files of an authentication server are sensitive
information and should not be world readable.

Created attachment 379473 [details]
rpmlint output for spec file and all built rpms for freeradius2-2.1.7-5

This is the output of rpmlint (run on F-12) for the freeradius-2.1.7-5 version under review. It is the output the previous comment refers to.

Hello, John.
I added some notes to bz ticket #225769

Peter, while your review of the Fedora Package Review bug is very much appreciated, this bug is for the Red Hat Enterprise Linux package review and must be owned by a Red Hat employee. 
Therefore I am removing you as the assignee and setting it back to nobody.
Please continue your Fedora review in bug 225769.

Updated files here:

http://people.redhat.com/jdennis/spec-review/freeradius2.spec
http://people.redhat.com/jdennis/spec-review/freeradius2-2.1.7-6.el5.src.rpm

Comments in https://bugzilla.redhat.com/show_bug.cgi?id=225769 have been
addressed in the above updated version.

Will attach latest run from rpmlint for your convenience.

Created attachment 379691 [details]
latest run of rpmlint

rpmlint output from the 2.1.7-6 version (run on F-12)

#  MUST: rpmlint must be run on every package. The output should be posted in the review.[1]

rpmlint on the SRPM freeradius2-2.1.7-6.el5.src.rpm
freeradius2.src: E: invalid-spec-name
freeradius2.src:19: W: unversioned-explicit-obsoletes freeradius-libs

The warning is fine, since you want to remove all versions. The error is caused by your package name being freeradius2. The spec file should be named freeradius2.spec inside the SRPM.

# MUST: The package must be named according to the Package Naming Guidelines .
PASS - This package must coexist in the repositories with the 1.x FreeRADIUS series.

# MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .
FAIL - See comment above about rpmlint

# MUST: The package must meet the Packaging Guidelines .
PASS

# MUST: The package must be licensed with a Fedora approved license and meet the Licensing Guidelines .
PASS

# MUST: The License field in the package spec file must match the actual license. [3]
PASS

# MUST: 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 must be included in %doc.[4]
FAIL - The src/lib/LICENSE file is not included in the %doc dir

# MUST: The spec file must be written in American English. [5]
PASS

# MUST: The spec file for the package MUST be legible. [6]
PASS

# MUST: The sources used to build the package must match the upstream source, as provided in the spec URL. Reviewers should use md5sum for this task. If no upstream URL can be specified for this package, please see the Source URL Guidelines for how to deal with this.
PASS

# MUST: The package MUST successfully compile and build into binary rpms on at least one primary architecture. [7]
PASS

# MUST: If the package does not successfully compile, build or work on an architecture, then those architectures should be listed in the spec in ExcludeArch. Each architecture listed in ExcludeArch MUST have a bug filed in bugzilla, describing the reason that the package does not compile/build/work on that architecture. The bug number MUST be placed in a comment, next to the corresponding ExcludeArch line. [8]
PASS

# MUST: All build dependencies must be listed in BuildRequires, except for any that are listed in the exceptions section of the Packaging Guidelines ; inclusion of those as BuildRequires is optional. Apply common sense.
PASS

# MUST: The spec file MUST handle locales properly. This is done by using the %find_lang macro. Using %{_datadir}/locale/* is strictly forbidden.[9]
PASS - no localization

# MUST: Every binary RPM package (or subpackage) which stores shared library files (not just symlinks) in any of the dynamic linker's default paths, must call ldconfig in %post and %postun. [10]

# MUST: Packages must NOT bundle copies of system libraries.[11]
PASS

# MUST: If the package is designed to be relocatable, the packager must state this fact in the request for review, along with the rationalization for relocation of that specific package. Without this, use of Prefix: /usr is considered a blocker. [12]
PASS - not relocatable

# MUST: A package must own all directories that it creates. If it does not create a directory that it uses, then it should require a package which does create that directory. [13]
PASS

# MUST: A Fedora package must not list a file more than once in the spec file's %files listings. [14]
PASS

# MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. Every %files section must include a %defattr(...) line. [15]
PASS

# MUST: Each package must have a %clean section, which contains rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [16]
PASS

# MUST: Each package must consistently use macros. [17]
PASS

# MUST: The package must contain code, or permissable content. [18]
PASS

# MUST: Large documentation files must go in a -doc subpackage. (The definition of large is left up to the packager's best judgement, but is not restricted to size. Large can refer to either size or quantity). [19]
PASS

# MUST: If a package includes something as %doc, it must not affect the runtime of the application. To summarize: If it is in %doc, the program must run properly if it is not present. [19]
PASS

# MUST: Header files must be in a -devel package. [20]
PASS

# MUST: Static libraries must be in a -static package. [21]
PASS

# MUST: Packages containing pkgconfig(.pc) files must 'Requires: pkgconfig' (for directory ownership and usability). [22]
PASS - no .pc files

# MUST: If a package contains library files with a suffix (e.g. libfoo.so.1.1), then library files that end in .so (without suffix) must go in a -devel package. [20]
PASS

# MUST: In the vast majority of cases, devel packages must require the base package using a fully versioned dependency: Requires: %{name} = %{version}-%{release} [23]
PASS

# MUST: Packages must NOT contain any .la libtool archives, these must be removed in the spec if they are built.[21]
PASS - .la archives are removed

# MUST: Packages containing GUI applications must include a %{name}.desktop file, and that file must be properly installed with desktop-file-install in the %install section. If you feel that your packaged GUI application does not need a .desktop file, you must put a comment in the spec file with your explanation. [24]
PASS - Not applicable

# MUST: Packages must not own files or directories already owned by other packages. The rule of thumb here is that the first package to be installed should own the files or directories that other packages may rely upon. This means, for example, that no package in Fedora should ever share ownership with any of the files or directories owned by the filesystem or man package. If you feel that you have a good reason to own a file or directory that another package owns, then please present that at package review time. [25]
PASS

# MUST: At the beginning of %install, each package MUST run rm -rf %{buildroot} (or $RPM_BUILD_ROOT). [26]
PASS

# MUST: All filenames in rpm packages must be valid UTF-8. [27]
PASS


*  SHOULD: If the source package does not include license text(s) as a separate file from upstream, the packager SHOULD query upstream to include it. [28]
N/A - Licenses included

* SHOULD: The description and summary sections in the package spec file should contain translations for supported Non-English languages, if available. [29]
No supported translations for the description and summary sections

* SHOULD: The reviewer should test that the package builds in mock. [30]
PASS

* SHOULD: The package should compile and build into binary rpms on all supported architectures. [31]
I only tested i386 and x86_64

* SHOULD: The reviewer should test that the package functions as described. A package should not segfault instead of running, for example.


* SHOULD: If scriptlets are used, those scriptlets must be sane. This is vague, and left up to the reviewers judgement to determine sanity. [32]
N/A

* SHOULD: Usually, subpackages other than devel should require the base package using a fully versioned dependency. [23]
PASS

* SHOULD: The placement of pkgconfig(.pc) files depends on their usecase, and this is usually for development purposes, so should be placed in a -devel pkg. A reasonable exception is that the main pkg itself is a devel tool not installed in a user runtime, e.g. gcc or gdb. [22]
N/A

* SHOULD: If the package has file dependencies outside of /etc, /bin, /sbin, /usr/bin, or /usr/sbin consider requiring the package which provides the file instead of the file itself. [33] 
N/A


In addition to the usual list, I also noticed this:
When doing the BuildRequires: perl[-devel], you test whether you're building on Fedora. This is unsafe, as this will have the wrong BuildRequires if this package is recompiled on RHEL6

The following were updated with the latest tweaks from the last review comments (including some comments from bz #225769)

http://people.redhat.com/jdennis/spec-review/freeradius2.spec
http://people.redhat.com/jdennis/spec-review/freeradius2-2.1.7-6.el5.src.rpm

This latest submission is APPROVED.