Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

On Tue, Apr 25, 2017 at 09:47:24AM -0500, Dan Williams wrote:
>On Tue, 2017-04-25 at 07:45 +0000, David Härdeman wrote:
>> April 24, 2017 5:49 PM, "Dan Williams" <d...@redhat.com> wrote:
>>> 
>>> It's not clear that the GNOME side was implemented correctly yet.
>>> Would be nice to see the sample code.
>>> 
>>> Dan
>> 
>> My GNOME-based client was based on the gdbus-example-objectmanager-
>> client.c so I hope it's correct, but sure, I'll try to pare down the
>> GNOME/gdbus-based client code and the (sd-bus based) server code to
>> two simple test cases and provide those later this week.
>
>Cool.  I'd be happy to take a look at it.  It may well turn out to be a
>gdbus bug, but without seeing the code it's not clear.

I've added test cases at:
https://github.com/Alphix/gdbus-sdbus-test

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

On Tue, Apr 25, 2017 at 09:21:19PM +0200, David Härdeman wrote:
>On Tue, Apr 25, 2017 at 09:54:45AM +0200, David Herrmann wrote:
>>No, it does not. sd-bus was inconsistent. See, there are 3 things
>>involved in the Object-Manager:
>>
>>Signal: InterfacesAdded
>>Signal: InterfacesRemoved
>>Call: GetManagedObjects
>>
>>The first two signals are used to add and remove objects (and their
>>respective interfaces) at runtime. The GetManagedObjects() call is
>>used to bootstrap, that is, to get the initial set of objects (and
>>their respective interfaces) when starting your application.
>>
>>So far, sd-bus reported the builtin interfaces correctly via the
>>signals, but did not return that information in the
>>GetManagedObjects() call. My patch fixed this omission.
>>The effect of this is that objects added/removed at runtime work fine,
>>but if an object was there already when your application starts, then
>>it will never be removed, since gdbus will see an interface-count
>>mismatch (as you observed above).
>>
>>Hence, the patch I provided fixes how sd-bus provides this
>>information. It does *not* change any policy.
>>
>>I think the 2 underlying problems you saw are bugs in both gdbus and
>>sd-bus. The one in sd-bus I tried to fix above, the one in gdbus is
>>only about counting interfaces and properly detecting negative
>>interface-counts. The gdbus bug is non-severe, though, since it
>>originates in a buggy remote client. So there is no hard reason to
>>change gdbus there.
>
>Ok, I've tried the patch. It seems to fix things insofar that
>InterfacesAdded/InterfacesRemoved/GetManagedObjects all report 5
>interfaces (checked with dbus-monitor).
>
>gdbus still gets royally confused though. It seems to report 5
>interfaces for objects which are added dynamically (i.e. signalled via
>InterfacesAdded) and 1 interface for objects which are generated via
>GetManagedObjects (even though the sd-bus server reports 5 interfaces).

Scratch that. With your patch gdbus seems to consistently report 5
interfaces. But there's still some confusion to address in gdbus (the
additional interfaces have no names for example).

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

On Tue, Apr 25, 2017 at 09:54:45AM +0200, David Herrmann wrote:
>On Tue, Apr 25, 2017 at 9:40 AM, David Härdeman <da...@hardeman.nu> wrote:
>> April 21, 2017 1:22 PM, "David Herrmann" <dh.herrm...@gmail.com> wrote:
>>> This change makes sure all objects have the built-in interfaces
>>> reported at all times. The GetManagedObjects() call didn't report them
>>> so far.
>>
>> Quite the contrary? If you look at the output from dbus-monitor above, 
>> you'll see that it is sd-bus that already *does* report all interfaces while 
>> gdbus doesnt?
>
>No, it does not. sd-bus was inconsistent. See, there are 3 things
>involved in the Object-Manager:
>
>Signal: InterfacesAdded
>Signal: InterfacesRemoved
>Call: GetManagedObjects
>
>The first two signals are used to add and remove objects (and their
>respective interfaces) at runtime. The GetManagedObjects() call is
>used to bootstrap, that is, to get the initial set of objects (and
>their respective interfaces) when starting your application.
>
>So far, sd-bus reported the builtin interfaces correctly via the
>signals, but did not return that information in the
>GetManagedObjects() call. My patch fixed this omission.
>The effect of this is that objects added/removed at runtime work fine,
>but if an object was there already when your application starts, then
>it will never be removed, since gdbus will see an interface-count
>mismatch (as you observed above).
>
>Hence, the patch I provided fixes how sd-bus provides this
>information. It does *not* change any policy.
>
>I think the 2 underlying problems you saw are bugs in both gdbus and
>sd-bus. The one in sd-bus I tried to fix above, the one in gdbus is
>only about counting interfaces and properly detecting negative
>interface-counts. The gdbus bug is non-severe, though, since it
>originates in a buggy remote client. So there is no hard reason to
>change gdbus there.

Ok, I've tried the patch. It seems to fix things insofar that
InterfacesAdded/InterfacesRemoved/GetManagedObjects all report 5
interfaces (checked with dbus-monitor).

gdbus still gets royally confused though. It seems to report 5
interfaces for objects which are added dynamically (i.e. signalled via
InterfacesAdded) and 1 interface for objects which are generated via
GetManagedObjects (even though the sd-bus server reports 5 interfaces).

I'll start working on a testcase...

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

April 25, 2017 9:54 AM, "David Herrmann" <dh.herrm...@gmail.com> wrote:

> Hi
> 
> On Tue, Apr 25, 2017 at 9:40 AM, David Härdeman <da...@hardeman.nu> wrote:
> 
>> April 21, 2017 1:22 PM, "David Herrmann" <dh.herrm...@gmail.com> wrote:
>>> On Fri, Apr 21, 2017 at 11:50 AM, David Härdeman <da...@hardeman.nu> wrote:
>> 
>> On Thu, Apr 20, 2017 at 02:19:22PM +0200, David Herrmann wrote:
>> On Thu, Apr 20, 2017 at 12:06 PM, David Härdeman <da...@hardeman.nu> wrote:
>> I'm implementing a server which creates an ObjectManager using the
>> sd-bus API and there seems to be some differences between how gdbus and
>> sd-bus implements the API.
>> 
>> I implemented a simple ObjectManager at /org/gnome/TestManager which
>> exports objects /org/gnome/TestManager/fooX with interface
>> org.gnome.TestManager.Device.
>> 
>> Under sd-bus, if an object is removed, the following signal is
>> generated:
>> 
>> signal time=1492642227.714223 sender=:1.104 -> destination=(null destination)
>> serial=90 path=/org/gnome/TestManager;
>> interface=org.freedesktop.DBus.ObjectManager;
>> member=InterfacesRemoved
>> object path "/org/gnome/TestManager/foo0"
>> array [
>> string "org.freedesktop.DBus.Peer"
>> string "org.freedesktop.DBus.Introspectable"
>> string "org.freedesktop.DBus.Properties"
>> string "org.freedesktop.DBus.ObjectManager"
>> string "org.gnome.TestManager.Device"
>> ]
>> 
>> If I implement the same simple server in gdbus, the signal is instead:
>> 
>> signal time=1492642227.714223 sender=:1.104 -> destination=(null destination)
>> serial=90 path=/org/gnome/TestManager;
>> interface=org.freedesktop.DBus.ObjectManager;
>> member=InterfacesRemoved
>> object path "/org/gnome/TestManager/foo0"
>> array [
>> string "org.gnome.TestManager.Device"
>> ]
>> 
>> The corresponding signals are also generated when an object is added.
>> 
>> Does the appended patch fix your issue?
>> (line-breaks might be screwed, sorry)
>> 
>> Haven't tried it yet, but just from reading the patch...it seems to do
>> the opposite of what I'd expect? I.e. add *more* interfaces?
>>> This change makes sure all objects have the built-in interfaces
>>> reported at all times. The GetManagedObjects() call didn't report them
>>> so far.
>> 
>> Quite the contrary? If you look at the output from dbus-monitor above, 
>> you'll see that it is sd-bus
>> that already *does* report all interfaces while gdbus doesnt?
> 
> No, it does not. sd-bus was inconsistent. See, there are 3 things
> involved in the Object-Manager:
> 
> Signal: InterfacesAdded
> Signal: InterfacesRemoved
> Call: GetManagedObjects
> 
> The first two signals are used to add and remove objects (and their
> respective interfaces) at runtime. The GetManagedObjects() call is
> used to bootstrap, that is, to get the initial set of objects (and
> their respective interfaces) when starting your application.
> 
> So far, sd-bus reported the builtin interfaces correctly via the
> signals, but did not return that information in the
> GetManagedObjects() call. My patch fixed this omission.
> The effect of this is that objects added/removed at runtime work fine,
> but if an object was there already when your application starts, then
> it will never be removed, since gdbus will see an interface-count
> mismatch (as you observed above).
> 
> Hence, the patch I provided fixes how sd-bus provides this
> information. It does *not* change any policy.

Oh, I see. Thanks. I'll try the patch ASAP.

Regards,
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

April 24, 2017 5:49 PM, "Dan Williams"  wrote:
> On Mon, 2017-04-24 at 16:50 +0200, Lennart Poettering wrote:
>> On Fri, 21.04.17 13:22, David Herrmann (dh.herrm...@gmail.com) wrote:
>> 
> Anyway, gdbus bugs aside, it seems that the interfaces
> reported by
> sd-bus should match what gdbus does? (assuming, of course,
> that gdbus
> can be considered the "reference" implementation).
 
 Does the appended patch fix your issue?
 (line-breaks might be screwed, sorry)
>>> 
>>> Haven't tried it yet, but just from reading the patch...it seems
>>> to do
>>> the opposite of what I'd expect? I.e. add *more* interfaces?
>> 
>> This change makes sure all objects have the built-in interfaces
>> reported at all times. The GetManagedObjects() call didn't report
>> them
>> so far.
>> 
>> Note that we really better report all interfaces an object
>> supports. I
>> don't know why glib does not do this, but I think it should.
>> 
>> Yeah, I#d agree with that. I think we should provide complete
>> information, and that means including built-in interfaces in all our
>> messages, in particular as some of them are optional. It appears to
>> me, that gdbus should be changed here, not sd-bus...
> 
> It's not clear that the GNOME side was implemented correctly yet.
> Would be nice to see the sample code.
> 
> Dan

