diff mbox series

dbus: Fix name of polkit function

Message ID 20190131194208.3855-1-plautrba@redhat.com (mailing list archive)
State Superseded
Headers show
Series dbus: Fix name of polkit function | expand

Commit Message

Petr Lautrbach Jan. 31, 2019, 7:42 p.m. UTC 
From: Dan Walsh <dwalsh@redhat.com>

Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
---
 dbus/org.selinux.policy | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Nicolas Iooss Feb. 6, 2019, 8:46 p.m. UTC | #1 
On Thu, Jan 31, 2019 at 8:42 PM Petr Lautrbach <plautrba@redhat.com> wrote:
>
> From: Dan Walsh <dwalsh@redhat.com>
>
> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>

Hi,
As I am unfamiliar with D-Bus code, I try to understand what is the
nature of this change (is-it a bugfix, an improvement, something
else?). Here are the facts:

- Currently, dbus/selinux_server.py uses «
@slip.dbus.polkit.require_auth("org.selinux.change_default_mode") »
for function « change_default_mode ».
- "org.selinux.change_default_mode" does not exist, and I do not know
whether that means that some kind of default policykit policy gets
applied.
- "org.selinux.change_policy_type" is defined in
dbus/org.selinux.policy, without any user.
- The commit that introduced dbus/selinux_server.py (commit
e6a1298e5421 ("These are massive changes involved in building new
GUI.")) added a function named « change_default_mode », without any
specific policykit access checking.
- Commit e8718ef51463 ("Make sure we do the polkit check on all dbus
interfaces.") added «
@slip.dbus.polkit.require_auth("org.selinux.change_default_mode") » in
policycoreutils/sepolicy/selinux_server.py.

If I understand correctly, this last commit is buggy and should have
added "org.selinux.change_default_mode" to
policycoreutils/sepolicy/org.selinux.policy. Therefore this new patch
fixes a bug and removes an unused policykit action. Is this right? If
yes, I suggest adding this to the commit description:

    Add missing action org.selinux.change_default_mode for
change_default_mode() and remove unused action
org.selinux.change_policy_type.

    Fixes: e8718ef51463 ("Make sure we do the polkit check on all dbus
interfaces.")

Would this be acceptable?

Thanks,
Nicolas

> ---
>  dbus/org.selinux.policy | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/dbus/org.selinux.policy b/dbus/org.selinux.policy
> index 01266102..9772127b 100644
> --- a/dbus/org.selinux.policy
> +++ b/dbus/org.selinux.policy
> @@ -70,9 +70,9 @@
>           <allow_active>auth_admin_keep</allow_active>
>          </defaults>
>      </action>
> -    <action id="org.selinux.change_policy_type">
> -        <description>SELinux write access</description>
> -        <message>System policy prevents change_policy_type access to SELinux</message>
> +    <action id="org.selinux.change_default_mode">
> +        <description>Change SELinux default enforcing mode</description>
> +        <message>System policy prevents change_default_policy access to SELinux</message>
>          <defaults>
>            <allow_any>no</allow_any>
>            <allow_inactive>no</allow_inactive>
> --
> 2.20.1
>
Petr Lautrbach Feb. 7, 2019, 9:09 a.m. UTC | #2 
Nicolas Iooss <nicolas.iooss@m4x.org> writes:

> On Thu, Jan 31, 2019 at 8:42 PM Petr Lautrbach 
> <plautrba@redhat.com> wrote:
>>
>> From: Dan Walsh <dwalsh@redhat.com>
>>
>> Signed-off-by: Petr Lautrbach <plautrba@redhat.com>
>
> Hi,
> As I am unfamiliar with D-Bus code, I try to understand what is 
> the
> nature of this change (is-it a bugfix, an improvement, something
> else?). Here are the facts:
>
> - Currently, dbus/selinux_server.py uses «
> @slip.dbus.polkit.require_auth("org.selinux.change_default_mode") 
> »
> for function « change_default_mode ».
> - "org.selinux.change_default_mode" does not exist, and I do not 
> know
> whether that means that some kind of default policykit policy 
> gets
> applied.
> - "org.selinux.change_policy_type" is defined in
> dbus/org.selinux.policy, without any user.
> - The commit that introduced dbus/selinux_server.py (commit
> e6a1298e5421 ("These are massive changes involved in building 
> new
> GUI.")) added a function named « change_default_mode », without 
> any
> specific policykit access checking.
> - Commit e8718ef51463 ("Make sure we do the polkit check on all 
> dbus
> interfaces.") added «
> @slip.dbus.polkit.require_auth("org.selinux.change_default_mode") 
> » in
> policycoreutils/sepolicy/selinux_server.py.
>
> If I understand correctly, this last commit is buggy and should 
> have
> added "org.selinux.change_default_mode" to
> policycoreutils/sepolicy/org.selinux.policy. Therefore this new 
> patch
> fixes a bug and removes an unused policykit action. Is this 
> right? If
> yes, I suggest adding this to the commit description:
>
>     Add missing action org.selinux.change_default_mode for
> change_default_mode() and remove unused action
> org.selinux.change_policy_type.
>
>     Fixes: e8718ef51463 ("Make sure we do the polkit check on 
>     all dbus
> interfaces.")
>
> Would this be acceptable?

You are completely right and the new message makes sense. I'll 
resend
the patch with updated commit message.

Thanks!

>
>> ---
>>  dbus/org.selinux.policy | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/dbus/org.selinux.policy b/dbus/org.selinux.policy
>> index 01266102..9772127b 100644
>> --- a/dbus/org.selinux.policy
>> +++ b/dbus/org.selinux.policy
>> @@ -70,9 +70,9 @@
>>           <allow_active>auth_admin_keep</allow_active>
>>          </defaults>
>>      </action>
>> -    <action id="org.selinux.change_policy_type">
>> -        <description>SELinux write access</description>
>> -        <message>System policy prevents change_policy_type 
>> access to SELinux</message>
>> +    <action id="org.selinux.change_default_mode">
>> +        <description>Change SELinux default enforcing 
>> mode</description>
>> +        <message>System policy prevents change_default_policy 
>> access to SELinux</message>
>>          <defaults>
>>            <allow_any>no</allow_any>
>>            <allow_inactive>no</allow_inactive>
>> --
>> 2.20.1
>>
diff mbox series

Patch

diff --git a/dbus/org.selinux.policy b/dbus/org.selinux.policy
index 01266102..9772127b 100644
--- a/dbus/org.selinux.policy
+++ b/dbus/org.selinux.policy
@@ -70,9 +70,9 @@ 
 	  <allow_active>auth_admin_keep</allow_active>
         </defaults>
     </action>
-    <action id="org.selinux.change_policy_type">
-        <description>SELinux write access</description>
-        <message>System policy prevents change_policy_type access to SELinux</message>
+    <action id="org.selinux.change_default_mode">
+        <description>Change SELinux default enforcing mode</description>
+        <message>System policy prevents change_default_policy access to SELinux</message>
         <defaults>
           <allow_any>no</allow_any>
           <allow_inactive>no</allow_inactive>