Partial fix for systemdunitproperty probe

[jan-cerny]

This is an attempt to fix systemdunitproperty probe. This probe produces incomplete results. Some unit properties are not collected at all. The results are inconsistent with systemctl show <unit> command. It is reported in https://bugzilla.redhat.com/show_bug.cgi?id=1819773. This PR somehow improves the situation, but it doesn't make the results on par with systemctl show output.

I have identified 4 main problems in the probe:

1. All systemd unit objects implement the generic org.freedesktop.systemd1.Unit interface. But, depending on the unit type they also implement one unit-type-specific interface. For example, services implement org.freedesktop.systemd1.Unit and org.freedesktop.systemd1.Service interface. More on that on https://www.freedesktop.org/wiki/Software/systemd/dbus/ where you can find all the interfaces for all systemd unit types. However, our probe queries only the org.freedesktop.systemd1.Unit interface, which means that properties specific only for a certain unit type are not retrieved.

2. The code that converts the received dbus messages (function dbus_value_to_string) doesn't account for container types and complex types, s.a. structures or nested arrays. But some unit properties that are useful are complex. For example, ExecStart service property has a dbus signature a(sasbttttuii) which means it's an array of structures where each structure contain string, array, string, boolean, 4 64-bit unsigned integers, 1 32-bit unsigned integer and 2 32-bit unsigned integers. The dbus spec https://dbus.freedesktop.org/doc/dbus-specification.html has a lot of data types. If our code can't process them it skips the property and the property doesn't appear in results.

3. Our probe gets "raw' data from dbus and populate them into OVAL items directly, without any processing, because it doesn't know anything about their meaning. That's a difference from systemctl show command that analyzes data from dbus and knows their meaning. In the aforementioned example of ExecStart service property, the systemctl show command when it recieves the aforementioned array of structures, it knows which member of structure means what, so it can add descriptions for the data. Then, it looks like this: ExecStart={ path=/sbin/auditd ; argv[]=/sbin/auditd ; ignore_errors=no ; start_time=[Wed 2020-05-20 06:55:41 CEST] ; stop_time=[Wed 2020-05-20 06:55:41 CEST] ; pid=3319 ; code=exited ; status=0 } This is possible because systemctl has a special code for presenting values of this property nicely instead of bumping raw data from dbus. For example here: https://github.com/systemd/systemd/blob/766507972b2e8ae1616a6db39231e19e4663f873/src/systemctl/systemctl.c#L5061

4. We don't care about special values - infinity, undefined, timestamps and we collect them in raw form as they are retreived from dbus.

I tried to address ad:

1. We can easily query also the type-specific interface. I have added a code that detects the unit type and then calls GetAll on both Unit and type-specific interface.

2. I have extended the code in dbus_value_to_string to recurse into container types. I'm not sure how to transform them to value elements because we don't have nested structures within OVAL items. So at this moment it keeps using the original mechanism of creating a single string of comma separated items, which means flattening of the structures. Hopefully that's acceptable.

3. I haven't fixed this and I have no idea how to fix that without having specific code and hard-coded values for many properties. That would effectively be copying a lot of code from systemd and in turn terrible to maintain.

4. I have only changed the boolean values format to be consistent with systemctl. We use "true" and "false" but systemctl displays it as "yes" and "no". Other things are harder. For example, you have to know the meaning of the property that a property is a timestamp, because there isn't a dedicated signature for timestamps and it's all integers.

For more details please see the commit message of each commit and comments in code.

Any thoughts?

[evgenyz]

First of all, the definition from OVAL is absurd: ...For more information see the output generated by systemctl show $unit.... There is no information about what version of systemctl one should use to produce aforementioned output. Neither there is any information about how to produce all possible output that should be supported by the probe. Such circumstances make this probe an indefinitely running target, which would be only compatible with some versions of systemctl in every single moment. And after that they would not want to accept the yamlfilecontent because, you know, the YAML Path specification is not good enough. Just great! I think that both the definition and (because of the definition) the implementation of this test are just broken.

Anyhow, the OVAL asks us to, mm-hmm, look at the output generated by systemctl show $unit and behave in the same way. I don't see any other route to conform to the OVAL, except by copying parts of systemd and using them to do what original systemctl is doing.

Other way around it might be in fixing the OVAL definition to declare exactly how DBus signatures should be interpreted by the probe. And how the probe should handle container types (nested structures). Most of the tests in OVAL just forbid querying for containers (save simple arrays of scalars).

My opinions in case we would want to do exactly what definition requires us to do:

1. Great, that is the only non-controversial problem and fix. It is as good as it comes.

2. On one hand probe has to obey the rule of being "like systemctl" and return such structures as we could see them in systemctl's output. On the other hand, it seems the author of the systemdunitproperty forgot that collecting OVAL entities is not the same as printing something to the stdout. All objects the probe would like to collect and return it has to place in a corresponding OVAL structure. There is no way to return such a container in OVAL result, and so there is no mapping rules in the systemdunitproperty definition.
   Again, the only way to conform to the definition would be to return path=/sbin/auditd ; argv[]=/sbin/auditd ; ignore_errors=no ; start_time=[Wed 2020-05-20 06:55:41 CEST] ; stop_time=[Wed 2020-05-20 06:55:41 CEST] ; pid=3319 ; code=exited ; status=0 as a string for ExecStart, because it is what systemctl does.

3. It's pretty much the same as item 2, with the same reasoning.

4. It might seem as a good idea to return booleans as strings with values aligned with what systemctl outputs. But what the probe should do with numbers then; are we collecting integers as strings as well? Shouldn't we instead return OVAL booleans for DBus booleans?

It might help if we could answer this question: 'the output generated by systemctl show $unit' is all strings or there are some actual types to work with?

In reality, I would say that we should: keep 1), replace comma with semicolon in 2), don't add description and return /sbin/auditd; [/sbin/auditd]; no; 112312312312313; 11233453453454345; 3319; exited; 0 in 3), and do the best effort in representing data from DBus in a string form in 4), without knowing anything about its semantics.

We will be the only users of that probe, we will know what the OVAL entities would mean.

[jan-cerny]

@evgenyz Thanks for excellent analysis. It seems that at the moment when the specification was created the systemd didn't use the complex types and the approved specification was good enough, but unfortunately, it evolved.

I agree with your proposal, it's the cheapest thing that we can do at this moment.

[evgenyz]

How about proposing an update for this test? We could use record/field result as in yamlfilecontent, sql57 and ldap57 probes.

[jan-cerny]

Yes, we could.