My GNOME-based client was based on the gdbus-example-objectmanager-client.c so 
I hope it's correct, but sure, I'll try to pare down the GNOME/gdbus-based 
client code and the (sd-bus based) server code to two simple test cases and 
provide those later this week.

Regards,
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

April 21, 2017 1:22 PM, "David Herrmann" <dh.herrm...@gmail.com> wrote:
> On Fri, Apr 21, 2017 at 11:50 AM, David Härdeman <da...@hardeman.nu> wrote:
>> On Thu, Apr 20, 2017 at 02:19:22PM +0200, David Herrmann wrote:
>>> On Thu, Apr 20, 2017 at 12:06 PM, David Härdeman <da...@hardeman.nu> wrote:
>> I'm implementing a server which creates an ObjectManager using the
>> sd-bus API and there seems to be some differences between how gdbus and
>> sd-bus implements the API.
>> 
>> I implemented a simple ObjectManager at /org/gnome/TestManager which
>> exports objects /org/gnome/TestManager/fooX with interface
>> org.gnome.TestManager.Device.
>> 
>> Under sd-bus, if an object is removed, the following signal is
>> generated:
>> 
>> signal time=1492642227.714223 sender=:1.104 -> destination=(null destination)
>> serial=90 path=/org/gnome/TestManager;
>> interface=org.freedesktop.DBus.ObjectManager;
>> member=InterfacesRemoved
>> object path "/org/gnome/TestManager/foo0"
>> array [
>> string "org.freedesktop.DBus.Peer"
>> string "org.freedesktop.DBus.Introspectable"
>> string "org.freedesktop.DBus.Properties"
>> string "org.freedesktop.DBus.ObjectManager"
>> string "org.gnome.TestManager.Device"
>> ]
>> 
>> If I implement the same simple server in gdbus, the signal is instead:
>> 
>> signal time=1492642227.714223 sender=:1.104 -> destination=(null destination)
>> serial=90 path=/org/gnome/TestManager;
>> interface=org.freedesktop.DBus.ObjectManager;
>> member=InterfacesRemoved
>> object path "/org/gnome/TestManager/foo0"
>> array [
>> string "org.gnome.TestManager.Device"
>> ]
>> 
>> The corresponding signals are also generated when an object is added.

>>> Does the appended patch fix your issue?
>>> (line-breaks might be screwed, sorry)
>> 
>> Haven't tried it yet, but just from reading the patch...it seems to do
>> the opposite of what I'd expect? I.e. add *more* interfaces?
> 
> This change makes sure all objects have the built-in interfaces
> reported at all times. The GetManagedObjects() call didn't report them
> so far.

Quite the contrary? If you look at the output from dbus-monitor above, you'll 
see that it is sd-bus that already *does* report all interfaces while gdbus 
doesnt?
 
> Note that we really better report all interfaces an object supports. I
> don't know why glib does not do this, but I think it should.

Maybe, I'm no dbus expert. And there does seem to be a bug in gdbus as well 
since the additional interfaces manage to confuse it. Anyway, it seems that 
either sd-bus should adapt the gdbus style or vice versa...maybe you could talk 
directly to the gdbus people?

Regards,
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

April 24, 2017 4:51 PM, "Lennart Poettering"  wrote:

> On Fri, 21.04.17 13:22, David Herrmann (dh.herrm...@gmail.com) wrote:
> 
>> Anyway, gdbus bugs aside, it seems that the interfaces reported by
>> sd-bus should match what gdbus does? (assuming, of course, that gdbus
>> can be considered the "reference" implementation).
>> 
>> Does the appended patch fix your issue?
>> (line-breaks might be screwed, sorry)
>> 
>> Haven't tried it yet, but just from reading the patch...it seems to do
>> the opposite of what I'd expect? I.e. add *more* interfaces?
>> 
>> This change makes sure all objects have the built-in interfaces
>> reported at all times. The GetManagedObjects() call didn't report them
>> so far.
>> 
>> Note that we really better report all interfaces an object supports. I
>> don't know why glib does not do this, but I think it should.
> 
> Yeah, I#d agree with that. I think we should provide complete
> information, and that means including built-in interfaces in all our
> messages, in particular as some of them are optional. It appears to
> me, that gdbus should be changed here, not sd-bus...

In that case, no changes are necessary to sd-bus since it already exposes all 
interfaces.

I've filed a bug report against gdbus:
https://bugzilla.gnome.org/show_bug.cgi?id=781524

Regards,
David
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

On Thu, Apr 20, 2017 at 02:19:22PM +0200, David Herrmann wrote:
>On Thu, Apr 20, 2017 at 12:06 PM, David Härdeman <da...@hardeman.nu> wrote:
>> Hi,
>>
>> I'm implementing a server which creates an ObjectManager using the
>> sd-bus API and there seems to be some differences between how gdbus and
>> sd-bus implements the API.
>>
>> I implemented a simple ObjectManager at /org/gnome/TestManager which
>> exports objects /org/gnome/TestManager/fooX with interface
>> org.gnome.TestManager.Device.
>>
>> Under sd-bus, if an object is removed, the following signal is
>> generated:
>>
>> signal time=1492642227.714223 sender=:1.104 -> destination=(null destination)
>> serial=90 path=/org/gnome/TestManager;
>> interface=org.freedesktop.DBus.ObjectManager;
>> member=InterfacesRemoved
>>object path "/org/gnome/TestManager/foo0"
>>array [
>>   string "org.freedesktop.DBus.Peer"
>>   string "org.freedesktop.DBus.Introspectable"
>>   string "org.freedesktop.DBus.Properties"
>>   string "org.freedesktop.DBus.ObjectManager"
>>   string "org.gnome.TestManager.Device"
>>]
>>
>> If I implement the same simple server in gdbus, the signal is instead:
>>
>> signal time=1492642227.714223 sender=:1.104 -> destination=(null destination)
>> serial=90 path=/org/gnome/TestManager;
>> interface=org.freedesktop.DBus.ObjectManager;
>> member=InterfacesRemoved
>>object path "/org/gnome/TestManager/foo0"
>>array [
>>   string "org.gnome.TestManager.Device"
>>]
>>
>> The corresponding signals are also generated when an object is added.
>>
>> Beside simply being different, this difference seems to confuse gdbus.
>> If I create a test client, it will report that any object which is
>> already existing when I start the client has 1 interface and any object
>> which is added/removed subsequently is reported as having 5 interfaces,
>> 4 of which are nameless (this would appear to be one or more gdbus
>> bug(s)).
>>
>> This bug in gdbus also means that it doesn't correctly catch the removal
>> of an object which existed at the time the client was started (because
>> of the interface count mismatch).
>>
>> Anyway, gdbus bugs aside, it seems that the interfaces reported by
>> sd-bus should match what gdbus does? (assuming, of course, that gdbus
>> can be considered the "reference" implementation).
>
>Does the appended patch fix your issue?
>(line-breaks might be screwed, sorry)

Haven't tried it yet, but just from reading the patch...it seems to do
the opposite of what I'd expect? I.e. add *more* interfaces?

>diff --git a/src/libsystemd/sd-bus/bus-objects.c
>b/src/libsystemd/sd-bus/bus-objects.c
>index 9bd07ffca..b6f5afe1b 100644
>--- a/src/libsystemd/sd-bus/bus-objects.c
>+++ b/src/libsystemd/sd-bus/bus-objects.c
>@@ -1057,6 +1057,22 @@ static int object_manager_serialize_path(
> if (r < 0)
> return r;
>
>+r = sd_bus_message_append(reply, "{sa{sv}}",
>"org.freedesktop.DBus.Peer", 0);
>+if (r < 0)
>+return r;
>+
>+r = sd_bus_message_append(reply, "{sa{sv}}",
>"org.freedesktop.DBus.Introspectable", 0);
>+    if (r < 0)
>+return r;
>+
>+r = sd_bus_message_append(reply, "{sa{sv}}",
>"org.freedesktop.DBus.Properties", 0);
>+if (r < 0)
>+return r;
>+
>+r = sd_bus_message_append(reply, "{sa{sv}}",
>"org.freedesktop.DBus.ObjectManager", 0);
>+if (r < 0)
>+return r;
>+
> found_something = true;
> }

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] sd-bus: ObjectManager difference with gdbus

[David Härdeman]

Hi,

I'm implementing a server which creates an ObjectManager using the
sd-bus API and there seems to be some differences between how gdbus and
sd-bus implements the API.

I implemented a simple ObjectManager at /org/gnome/TestManager which
exports objects /org/gnome/TestManager/fooX with interface
org.gnome.TestManager.Device.

Under sd-bus, if an object is removed, the following signal is
generated:

signal time=1492642227.714223 sender=:1.104 -> destination=(null destination)
serial=90 path=/org/gnome/TestManager;
interface=org.freedesktop.DBus.ObjectManager;
member=InterfacesRemoved
   object path "/org/gnome/TestManager/foo0"
   array [
  string "org.freedesktop.DBus.Peer"
  string "org.freedesktop.DBus.Introspectable"
  string "org.freedesktop.DBus.Properties"
  string "org.freedesktop.DBus.ObjectManager"
  string "org.gnome.TestManager.Device"
   ]

If I implement the same simple server in gdbus, the signal is instead:

signal time=1492642227.714223 sender=:1.104 -> destination=(null destination) 
serial=90 path=/org/gnome/TestManager;
interface=org.freedesktop.DBus.ObjectManager;
member=InterfacesRemoved
   object path "/org/gnome/TestManager/foo0"
   array [
  string "org.gnome.TestManager.Device"
   ]

The corresponding signals are also generated when an object is added.

Beside simply being different, this difference seems to confuse gdbus.
If I create a test client, it will report that any object which is
already existing when I start the client has 1 interface and any object
which is added/removed subsequently is reported as having 5 interfaces,
4 of which are nameless (this would appear to be one or more gdbus
bug(s)).

This bug in gdbus also means that it doesn't correctly catch the removal
of an object which existed at the time the client was started (because
of the interface count mismatch).

