policykit should support netgroups

[Steve Atwell]

Binary package hint: policykit-1

PolicyKit doesn't appear to have any support for netgroups. In particular, you can't use netgroups to specify which users can act as administrators. We have a setup which uses per-host netgroups to provide central control over who has administrative privileges to each host. Things like pam_access and sudoers have netgroup support already, so it seems fitting that PolicyKit would support them as well.

Description: Ubuntu 10.04 LTS
Release: 10.04

policykit-1:
  Installed: 0.96-2
  Candidate: 0.96-2

[Launchpad Janitor (janitor)]

Status changed to 'Confirmed' because the bug affects multiple users.

[Nikki VonHollen (vonhollen)]

Same as:
https://bugs.launchpad.net/ubuntu/+source/policykit-1/+bug/724052

Netgroup support in Policy Kit would work similarly to unix groups. Where one might use the identity "unix-group:foo", the netgroup version would allow "netgroup:foo".

Netgroup entries only acknowledge the username portion of the netgroup (hostname, username, domainname) triple.

An example /etc/netgroup file would look like:
somegroup (-,john,) (-,jane,) othergroup
othergroup (-,mary,) (-,bill,)

Which puts "unix-user:mary" in netgroups "somegroup" and "othergroup".

I'll be submitting a patch in the next week or two to add this functionality. The only problem is I'll have to re-factor local authority and identity code to remove the need for getgrouplist which doesn't exist for netgroups (see get_groups_for_user in polkitbackendlocalauthority.c).

The new tests I'm adding for bug #43608 cover all the code I'm re-factoring.

[Nikki VonHollen (vonhollen)]

Created attachment 54448
Basic unittest support and a few tests

Adds basic unit tests for:
PolkitIdentity, PolkitUnixUser, PolkitUnixGroup, PolkitBackendLocalAuthorizationStore, and PolkitBackendLocalAuthority.

[Nikki VonHollen (vonhollen)]

Created attachment 54450
Added netgroup support and expanded unit tests with MockLibc

Please ignore the previous comment/attachment. I got the two bug numbers mixed up.

This patch relies on 'policykit-unittest.patch' from bug #43608.

There are a few issues I ran into implementing netgroups:
1. Netgroups can't support glob syntax like other identities do in *.pkla
files.
2. A netgroup with a wildcard entry like (,,), which means any
user/host/domain, will be ignored in the admin identities list.
3. I use git submodules to include the MockLibc source code, but I'm not sure
if that's what we should stick with. I can include code

Since I'm using submodules, the first commit with the submodule needs to run
'git submodule add' to put special metadata in the repo which can't happen via
patch AFAIK. For eaxmple:
cd PolicyKit
patch -p1 < ~/policykit-unittest.patch
patch -p1 < ~/policykit-netgroup.patch
git submodule add -b v1 https://code.google.com/p/mocklibc/ noinst/mocklibc

[Zeuthen (zeuthen)]

Comment on attachment 54450
Added netgroup support and expanded unit tests with MockLibc

Review of attachment 54450:
-----------------------------------------------------------------

