Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1058812 - Review Request: mod_lookup_identity - Apache module to retrieve additional information about the authenticated user.
Summary: Review Request: mod_lookup_identity - Apache module to retrieve additional in...
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Jakub Hrozek
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-01-28 14:49 UTC by Jan Pazdziora
Modified: 2014-02-22 00:34 UTC (History)
5 users (show)

Fixed In Version: mod_lookup_identity-0.8.3-1.fc20
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2014-02-22 00:34:19 UTC
jhrozek: fedora-review+
gwync: fedora-cvs+


Attachments (Terms of Use)

Description Jan Pazdziora 2014-01-28 14:49:21 UTC
Spec URL: http://fedorapeople.org/cgit/adelton/public_git/mod_lookup_identity.git/plain/mod_lookup_identity.spec
SRPM URL: http://copr-be.cloud.fedoraproject.org/results/adelton/identity_demo/fedora-19-x86_64/mod_lookup_identity-0.8.1-2.fc19/mod_lookup_identity-0.8.1-2.fc19.src.rpm
Description: mod_lookup_identity can retrieve GECOS and group information about
user authenticated in Apache httpd server, as well as attributes
about the user provided by local sssd, and store these values
in notes/environment variables to be consumed by web applications.
Use of REMOTE_USER_* environment variables is recommended.
Fedora Account System Username: adelton

Comment 1 Jakub Hrozek 2014-01-29 21:59:32 UTC
As agreed on IRC, I'm stealing this package review from Nathaniel.

Comment 2 Jakub Hrozek 2014-01-29 22:46:19 UTC
The specfile mostly looks good and both build and the binary works as advertised. The package builds cleanly in Koji:
http://koji.fedoraproject.org/koji/taskinfo?taskID=6470246

I have some comments and questions, though:

* You shouldn't Require httpd itself, but rather httpd-mmn. "mmn" stands for "module magic number" and will ensure that you only Require the version of httpd that has compatible module API. httpd-mmn is provided by httpd, it's just versioned, so the dependency tree stays the same. You can use the following one-liner:
%{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat %{_includedir}/httpd/.mmn || echo 0-0)}}
And then:
Requires:       httpd-mmn = %{_httpd_mmn}

* Why do you filter the auto-provides? I think the comment should also explain "why", not only "what".

* I think the description could be improved, too. Currently it says:
"""
mod_lookup_identity can retrieve GECOS and group information about user authenticated in Apache httpd server.
"""
I don't think the descrption should pitch GECOS in particular, but rather only talk about generic attributes, as it does later.

* Most importantly -- you don't use the standard Fedora flags during %build, doesn't that also omit flags that improve security? I think an Apache module should be compiled in total paranoia mode.. For a local build, the following worked for me:
%{_httpd_apxs} -c -Wc,"%{optflags} -Wall -pedantic $(pkg-config --cflags dbus-1)" $(pkg-config --libs dbus-1) mod_lookup_identity.c

(I also moved the source file name to the end to separate the flags from the source on the build command line.)

* When the sssd-ifp package is in Fedora, the Requires should change from dbus-libs to sssd-ifp. It's OK to keep the module as-is for now, but it's something we should keep in mind for the future.

Comment 3 Jan Pazdziora 2014-01-30 08:21:42 UTC
(In reply to Jakub Hrozek from comment #2)
> 
> * You shouldn't Require httpd itself, but rather httpd-mmn. "mmn" stands for
> "module magic number" and will ensure that you only Require the version of
> httpd that has compatible module API. httpd-mmn is provided by httpd, it's
> just versioned, so the dependency tree stays the same. You can use the
> following one-liner:
> %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat
> %{_includedir}/httpd/.mmn || echo 0-0)}}
> And then:
> Requires:       httpd-mmn = %{_httpd_mmn}

Per https://fedoraproject.org/wiki/PackagingDrafts/ApacheHTTPModules I've attempted to define the macro as

%{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat %{_includedir}/httpd/.mmn || echo missing-httpd-devel)}}

My problem however is that I don't seem to be able to get httpd-devel installed on Fedora 21 in koji:

http://kojipkgs.fedoraproject.org//work/tasks/930/6470930/root.log

The net effect is that in build.log, there is then

cat: /usr/include/httpd/.mmn: No such file or directory
error: line 20: Double separator '-' in: missing-httpd-devel: Requires: httpd-mmn = missing-httpd-devel
Building target platforms: x86_64

I could go with 0-0 as you propose but I wonder why the behaviour is different on Fedora 21. Also, having the missing-httpd-devel acts as nice safeguard against broken setup ... but I am not sure how to fix the setup.

Note that on Fedora 20 it works fine:

http://kojipkgs.fedoraproject.org//work/tasks/926/6470926/root.log

