Bug 823446 - Review Request: libradius - This is a library to generate RADIUS authentication request
Summary: Review Request: libradius - This is a library to generate RADIUS authenticati...
Keywords:
Status: CLOSED WONTFIX
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Stephen Gallagher
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2012-05-21 09:47 UTC by Simone Caronni
Modified: 2014-06-16 11:21 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2012-06-14 07:31:00 UTC
Type: ---
Embargoed:
sgallagh: fedora-review?


Attachments (Terms of Use)
Add support for using NSS for MD5 hashes (5.08 KB, patch)
2012-05-29 19:01 UTC, Stephen Gallagher
negativo17: review+
Details | Diff

Description Simone Caronni 2012-05-21 09:47:13 UTC
Spec URL: http://slaanesh.fedorapeople.org/libradius.spec
SRPM URL: http://slaanesh.fedorapeople.org/libradius-20040827-1.fc17.src.rpm
Description: This is a library to generate RADIUS authentication request.
Fedora Account System Username: slaanesh

Comment 1 Gwyn Ciesla 2012-05-25 14:33:39 UTC
Stephen, FYI, this requires libmd.

https://bugzilla.redhat.com/show_bug.cgi?id=823444

Comment 2 Stephen Gallagher 2012-05-25 15:50:29 UTC
tl;dr version:
The libradius(3) belongs in the -devel package. It's not useful for runtime.
The version is wrong, doesn't match upstream and doesn't match the %changelog

Not a blocker to packaging:
It would be better to avoid the dep on libmd if possible. There are many other libraries in Fedora already that can do MD5 hashes. Consider porting the dependency to the 'nss' package for crypto certification. See http://crypto.stanford.edu/firefox-rhash/ for a good starting point.






MUST: rpmlint must be run on the source rpm and all binary rpms the build produces. The output should be posted in the review.[1]

[sgallagh@sgallagh520 SRPMS]$ rpmlint ./libradius-20040827-1.fc17.src.rpm ~/rpmbuild/RPMS/x86_64/libradius-*
libradius.x86_64: W: incoherent-version-in-changelog 0.1-2 ['20040827-1.fc17', '20040827-1']
libradius-devel.x86_64: W: no-documentation
4 packages and 0 specfiles checked; 0 errors, 2 warnings.



MUST: The package must be named according to the Package Naming Guidelines .
FAIL: The package version should be based on the upstream version, which is 20040827


MUST: The spec file name must match the base package %{name}, in the format %{name}.spec unless your package has an exemption. [2] .
PASS



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]
N/A



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 (tested on F17 x86_64)



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]
N/A



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]
N/A



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]
PASS



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]
N/A



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]
N/A



MUST: A Fedora package must not list a file more than once in the spec file's %files listings. (Notable exception: license texts in specific situations)[14]
PASS



MUST: Permissions on files must be set properly. Executables should be set with executable permissions, for example. [15]
PASS



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



MUST: The package must contain code, or permissable content. [17]
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). [18]
N/A



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. [18]
PASS



MUST: Static libraries must be in a -static package. [19]
N/A



MUST: Development files must be 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}%{?_isa} = %{version}-%{release} [21]
PASS



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



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. [22]
N/A



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. [23]
PASS



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

Comment 3 Gwyn Ciesla 2012-05-25 15:53:21 UTC
The only reason that this uses libmd is because they're from the same upstream, and it's known to currently work.  I don't love it either, but it seemed better than more bundling.