Anyway, gdbus bugs aside, it seems that the interfaces reported by
sd-bus should match what gdbus does? (assuming, of course, that gdbus
can be considered the "reference" implementation).

Regards,
David

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus object manager question

[David Härdeman]

On Sat, Jun 27, 2015 at 09:22:45AM +0200, David Härdeman wrote:
On Thu, Jun 25, 2015 at 04:25:29PM +0200, David Herrmann wrote:
On Tue, Jun 23, 2015 at 2:41 PM, David Härdeman da...@hardeman.nu wrote:
...
 Now, a question...how is an object manager supposed to be implemented in
 sd-bus?

 I've seen that there's a sd_bus_add_object_manager() function in sd-bus.h,
 but how would I notify the object manager when objects are added and
 removed?

 Two likely candidates seemed to be:
 sd_bus_emit_object_added()
 sd_bus_emit_object_removed()

 But calling them did not seem to do the right thing. And there seems to be
 zero usage of sd_bus_add_object_manager() in the systemd tree itself (though
 plenty of calls to sd_bus_add_node_enumerator() which might be related?).

Not sure where the confusion is. Those calls generate the
InterfacesAdded/Removed signals that the objectmanager API specifies.
They collect the interfaces based on the vtables you registered on a
given path. have a look at test-bus-objects.c which uses these
interfaces.

I've done some more experimentation and I think I've got a better idea
of what's going on. Sorry if my first email was a bit confusing.

My problem is that I couldn't get the sd-bus object manager to properly
notify clients when objects are added or removed.

I'm testing my sd-bus server with a gdbus client. The
GDBusObjectManagerClient should normally generate signals (gobject
signals, that is) when objects are added or removed.

Anyway, I tried implementing an object manager server with gdbus as
well, and when talking to the gdbus server, the client is notified of
object additions/removals, so I used used busctl to monitor the signals
that were generated. I think I've found the problem.

Assume there's an object manager at /org/example/mgr and that a new
object is created at /org/example/mgr/object1.

With sd-bus, I'd call:
sd_bus_emit_object_added(bus, /org/example/mgr/object1);

The generated signal would be:
‣ Type=signal  Endian=l  Flags=1  Version=1  Priority=0 Cookie=11
  Sender=:1.297  Path=/org/example/mgr/object1
Interface=org.freedesktop.DBus.ObjectManager  Member=InterfacesAdded
  UniqueName=:1.297
  MESSAGE oa{sa{sv}} {
  OBJECT_PATH /org/example/mgr/object1;

With gdbus, the generated signal is instead:
The generated signal would be:
‣ Type=signal  Endian=l  Flags=1  Version=1  Priority=0 Cookie=21
  Sender=:1.297  Path=/org/example/mgr
Interface=org.freedesktop.DBus.ObjectManager  Member=InterfacesAdded
  UniqueName=:1.297
  MESSAGE oa{sa{sv}} {
  OBJECT_PATH /org/example/mgr/object1;

Note the difference in the sender path.

The GDBusObjectManagerClient seems to ignore the signal sd-bus generates
because of the different path.

I think that means that sd_bus_emit_object_added() would need to take
both the object manager path and the object path as arguments instead of
a single path?

Ping?
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] sd-bus object manager question

[David Härdeman]

On Thu, Jun 25, 2015 at 04:25:29PM +0200, David Herrmann wrote:
On Tue, Jun 23, 2015 at 2:41 PM, David Härdeman da...@hardeman.nu wrote:
...
 Now, a question...how is an object manager supposed to be implemented in
 sd-bus?

 I've seen that there's a sd_bus_add_object_manager() function in sd-bus.h,
 but how would I notify the object manager when objects are added and
 removed?

 Two likely candidates seemed to be:
 sd_bus_emit_object_added()
 sd_bus_emit_object_removed()

 But calling them did not seem to do the right thing. And there seems to be
 zero usage of sd_bus_add_object_manager() in the systemd tree itself (though
 plenty of calls to sd_bus_add_node_enumerator() which might be related?).

Not sure where the confusion is. Those calls generate the
InterfacesAdded/Removed signals that the objectmanager API specifies.
They collect the interfaces based on the vtables you registered on a
given path. have a look at test-bus-objects.c which uses these
interfaces.

I've done some more experimentation and I think I've got a better idea
of what's going on. Sorry if my first email was a bit confusing.

My problem is that I couldn't get the sd-bus object manager to properly
notify clients when objects are added or removed.

I'm testing my sd-bus server with a gdbus client. The
GDBusObjectManagerClient should normally generate signals (gobject
signals, that is) when objects are added or removed.

Anyway, I tried implementing an object manager server with gdbus as
well, and when talking to the gdbus server, the client is notified of
object additions/removals, so I used used busctl to monitor the signals
that were generated. I think I've found the problem.

Assume there's an object manager at /org/example/mgr and that a new
object is created at /org/example/mgr/object1.

With sd-bus, I'd call:
sd_bus_emit_object_added(bus, /org/example/mgr/object1);

The generated signal would be:
‣ Type=signal  Endian=l  Flags=1  Version=1  Priority=0 Cookie=11
  Sender=:1.297  Path=/org/example/mgr/object1
Interface=org.freedesktop.DBus.ObjectManager  Member=InterfacesAdded
  UniqueName=:1.297
  MESSAGE oa{sa{sv}} {
  OBJECT_PATH /org/example/mgr/object1;

With gdbus, the generated signal is instead:
The generated signal would be:
‣ Type=signal  Endian=l  Flags=1  Version=1  Priority=0 Cookie=21
  Sender=:1.297  Path=/org/example/mgr
Interface=org.freedesktop.DBus.ObjectManager  Member=InterfacesAdded
  UniqueName=:1.297
  MESSAGE oa{sa{sv}} {
  OBJECT_PATH /org/example/mgr/object1;

Note the difference in the sender path.

The GDBusObjectManagerClient seems to ignore the signal sd-bus generates
because of the different path.

I think that means that sd_bus_emit_object_added() would need to take
both the object manager path and the object path as arguments instead of
a single path?

Regards,
David

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] sd-bus object manager question

[David Härdeman]

Hi,

as an experiment I've tried porting a toy dbus daemon over to sd-bus. It 
seems to be working well so far and I have to say I really like the 
sd-bus API over the gdbus one (sd-bus feels like I'm still writing 
C...can't say the same thing about gdbus).


Now, a question...how is an object manager supposed to be implemented in 
sd-bus?


I've seen that there's a sd_bus_add_object_manager() function in 
sd-bus.h, but how would I notify the object manager when objects are 
added and removed?


Two likely candidates seemed to be:
sd_bus_emit_object_added()
sd_bus_emit_object_removed()

But calling them did not seem to do the right thing. And there seems to 
be zero usage of sd_bus_add_object_manager() in the systemd tree itself 
(though plenty of calls to sd_bus_add_node_enumerator() which might be 
related?).


So, could anyone provide me with an example of how to use 
sd_bus_add_object_manager()?


Regards,
David

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Debian Bug#618862: systemd: ignores keyscript in crypttab - a possible solution

[David Härdeman]

On 2015-05-26 00:05, Alberto Bertogli wrote:
I hit this issue after upgrading a system that used keyscript to 
Jessie,

and it would no longer boot with systemd [1].

That led me to look into adding a password agent for my use case, 
and/or

creating a generic one that would invoke keyscripts as a workaround...



...

On Wed, Feb 05, 2014 at 12:16:00AM +0100, Lennart Poettering wrote:

On Thu, 30.01.14 10:40, David Härdeman (da...@hardeman.nu) wrote:
 b) the password agent implementation in systemd doesn't seem to
 handle binary strings (i.e. strings with '\0'), as can be seen by
 calls to e.g. strlen()...

 Whether making it binary safe would be a major change or not is
 something I haven't researched yet but it seems like a change that
 should be generally useful upstream...

I'd be OK with this, as discussed at FOSDEM, and I see you already
posted a ptach for this.


Has this been merged?


No, the last word was basically this thread:
http://lists.freedesktop.org/archives/systemd-devel/2014-July/021246.html

I don't have the time to implement a complete solution...


Is it safe for a password agent to write content with \0 back to the
socket?


Haven't checked but I'd be surprised if that was the case.

//David

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] gummiboot: freeing the wrong variable

[David Härdeman]

The error path in efivar_get free's what would have been
the copy of the string if the strcpy had succeeded, which
it did not (or we wouldn't be in the error path).

Signed-off-by: David Härdeman da...@hardeman.nu
---
 src/efi/util.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/efi/util.c b/src/efi/util.c
index 4715c57..7cb8e0f 100644
--- a/src/efi/util.c
+++ b/src/efi/util.c
@@ -102,7 +102,7 @@ EFI_STATUS efivar_get(CHAR16 *name, CHAR16 **value) {
 
 val = StrDuplicate((CHAR16 *)buf);
 if (!val) {
-FreePool(val);
+FreePool(buf);
 return EFI_OUT_OF_RESOURCES;
 }
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] [RFC] Add binary password agent protocol

[David Härdeman]

On Thu, Jul 03, 2014 at 01:41:43PM +0200, Lennart Poettering wrote:
On Fri, 27.06.14 01:54, David Härdeman (da...@hardeman.nu) wrote:

 Add binary string handling functions and extend the password agent
 protocol to support binary strings (using = as a string prefix
 instead of +).

I am feeling a bit uneasy about this one. I have the suspicion that the
entire password logic should be changed around: we should never transfer
the passwords in userspace, but use the kernel keyring for this. And the
queries should probably be triggered via dbus (as soon as kdbus is done,
and we can use dbus in early-boot). 

THis all makes me want to stay away from this for now. The kernel
keyring is binary-safe anyway, so half the problem goes away there. The
kernel also already has a key request API iirc (though a weird one, from
a cursory look), so we really should align ourselves a lot more with
that.

Sorry if this sounds disappointing, but I think the proper fix to get
binary passwords done is the kernel keyring, not just polishing what we
have right now.

No problem. Getting it right is part of the systemd philosophy...that
sometime takes longer

Anyhow, I've looked at the in-kernel keyring stuff before. Basically
userspace can request a key via a syscall (or in-kernel code can do
pretty much the same thing via a similar function call).

If the key is missing, a placeholder gets created and /sbin/request-key
is invoked to fill that placeholder with a proper key. The current
request-key uses config files under /etc to determine how to handle the
key request.

The key can then be properly constructed by the add_key sysctl (to write
the payload to the key).

As far as I can tell, this could replace the sockets that are currently
used in systemd, but not much more. All the information in the ask.
files would still need to be conveyed separately (e.g. by still creating
the ask. file and providing the path to the file as the callout_info
in request_key).

I'm guessing you'd like the dbus API to be a higher-level API that
userspace can use to get systemd to create the ask. file and call
request_key()? Are there any advantages of that redirection over doing
it straight away in the app?

Also, I guess this means that systemd would have to add a homegrown
version of /sbin/request-key (external dependencies for early boot does
not seem to be part of the systemd way of doing things)?

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH] [RFC] Add binary password agent protocol

[David Härdeman]

index 000..118e0aa
--- /dev/null
+++ b/src/shared/bstrv.c
@@ -0,0 +1,194 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+/***
+  This file is part of systemd.
+
+  Copyright 2014 David Härdeman da...@hardeman.nu
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or
+  (at your option) any later version.
+
+  systemd is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  Lesser General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public License
+  along with systemd; If not, see http://www.gnu.org/licenses/.
+***/
+
+#include assert.h
+#include stdlib.h
+#include stdarg.h
+#include string.h
+#include errno.h
+
+#include util.h
+#include strv.h
+#include bstrv.h
+
+struct bstr {
+size_t len;
+uint8_t data[];
+};
+
+static void bstr_free(bstr *l) {
+if (l) {
+/* FIXME: This should be something like explicit_bzero() */
+memzero(l-data, l-len);
+free(l);
+}
+}
+
+void bstrv_free(bstr **l) {
+bstr **k;
+
+if (!l)
+return;
+
+for (k = l; *k; k++)
+bstr_free(*k);
+
+free(l);
+}
+
+bstr **bstrv_new(const uint8_t *data, size_t len) {
+bstr **a;
+
+if (len  0)
+assert(data);
+
+a = new0(bstr *, 2);
+if (!a)
+return NULL;
+
+a[0] = malloc(sizeof(bstr) + len);
+if (!a[0]) {
+free(a);
+return NULL;
+}
+a[0]-len = len;
+memcpy(a[0]-data, data, len);
+
+return a;
+}
+
+bstr **bstrv_remove(bstr **l, const uint8_t *data, size_t len) {
+bstr **f, **t;
+
+if (!l)
+return NULL;
+
+if (len  0)
+assert(data);
+
+for (f = t = l; *f; f++) {
+if ((*f)-len == len 
+memcmp((*f)-data, data, len) == 0)
+bstr_free(*f);
+else
+*(t++) = *f;
+}
+
+*t = NULL;
+return l;
+}
+
+bstr **bstrv_uniq(bstr **l) {
+bstr **i;
+
+/* Drops duplicate entries. The first identical binary string will be
+ * kept, the others dropped */
+
+BSTRV_FOREACH(i, l)
+bstrv_remove(i + 1, (*i)-data, (*i)-len);
+
+return l;
+}
+
+int bstrv_push(bstr ***l, const uint8_t *data, size_t len) {
+bstr **c;
+bstr *k;
+size_t n;
+
+if (len  0)
+assert(data);
+
+k = malloc(sizeof(*k) + len);
+if (!k)
+return -ENOMEM;
+
+n = bstrv_length(*l);
+c = realloc(*l, sizeof(bstr *) * (n + 2));
+if (!c) {
+free(k);
+return -ENOMEM;
+}
+
+k-len = len;
+memcpy(k-data, data, len);
+
+c[n] = k;
+c[n + 1] = NULL;
+
+*l = c;
+return 0;
+}
+
+bstr **bstrv_parse_nulstr(const uint8_t *s, size_t l) {
+_cleanup_strv_free_ char **v = NULL;
+char **k;
+bstr **b = NULL;
+
+v = strv_parse_nulstr((const char *)s, l);
+if (!v)
+return NULL;
+
+STRV_FOREACH(k, v) {
+if (bstrv_push(b, (const uint8_t *)*k, strlen(*k))  0) {
+bstrv_free(b);
+return NULL;
+}
+}
+
+return b;
+}
+
+unsigned bstrv_length(bstr * const *l) {
+unsigned n = 0;
+
+if (!l)
+return 0;
+
+for (; *l; l++)
+n++;
+
+return n;
+}
+
+unsigned bstr_length(const bstr *l) {
+if (l)
+return l-len;
+return 0;
+}
+
+uint8_t *bstr_data(bstr *l) {
+if (l)
+return l-data;
+return NULL;
+}
+
+char *bstr_cdata(bstr *l) {
+return (char *)bstr_data(l);
+}
+
+bool bstreq(const bstr *a, const bstr *b) {
+if (!a || !b)
+return false;
+if (a-len != b-len)
+return false;
+return memcmp(a-data, b-data, a-len) == 0;
+}
+
diff --git a/src/shared/bstrv.h b/src/shared/bstrv.h
new file mode 100644
index 000..e08c215
--- /dev/null
+++ b/src/shared/bstrv.h
@@ -0,0 +1,50 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+  This file is part of systemd.
+
+  Copyright 2014 David Härdeman
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License

Re: [systemd-devel] Cache passphrase for cryptsetup?

[David Härdeman]

On Sun, Apr 20, 2014 at 09:55:11PM +0300, Mantas Mikulėnas wrote:
On 2014-04-20 21:45, Matthew Monaco wrote:
 And of course, the third option would be to submit a patch. The 
 src/cryptsetup
 stuff is pretty straightforward.

Wasn't one submitted just a month ago?

Yes, patches 1/3 and 2/3 were committed very recently and I still need
to post patch 3/3. Then a separate patch is necessary for the cryptsetup
package in Debian and after that, keyscript= will work for Debian at
least.

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 4/4] Add binary password agent protocol

