[PATCH 0/2] connman: Always depend on xuser-account & fix D-Bus policy

[PATCH 0/2] connman: Always depend on xuser-account & fix D-Bus policy

[Jussi Kukkonen]

Review on the policy change is appreciated: it looks clearly correct to me but
maybe there's some context I've missed.

Cheers,
  Jussi



The following changes since commit 9f66aa1a11b5b45280aa41bf4517ec56ee5f2ab2:

  bitbake: toaster: start script warning text formatting small improvement (2015-09-23 22:44:56 +0100)

are available in the git repository at:

  git://git.yoctoproject.org/poky-contrib jku/connman-policy
  http://git.yoctoproject.org/cgit.cgi/poky-contrib/log/?h=jku/connman-policy

Jussi Kukkonen (2):
  connman: Depend on xuser-account unconditionally
  connman: Don't use a blanket "allow" D-Bus policy

 meta/recipes-connectivity/connman/connman.inc      |  8 +------
 .../connman/add_xuser_dbus_permission.patch        | 28 +++++++++++++++++++---
 2 files changed, 26 insertions(+), 10 deletions(-)

-- 
2.1.4

[PATCH 1/2] connman: Depend on xuser-account unconditionally

[Jussi Kukkonen]

This means dragging in xuser-account even when it's not used but
that's a lesser evil than the recipe depending on machine specific
settings.

This also prevents a warning on connman service startup when
ROOTLESS_X is not set:
  Unknown username "xuser" in message bus

