Bug 538615

*** Bug 538779 has been marked as a duplicate of this bug. ***

I suggest /etc/security/polkit as the best location for these files (modeled after the existing console.apps configuration infrastructure).

1. polkit installs no files below /var/lib/polkit-1/
2. polkit-desktop-policy does install files there, but they are not config files

Re comment #3: okay, fair enough. But:

1. It installs "do-not-edit" files in /etc, which in turn point to the man page, which points to /var/lib as the location for config files.

This is the issue; the intended-for-a-package-system config files should be read from /usr/share/polkit/[...], and the intended-to-be-edited-or-created-manually files should be in /etc/security/polkit/[...].

2. And that's what should go in /usr/share, yeah?

Some of the directories below /var/lib/polkit-1/localauthority.d are machine-specific and thus not appropriate below /usr/share

One model in that case is for the files to be installed in /usr/share and for config files in /etc to point to the ones intended to be active for that machine.

Alternately, I think there's enough real-world precedent for putting rpm-managed config files in /etc, and the localauthority subdirectory scheme well-defined enough, that moving that whole tree to /etc/security/polkit-1/localauthority.d would be an improvement over the current state. (Example: fontconfig, and also how gdm used to be.)

In my mind, that's a much less serious problem than vital system policy configuration (as opposed to system state info) unexpectedly living in /var.

the .pkla files are edited by humans - much like yum .repo files. or pam config files.

If you want to have a hierarchy of them then have the hierarchy be:

/usr/share/polkit/ <-- system-installed defaults
/etc/security/polkit/localauthority.d/{10-vendor.d  20-org.d  30-site.d  50-local.d  90-mandatory.d}

and do away with the /var/lib/polkit entries altogether.

Oh, I should add: the file installed by polkit-desktop-policy clearly *is* a
config file. It's just not a config file that's intended to be user-modified.

Rather than just "# DO NOT EDIT THIS FILE, it will be overwritten on update.",
the comment at the top of that file might be more helpful as:

# DO NOT EDIT THIS FILE; it will be overwritten on update.
#
# Please refer to the pklocalauthority man page for appropriate
# locations for system- and site-specific .pkla files.
#

Looking at what's in the /var/lib/polkit-1 hierarchy, it definitely does not belong in /var/.  Here's the FHS entry for /var/lib:

"""
This hierarchy holds state information pertaining to an application or the system. State information is data that programs modify while they run, and that pertains to one specific host. Users must never need to modify files in /var/lib to configure a package's operation.
"""
  http://www.pathname.com/fhs/pub/fhs-2.3.html#VARLIBVARIABLESTATEINFORMATION

Having users place new files under directories in the /var/ hierarchy in order to configure the service is definitely included in the spirit of "never need to modify files in /var/lib/ to configure a package".  Where to put the files is not so clear but I see two methods.  Either follow gconf's somewhat contentious example and put everything under /etc/ or follow Matthew Miller's suggestion and put the defaults under /usr/share and the user editable files that override the defaults in /etc/.  The latter is what I recommend as Debian has moved the gconf default schema files to /usr/share and Havoc has stated that putting gconf defaults in /etc/ was probably a mistake so it would be better that we don't repeat the same mistake here.

Note, for this comment:
> Some of the directories below /var/lib/polkit-1/localauthority.d are
> machine-specific and thus not appropriate below /usr/share  

There's several options.

1) Only 10-vendor.d goes below /usr/share.  Those are rpm managed files provided by the vendor and intended to be overridden by the local admins in /etc/.  Any policy files that applications or fedora packages should end up in 10-vendor.d
2) Parallel directories in /usr/share and /etc.  Vendor and local admins use a package manager to provision the vendor/org/site defaults in /usr/share/.  Admins of the specific machine make changes in /etc/.

I think the confusion factor is very high in option 2.  Option 1 is the better choice here.

Note 2: It was suggested that we want to have different policies for Fedora Desktop, Fedora Server, Fedora, etc spins.  Do we need two vendor directories?  So that the application package can drop a default policy into one vendor dir and the polkit-desktop-policy package canoverride those if necessary in the second vendor dir?

I have no interest in fighting FHS compliance wars here, thats really a waste of time. 

What is or is not a config file is really pretty much in the eye of the beholder. Something thats data to app A can very well be configuration for app B, and something can be configuration on a distro-level without being meant to be edited locally.
 
David will certainly justify his choices when he comes back from vacation.

> Note 2: It was suggested that we want to have different policies for Fedora
> Desktop, Fedora Server, Fedora, etc spins.  Do we need two vendor directories? 
> So that the application package can drop a default policy into one vendor dir
> and the polkit-desktop-policy package can override those if necessary in the
> second vendor dir?  