Comment 4 Stephen Gallagher 2012-05-25 16:03:54 UTC
(In reply to comment #3)
> The only reason that this uses libmd is because they're from the same
> upstream, and it's known to currently work.  I don't love it either, but it
> seemed better than more bundling.

Sure, but looking at the code it seems like a near-trivial effort to port to nss. I may contribute such a patch if we decide to start using this library. (I don't want to pull in libmd, potentially into RHEL, just for this redundant library).

Comment 5 Gwyn Ciesla 2012-05-25 17:17:50 UTC
That was my concern as well but I'm not familiar enough with that code to make that assessment.  If you were to do that in the near term, we could avoid libmd altogether, but that's another issue.

Comment 6 Simone Caronni 2012-05-26 08:37:13 UTC
Hello,

I will try to see what can be done regarding the nss library on Monday when I come back to the office and fix the version number.

If someone wants to provide the patch I'll be much grateful as I would like as well to reduce dependency on other libraries.

Thanks,
--Simone

(typed on the phone)dw

Comment 7 Stephen Gallagher 2012-05-26 12:28:47 UTC
Re-setting the NEEDSINFO flag per the review guidelines (since we're still waiting on the review to be resubmitted).

Comment 8 Simone Caronni 2012-05-28 13:22:22 UTC
(In reply to comment #2)
> tl;dr version:
> The libradius(3) belongs in the -devel package. It's not useful for runtime.
> The version is wrong, doesn't match upstream and doesn't match the %changelog
>
> Not a blocker to packaging:
> It would be better to avoid the dep on libmd if possible. There are many
> other libraries in Fedora already that can do MD5 hashes. Consider porting
> the dependency to the 'nss' package for crypto certification. See
> http://crypto.stanford.edu/firefox-rhash/ for a good starting point.

Updated package with the fixes:

Spec URL: http://slaanesh.fedorapeople.org/libradius.spec
SRPM URL: http://slaanesh.fedorapeople.org/libradius-20040827-2.fc17.src.rpm

- Fixed changelog version that was causing rpmlint errors.
- Moved libradius(3) manpage in -devel package.

I'm not able to port the MD5 calls to the nss library; if you can supply the patch to remove the dependency that would be great. As it's not a blocker to packaging that can also be set later on by setting an Obsolete flag in the spec file wrt the libmd library as this is the only package requiring it.

Thanks,
--Simone

Comment 9 Stephen Gallagher 2012-05-29 19:01:40 UTC
Created attachment 587487 [details]
Add support for using NSS for MD5 hashes

Attached is a completely untested patch for adding NSS support. I don't have a RADIUS server available at the moment to test against. Could you give it a try (it applies atop your existing patch) and let me know if it works? If so, we can drop the libmd requirement.

Comment 10 Simone Caronni 2012-05-30 08:51:51 UTC
Many thanks,

here is the updated package. I've merged libnss and libmd patches as libmd should be removed and the patch you provided relies on top of the changes included in the libmd patch.

Spec URL: http://slaanesh.fedorapeople.org/libradius.spec
SRPM URL: http://slaanesh.fedorapeople.org/libradius-20040827-3.fc17.src.rpm

- Merge patch from Stephen Gallagher with libmd patch.
- Remove libmd BR and add nss and nspr.

I'll post an updated mod_auth_xradius soon, I'm applying some small changes as I find a couple of leftovers from the unbundling of libradius.

Comment 11 Simone Caronni 2012-05-30 10:01:03 UTC
There is one problem: the library doesn't work even though linked by the Apache module.

When loading the apache module, I got the following:

/etc/httpd/modules/mod_auth_xradius.so: undefined symbol: xrad_put_string

By looking at the source code, I see that libradius bundled with mod_auth_xradius is different than the one used here. All the functions have "xrad_" as a prefix while in this one they have all "rad_".

Here is a diff file between the two implementations (radius code and man pages only; so no READMEs or md5 stuff):

http://slaanesh.fedorapeople.org/libradius-vs-mod_auth_xradius.diff

Regards,
--Simone

Comment 12 Stephen Gallagher 2012-05-30 11:47:04 UTC
Simone, assuming that you have tested my libnss patch and it's working with consumers other than mod_auth_xradius, I'm willing to mark this package as approved.

The issues with mod_auth_xradius are in that package, and have nothing to do with packaging libradius itself, so those should be resolved elsewhere.

Can you confirm that you can successfully communicate with a RADIUS server with the current package? As I've said, I have no infrastructure available to test this myself.

Comment 13 Simone Caronni 2012-05-30 13:04:46 UTC
For now I cannot confirm it works as the only consumer I have is mod_auth_xradius which doesn't as I didn't notice all the functions were renamed to xrad_.

Since the spin-off was only to have mod_auth_xradius in Fedora I don't see much benefit into this.

I'm trying now to make them work together; will post an update later today.

Now I have a question: if both libradius and mod_auth_xradius end up being so different from the original sources, wouldn't be better to host the code somewhere else (fork) and simply put all the patches in (github, git.fedorahosted.org)? Last updates are both dated 2004.

Thanks,
--Simone

Comment 14 Stephen Gallagher 2012-05-30 14:35:06 UTC
Forking is always an option. I'd recommend trying to contact the original upstream first to see if you can negotiate pushing such changes there. They may even opt to give you stewardship of the original upstream.

Comment 15 Dmitri Pal 2012-05-30 20:49:30 UTC
The xauth is a fork but it is more enhanced than the original one. I reviewed the code and I would prefer going with the xauth version if it is available.

Comment 16 Dmitri Pal 2012-06-07 22:59:54 UTC
It seems that the mod_auth_xradius package includes the library so I am puzzled a bit why you have a problem of importing a separate library.
See http://www.outoforder.cc/projects/apache/mod_auth_xradius/
Is this the mod_auth_xradius that you are trying to use?

Comment 17 Simone Caronni 2012-06-08 07:14:08 UTC
Please read the bug for mod_auth_xradius, the whole explanation is there; I was not given permission to bundle the xradius library and this was an attempt to un-bundle it.

There's also a Fedora Package Committee ticket filed in.

https://bugzilla.redhat.com/show_bug.cgi?id=820488

Regards,
--Simone

Comment 18 Stephen Gallagher 2012-06-08 11:32:55 UTC
I think we're talking past each other here. Please see my comments in https://bugzilla.redhat.com/show_bug.cgi?id=820488

I think what we need to do is scrap this separate effort and build libxradius as a subpackage of mod_auth_xradius.


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