[David Härdeman]

On Tue, Mar 25, 2014 at 01:58:27AM +0100, Lennart Poettering wrote:
On Tue, 04.02.14 00:57, David Härdeman (da...@hardeman.nu) wrote:

Sorry for the later review!

 Add binary string handling functions and extend the password agent
 protocol to support binary strings (using = as a string prefix
 instead of +).

Hmm, I wished there was a different way to implement this, rather than
having to introduce bstrv... But I see no other way, I fear.. :-(

(Well, we could always just pass an escaped string around, never the
binary data, and then only convert to binary data only as last step. But
I figure that would just lying to ourselves...)

I figure this has to do then. Could you please rebase to current git?

This one will take a bit longer. First I wanted to see that you agree
with the direction of the patch. Since you do, I'll spend the time to
write up some test cases before I resubmit this patch.

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 1/3] Add more password agent information

[David Härdeman]

On Tue, Mar 25, 2014 at 09:50:10AM +0100, Benjamin SANS wrote:
* On Tuesday, 25 March 2014 01:46, David Härdeman da...@hardeman.nu wrote:
 Bejamin's approach does not seem to solve the binary key part of the
 puzzle either...(passing binary keys from the keyscript, as opposed to
 passphrases).

Actually it does, but I'm not very proud of the fix...
Here is an explanation:

- When using a keyscript, the agent creates a temporary file like so:

char temp[] = /run/systemd/ask-password/tmp.XX;
int fd = mkostemp(temp, O_WRONLY|O_CLOEXEC);

- It then forks, redirect the standard output of the child to this temporary
  file, and execv the keyscript.

- Finally, it returns via the socket the path of this temporary file.

But all of this is based on the assumption that /run is a tmpfs and that it is
safe enough to temporary store the key.


Ah, mea culpa. I missed that you proposed changes to systemd's own
agent as well. Myopia, since my approach was to use an additional (new)
agent which only handles the keyscript= case, I just assumed you did as
well :)

BTW, it should be noted that since the agent API allows for concurrent
agents, it's still possible to use e.g. both a keyscript and keyboard
input as a backup with my approach...the systemd agent is smart enough
to remove the TTY passphrase prompt once an answer has been provided via
the other agent.

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/2] Fix keysize handling in cryptsetup (bits vs. bytes)

[David Härdeman]

The command line key-size is in bits but the libcryptsetup API expects bytes.

Note that the modulo 8 check is in the original cryptsetup binary as well, so
it's no new limitation.

(v2: changed the point at which the /= 8 is performed, rebased, removed tabs)
---
 src/cryptsetup/cryptsetup.c |   11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index a647a94..812b32f 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -88,6 +88,13 @@ static int parse_one_option(const char *option) {
 return 0;
 }
 