To answer this part: No. Application defaults are contained in the action definitions in /usr/share/polkit-1/actions. And the idea is that these will be changed to be very restrictive. And then there will probably be a single policy package installed, depending on the purpose of the installation (a desktop policy or a server policy,...), and local- or site-wide tweaks to that installed policy can be done without conflicts by dropping additional .pkla files in the other directories in /var/lib/polkit-1/localauthority.d.

To prevent multiple policy packages being installed in parallel, we might make them conflict with each other,

> I have no interest in fighting FHS compliance wars here, thats really a waste
of time. 

I don't think there's any need to fight; let's just fix it. The situation is pretty clear cut, with a few details here and there.

it seems to me that pkgs which are also critical path should comply with all the pkging guidelines. FHS compliance is a requirement. This should probably be an F13Blocker.

critical path is a useful concept. Please don't devalue it by using it for silly arguments.

Whether or not it's a configuration file might be in the eye of the beholder. However the fact that I had to edit stuff in a file somewhere in /var/lib after Fedora 12 release to me was a clue that it should not belong there. No other packages have "configuration" or "policy" stuff there that is expected to be tweaked by the site- or machine-administrator (that I know of); at least this is the first package that I found out that needed it.

I do not really care whether the "do not edit" files are also below /etc but as an administrator, things that I'm expected to edit should be below /etc in my opinion, whether or not the FHS agrees.

Seth's suggestion in comment #7 would be perfect.

Made a patch; will post after testing. A couple of quick comments, though:

1) The config file in /etc which says "do not modify" is a different _type_ of file from the .pkla files. Only one key, AdminIdentities, is read from the single Configuration group. (This is where, for example, a group named "desktop_admin_r" can be configured to act like the traditional "wheel" group.) 

But although it shouldn't be modified, it can be overridden with a separate file. That seems okay, but the file comments should explain that for the sake of transparency.

2) Arguably, 10-vendor.d should go into /usr/share, but it seems like having entirely different places to look could lead to more confusion. Perhaps the best approach is to move everything to /etc/security/polkit-1 but make that particular directory a symlink into /usr/share/polkit-1. My patch / updated specfile doesn't actually mess with that, though.

3) Current patch leaves reading from /var/lib/polkit-1; that's part of what I need to test.

So, off to test. Update soon.

Created attachment 373472 [details]
Patch to use /etc/security/polkit-1 instead of /var/lib/polkit-1

Note that *something* has to get restarted for this to take effect, and I'm not sure exactly what. I ended up rebooting and that worked.

matthew, is it the inotify task watching for file changes in the /var/lib/polkit dir that maybe needs to move to watching /etc?

Created attachment 373474 [details]
specfile patch

re comment #18: yeah, possibly. not clear where that task is coming from, since there's no references to inotify in the source. And *having* rebooted, changes to files in the new location take effect instantly.

Annnnd I should also note that I have selinux turned off on my development box, and there's probably selinux policy that needs to be updated here.

Created attachment 373483 [details]
patch against current (as of Nov 13th) git

This is actually even simpler because of the removal of pklalockdown.

diffstat polkit-0.95-git-use_etc_for_pkla.patch
 configure.ac                                    |    2 +-
 docs/man/pklocalauthority.xml                   |    2 +-
 src/polkitbackend/Makefile.am                   |    6 +++---
 src/polkitbackend/polkitbackendlocalauthority.c |    9 +++++++--
 4 files changed, 12 insertions(+), 7 deletions(-)

Note that patch against git isn't actually tested, but I Can't See What Possibly Could Go Wrong™.

Another note: this would be more work, but it seems like putting the locations of the Local Authority directories in the localauthority.conf file would be architecturally the right thing to do, rather than hard-coding the directories (with their apparently "magical" priority numbers).

One step at a time, though -- my suggestion is to use the current change at this point, and then add that as a next phase.

> rather than hard-coding the directories
> (with their apparently "magical" priority numbers).

The one place where we are using a time-honored Unix tradition of relying on lex ordering, and now you are complaining... :-)

Ha. :)

But actually, that's not the complaint. In fact, the opposite -- it's designed to _look_ like a "typical" directory where arbitrary subdirectories are read in lex order, but they're actually not. Specifically-named directories are hard-coded in a list (const gchar *store_locations[]) , and then those are stuffed into the authorization_stores list in order (and then the order is reversed). I don't think they're actually ever sorted, although it may be somewhere in the code I haven't looked.

But you can't actually create polkit-1/localauthority/42-whatever.d/ and have it work.

This is arguably bad, because it looks like a traditional pattern (for better or for worse!), but isn't actually. A reasonable user/sysadmin could easily be fooled. But it's also small potatoes -- and can easily be addressed by the suggestion of putting the list in localauthority.conf. (Or I guess by actually using readdir and doing the lex sort!)

Ever tried to create /etc/rc42.d/ ? Did that do what you want ?

Matthias,
 Init dirs aren't sorted/run like that at all. Those directories don't get compiled into a single use.

However, if you look inside those directories you'll note that the numbers on the files there are sorted like that.

so that if you have:
S23Foo, S26Bar