Generally the patch looks very good, good job! High-level comments

 - I'm not sure I like git submodules (but TBH I don't have any good reason
   except "it's different!" ... but I think that's reason enough).

   So at this point I would prefer just adding the C and H files from mocklibc
   into tests/mocklibc/*.[ch] and instead of a full buildsystem (with configure.ac
   and all that entails) just having tests/mocklibc/Makefile.am.

   We can always sync with upstream mocklibc if there are bug-fixes or new
   features needed. I just think it's a lot easier to do it this way.

 - Please use 'git format-patch' to format the patch. That way I don't have
   to a) use 'git add' on all the new files; and b) write the commit message.
   (As other developers, I too am lazy!)

 - I think we should probably call it PolkitUnixNetgroup (and
   polkit_unix_netgroup and unix-netgroup:name) instead of
   PolkitNetGroup (and polkit_net_group and netgroup:name).
   Yeah, I agree it's a bit superfluous with the Unix prefix but we
   already do this for users and groups

 - The coding standards generally use "p != NULL" and "p == NULL" to make
   it explicit that p is a pointer and not a boolean. It's not a must to
   follow these conventions in new code but I would appreciate if we did
   in existing code (consistency matters etc.)

 - Please avoid using C++-style comments .. the main reason for this is
   that I use "//" only when temporarily commenting out code. This makes
   it easy to search for it. Yeah, it's a weird reason but bear with me :-)

::: test/polkit/polkitidentitytest.c
@@ +28,5 @@
> +struct comparison_test_data_type {
> + const gchar *subject_a;
> + const gchar *subject_b;
> + gboolean equal;
> +};

Not a biggie but the code generally uses CamelCase and typedef ... and _type is a bit unneeded... so this would simply be "type struct { } ComparisonTestData;"

[Zeuthen (zeuthen)]

Btw, the patch is also missing modifications to

 docs/polkit/polkit-1-sections.txt
 docs/polkit/polkit-1.types
 docs/polkit/polkit-1-docs.xml

to make the newly added type and functions appear in the docs.

[Nikki VonHollen (vonhollen)]

Created attachment 54617
Added netgroup support and expanded unit tests with MockLibc

I fixed all the style/naming problems you mentioned, and I updated the docs for the API and pklocalauthority.

I also included the entire source of MockLibc, but polkit's ./configure still calls the ./configure for MockLibc. I'll submit another patch without the recursive configure if you want, but that may make updating MockLibc a pain. I'll try to work more on that tomorrow.

This set of patches relies on unitttest support but not the previous netgroup patch I posted, so it should be applied against the current public trunk. I used 'git format-patch --stdout <RANGE>' so the patch is in mbox format.

Thanks!

[Zeuthen (zeuthen)]

(In reply to comment #5)
> I'll try to work more on that tomorrow.

Sounds great, thanks! Btw, release-wise I'm shooting for a release on Friday (even though it's a bad idea to release software just before an weekend or, worse, long holiday).

[Nikki VonHollen (vonhollen)]

Created attachment 54662
Updated MockLibc to fix srcdir != builddir bug

I fixed the bug in MockLibc where builds fail if srcdir != builddir. It should work fine for PolicyKit, but the current PolicyKit head fails to build in that situation with the error "ERROR: polkit.h: no such a file or directory" in src/polkit. I'm looking into that issue now.

[Zeuthen (zeuthen)]

(In reply to comment #7)
> Created attachment 54662 [details] [review]
> Updated MockLibc to fix srcdir != builddir bug
>
> I fixed the bug in MockLibc where builds fail if srcdir != builddir. It should
> work fine for PolicyKit, but the current PolicyKit head fails to build in that
> situation with the error "ERROR: polkit.h: no such a file or directory" in
> src/polkit. I'm looking into that issue now.

OK, but please don't spend too much time on buildsystems! Also, as per comment 3 the mocklib code should go into test/mocklibc (or test/tools/mocklibc if you want), not noinst/mocklibc. Thanks.

[1] : as a general sentiment, I think that people who are capable of writing code should spend time on doing just that - not fighting arcane build systems from the 1980 era!

[Nikki VonHollen (vonhollen)]

Created attachment 54715
Netgroup support and additional testing with MockLibc

David: This patch set is everything you need to apply to mainline with the previous changes and the noinst/mocklibc -> test/mocklibc move you requested.

[Nikki VonHollen (vonhollen)]

Created attachment 54727
Netgroup support and additional testing with MockLibc

Please ignore the last patch which didn't have all the changes.

This is a single patch (git rebase) with netgroup/testing support and also fixes another srcdir != builddir problem with my tests.

[Zeuthen (zeuthen)]

(In reply to comment #10)
> Created attachment 54727 [details] [review]
> Netgroup support and additional testing with MockLibc
>
> Please ignore the last patch which didn't have all the changes.
>
> This is a single patch (git rebase) with netgroup/testing support and also
> fixes another srcdir != builddir problem with my tests.

Looks good, I just pushed the patch with a s-o-b

 http://cgit.freedesktop.org/PolicyKit/commit/?id=674357c20c1b6cb421fea6eb6924b274ec477c0e

Btw, I promised to make a release today but as I'm not working today (just found out this morning that it's a company holiday!) it will have to wait to next week or the week after (Red Hat is shut down over the holidays but I might find a little bit of time to do the release). Thanks for your patch!

[Launchpad Janitor (janitor)]

This bug was fixed in the package policykit-1 - 0.104-1

---------------
policykit-1 (0.104-1) unstable; urgency=low

  * New upstream release.
    - Add support for netgroups. (LP: #724052)
  * debian/rules: Disable systemd support, continue to work with ConsokeKit.
  * 05_revert-admin-identities-unix-group-wheel.patch: Refresh to apply
    cleanly.
  * debian/libpolkit-gobject-1-0.symbols: Add new symbols from this new
    release.
  * debian/rules: Do not let test failures fail the build. The new test suite
    also runs a test against the system D-BUS/ConsoleKit, which can't work on
    buildds.
 -- Martin Pitt <email address hidden> Fri, 06 Jan 2012 12:28:54 +0100