+if (arg_key_size % 8) {
+log_error(size= not a multiple of 8, ignoring.);
+return 0;
+}
+
+arg_key_size /= 8;
+
 } else if (startswith(option, key-slot=)) {
 
 arg_type = CRYPT_LUKS1;
@@ -414,7 +421,7 @@ static int attach_luks_or_plain(struct crypt_device *cd,
 /* for CRYPT_PLAIN limit reads
  * from keyfile to key length, and
  * ignore keyfile-size */
-arg_keyfile_size = arg_key_size / 8;
+arg_keyfile_size = arg_key_size;
 
 /* In contrast to what the name
  * crypt_setup() might suggest this
@@ -577,7 +584,7 @@ int main(int argc, char *argv[]) {
 else
 until = 0;
 
-arg_key_size = (arg_key_size  0 ? arg_key_size : 256);
+arg_key_size = (arg_key_size  0 ? arg_key_size : (256 / 8));
 
 if (key_file) {
 struct stat st;

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 0/2] Password agent fixes

[David Härdeman]

The following series contains patch 1 - 2 of the previous patchset that
I've posted, hopefully all of Lennart's comments should have been taken
into account (patch 3 was already applied in a reworked fashion).

I've left out the binary protocol patch until I've written unit tests.

---

David Härdeman (2):
  Add more password agent information
  Fix keysize handling in cryptsetup (bits vs. bytes)


 src/ask-password/ask-password.c |   14 +++---
 src/cryptsetup/cryptsetup.c |   25 +
 src/shared/ask-password-api.c   |9 +++--
 src/shared/ask-password-api.h   |6 --
 4 files changed, 43 insertions(+), 11 deletions(-)

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 1/2] Add more password agent information

[David Härdeman]

Add an (optional) Id key in the password agent .ask files. The Id is
supposed to be a simple string in subsystem:target form which
is used to provide more information on what the requested passphrase
is to be used for (which e.g. allows an agent to only react to cryptsetup
requests).

(v2: rebased, fixed indentation, escape name, use strappenda)
---
 src/ask-password/ask-password.c |   14 +++---
 src/cryptsetup/cryptsetup.c |   14 --
 src/shared/ask-password-api.c   |9 +++--
 src/shared/ask-password-api.h   |6 --
 4 files changed, 34 insertions(+), 9 deletions(-)

diff --git a/src/ask-password/ask-password.c b/src/ask-password/ask-password.c
index ea0c623..4d5690c 100644
--- a/src/ask-password/ask-password.c
+++ b/src/ask-password/ask-password.c
@@ -43,6 +43,7 @@
 #include def.h
 
 static const char *arg_icon = NULL;
+static const char *arg_id = NULL;
 static const char *arg_message = NULL;
 static bool arg_use_tty = true;
 static usec_t arg_timeout = DEFAULT_TIMEOUT_USEC;
@@ -58,7 +59,8 @@ static int help(void) {
 --timeout=SEC   Timeout in sec\n
 --no-ttyAsk question via agent even on TTY\n
 --accept-cached Accept cached passwords\n
---multiple  List multiple passwords if available\n,
+--multiple  List multiple passwords if available\n
+--id=ID Query identifier (e.g. 
cryptsetup:/dev/sda5)\n,
program_invocation_short_name);
 
 return 0;
@@ -71,7 +73,8 @@ static int parse_argv(int argc, char *argv[]) {
 ARG_TIMEOUT,
 ARG_NO_TTY,
 ARG_ACCEPT_CACHED,
-ARG_MULTIPLE
+ARG_MULTIPLE,
+ARG_ID
 };
 
 static const struct option options[] = {
@@ -81,6 +84,7 @@ static int parse_argv(int argc, char *argv[]) {
 { no-tty,no_argument,   NULL, ARG_NO_TTY
},
 { accept-cached, no_argument,   NULL, ARG_ACCEPT_CACHED 
},
 { multiple,  no_argument,   NULL, ARG_MULTIPLE  
},
+{ id,required_argument, NULL, ARG_ID
},
 {}
 };
 
@@ -119,6 +123,10 @@ static int parse_argv(int argc, char *argv[]) {
 arg_multiple = true;
 break;
 
+case ARG_ID:
+arg_id = optarg;
+break;
+
 case '?':
 return -EINVAL;
 
@@ -162,7 +170,7 @@ int main(int argc, char *argv[]) {
 } else {
 char **l;
 
-if ((r = ask_password_agent(arg_message, arg_icon, timeout, 
arg_accept_cached, l)) = 0) {
+if ((r = ask_password_agent(arg_message, arg_icon, arg_id, 
timeout, arg_accept_cached, l)) = 0) {
 char **p;
 
 STRV_FOREACH(p, l) {
diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index 9b9074c..a647a94 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -257,6 +257,8 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 int r;
 char **p;
 _cleanup_free_ char *text = NULL;
+_cleanup_free_ char *escaped_name = NULL;
+char *id;
 
 assert(name);
 assert(passwords);
@@ -264,7 +266,13 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 if (asprintf(text, Please enter passphrase for disk %s!, name)  0)
 return log_oom();
 
-r = ask_password_auto(text, drive-harddisk, until, accept_cached, 
passwords);
+escaped_name = cescape(name);
+if (!escaped_name)
+return log_oom();
+
+id = strappenda(cryptsetup:, escaped_name);
+
+r = ask_password_auto(text, drive-harddisk, id, until, 
accept_cached, passwords);
 if (r  0) {
 log_error(Failed to query password: %s, strerror(-r));
 return r;
@@ -278,7 +286,9 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 if (asprintf(text, Please enter passphrase for disk %s! 
(verification), name)  0)
 return log_oom();
 
-r = ask_password_auto(text, drive-harddisk, until, false, 
passwords2);
+id = strappenda(cryptsetup-verification:, escaped_name);
+
+r = ask_password_auto(text, drive-harddisk, id, until, 
false, passwords2);
 if (r  0) {
 log_error(Failed to query verification password: %s, 
strerror(-r));
 return r;
diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index eb40995..c3c78b6 100644
--- a/src/shared/ask-password-api.c
+++ 

Re: [systemd-devel] [PATCH 1/3] Add more password agent information

[David Härdeman]

On Tue, Mar 25, 2014 at 01:03:48AM +0100, Lennart Poettering wrote:
On Wed, 12.02.14 23:55, David Härdeman (da...@hardeman.nu) wrote:
 Add an (optional) Id key in the password agent .ask files. The Id is
 supposed to be a simple string in subsystem:target form which
 is used to provide more information on what the requested passphrase
 is to be used for (which e.g. allows an agent to only react to cryptsetup
 requests).

I wonder if this is related to the keyhandler stuff Benjamin Sans has
asked for.

I think Benjamin and I have basically both come up with the same
solution (though I haven't changed the option from keyscript= to
keyhandler= since that would break backwards compatibility...which is
partly the point of the whole exercise)...

Bejamin's approach does not seem to solve the binary key part of the
puzzle either...(passing binary keys from the keyscript, as opposed to
passphrases).

My proposed approach is provided in more detail in the corresponding
Debian bug report, see:
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=618862#44

So yes, I think they're related...as in they are independent
implementations of the same thing :)


http://lists.freedesktop.org/archives/systemd-devel/2014-March/017869.html

Benjamin, can you comment?

 -r = ask_password_auto(text, drive-harddisk, until, accept_cached, 
 passwords);
 +if (asprintf(id, cryptsetup:%s, name)  0)
 +return log_oom();
 +
 +r = ask_password_auto(text, drive-harddisk, id, until,
 accept_cached, passwords);

Hmm, no tabs please...

Also, please use strappend() for cases like this, where we just want to
concatenate two strings.

That said, I wodner, if we should escape the second part of the string,
just to be sure. Using cescape() here would suffice?

And risk breaking backward compatibility? (I was too lazy to check
exactly what cescape() handles right now)...

The name (second part of the string) comes from /etc/crypttab, which
is writeable by root only on any sane system? (otherwise it'd be dead
easy to insert a keyscript in between which does some keylogging).


  if (r  0) {
  log_error(Failed to query password: %s, strerror(-r));
  return r;
 @@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, 
 bool accept_cached, char
  if (asprintf(text, Please enter passphrase for disk %s! 
 (verification), name)  0)
  return log_oom();
  
 -r = ask_password_auto(text, drive-harddisk, until, false, 
 passwords2);
 +if (asprintf(id, cryptsetup-verification:%s, name)  0)
 +return log_oom();
 +

Similar here.

Otherwise this looks good to go, but I'd like to see a comment by
Benjamin, to see if this would work for him, too!

Lennart

-- 
Lennart Poettering, Red Hat


-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH 1/3] Add more password agent information

[David Härdeman]

On Tue, Mar 25, 2014 at 01:03:48AM +0100, Lennart Poettering wrote:
On Wed, 12.02.14 23:55, David Härdeman (da...@hardeman.nu) wrote:
 -r = ask_password_auto(text, drive-harddisk, until, accept_cached, 
 passwords);
 +if (asprintf(id, cryptsetup:%s, name)  0)
 +return log_oom();
 +
 +r = ask_password_auto(text, drive-harddisk, id, until,
 accept_cached, passwords);

Hmm, no tabs please...

I've told vim to respect your indentation :)

Also, please use strappend() for cases like this, where we just want to
concatenate two strings.

Hmmm, the asprinf use there matches the style of the code of the rest of
the functionfor example, with the patch applied that part reads:

if (asprintf(text, Please enter passphrase for disk %s!, name)  0)
return log_oom();

if (asprintf(id, cryptsetup:%s, name)  0)
return log_oom();

Changing the second asprintf to use strappend and cescape wouldn't
really make it more readable, would it?


  if (r  0) {
  log_error(Failed to query password: %s, strerror(-r));
  return r;
 @@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, 
 bool accept_cached, char
  if (asprintf(text, Please enter passphrase for disk %s! 
 (verification), name)  0)
  return log_oom();
  
 -r = ask_password_auto(text, drive-harddisk, until, false, 
 passwords2);
 +if (asprintf(id, cryptsetup-verification:%s, name)  0)
 +return log_oom();
 +

Similar here.

Ibid

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 0/3] Password agent fixes

[David Härdeman]

The following series contains patch 1 - 3 of the previous patchset that
I've posted. Lennart wanted a single Id= instead of the two Purpose=
and Target= strings in the .ask files, so I've changed it accordingly.

The two following patches are just small fixes.

I've left out the binary protocol patch for now.

---

David Härdeman (3):
  Add more password agent information
  Fix keysize handling in cryptsetup (bits vs. bytes)
  Fix askpass buffer overflow


 src/ask-password/ask-password.c |   14 +++---
 src/cryptsetup/cryptsetup.c |   19 +++
 src/shared/ask-password-api.c   |   14 --
 src/shared/ask-password-api.h   |6 --
 4 files changed, 42 insertions(+), 11 deletions(-)

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 3/3] Fix askpass buffer overflow

[David Härdeman]

Fix askpass overflow in reading a passphrase from a tty.
Doesn't seem security sensitive, but add a check for correctness.
---
 src/shared/ask-password-api.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index 1c18274..045cff2 100644
--- a/src/shared/ask-password-api.c
+++ b/src/shared/ask-password-api.c
@@ -213,6 +213,11 @@ int ask_password_tty(
 loop_write(ttyfd, *, 1, false);
 
 dirty = true;
+
+   if (p = (sizeof(passphrase) - 1)) {
+   loop_write(ttyfd, \n, 1, false);
+   break;
+   }
 }
 }
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 1/3] Add more password agent information

[David Härdeman]

Add an (optional) Id key in the password agent .ask files. The Id is
supposed to be a simple string in subsystem:target form which
is used to provide more information on what the requested passphrase
is to be used for (which e.g. allows an agent to only react to cryptsetup
requests).
---
 src/ask-password/ask-password.c |   14 +++---
 src/cryptsetup/cryptsetup.c |   11 +--
 src/shared/ask-password-api.c   |9 +++--
 src/shared/ask-password-api.h   |6 --
 4 files changed, 31 insertions(+), 9 deletions(-)

diff --git a/src/ask-password/ask-password.c b/src/ask-password/ask-password.c
index ea0c623..f939eab 100644
--- a/src/ask-password/ask-password.c
+++ b/src/ask-password/ask-password.c
@@ -43,6 +43,7 @@
 #include def.h
 
 static const char *arg_icon = NULL;
+static const char *arg_id = NULL;
 static const char *arg_message = NULL;
 static bool arg_use_tty = true;
 static usec_t arg_timeout = DEFAULT_TIMEOUT_USEC;
@@ -58,7 +59,8 @@ static int help(void) {
 --timeout=SEC   Timeout in sec\n
 --no-ttyAsk question via agent even on TTY\n
 --accept-cached Accept cached passwords\n
---multiple  List multiple passwords if available\n,
+--multiple  List multiple passwords if available\n
+--id=ID Query identifier (e.g. 
cryptsetup:/dev/sda5)\n,
program_invocation_short_name);
 
 return 0;
@@ -71,7 +73,8 @@ static int parse_argv(int argc, char *argv[]) {
 ARG_TIMEOUT,
 ARG_NO_TTY,
 ARG_ACCEPT_CACHED,
-ARG_MULTIPLE
+ARG_MULTIPLE,
+ARG_ID
 };
 
 static const struct option options[] = {
@@ -81,6 +84,7 @@ static int parse_argv(int argc, char *argv[]) {
 { no-tty,no_argument,   NULL, ARG_NO_TTY
},
 { accept-cached, no_argument,   NULL, ARG_ACCEPT_CACHED 
},
 { multiple,  no_argument,   NULL, ARG_MULTIPLE  
},
+{ id,required_argument, NULL, ARG_ID
},
 {}
 };
 
@@ -119,6 +123,10 @@ static int parse_argv(int argc, char *argv[]) {
 arg_multiple = true;
 break;
 
+   case ARG_ID:
+   arg_id = optarg;
+break;
+
 case '?':
 return -EINVAL;
 
@@ -162,7 +170,7 @@ int main(int argc, char *argv[]) {
 } else {
 char **l;
 
-if ((r = ask_password_agent(arg_message, arg_icon, timeout, 
arg_accept_cached, l)) = 0) {
+if ((r = ask_password_agent(arg_message, arg_icon, arg_id, 
timeout, arg_accept_cached, l)) = 0) {
 char **p;
 
 STRV_FOREACH(p, l) {
diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index 033c0cd..f72cf9f 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -260,6 +260,7 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 int r;
 char **p;
 _cleanup_free_ char *text = NULL;
+_cleanup_free_ char *id = NULL;
 
 assert(name);
 assert(passwords);
@@ -267,7 +268,10 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 if (asprintf(text, Please enter passphrase for disk %s!, name)  0)
 return log_oom();
 
-r = ask_password_auto(text, drive-harddisk, until, accept_cached, 
passwords);
+   if (asprintf(id, cryptsetup:%s, name)  0)
+   return log_oom();
+
+r = ask_password_auto(text, drive-harddisk, id, until, 
accept_cached, passwords);
 if (r  0) {
 log_error(Failed to query password: %s, strerror(-r));
 return r;
@@ -281,7 +285,10 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 if (asprintf(text, Please enter passphrase for disk %s! 
(verification), name)  0)
 return log_oom();
 
-r = ask_password_auto(text, drive-harddisk, until, false, 
passwords2);
+   if (asprintf(id, cryptsetup-verification:%s, name)  0)
+   return log_oom();
+
+r = ask_password_auto(text, drive-harddisk, id, until, 
false, passwords2);
 if (r  0) {
 log_error(Failed to query verification password: %s, 
strerror(-r));
 return r;
diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index a328f14..1c18274 100644
--- a/src/shared/ask-password-api.c
+++ b/src/shared/ask-password-api.c
@@ -298,6 +298,7 @@ fail:
 int ask_password_agent(
 const char *message,

[systemd-devel] [PATCH 2/3] Fix keysize handling in cryptsetup (bits vs. bytes)

[David Härdeman]

The command line key-size is in bits but the libcryptsetup API expects bytes.
---
 src/cryptsetup/cryptsetup.c |8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index f72cf9f..e7e8066 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -414,7 +414,7 @@ static int attach_luks_or_plain(struct crypt_device *cd,
 /* for CRYPT_PLAIN limit reads
  * from keyfile to key length, and
  * ignore keyfile-size */
-opt_keyfile_size = opt_key_size / 8;
+opt_keyfile_size = opt_key_size;
 
 /* In contrast to what the name
  * crypt_setup() might suggest this
@@ -577,7 +577,11 @@ int main(int argc, char *argv[]) {
 else
 until = 0;
 
-opt_key_size = (opt_key_size  0 ? opt_key_size : 256);
+if (opt_key_size % 8) {
+log_warning(Key size invalid (not a multiple of 8).);
+goto finish;
+}
+opt_key_size = (opt_key_size  0 ? opt_key_size : 256) / 8;
 
 if (key_file) {
 struct stat st;

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] Debian Bug#618862: systemd: ignores keyscript in crypttab - a possible solution

[David Härdeman]

On Wed, Feb 05, 2014 at 12:16:00AM +0100, Lennart Poettering wrote:
On Thu, 30.01.14 10:40, David Härdeman (da...@hardeman.nu) wrote:
 This issue is fixable with minor upstream changes, e.g. by extending
 the PasswordAgent protocol to add Subsystem=cryptsetup and
 Target=diskname entries to the ask. file.

I'd be fine with adding a field Id= or so, which then is filled by an
identifier of some kind be the cryptsetup tool that is useful to
identify the device to query things on. for example:
Id=cryptsetup:/dev/sda5 or so could be one way how this could be
filled in. We wouldn't enfoce any kind of syntax on this, just suggest
some common sense so that people choose identifiers that are unlikely to
clash with other subsystems, and somewhat reasonable to read...

In the patches I sent I split it into Purpose and Target and my
thinking was more or less the same as yours...i.e. that there's no
particular syntax and that the meaning of the string is subsystem
specific.

I'd be happy to change the patch to use Id=subsystem:target if
you'd prefer.

 b) the password agent implementation in systemd doesn't seem to
 handle binary strings (i.e. strings with '\0'), as can be seen by
 calls to e.g. strlen()...
 
 Whether making it binary safe would be a major change or not is
 something I haven't researched yet but it seems like a change that
 should be generally useful upstream...

I'd be OK with this, as discussed at FOSDEM, and I see you already
posted a ptach for this.

Yes. I take it you're pretty busy with the kdbus stuff right now but a
review of those patches would be nice when you have the time.

-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 0/4] Preparation series for binary passphrases

[David Härdeman]

The following series contains some preparatory patches to support
binary strings as passphrases when using password agents (my
goal is to implement support for the keyscript= parameter
for cryptsetup using an agent).

These patches are very lightly tested since I wanted to get some
feedback and see if the overall approach is considered ok or not first.

---

David Härdeman (4):
  Add more password agent information
  Fix keysize handling in cryptsetup (bits vs. bytes)
  Fix askpass buffer overflow
  Add binary password agent protocol


 Makefile.am|2 
 src/ask-password/ask-password.c|   53 +++--
 src/cryptsetup/cryptsetup.c|   72 ---
 src/shared/ask-password-api.c  |   74 
 src/shared/ask-password-api.h  |9 +
 src/shared/bstrv.c |  194 
 src/shared/bstrv.h |   50 +
 .../tty-ask-password-agent.c   |   11 +
 8 files changed, 364 insertions(+), 101 deletions(-)
 create mode 100644 src/shared/bstrv.c
 create mode 100644 src/shared/bstrv.h

-- 
David Härdeman

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 2/4] Fix keysize handling in cryptsetup (bits vs. bytes)

[David Härdeman]

The command line key-size is in bits but the libcryptsetup API expects bytes.
---
 src/cryptsetup/cryptsetup.c |8 ++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index 4a32856..c01ed01 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -407,7 +407,7 @@ static int attach_luks_or_plain(struct crypt_device *cd,
 /* for CRYPT_PLAIN limit reads
  * from keyfile to key length, and
  * ignore keyfile-size */
-opt_keyfile_size = opt_key_size / 8;
+opt_keyfile_size = opt_key_size;
 
 /* In contrast to what the name
  * crypt_setup() might suggest this
@@ -570,7 +570,11 @@ int main(int argc, char *argv[]) {
 else
 until = 0;
 
-opt_key_size = (opt_key_size  0 ? opt_key_size : 256);
+if (opt_key_size % 8) {
+log_warning(Key size invalid (not a multiple of 8).);
+goto finish;
+}
+opt_key_size = (opt_key_size  0 ? opt_key_size : 256) / 8;
 
 if (key_file) {
 struct stat st;

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 1/4] Add more password agent information

[David Härdeman]

Add (optional) Purpose and Target keys in the password agent .ask
files. These are used to provide more information on what the requested
passphrase is to be used for (which e.g. allows an agent to only listen
to cryptsetup requests).
---
 src/ask-password/ask-password.c |   22 +++---
 src/cryptsetup/cryptsetup.c |4 ++--
 src/shared/ask-password-api.c   |   13 +++--
 src/shared/ask-password-api.h   |6 --
 4 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/src/ask-password/ask-password.c b/src/ask-password/ask-password.c
index ea0c623..ca337b6 100644
--- a/src/ask-password/ask-password.c
+++ b/src/ask-password/ask-password.c
@@ -43,6 +43,8 @@
 #include def.h
 
 static const char *arg_icon = NULL;
+static const char *arg_purpose = NULL;
+static const char *arg_target = NULL;
 static const char *arg_message = NULL;
 static bool arg_use_tty = true;
 static usec_t arg_timeout = DEFAULT_TIMEOUT_USEC;
@@ -58,7 +60,9 @@ static int help(void) {
 --timeout=SEC   Timeout in sec\n
 --no-ttyAsk question via agent even on TTY\n
 --accept-cached Accept cached passwords\n
---multiple  List multiple passwords if available\n,
+--multiple  List multiple passwords if available\n
+--purpose=TXT   Passphrase subsystem/purpose, e.g. 
cryptsetup\n
+--target=TXTE.g. device to unlock, subsystem 
specific\n,
program_invocation_short_name);
 
 return 0;
@@ -71,7 +75,9 @@ static int parse_argv(int argc, char *argv[]) {
 ARG_TIMEOUT,
 ARG_NO_TTY,
 ARG_ACCEPT_CACHED,
-ARG_MULTIPLE
+ARG_MULTIPLE,
+ARG_PURPOSE,
+ARG_TARGET
 };
 
 static const struct option options[] = {
@@ -81,6 +87,8 @@ static int parse_argv(int argc, char *argv[]) {
 { no-tty,no_argument,   NULL, ARG_NO_TTY
},
 { accept-cached, no_argument,   NULL, ARG_ACCEPT_CACHED 
},
 { multiple,  no_argument,   NULL, ARG_MULTIPLE  
},
+{ purpose,   required_argument, NULL, ARG_PURPOSE   
},
+{ target,required_argument, NULL, ARG_TARGET
},
 {}
 };
 
@@ -119,6 +127,14 @@ static int parse_argv(int argc, char *argv[]) {
 arg_multiple = true;
 break;
 
+case ARG_PURPOSE:
+arg_purpose = optarg;
+break;
+
+case ARG_TARGET:
+arg_target = optarg;
+break;
+
 case '?':
 return -EINVAL;
 
@@ -162,7 +178,7 @@ int main(int argc, char *argv[]) {
 } else {
 char **l;
 
-if ((r = ask_password_agent(arg_message, arg_icon, timeout, 
arg_accept_cached, l)) = 0) {
+if ((r = ask_password_agent(arg_message, arg_icon, 
arg_purpose, arg_target, timeout, arg_accept_cached, l)) = 0) {
 char **p;
 
 STRV_FOREACH(p, l) {
diff --git a/src/cryptsetup/cryptsetup.c b/src/cryptsetup/cryptsetup.c
index 033c0cd..4a32856 100644
--- a/src/cryptsetup/cryptsetup.c
+++ b/src/cryptsetup/cryptsetup.c
@@ -267,7 +267,7 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 if (asprintf(text, Please enter passphrase for disk %s!, name)  0)
 return log_oom();
 
-r = ask_password_auto(text, drive-harddisk, until, accept_cached, 
passwords);
+r = ask_password_auto(text, drive-harddisk, cryptsetup, name, 
until, accept_cached, passwords);
 if (r  0) {
 log_error(Failed to query password: %s, strerror(-r));
 return r;
@@ -281,7 +281,7 @@ static int get_password(const char *name, usec_t until, 
bool accept_cached, char
 if (asprintf(text, Please enter passphrase for disk %s! 
(verification), name)  0)
 return log_oom();
 
-r = ask_password_auto(text, drive-harddisk, until, false, 
passwords2);
+r = ask_password_auto(text, drive-harddisk, cryptsetup, 
name, until, false, passwords2);
 if (r  0) {
 log_error(Failed to query verification password: %s, 
strerror(-r));
 return r;
diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index a328f14..553debc 100644
--- a/src/shared/ask-password-api.c
+++ b/src/shared/ask-password-api.c
@@ -298,6 +298,8 @@ fail:
 int ask_password_agent(
 const char *message,
 const char *icon,
+const char *purpose,
+const char *target,
   

[systemd-devel] [PATCH 3/4] Fix askpass buffer overflow

[David Härdeman]

Fix askpass overflow in reading a passphrase from a tty.
Doesn't seem security sensitive, but add a check for correctness.
---
 src/shared/ask-password-api.c |5 +
 1 file changed, 5 insertions(+)

diff --git a/src/shared/ask-password-api.c b/src/shared/ask-password-api.c
index 553debc..499ec84 100644
--- a/src/shared/ask-password-api.c
+++ b/src/shared/ask-password-api.c
@@ -213,6 +213,11 @@ int ask_password_tty(
 loop_write(ttyfd, *, 1, false);
 
 dirty = true;
+
+   if (p = (sizeof(passphrase) - 1)) {
+   loop_write(ttyfd, \n, 1, false);
+   break;
+   }
 }
 }
 

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

[systemd-devel] [PATCH 4/4] Add binary password agent protocol

[David Härdeman]

; indent-tabs-mode: nil -*-*/
+
+/***
+  This file is part of systemd.
+
+  Copyright 2014 David Härdeman da...@hardeman.nu
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License, or
+  (at your option) any later version.
+
+  systemd is distributed in the hope that it will be useful, but
+  WITHOUT ANY WARRANTY; without even the implied warranty of
+  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+  Lesser General Public License for more details.
+
+  You should have received a copy of the GNU Lesser General Public License
+  along with systemd; If not, see http://www.gnu.org/licenses/.
+***/
+
+#include assert.h
+#include stdlib.h
+#include stdarg.h
+#include string.h
+#include errno.h
+
+#include util.h
+#include strv.h
+#include bstrv.h
+
+struct bstr {
+size_t len;
+uint8_t data[];
+};
+
+void bstrv_free(bstr **l) {
+bstr **k;
+
+if (!l)
+return;
+
+for (k = l; *k; k++) {
+memzero((*k)-data, (*k)-len);
+free(*k);
+}
+
+printf(Freeing %p\n, l);
+free(l);
+}
+
+bstr **bstrv_new(const uint8_t *data, size_t len) {
+size_t n = 1;
+bstr **a;
+
+if (data  len  0)
+n++;
+
+a = new0(bstr *, n);
+if (!a)
+return NULL;
+
+if (data  len  0) {
+a[0] = malloc(sizeof(bstr) + len);
+if (!a[0]) {
+free(a);
+return NULL;
+}
+a[0]-len = len;
+memcpy(a[0]-data, data, len);
+}
+
+return a;
+}
+
+bstr **bstrv_remove(bstr **l, const uint8_t *data, size_t len) {
+bstr **f, **t;
+
+if (!l)
+return NULL;
+
+assert(len  0);
+assert(data);
+
+for (f = t = l; *f; f++) {
+if ((*f)-len == len 
+memcmp((*f)-data, data, len) == 0)
+free(*f);
+else
+*(t++) = *f;
+}
+
+*t = NULL;
+return l;
+}
+
+bstr **bstrv_uniq(bstr **l) {
+bstr **i;
+
+/* Drops duplicate entries. The first identical binary string will be
+ * kept, the others dropped */
+
+BSTRV_FOREACH(i, l)
+bstrv_remove(i + 1, (*i)-data, (*i)-len);
+
+return l;
+}
+
+int bstrv_push(bstr ***l, const uint8_t *data, size_t len) {
+bstr **c;
+bstr *k;
+size_t n;
+
+if (!data || len  1)
+return 0;
+
+k = malloc(sizeof(*k));
+if (!k)
+return -ENOMEM;
+printf(Allocated new bstr %p\n, k);
+
+n = bstrv_length(*l);
+printf(bstrv_length is %zu, reallocing\n, n);
+c = realloc(*l, sizeof(bstr *) * (n + 2));
+if (!c) {
+free(k);
+return -ENOMEM;
+}
+printf(bstrv is now %p\n, c);
+k-len = len;
+memcpy(k-data, data, len);
+
+c[n] = k;
+c[n + 1] = NULL;
+
+*l = c;
+return 0;
+}
+
+bstr **bstrv_parse_nulstr(const uint8_t *s, size_t l) {
+_cleanup_strv_free_ char **v = NULL;
+char **k;
+bstr **b = NULL;
+
+v = strv_parse_nulstr((const char *)s, l);
+if (!v)
+return NULL;
+
+STRV_FOREACH(k, v) {
+if (bstrv_push(b, (const uint8_t *)*k, strlen(*k))  0) {
+bstrv_free(b);
+return NULL;
+}
+}
+
+return b;
+}
+
+unsigned bstrv_length(bstr * const *l) {
+unsigned n = 0;
+
+if (!l)
+return 0;
+
+for (; *l; l++)
+n++;
+
+return n;
+}
+
+unsigned bstr_length(const bstr *l) {
+if (l)
+return l-len;
+return 0;
+}
+
+uint8_t *bstr_data(bstr *l) {
+if (l)
+return l-data;
+return NULL;
+}
+
+char *bstr_cdata(bstr *l) {
+return (char *)bstr_data(l);
+}
+
+bool bstreq(const bstr *a, const bstr *b) {
+if (!a || !b)
+return false;
+if (a-len != b-len)
+return false;
+return memcmp(a-data, b-data, a-len) == 0;
+}
+
diff --git a/src/shared/bstrv.h b/src/shared/bstrv.h
new file mode 100644
index 000..e08c215
--- /dev/null
+++ b/src/shared/bstrv.h
@@ -0,0 +1,50 @@
+/*-*- Mode: C; c-basic-offset: 8; indent-tabs-mode: nil -*-*/
+
+#pragma once
+
+/***
+  This file is part of systemd.
+
+  Copyright 2014 David Härdeman
+
+  systemd is free software; you can redistribute it and/or modify it
+  under the terms of the GNU Lesser General Public License as published by
+  the Free Software Foundation; either version 2.1 of the License

Re: [systemd-devel] Debian Bug#618862: systemd: ignores keyscript in crypttab - a possible solution

[David Härdeman]

On 2013-06-25 17:47, Michael Biebl wrote:

Am 25.06.2013 13:13, schrieb Harald Jenny:

Dear Michael Biebl,

following the systemd survey and discussion I think these mails might 
be

of interest to you concerning possible ways to solve the current issue
(not only in Debian but also SuSE/upstream interest).

http://lists.freedesktop.org/archives/systemd-devel/2012-June/thread.html#5693
http://lists.freedesktop.org/archives/systemd-devel/2012-July/thread.html#5835


I personally don't own such hardware, and I never have userd
cryptsetup's keyscript support. So I'm probably not the most qualified
to evaluate the situation.
That said, reading the upstream discussion, I guess we have 3 options
a/ do nothing about it
b/ apply the patch from David Härdeman downstream and maintaining it as
a downstream patch forever
c/ try to implement keyscript support based on the PasswordAgent 
interface


a/ is obviously not very compelling. As for b/, we try to avoid
downstream patches as much as possible.
Regarding c/, I dunno how much effort that would be.

Bringing David into the loop here. Maybe he has some further insight on
this matter.


I found some time to consider how to solve this and I think I have a 
pretty good solution that'd require a minimum amount of changes 
upstream.


What I've hacked together is:

First, a patch to the askpass binary in cryptsetup (the Debian 
package, not systemd's own stuff) so that it'll detect that systemd is 
running, in which case it'll use systemd's own password agent system for 
requesting a password from the user.


Second, a new systemd password agent. It waits for a password request 
from systemd's own cryptsetup implementation. The password agent then 
re-parses /etc/crypttab to find the corresponding entry and checks if it 
includes a keyscript= option. If no keyscript option is present the 
agent does nothing but if it *is* present, the agent recreates the 
environment created by the current cryptsetup scripts, launches the 
keyscript and sends the output back via the appropriate socket provided 
by systemd.


That the changes to askpass and the introduction of the new password 
agent are unrelated but both are necessary to not break existing 
keyscripts. askpass is used in keyscripts to get an actual key from 
the user. The password agent makes sure that systemd's own cryptsetup 
stuff honors the keyscript.


The new agent is not production ready yet (I plan to do some more work 
on it during FOSDEM), the two most important issues are:


a) getting the name of the cryptdev that the password request 
corresponds to currently involves parsing the prompt message (Please 
enter passphrase for disk %s!) which is obviously not a real 
solution...


This issue is fixable with minor upstream changes, e.g. by extending the 
PasswordAgent protocol to add Subsystem=cryptsetup and 
Target=diskname entries to the ask. file.


b) the password agent implementation in systemd doesn't seem to handle 
binary strings (i.e. strings with '\0'), as can be seen by calls to e.g. 
strlen()...


Whether making it binary safe would be a major change or not is 
something I haven't researched yet but it seems like a change that 
should be generally useful upstream...




Minor detail: the additional agent could be shipped either in (I have no 
real preference):


a) the cryptsetup package

b) as part of the Debian systemd package

c) upstream systemd


Feedback welcome.

Regards,
David

[1] http://www.freedesktop.org/wiki/Software/systemd/PasswordAgents/

___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel

Re: [systemd-devel] [PATCH] add keyscript support to cryptsetup

[David Härdeman]

On Tue, 10 Jul 2012 16:25:50 +0200, Lennart Poettering
lenn...@poettering.net wrote:
 On Tue, 10.07.12 10:35, David Härdeman (da...@hardeman.nu) wrote:
 Whenever a user comes up with another scheme for storing keys (you've
 already seen some...like storing keys between the MBR and first
 partition using the keyfile-offset= parameter), it is possible to ask
 them to use a keyscript= instead of extending the cryptsetup logic.
 
 Well, but if this is all dependent on some other hw then the synchronous
 nature of keyscript= doesn't work anyway... (see other mail about that)

I think you're making a much bigger issue out of the synchronous nature
than warranted.

In the current systemd solution, the systemd-native cryptsetup binary will
request a password using a /run/systemd/ask-password/ask.foobar file, then
wait until it gets one or more answers back via a socket. If some password
agent used e.g. a USB smartcard reader, it would naturally wait (exactly
how it waits is not that important) until said reader is available before
it could theoretically provide said answer. If the partition is necessary
for booting the system, the boot will wait until the key is provided and
the partition is mounted.

In the Debian cryptsetup init scripts, the keyscript will be executed and
the cryptsetup binary will wait until it gets an answer back.

Having keyscripts check and wait for certain /dev files to be present is
not much of an issue and it makes very little difference in the
asynchronous nature of the solution. The scsi_wait module and/or commands
like udevadm settle, etc, are not strictly necessary in a keyscript.

So, neither solution is, as far as I can tell, more asynchronous.

 Both yubikeys and most smartcard readers are USB devices, so you always
 have the enumeration issues, which means a synchronous solution wouldn't
 work wihtout races anyway and the async agent stuff is much preferable.

See above.

 http://www.freedesktop.org/wiki/Software/systemd/PasswordAgents
 
 I also dislike additional complexity and realize that systemd is an
 excellent oppurtinity of providing some much needed spring cleaning in
 distribution-specific boot scripts - but there are a few problems with
 using password agents:
 
 Well, the complexity comes for a reason: correctness.

I think you misunderstood me. I wasn't referring to systemd password
agents when I said complexity, I was referring to the legacy features
such as keyscript=.

 1) Backwards compatibility
 
 keyscript has been supported by the Debian cryptsetup package for a
 long time and people already have keyscripts. Some of these are used
for
 boot-critical partitions. Just telling everyone to rewrite whatever
they
 have as passwordagents is unlikely to be accepted.
 
 We do take the liberty to break compatibility  in some cases, where we
 believe the previous choice was broken or where we have really good
 reasons to. We do document them though (for example
 http://www.freedesktop.org/wiki/Software/systemd/Incompatibilities) so
 maybe that's one of those things where we should break the compat and
 document it?

That's fine by me. I understand that systemd has some ambitious goals wrt.
cruft removal and distribution consolidation, both of which I applaud. As
long a some other scheme is in place which allows similar functionality.

 Note that if compat is really that important it should be possible to
 write an agent that can invoke keyscripts like this. If you did that you
 would gain, among other things the ability to query for passwords at the
 same time via different methods. With a bit of C code it should be
 fairly easy to hack up an agent that just invokes a keyscript.

The biggest question in my mind right now is how the keyscript password
agent should be implemented and how it should interact with the other
agents.

Imagine the scenario where crypsetup wants to set up a partition using a
binary key from a smartcard, which in turn requires a PIN to be entered:

1) Following the current logic, the systemd cryptsetup binary would ask
for the key to the partition using /run/systemd/ask-password/ask.foo. (That
file could potentially be extended to add something like an Options=
field where we could provide additional information about the password
request to the agents).

2a) The keyscript agent would read the ask.foo file and determine that
it can provide the requested key from a smartcard (I assume this knowledge
comes from some config file)

2b) At the same time, the other agents provide normal password input
prompt(s) to the user (e.g. on the tty)...which are pointless since a
binary key is expected.

3) Soon afterwards, the keyscript agent in turn uses
/run/systemd/ask-password/ask.bar to request the PIN for the smartcard.

4) The user would be confronted with two passphrase prompts. One
(pointless) one to provide a binary key using the keyboard, and one (real)
prompt for the PIN to the smartcard.

I'm not sure what kind of changes to the password agent scheme

Re: [systemd-devel] [PATCH] add keyscript support to cryptsetup

[David Härdeman]

On Mon, Jul 09, 2012 at 10:49:56PM +0200, Lennart Poettering wrote:
On Fri, 29.06.12 00:56, David Härdeman (da...@hardeman.nu) wrote:

 Debian's cryptsetup package supports the keyscript= option in /etc/crypttab
 
 This patch is a first attempt at implementing support for the same option
 in systemd. It is not at exact feature parity yet (environment variables
 are missing and relative paths are not supported), but it's a start.
 
 I'm not sure if the (somewhat complicated dance) with fds is considered
 acceptable or if I should use something else?

Humpf. So I am really not convinced that supporting this is really such
a good idea. I am not a fan at all of this scriptlogic. (Starting with
the fact that this is called keyscript=, i.e. as if this really needs to
be a script...).

The name of the option (keyscript) doesn't mean that it needs to be e.g.
a bash script, it could be any executable.

I wonder what the precise usecases for this are, and whether we can't
find better solutions for these usecases... 

I originally implemented the keyscript= support in Debian, and the way I
see it there are two different usecases depending on your point of view.

As a package maintainer, or in this case systemd maintainer, the
keyscript= functionality was a good way of keeping bloat out of the
cryptsetup scripts (granted, some would probably say that the cryptsetup
scripts in Debian are already bloated, but without this functionality
they would be even more so).

Whenever a user comes up with another scheme for storing keys (you've
already seen some...like storing keys between the MBR and first
partition using the keyfile-offset= parameter), it is possible to ask
them to use a keyscript= instead of extending the cryptsetup logic.

From a user point of view it is of course additional flexibility which
is the usecase. I've seen keyscripts to use Yubikeys, keyscripts to get
keys off a smartcard (and the PIN for the smartcard could be requested
via systemd passwordagents or any other scheme), scripts which mount
different filesystems and grab the key off them, etc.

But yes, it might of course be possible to find a better
(non-backwards-compatible) solution. Starting with introducing support
for binary strings in PasswordAgents (see below).

I mean, we already have the password agent logic, that is asynchronous,
and way more powerful:

http://www.freedesktop.org/wiki/Software/systemd/PasswordAgents

I also dislike additional complexity and realize that systemd is an
excellent oppurtinity of providing some much needed spring cleaning in
distribution-specific boot scripts - but there are a few problems with
using password agents:

1) Backwards compatibility

keyscript has been supported by the Debian cryptsetup package for a
long time and people already have keyscripts. Some of these are used for
boot-critical partitions. Just telling everyone to rewrite whatever they
have as passwordagents is unlikely to be accepted.

2) PasswordAgents can't handle binary strings

keyscripts typically generate the key for the device and pass it to
stdout. PasswordAgents can't handle binary strings which contain NUL
characters (for example).

3) systemd specific solution

Converting keyscript= scripts to password agents introduce a strong
dependency on systemd. I realize that you don't consider it to be a
problem but I'm guessing it wouldn't be acceptable to Debian (where
systemd is not mandatory).


-- 
David Härdeman
___
systemd-devel mailing list
systemd-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/systemd-devel