You can boot S24Quux in there and it will run between Foo and Bar.

The suggestion from the polkit docs is that those dirs are sorted in order. So the expectation is that it's just using readdir.

What Seth said. It's a different pattern. But having said that, can we take that aspect of this offline or put it into another bug? I don't want the basic thing obscured: 1) let's follow the FHS for config files, and 2) hey, here's a patch that does just that!

This was dealt with (and rejected) upstream.

Wait, the upstream (which is you, right?) decided that violating the FHS was acceptable?

Reopening, because if so, it is not acceptable for Fedora, and we need to look into taking this patch for our own use.

(In reply to comment #31)
> Wait, the upstream (which is you, right?)

Yes.

> decided that violating the FHS was
> acceptable?

I've already explained upstream that this is not the case.
 
> Reopening, because if so, it is not acceptable for
> Fedora, and we need to look into taking this patch
> for our own use.  

No. Tom, I'd appreciate if you kept these two things in mind

 - you should know that this Bugzilla is not the right place
   to discuss upstream features. Take your complaints to upstream.

 - I'm closing. And please don't be childish and reopen it again.
   Thanks.

> I've already explained upstream that this is not the case.

I hope I'm not being unfair, but I think a reasonable paraphrase of the arguments you gave me are:

1) these configuration files aren't really configuration files, because they're too security-sensitive for users to mess with.

2) the intended scheme for manipulation of these files is by RPM, and therefore they should be hidden away somewhere so that no other scheme is used (puppet, cfengine, local changes)

and

3) If someone implements and configures a different polkit authority rather than the Local Authority, the local authority config files will be meaningless, and therefore they should be hidden so they don't confuse people.

Is that a fair representation?

Since it's "childish" to reopen this bug report, could we make a more concerted effort to work with "upstream" on this?  I think that putting config files in /var is clearly wrong.  If Matt's characterization is correct ("they're
too security-sensitive for users to mess with", etc.), then I'm really confused.  How are the same people saying that sysadmins should just edit their config files (in last week's highly-publicized discussion on the PackageKit issue last week), but they should never edit those same config files (in this discussion).

(In reply to comment #34)
> Since it's "childish" to reopen this bug report, could we make a more concerted
> effort to work with "upstream" on this?  

Sounds great.. but if you want to work with upstream, why are you posting your thoughts here? The thread is here

http://lists.freedesktop.org/archives/polkit-devel/2009-November/000258.html

FWIW, I already posted a suggestion on how to deal with it upstream. It's kind of a cop-out from my side but hopefully it will let us all move on to more productive things than debating FHS "compliance" and "configuration data" vs. "application data"

 http://lists.freedesktop.org/archives/polkit-devel/2009-November/000270.html

Thanks,
David

I can't see how there is parity between telling people to edit the .pkli files to change the Policy for an application and saying that they are not configuration files.

Since this issue affects Fedora, and is not yet resolved in Fedora (which adheres to the FHS, regardless of your personal feelings about it), I'm reopening this bug. Feel free to work on the issue upstream, but we need to track its progress here.

(In reply to comment #36)
> I can't see how there is parity between telling people to edit the .pkli files
> to change the Policy for an application and saying that they are not
> configuration files.

I never told people to do that. And it wasn't the way the PackageKit mess was fixed. I guess that people are so used to thinking everything is a config file that they forgot there's a thing called "application data".

> Since this issue affects Fedora, and is not yet resolved in Fedora (which
> adheres to the FHS, regardless of your personal feelings about it), I'm
> reopening this bug. Feel free to work on the issue upstream, but we need to
> track its progress here.

No, the FHS is still not being violated. As I already said, Tom, we will add functionality upstream so _your_ view on the FHS and polkit won't be violated. And, Tom, if you want to track this enhancement you need to open a new bug or rename this one. Instead of closing this bug and making you open a new one I'm going to go ahead and just rename it for you.

(And, Tom, in the future please don't be a dick and drag phrases like "personal feelings" into this. It's slightly offensive and not very professional. Thanks.)

Upstream bug filed at https://bugs.freedesktop.org/show_bug.cgi?id=25367, and
David has tentatively agreed to support reading from /etc.

So I think we're all good with no further discussion really needed here.

I think this particular bug is still useful though, for the specfile patch. I'm
going to go ahead and rename the issue again to reflect that.

The current state is: patch is upstream waiting review. Once that's in, we'll
want to update the Fedora package to match, using the proposed changes in
attachment #373474 [details].

According to https://bugs.freedesktop.org/show_bug.cgi?id=25367 the bug has been fixed upstream with the following git commit:
  http://cgit.freedesktop.org/PolicyKit/commit/?id=8e0b9b47d1fc1a4ab6020770e4b3084ddd45b71d

Yes. Now just needs the matching specfile changes.

The changes to also read from /etc are in polkit 0.96 which will be in F13.

Thank you and David both very much.