[YOCTO #8005]

Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
---
 meta/recipes-connectivity/connman/connman.inc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/meta/recipes-connectivity/connman/connman.inc b/meta/recipes-connectivity/connman/connman.inc
index fd9640e..6c062ae 100644
--- a/meta/recipes-connectivity/connman/connman.inc
+++ b/meta/recipes-connectivity/connman/connman.inc
@@ -113,7 +113,7 @@ RPROVIDES_${PN} = "\
 
 RDEPENDS_${PN} = "\
 	dbus \
-	${@base_conditional('ROOTLESS_X', '1', 'xuser-account', '', d)} \
+	xuser-account \
 	"
 
 PACKAGES_DYNAMIC += "^${PN}-plugin-.*"
-- 
2.1.4

[PATCH 2/2] connman: Don't use a blanket "allow" D-Bus policy

[Jussi Kukkonen]

There are already "allow" rules for root and conditionally xuser to
send messages to connman: there should be no reason for a default
allow policy.

Also, conditionally add a policy to allow xuser to send to the
connman vpn service (similar to main service).

Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
---
 meta/recipes-connectivity/connman/connman.inc      |  6 -----
 .../connman/add_xuser_dbus_permission.patch        | 28 +++++++++++++++++++---
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/meta/recipes-connectivity/connman/connman.inc b/meta/recipes-connectivity/connman/connman.inc
index 6c062ae..1712af3 100644
--- a/meta/recipes-connectivity/connman/connman.inc
+++ b/meta/recipes-connectivity/connman/connman.inc
@@ -70,13 +70,7 @@ SYSTEMD_SERVICE_${PN} = "connman.service"
 SYSTEMD_SERVICE_${PN}-vpn = "connman-vpn.service"
 SYSTEMD_WIRED_SETUP = "ExecStartPre=-${libdir}/connman/wired-setup"
 
-# This allows *everyone* to access ConnMan over DBus, without any access
-# control.  Really the at_console flag should work, which would mean that
-# both this and the xuser patch can be dropped.
 do_compile_append() {
-	sed -i -e s:deny:allow:g ${S}/src/connman-dbus.conf
-	sed -i -e s:deny:allow:g ${S}/vpn/vpn-dbus.conf
-
 	sed -i "s#ExecStart=#${SYSTEMD_WIRED_SETUP}\nExecStart=#" ${B}/src/connman.service
 }
 
diff --git a/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch b/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch
index 707b3ca..15a191d 100644
--- a/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch
+++ b/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch
@@ -1,9 +1,14 @@
-Because Poky doesn't support at_console we need to special-case the session
-user.
+Because Poky doesn't support at_console we need to
+special-case the session user.
 
 Upstream-Status: Inappropriate [configuration]
 
-Signed-off-by: Ross Burton <ross.burton@intel.com>
+Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
+
+---
+ src/connman-dbus.conf | 3 +++
+ vpn/vpn-dbus.conf     | 3 +++
+ 2 files changed, 6 insertions(+)
 
 diff --git a/src/connman-dbus.conf b/src/connman-dbus.conf
 index 98a773e..466809c 100644
@@ -19,3 +24,20 @@ index 98a773e..466809c 100644
      <policy at_console="true">
          <allow send_destination="net.connman"/>
      </policy>
+diff --git a/vpn/vpn-dbus.conf b/vpn/vpn-dbus.conf
+index 0f0c8da..9ad05b9 100644
+--- a/vpn/vpn-dbus.conf
++++ b/vpn/vpn-dbus.conf
+@@ -6,6 +6,9 @@
+         <allow send_destination="net.connman.vpn"/>
+         <allow send_interface="net.connman.vpn.Agent"/>
+     </policy>
++    <policy user="xuser">
++        <allow send_destination="net.connman.vpn"/>
++    </policy>
+     <policy at_console="true">
+         <allow send_destination="net.connman.vpn"/>
+     </policy>
+-- 
+2.1.4
+
-- 
2.1.4

Re: [PATCH 1/2] connman: Depend on xuser-account unconditionally

[Andreas Oberritter]

Hello Jussi,

On 25.09.2015 13:14, Jussi Kukkonen wrote:
> This means dragging in xuser-account even when it's not used but
> that's a lesser evil than the recipe depending on machine specific
> settings.

that's quite ugly, sorry.

It's true that machine specific variables shouldn't be used in this
recipe, but adding privileged user accounts with regular home
directories is evil as well and confusing for real users.

There must be a better way to solve this.

Whether to add user accounts or not should be a decision made by the
distro. If this setting is machine specific, which to be honest
surprised me, why don't you just let the machine specific xorg video
driver depend on xuser-account unconditionally? Connman really doesn't
need this account.

> This also prevents a warning on connman service startup when
> ROOTLESS_X is not set:
>   Unknown username "xuser" in message bus
> 
> [YOCTO #8005]

This bugzilla entry is just about the warning, which could as well just
get ignored or removed from dbus.

Regards,
Andreas

> 
> Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
> ---
>  meta/recipes-connectivity/connman/connman.inc | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/meta/recipes-connectivity/connman/connman.inc b/meta/recipes-connectivity/connman/connman.inc
> index fd9640e..6c062ae 100644
> --- a/meta/recipes-connectivity/connman/connman.inc
> +++ b/meta/recipes-connectivity/connman/connman.inc
> @@ -113,7 +113,7 @@ RPROVIDES_${PN} = "\
>  
>  RDEPENDS_${PN} = "\
>  	dbus \
> -	${@base_conditional('ROOTLESS_X', '1', 'xuser-account', '', d)} \
> +	xuser-account \
>  	"
>  
>  PACKAGES_DYNAMIC += "^${PN}-plugin-.*"
> 

Re: [PATCH 2/2] connman: Don't use a blanket "allow" D-Bus policy

[Andreas Oberritter]

On 25.09.2015 13:14, Jussi Kukkonen wrote:
> There are already "allow" rules for root and conditionally xuser to
> send messages to connman: there should be no reason for a default
> allow policy.
> 
> Also, conditionally add a policy to allow xuser to send to the
> connman vpn service (similar to main service).
> 
> Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
> ---
>  meta/recipes-connectivity/connman/connman.inc      |  6 -----
>  .../connman/add_xuser_dbus_permission.patch        | 28 +++++++++++++++++++---
>  2 files changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/meta/recipes-connectivity/connman/connman.inc b/meta/recipes-connectivity/connman/connman.inc
> index 6c062ae..1712af3 100644
> --- a/meta/recipes-connectivity/connman/connman.inc
> +++ b/meta/recipes-connectivity/connman/connman.inc
> @@ -70,13 +70,7 @@ SYSTEMD_SERVICE_${PN} = "connman.service"
>  SYSTEMD_SERVICE_${PN}-vpn = "connman-vpn.service"
>  SYSTEMD_WIRED_SETUP = "ExecStartPre=-${libdir}/connman/wired-setup"
>  
> -# This allows *everyone* to access ConnMan over DBus, without any access
> -# control.  Really the at_console flag should work, which would mean that
> -# both this and the xuser patch can be dropped.
>  do_compile_append() {
> -	sed -i -e s:deny:allow:g ${S}/src/connman-dbus.conf
> -	sed -i -e s:deny:allow:g ${S}/vpn/vpn-dbus.conf
> -
>  	sed -i "s#ExecStart=#${SYSTEMD_WIRED_SETUP}\nExecStart=#" ${B}/src/connman.service
>  }
>  
> diff --git a/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch b/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch
> index 707b3ca..15a191d 100644
> --- a/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch
> +++ b/meta/recipes-connectivity/connman/connman/add_xuser_dbus_permission.patch
> @@ -1,9 +1,14 @@
> -Because Poky doesn't support at_console we need to special-case the session
> -user.
> +Because Poky doesn't support at_console we need to
> +special-case the session user.

Here you can see that it really is poky's distro policy that slipped
into OE-Core. How about removing ROOTLESS_X and xuser from OE-Core and
putting it into a layer that actually sets the variable?

Regards,
Andreas

>  
>  Upstream-Status: Inappropriate [configuration]
>  
> -Signed-off-by: Ross Burton <ross.burton@intel.com>
> +Signed-off-by: Jussi Kukkonen <jussi.kukkonen@intel.com>
> +
> +---
> + src/connman-dbus.conf | 3 +++
> + vpn/vpn-dbus.conf     | 3 +++
> + 2 files changed, 6 insertions(+)
>  
>  diff --git a/src/connman-dbus.conf b/src/connman-dbus.conf
>  index 98a773e..466809c 100644
> @@ -19,3 +24,20 @@ index 98a773e..466809c 100644
>       <policy at_console="true">
>           <allow send_destination="net.connman"/>
>       </policy>
> +diff --git a/vpn/vpn-dbus.conf b/vpn/vpn-dbus.conf
> +index 0f0c8da..9ad05b9 100644
> +--- a/vpn/vpn-dbus.conf
> ++++ b/vpn/vpn-dbus.conf
> +@@ -6,6 +6,9 @@
> +         <allow send_destination="net.connman.vpn"/>
> +         <allow send_interface="net.connman.vpn.Agent"/>
> +     </policy>
> ++    <policy user="xuser">
> ++        <allow send_destination="net.connman.vpn"/>
> ++    </policy>
> +     <policy at_console="true">
> +         <allow send_destination="net.connman.vpn"/>
> +     </policy>
> +-- 
> +2.1.4
> +
> 

Re: [PATCH 1/2] connman: Depend on xuser-account unconditionally

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 697 bytes --]

On 25 September 2015 at 16:59, Andreas Oberritter <obi@opendreambox.org>
wrote:

> Whether to add user accounts or not should be a decision made by the
> distro. If this setting is machine specific, which to be honest
> surprised me, why don't you just let the machine specific xorg video
> driver depend on xuser-account unconditionally? Connman really doesn't
> need this account.
>

connman does need the account as it uses it to allow a non-root X user to
actually use connman.  DBus should warn if you try and set policy that uses
users that entirely don't exist.

I wonder if dbus lets multiple files assign policy, so the dbus rules could
go into the xuser package?

Ross

[-- Attachment #2: Type: text/html, Size: 1179 bytes --]

Re: [PATCH 2/2] connman: Don't use a blanket "allow" D-Bus policy

[Burton, Ross]

[-- Attachment #1: Type: text/plain, Size: 560 bytes --]

On 25 September 2015 at 17:06, Andreas Oberritter <obi@opendreambox.org>
wrote:

> Here you can see that it really is poky's distro policy that slipped
> into OE-Core. How about removing ROOTLESS_X and xuser from OE-Core and
> putting it into a layer that actually sets the variable?
>

There's nothing about Poky here.  meta-intel used to set it for several
BSPs, in the consolidation that happened recently that obviously got lost.
Part of me does want to get ROOTLESS_X removed and have it done properly,
but it's far too late for that.

Ross

[-- Attachment #2: Type: text/html, Size: 1013 bytes --]

Re: [PATCH 1/2] connman: Depend on xuser-account unconditionally

[Jussi Kukkonen]

[-- Attachment #1: Type: text/plain, Size: 1221 bytes --]

On 25 September 2015 at 19:12, Burton, Ross <ross.burton@intel.com> wrote:
>
> On 25 September 2015 at 16:59, Andreas Oberritter <obi@opendreambox.org>
> wrote:
>
>> Whether to add user accounts or not should be a decision made by the
>> distro. If this setting is machine specific, which to be honest
>> surprised me, why don't you just let the machine specific xorg video
>> driver depend on xuser-account unconditionally? Connman really doesn't
>> need this account.
>>
>
> connman does need the account as it uses it to allow a non-root X user to
> actually use connman.  DBus should warn if you try and set policy that uses
> users that entirely don't exist.
>
> I wonder if dbus lets multiple files assign policy, so the dbus rules
> could go into the xuser package?
>

I see the patch is in master now but I'll just note here that Ross' idea
here seems like a good solution (even if it makes figuring out the overall
policy a little trickier for humans) and I see nothing that would prevent
it from working. I just got side tracked with other D-Bus policy bugs while
look at this: After I'm done with those I'll make a patch to move the xuser
connman policy to the xuser recipe.

Jussi

[-- Attachment #2: Type: text/html, Size: 1947 bytes --]

Re: [PATCH 1/2] connman: Depend on xuser-account unconditionally

[Andreas Oberritter]

On 30.09.2015 09:08, Jussi Kukkonen wrote:
> I see the patch is in master now but I'll just note here that Ross' idea
> here seems like a good solution (even if it makes figuring out the
> overall policy a little trickier for humans) and I see nothing that
> would prevent it from working. I just got side tracked with other D-Bus
> policy bugs while look at this: After I'm done with those I'll make a
> patch to move the xuser connman policy to the xuser recipe.

Thank you, Jussi!

Regards,
Andreas