Comment 4 Jan Kaluža 2014-01-30 10:55:46 UTC
(In reply to Jan Pazdziora from comment #3)
> cat: /usr/include/httpd/.mmn: No such file or directory
> error: line 20: Double separator '-' in: missing-httpd-devel: Requires:
> httpd-mmn = missing-httpd-devel
> Building target platforms: x86_64

This is caused by change in rpmbuild and we have fixed this problem in all packages depending on httpd in rawhide last week. I've just updated the documentation you've been using.

Comment 5 Jakub Hrozek 2014-01-30 10:56:29 UTC
(In reply to Jan Pazdziora from comment #3)
> (In reply to Jakub Hrozek from comment #2)
> > 
> > * You shouldn't Require httpd itself, but rather httpd-mmn. "mmn" stands for
> > "module magic number" and will ensure that you only Require the version of
> > httpd that has compatible module API. httpd-mmn is provided by httpd, it's
> > just versioned, so the dependency tree stays the same. You can use the
> > following one-liner:
> > %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat
> > %{_includedir}/httpd/.mmn || echo 0-0)}}
> > And then:
> > Requires:       httpd-mmn = %{_httpd_mmn}
> 
> Per https://fedoraproject.org/wiki/PackagingDrafts/ApacheHTTPModules I've
> attempted to define the macro as
> 

I pinged one of the httpd developers. This draft was just outdated because of changes they did in rawhide recently. They've fixed the draft to recommend 0-0 now.

> %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat
> %{_includedir}/httpd/.mmn || echo missing-httpd-devel)}}
> 
> My problem however is that I don't seem to be able to get httpd-devel
> installed on Fedora 21 in koji:
> 
> http://kojipkgs.fedoraproject.org//work/tasks/930/6470930/root.log
> 

I'm not sure what to look for in that log, I don't even see httpd-devel requested.

Comment 6 Jan Kaluža 2014-01-30 10:59:07 UTC
(In reply to Jakub Hrozek from comment #5)
> (In reply to Jan Pazdziora from comment #3)
> > %{!?_httpd_mmn: %{expand: %%global _httpd_mmn %%(cat
> > %{_includedir}/httpd/.mmn || echo missing-httpd-devel)}}
> > 
> > My problem however is that I don't seem to be able to get httpd-devel
> > installed on Fedora 21 in koji:
> > 
> > http://kojipkgs.fedoraproject.org//work/tasks/930/6470930/root.log
> > 
> 
> I'm not sure what to look for in that log, I don't even see httpd-devel
> requested.

This gets fixed automatically once you will use 0-0 instead of missing-httpd-devel.

Comment 8 Jakub Hrozek 2014-01-31 23:30:34 UTC
Thanks for the changes. The package now looks good and builds fine.

There is one more change I'd like to see before I approve the package. The spec file explicitly requires dbus-libs, but that's not necessary, RPM does the right thing on its own:

$ rpm -qp --requires /home/remote/jhrozek/rpmbuild/RPMS/x86_64/mod_lookup_identity-0.8.2-1.fc20.x86_64.rpm | grep dbus
dbus-libs
libdbus-1.so.3()(64bit)

(the former is explicit, the latter autogenerated)

actually even rpmdiff warns about the explicit requires:
$ rpmlint /home/remote/jhrozek/rpmbuild/RPMS/x86_64/mod_lookup_identity-0.8.2-1.fc20.x86_64.rpm
mod_lookup_identity.x86_64: E: explicit-lib-dependency dbus-libs
mod_lookup_identity.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up

The second warning is a false positive.

Comment 10 Jakub Hrozek 2014-02-03 22:07:11 UTC
The spec file looks good to me, file ownership is sane, license matches the upstream, the package passed my sanity test.

The tarball matches:
 * rpm: 16c9377e839a959172a2dfe7d32de5bf
 * upstream [*]: 16c9377e839a959172a2dfe7d32de5bf

The package builds fine in Mock:
 * http://koji.fedoraproject.org/koji/taskinfo?taskID=6487502

Full rpmlint output:
mod_lookup_identity.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up
mod_lookup_identity-debuginfo.x86_64: W: spelling-error Summary(en_US) lookup -> lockup, hookup, look up
mod_lookup_identity-debuginfo.x86_64: W: spelling-error %description -l en_US lookup -> lockup, hookup, look up

This package is APPROVED.

[*] You should fix two bugs on your website
 - there is no link to 0.8.3 on http://www.adelton.com/apache/mod_lookup_identity/
 - following the link to "mod_lookup_identity 0.8.2" takes me to "http://www.adelton.com/apache/mod_authnz_pam/"

Comment 11 Jan Pazdziora 2014-02-10 14:11:54 UTC
(In reply to Jakub Hrozek from comment #10)
> 
> This package is APPROVED.

Thanks.

> [*] You should fix two bugs on your website
>  - there is no link to 0.8.3 on
> http://www.adelton.com/apache/mod_lookup_identity/
>  - following the link to "mod_lookup_identity 0.8.2" takes me to
> "http://www.adelton.com/apache/mod_authnz_pam/"

Both fixed.

New Package SCM Request
=======================
Package Name: mod_lookup_identity
Short Description: Apache module to retrieve additional information about the authenticated user
Owners: adelton
Branches: f20
InitialCC:

Comment 12 Gwyn Ciesla 2014-02-10 14:13:30 UTC
Git done (by process-git-requests).

Comment 13 Fedora Update System 2014-02-10 14:40:35 UTC
mod_lookup_identity-0.8.3-1.fc20 has been submitted as an update for Fedora 20.
https://admin.fedoraproject.org/updates/mod_lookup_identity-0.8.3-1.fc20

Comment 14 Fedora Update System 2014-02-11 23:04:06 UTC
mod_lookup_identity-0.8.3-1.fc20 has been pushed to the Fedora 20 testing repository.

Comment 15 Fedora Update System 2014-02-22 00:34:19 UTC
mod_lookup_identity-0.8.3-1.fc20 has been pushed to the Fedora 20 stable repository.


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