Bug 506355 - Review Request: munge - Uid 'N' Gid Emporium
Summary: Review Request: munge - Uid 'N' Gid Emporium
Status: CLOSED NEXTRELEASE
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
low
medium
Target Milestone: ---
Assignee: Mamoru TASAKA
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Keywords:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2009-06-16 20:31 UTC by Steve Traylen
Modified: 2009-10-21 16:19 UTC (History)
5 users (show)

(edit)
Clone Of:
(edit)
Last Closed: 2009-07-30 09:19:25 UTC
mtasaka: fedora-review+
tibbs: fedora-cvs+


Attachments (Terms of Use)

Description Steve Traylen 2009-06-16 20:31:35 UTC
Spec URL: http://cern.ch/straylen/munge-rpms/munge.spec
SRPM URL: http://cern.ch/straylen/munge-rpms/munge-0.5.8-1.fc11.src.rpm

Description: 
MUNGE (MUNGE Uid 'N' Gid Emporium) is an authentication service for creating
and validating credentials. It is designed to be highly scalable for use
in an HPC cluster environment.
It allows a process to authenticate the UID and GID of another local or
remote process within a group of hosts having common users and groups.
These hosts form a security realm that is defined by a shared cryptographic
key. Clients within this security realm can create and validate credentials
without the use of root privileges, reserved ports, or platform-specific
methods.


There is koji build of munge here.
http://koji.fedoraproject.org/koji/taskinfo?taskID=1418953

The remaining rpmlint errors I believe are all justified.
Many of the directories are to be exclusivly read by the
munge user.

$ rpmlint munge-*
munge.x86_64: W: non-standard-uid /var/lib/munge munge
munge.x86_64: W: non-standard-gid /var/lib/munge munge
munge.x86_64: E: non-standard-dir-perm /var/lib/munge 0711
munge.x86_64: W: non-standard-uid /var/log/munge munge
munge.x86_64: W: non-standard-gid /var/log/munge munge
munge.x86_64: E: non-standard-dir-perm /var/log/munge 0700
munge.x86_64: W: non-standard-uid /etc/munge munge
munge.x86_64: W: non-standard-gid /etc/munge munge
munge.x86_64: E: non-standard-dir-perm /etc/munge 0700
munge.x86_64: W: non-standard-uid /var/run/munge munge
munge.x86_64: W: non-standard-gid /var/run/munge munge
munge.x86_64: W: incoherent-subsys /etc/rc.d/init.d/munge $RH_SUBSYS_BASE
munge-devel.x86_64: W: no-documentation

I require a package sponsor.

Comment 1 Mamoru TASAKA 2009-07-10 17:42:35 UTC
Some notes:

* src.rpm itself
  - All files in src.rpm should have 0644 permission.

* License
  - As far as I checked the whole source code, the license
    tag should be "GPLv2+".

* %description
  - You don't have to repeat the same explanation on -devel
    subpackage which already appears in the %description of
    main package.

* Fedora specific compilation flags
  https://fedoraproject.org/wiki/Packaging/Guidelines#Compiler_flags
  - Fedora specific compilation flags are not correctly honored:
--------------------------------------------------------
   217  make[2]: Entering directory `/builddir/build/BUILD/munge-0.5.8/src/libcommon'
   218  if /bin/sh ../../libtool --tag=CC --mode=compile gcc -DHAVE_CONFIG_H -I. -I. -I../../config  -I../../src/libmunge   -D_GNU_SOURCE -MT fd.lo -MD -MP -MF ".deps/fd.Tpo" -c -o fd.lo fd.c; \
--------------------------------------------------------
    ( By the way, please check what %configure actually does
      by $ rpm --eval %configure )

* Timestamps
  https://fedoraproject.org/wiki/Packaging/Guidelines#Timestamps
  - When using "install" or "cp" commands, add "-p" option to keep
    timestamps on installed files.

* Scriptlets
  - Calling /sbin/ldconfig on -devel %post(un) scriptlets is not needed.
  - Use %_var or %_localstatedir instead of directly using "/var"
  - This is not always needed, however would you check if
    "condrestart" is not needed at %postun?
    https://fedoraproject.org/wiki/Packaging/SysVInitScript#InitscriptScriptlets

* %files
  - "INSTALL" file is usually for people who want to build and install
    software by themselves and not needed for rpm users.

  - From the contents of create-munge-key, it seems that
    %files should have %ghost %_sysconfdir/%name/%name.key entry.

  - Usually man pages of entry 3 is for the explanation of APIs of
    functions in libraries or so and should be in -devel subpackage.

  - %defattr must be set on -devel subpackage (please use rpmlint)

  - Currently %{_var}/lib/munge has 0711 permission, which is not
    usual, however explicit %attr is not set for this directory.
    If this permission is correct, set %attr on this directory
    explicitly. Otherwise fix the permission on this directory
    at %install.
    The same issue also applies to %{_sysconfdir}/munge, %{_var}/log/munge

* -devel subpackage
  - munge.h does not work as:
--------------------------------------------------------------------
+++++ TEMP.c ++++++++
#include <munge.h>

int foo (munge_enum_t type){
	return munge_enum_is_valid(type, 0);
}
+++++++++++++++++++++++

$ env LANG=C gcc -c TEMP.c 
In file included from TEMP.c:1:
/usr/include/munge.h:39:4: error: #error By linking against libmunge, the derivative
/usr/include/munge.h:40:4: error: #error work becomes licensed under the terms of the
/usr/include/munge.h:41:4: error: #error GNU General Public License. Acknowledge by
/usr/include/munge.h:42:4: error: #error defining the GPL_LICENSED preprocessor macro.
--------------------------------------------------------------------

* service
--------------------------------------------------------------------
[root@localhost ~]# env LANG=C service munge status
[root@localhost ~]# (returns nothing)
--------------------------------------------------------------------

  - Usually this should be something like:
--------------------------------------------------------------------
[root@localhost ~]# service xttpd status
xttpd is stopped
--------------------------------------------------------------------

Comment 2 Bill Nottingham 2009-07-10 19:30:17 UTC
(In reply to comment #1)

> $ env LANG=C gcc -c TEMP.c 
> In file included from TEMP.c:1:
> /usr/include/munge.h:39:4: error: #error By linking against libmunge, the
> derivative
> /usr/include/munge.h:40:4: error: #error work becomes licensed under the terms
> of the
> /usr/include/munge.h:41:4: error: #error GNU General Public License.
> Acknowledge by
> /usr/include/munge.h:42:4: error: #error defining the GPL_LICENSED preprocessor
> macro.

Hey, can we patch this out?

1) It's terribly passive-aggressive, and I think our code shouldn't do that
2) We don't do this in any other libraries; we assume users can figure out licensing on their own
3) It's not even correct.
a) the licensing only covers distribution. If you don't distribute, it doesn't really matter
b) is that even a legally enforceable 'agreement'?

Comment 3 Chris Dunlap 2009-07-10 19:54:46 UTC
Could you change the munge.spec summary field to "MUNGE Uid 'N' Gid Emporium"?  MUNGE is a recursive acronym.

Please drop the dont-exit-form-lib.patch.  All it does is comment out the exit() call in the _log_die routine that it supposed to log a message and then die.  The code expects execution to stop once a fatal error message is logged.  Continuing after that will cause unexpected and likely undesired behavior.

As for GPL_LICENSED, it's fine with me if you patch it out.

Comment 4 Chris Dunlap 2009-07-10 20:07:39 UTC
You don't need the libgcrypt-devel BuildRequires.  MUNGE requires either OpenSSL or libgcrypt -- not both.  You can force the selection with configure's --with-crypto-lib option, but it will use OpenSSL if it finds it.

Comment 5 Mamoru TASAKA 2009-07-22 19:06:40 UTC
ping?

Comment 6 Steve Traylen 2009-07-22 23:08:04 UTC
First Mamaro and Chris thanks for you comments.

The majority are addressed:

- Correct License to GPLv2+
- Move man5 pages to the devel package.
- Remove +x bit from create-munge-key source.
- Preserve timestamps when installing files.
- ldconfig not needed on -devel package.
- Do a condrestart when upgrading.
- Remove redundant files from docs.
- chmod /var/lib/munge /var/log/munge and /etc/munge to 700.
- Apply patch to not error when GPL_LICENSED is not set.
- Patch service script to print error on if munge.key not present
  on start only and with a better error.
- Remove dont-exit-form-lib.patch. munge is expecting libmunge to
  do this.
- Remove libgcrypt-devel from BuildRequires, uses openssl by
  default anyway.
- Mark the munge.key as a ghost file.

Remaining items

1) I need to check what is going on the compilation flags. I'll get
   to this shortly.

2) Reading the Summary: specification again it is meant to be informative 
   so neither 
   Uid 'N' Gid Emporium
   nor 
   MUNGE is a recursive acronym

  is very good.
 
  For now I've set it to 
  "Enables uid & gid authentication across a host cluster"
  but that is not particularly descriptive either? Will have a think.

Spec and .srm.rpm:
http://cern.ch/steve.traylen/munge-rpms/munge-0.5.8-2.fc11.src.rpm
http://cern.ch/steve.traylen/munge-rpms/munge.spec

$ rpmlint SPECS/munge.spec SRPMS/munge-0.5.8-2.fc11.src.rpm \
        RPMS/x86_64/munge-0.5.8-2.fc11.x86_64.rpm \
        RPMS/x86_64/munge-devel-0.5.8-2.fc11.x86_64.rpm \
        RPMS/x86_64/munge-debuginfo-0.5.8-2.fc11.x86_64.rpm  \
munge.x86_64: W: shared-lib-calls-exit /usr/lib64/libmunge.so.2.0.0
exit@GLIBC_2.2.5
munge.x86_64: W: non-standard-uid /var/lib/munge munge
munge.x86_64: W: non-standard-gid /var/lib/munge munge
munge.x86_64: E: non-standard-dir-perm /var/lib/munge 0700
munge.x86_64: W: non-standard-uid /etc/munge/munge.key munge
munge.x86_64: W: non-standard-gid /etc/munge/munge.key munge
munge.x86_64: E: non-readable /etc/munge/munge.key 0400
munge.x86_64: W: non-standard-uid /etc/munge munge
munge.x86_64: W: non-standard-gid /etc/munge munge
munge.x86_64: E: non-standard-dir-perm /etc/munge 0700
munge.x86_64: W: non-standard-uid /var/log/munge munge
munge.x86_64: W: non-standard-gid /var/log/munge munge
munge.x86_64: E: non-standard-dir-perm /var/log/munge 0700
munge.x86_64: W: non-standard-uid /var/run/munge munge
munge.x86_64: W: non-standard-gid /var/run/munge munge
munge.x86_64: W: incoherent-subsys /etc/rc.d/init.d/munge $RH_SUBSYS_BASE
munge-debuginfo.x86_64: E: debuginfo-without-sources
4 packages and 1 specfiles checked; 5 errors, 12 warnings.

They can all be explained I feel as mostly private directories
and files for the munged owned process. The last one of course should be 
fixed up with the compilation corrections.

Builds for fc11, fc12, el4 and el5:
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493445
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493450
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493455
https://koji.fedoraproject.org/koji/taskinfo?taskID=1493460

Steve.

Comment 7 Steve Traylen 2009-07-23 12:04:19 UTC
Updated packages now with correct compiler flags. From the comments
so far I think this is everything so far.

CFLAGS=%{optflags} -DGNU_SOURCE 

is added NOT for the case of .el4 and .el5 where it compiles with
out -DGNU_SOURCE anyway.

http://cern.ch/steve.traylen/munge-rpms/munge-0.5.8-3.fc11.src.rpm
http://cern.ch/steve.traylen/munge-rpms/munge.spec

http://koji.fedoraproject.org/koji/taskinfo?taskID=1494401
http://koji.fedoraproject.org/koji/taskinfo?taskID=1494406
http://koji.fedoraproject.org/koji/taskinfo?taskID=1494413
http://koji.fedoraproject.org/koji/taskinfo?taskID=1494417


$ rpmlint SPECS/munge.spec SRPMS/munge-0.5.8-3.fc11.src.rpm \
    RPMS/x86_64/munge-0.5.8-3.fc11.x86_64.rpm \
    RPMS/x86_64/munge-debuginfo-0.5.8-3.fc11.x86_64.rpm \
    RPMS/x86_64/munge-devel-0.5.8-3.fc11.x86_64.rpm 

munge.x86_64: W: shared-lib-calls-exit /usr/lib64/libmunge.so.2.0.0 exit@GLIBC_2.2.5
munge.x86_64: W: non-standard-uid /var/lib/munge munge
munge.x86_64: W: non-standard-gid /var/lib/munge munge
munge.x86_64: E: non-standard-dir-perm /var/lib/munge 0700
munge.x86_64: W: non-standard-uid /etc/munge/munge.key munge
munge.x86_64: W: non-standard-gid /etc/munge/munge.key munge
munge.x86_64: E: non-readable /etc/munge/munge.key 0400
munge.x86_64: W: non-standard-uid /etc/munge munge
munge.x86_64: W: non-standard-gid /etc/munge munge
munge.x86_64: E: non-standard-dir-perm /etc/munge 0700
munge.x86_64: W: non-standard-uid /var/log/munge munge
munge.x86_64: W: non-standard-gid /var/log/munge munge
munge.x86_64: E: non-standard-dir-perm /var/log/munge 0700
munge.x86_64: W: non-standard-uid /var/run/munge munge
munge.x86_64: W: non-standard-gid /var/run/munge munge
munge.x86_64: W: incoherent-subsys /etc/rc.d/init.d/munge $RH_SUBSYS_BASE
4 packages and 1 specfiles checked; 4 errors, 12 warnings.

As mentioned previously munge is expecting libmunge to exist in this case.
The permissions warnings and errors are private directories and files
that the munge user owns.

The incoherent-subsys warning is false  because there is a variable in
that line.

Steve

Comment 8 Mamoru TASAKA 2009-07-23 15:49:19 UTC
Well, for -3:

* Misc issue
  - We now recommend to use %defattr(-,root,root,-)

  - If the permission of files/directories on %files list
    are not 0644 (for file) or 0755 (for directory), 
    I recomment to write explicit permissions as %attr in %files 
    like
----------------------------------------------------
%attr(0700,munge,munge)  %dir %{_var}/run/munge
----------------------------------------------------
    ( even if you change the permissions of these files/directories
       at %install )

  - At %changelog:
----------------------------------------------------
- Move man5 pages to the devel package.
----------------------------------------------------
    Perhaps "man3" pages

Comment 9 Steve Traylen 2009-07-23 16:49:04 UTC
Thanks Mamoru again:

http://cern.ch/steve.traylen/munge-rpms/munge-0.5.8-4.fc11.src.rpm
http://cern.ch/steve.traylen/munge-rpms/munge.spec

has the 3 changes from Comment #8.

Steve

Comment 10 Mamoru TASAKA 2009-07-23 17:12:57 UTC
Well, okay:

----------------------------------------------------------
  This package (munge) is APPROVED by mtasaka
----------------------------------------------------------

It seems that Dennis is going to sponsor you. When you really
get sponsored, would you notice it on this bugzilla?

Comment 11 Steve Traylen 2009-07-23 17:20:51 UTC
Yes I'll notice for sure.
Thanks again.

Comment 12 Mamoru TASAKA 2009-07-29 18:16:45 UTC
Now that it seems that you are sponsored, please write
CVS request.

Comment 13 Steve Traylen 2009-07-29 20:27:46 UTC
New Package CVS Request
=======================
Package Name: munge
Short Description: Enables uid & gid authentication across a host cluster
Owners: stevetraylen
Branches: F-10 F-11 EL-4 EL-5
InitialCC:

Comment 14 Jason Tibbitts 2009-07-29 22:16:58 UTC
CVS done.

Comment 15 Fedora Update System 2009-07-30 08:49:58 UTC
munge-0.5.8-4.fc11 has been submitted as an update for Fedora 11.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.fc11

Comment 16 Fedora Update System 2009-07-30 08:58:16 UTC
munge-0.5.8-4.fc10 has been submitted as an update for Fedora 10.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.fc10

Comment 17 Fedora Update System 2009-07-30 09:07:48 UTC
munge-0.5.8-4.el5 has been submitted as an update for Fedora EPEL 5.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.el5

Comment 18 Fedora Update System 2009-07-30 09:17:06 UTC
munge-0.5.8-4.el4 has been submitted as an update for Fedora EPEL 4.
http://admin.fedoraproject.org/updates/munge-0.5.8-4.el4

Comment 19 Steve Traylen 2009-07-30 09:19:25 UTC
Submitted now for el4,5 fc10,11.

As I understand this will get to rawhide by itself from the devel build.
Steve.

Comment 20 Fedora Update System 2009-08-03 19:22:03 UTC
munge-0.5.8-4.fc11 has been pushed to the Fedora 11 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 21 Fedora Update System 2009-08-03 19:23:22 UTC
munge-0.5.8-4.fc10 has been pushed to the Fedora 10 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 22 Fedora Update System 2009-08-05 17:24:10 UTC
munge-0.5.8-4.el4 has been pushed to the Fedora EPEL 4 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 23 Fedora Update System 2009-08-25 16:02:46 UTC
munge-0.5.8-4.el5 has been pushed to the Fedora EPEL 5 stable repository.  If problems still persist, please make note of it in this bug report.

Comment 24 Christopher D. Maestas 2009-10-20 22:53:14 UTC
When I download munge from its homepage and run:

  rpmbuild --rebuild munge-0.5.8-1.src.rpm

I get the following rpms:

munge-0.5.8-1.x86_64.rpm
munge-debuginfo-0.5.8-1.x86_64.rpm
munge-devel-0.5.8-1.x86_64.rpm
munge-libs-0.5.8-1.x86_64.rpm


Where is the munge-libs rpm in epel?  This is needed if one wants to leverage this package in building slurm, which is an advanced resource manager.

Comment 25 Steve Traylen 2009-10-21 07:46:33 UTC
Hi Chris,

The libs are in the munge package.

$ rpm -pql munge-0.5.8-6.fc11.i586.rpm  |grep lib
/usr/lib/libmunge.so.2
/usr/lib/libmunge.so.2.0.0
/var/lib/munge

Concerning slurm I have meant to get around to building it but have not
yet. Last time I looked I got bogged down deciding which options to
enable but it was a while ago now I looked at it so can't remember the
details.

Anyway I can split up munge into munge and munge-libs for sure if it
makes compatibility with the homepage rpms better but would 
like to know why.

It's not obvious to me why you would only need the libs and not 
the rest of munge.

Also since this is released it makes more sense if you can open a new
bug if that is okay.

Thanks for the comments.

Steve

Comment 26 Christopher D. Maestas 2009-10-21 15:05:20 UTC
Steve,

I think Chris Dunlap (cdunlap@llnl.gov), who is on the cc list for this bug (and a point of contact for munge), can elaborate on why it builds that way.  Here's what I see in my x86_64 package.

#  rpm -qpl XCXV4.X-MUNGE/munge-libs-0.5.8-1.x86_64.rpm
/usr/lib64/libmunge.so.2
/usr/lib64/libmunge.so.2.0.0

BTW, it was great to see munge in epel and I hope we can resolve this issue!

-Chris

Comment 27 Steve Traylen 2009-10-21 15:19:58 UTC
Yes I read Chris and assumed the same Chris of course.

Okay I understand there is a difference that the libs 

/usr/lib64/libmunge.so.2
/usr/lib64/libmunge.so.2.0.0

are in epel:munge
rather than upstream:munge-libs.

but when does a problem actually occur with this?

Do you get an error message or something when building slurm?

Steve

Comment 28 Christopher D. Maestas 2009-10-21 15:33:48 UTC
Here's what I see when attempting to build slurm:
---
# rpm -qa | grep munge
munge-0.5.8-6.el5
munge-0.5.8-6.el5
munge-devel-0.5.8-6.el5
munge-devel-0.5.8-6.el5
# rpmbuild -tb slurm-2.0.7.tar.bz2
error: Failed build dependencies:
        munge-libs is needed by slurm-2.0.7-1.x86_64
---

So I think SLURM developers are using how munge builds out of the box.  My fear is that if we start to change scope now with SLURM to build using the epel packages, we start down a hairy path.  It would be nice if we could just match how things build with:
   rpmbuild --rebuild PACKAGENAME

But perhaps there were issues with the munge spec file in the beginning and this is why it was packaged like this?

-cdm

Comment 29 Steve Traylen 2009-10-21 16:01:32 UTC
Okay,

So the "real" solution is that slurm should not be requiring munge-libs
at all and should be requiring munge-devel and in: 

epel case)
    this would also pull in munge.
upstream case)
    this would pull in munge-libs

the result being the same.

But of course I can change the epel ones since the upstream ones are well
established and it is not bad packaging or anything, just different and
common technique. In fact there are other advantages since it enables
both i386 and x96_64 libs to be installed on x86_64 bit as well.

Please could you submit a new bug with this request. Essentially
so I have something to close as the package makes its way through
release. 

Steve

Comment 30 Christopher D. Maestas 2009-10-21 16:19:46 UTC
OK, I have created:

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


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