A few random notes below just to start from something.

I think that the goal should be to log every event that changes the
persistent configuration or the current non-persistent status of NM
and networking. So every invocation of D-Bus methods or write to D-Bus
property should be included; this covers changes to connection
settings, other global settings like radio/networking/loglevel and
changes to connections/devices status. Maybe it would also be useful
to log reception of signals and event on file watches.

Audit log should not include other events that are not the direct
result of user intervention, like a connection going down due to link
change.

For each event we should log a timestamp, a description of the event
including relevant context information, the user who performed it and
the result. The context information should be limited to necessary; as
an example, for a change in connection settings the log should report
only the changed properties and their new values, together with the
connection name and uuid.

Since most of (all) these actions go through D-Bus, probably it would
be possible to log everything automatically (or everything we're
interested in, like a list of predefined methods and properties) by
logging the parameters of methods invocations and the new values of
properties. But perhaps this isn't so useful compared to having
specialized logging for each action, which could format a log entry
containing only the relevant information (see also the note below
about journald metadata).

Regarding the output and storage method for the log, someone (I don't
remember exactly who) suggested to use journald, which provides a
native API to save structured logs; each log entry can have, beside
the usual message, other metadata which are stored as a set of
key=>value tuples. So a modification of a connection setting could be
saved as:

    MESSAGE="Connection eth0-static modified by user root: ipv4.method=auto"
    CONNECTION_UUID=04aeacb0-b811-4cc4-9fbd-4b826eb5339f
    CONNECTION_NAME=eth0-static
    USER=root
    ACTION=modify-connection
    CHANGED_PROPERTIES='ipv4.method'='auto'

This additional log data can be used in searches and would be useful
to administrators to quickly identify the source of every change.

Using journald means linking against libsystemd which would become a
build time dependency; anyway we can add a configure option to choose
between journald and other methods as log storage. An alternative
output method can be a simple text file with one line per entry
formatted as the MESSAGE field of the journald example above.

Other ideas?

We should also evaluate how other services implement audit logging.

If we leverage journald, we should first implement a nm-logging backend that uses journald directly. That would be useful anyway and something we should do.

(In reply to Thomas Haller from comment #2)
> We should also evaluate how other services implement audit logging.

Some daemons as openssh, dbus, and libvirt interface with the audit
subsystem through libaudit to log information on system configuration
changes. For example this is the documentation of audit support in
libvirt:

  https://libvirt.org/auditlog.html

There is the possibility to enable or disable audit logging from
configuration and to choose to dispatch audit messages to the standard
logging layer as well (which supports multiple backends as syslog,
journald, log file); messages have a format which includes a type from
a predefined set and some fields to describe the reason of the change,
the resources involved and what changed.

I think this approach is interesting because it relies on the existing
audit framework which provides a centralized mechanism to store all
important system modifications and tools to analyze them; but it also
allows to fall back to normal logging.

> If we leverage journald, we should first implement a nm-logging backend that
> uses journald directly. That would be useful anyway and something we should
> do.

Yes, native journald support would be great to have in any case.

(In reply to Beniamino Galvani from comment #3)
> (In reply to Thomas Haller from comment #2)
> > We should also evaluate how other services implement audit logging.
> 
> Some daemons as openssh, dbus, and libvirt interface with the audit
> subsystem through libaudit to log information on system configuration
> changes. For example this is the documentation of audit support in
> libvirt:
> 
>   https://libvirt.org/auditlog.html
> 
> There is the possibility to enable or disable audit logging from
> configuration and to choose to dispatch audit messages to the standard
> logging layer as well (which supports multiple backends as syslog,
> journald, log file); messages have a format which includes a type from
> a predefined set and some fields to describe the reason of the change,
> the resources involved and what changed.
> 
> I think this approach is interesting because it relies on the existing
> audit framework which provides a centralized mechanism to store all
> important system modifications and tools to analyze them; but it also
> allows to fall back to normal logging.

Yes, using libaudit sounds good. And I agree, we should also duplicate the messages to our internal logging (configurable via a new logging domain LOGD_AUDIT).


> 
> > If we leverage journald, we should first implement a nm-logging backend that
> > uses journald directly. That would be useful anyway and something we should
> > do.
> 
> Yes, native journald support would be great to have in any case.

added that in bug 752136, branch th/logging-sd-journal-bgo752136

Branch bg/audit-bgo749364 introduces some audit primitives that can dispatch messages to different backends including libaudit and standard logging. It also adds audit log calls where there are relevant changes to the system configuration; probably they are still missing in some other places, but before proceeding I'd like to have some feedback on the general approach.

>> build: remove unneeded AC_SUBST macros after PKG_CHECK_MODULES

   The PKG_CHECK_MODULES macro shipped with modern versions (at least
    0.24) 

do we have this as a minimal requirement? How old is 0.24?

Can you rebase it on master and do the same for SYSTEMD_JOURNAL defines?




+ priv->audit_backends = g_key_file_get_string_list (priv->keyfile, NM_CONFIG_KEYFILE_GROUP_MAIN, "audit-backends", NULL, NULL);

_nm_utils_strv_cleanup()?




+    if (nm_config_data_get_audit_backends (old_data) != nm_config_data_get_audit_backends (new_data))
+         changes |= NM_CONFIG_CHANGE_AUDIT_BACKENDS;

Cannot compare char** with ==.



BACKEND_LOG:

shouldn't we enable nm-logging backend always, and control it via our usual mechanism (LOGD_AUDIT)?




in nm_audit_init(), could you refactor the two "if" like:


 #if HAVE_LIBAUDIT
     if (NM_FLAGS_HAS (audit_backends, BACKEND_AUDITD)) {
         if (audit_fd <= 0) {
         }
     } else {
         if (autid_fd > 0) {
         }
     }




+enum { /*< skip >*/

the enum is in a C-file, no need for <skip>. Anyway, it's fine too.

Maybe give the enum a name and use it instead of int?




/* g_ptr_array_insert() is only available since glib 2.40 */

include nm-glib-compat.h and it will work.



+ ...gulong pid, gulong uid,

+    if (pid > 0)
+         g_ptr_array_add (fields, FIELD_UINT ("pid", pid));
+    if (uid < G_MAXULONG)
+         g_ptr_array_add (fields, FIELD_UINT ("uid", uid));


there are types pid_t, uid_t,.. instead you accept gulong, and assign them in the end to guint field. I don't mind, uint might be large enough, but something consistent?




I think the audit-functions should be macros and pass also __FILE__, __LINE__ and func. They are useful for some backends (like BACKEND_LOG).


I would engage in some micro-optimization and make GPtrArray *fields stack-allocated... but an easy win seems to use g_slice_new() instead of g_new() for AuditField.


+    const char *str_value;
+    guint uint_value;
+    gboolean is_string;

the *_value; fields can be a union.
How about instead having a GValue there?

(In reply to Thomas Haller from comment #6)
> >> build: remove unneeded AC_SUBST macros after PKG_CHECK_MODULES
> 
>    The PKG_CHECK_MODULES macro shipped with modern versions (at least
>     0.24) 
> 
> do we have this as a minimal requirement? How old is 0.24?

More than 5 years:

[ ]     pkg-config-0.23.tar.gz  16-Jan-2008 14:54       1.0M
[ ]     pkg-config-0.24.tar.gz  23-May-2010 14:33       970K

If we need to support such old versions, I'll change the patch to add the AC_SUBSTs after PKG_CHECK_MODULES where they're missing.

Fixed all other points and repushed to bg/audit-bgo749364.

(In reply to Beniamino Galvani from comment #7)
> (In reply to Thomas Haller from comment #6)
> > >> build: remove unneeded AC_SUBST macros after PKG_CHECK_MODULES
> > 
> >    The PKG_CHECK_MODULES macro shipped with modern versions (at least
> >     0.24) 
> > 
> > do we have this as a minimal requirement? How old is 0.24?
> 
> More than 5 years:
> 
> [ ]     pkg-config-0.23.tar.gz  16-Jan-2008 14:54       1.0M
> [ ]     pkg-config-0.24.tar.gz  23-May-2010 14:33       970K
> 
> If we need to support such old versions, I'll change the patch to add the
> AC_SUBSTs after PKG_CHECK_MODULES where they're missing.

The only question here is which is the oldest version we want to support (I don't know). Then I agree with whatever fix follows (one way or the other).

But if unsure, I'd rather play it save and add the AC_SUBSTs.


>> core: introduce audit support

The whole audit-thing is not a GObject. I guess that is ok, it is in that sense similar to nm-logging.c and it might be actually less code.  Blame glib, needs so much boilerplate just to create a trivial class.

An alternative idea would be to have a singleton instance. I usually prefer that because it then it has more this feeling of individual objects that interact.

Anyway, fine with me as is (unless you want to change it).



#ifndef NM_AUDIT_H

I think we are not consistent about include guards. But I always add them in __NM_AUTID_H__ style.



We only support backend "audit" (and nm-logging). To me nm-logging + journal is already great, having libaudit too is even better. But do you really expect to ever add another backend? What could that be?

Maybe instead of having a configuration option main.audit-backends, add instead a section [auditd] with (for now) the only option enabled=yes|no.
Later we could add more options there -- especially for the auditd-backend.
That is then consistent with our [logging] section.

If you do that, how about passing NMConfigData directly to:

  nm_autid_init (const NMConfigData *config_data);

then nm_audit_init() gets all the relevant arguments out of @config_data -- instead of NMManager taking them out from NMConfigData and passing them explicitly to nm_autid_init().
-- note taht NMConfigData is immutable. If it is useful to you, you can safely keep a reference to that instance.


Please also explain the option in man/NM.conf.xml.in and add an commented-out example to contrib/rpm/fedora/NetworkManager.conf.
Ah, you do that in the next commit. Maybe just squash them as they really belong together?




I think what you said yesterday, about logging the individual fields to journal is really great. But that would imply that you split build_message() in build_message_auditd() and one for nm-logging. Hence, AuditField would have to become public...





Pushed minor fixups. Overall, looks great!!


As there is no object, can we wrap the global variables in a struct?
I always find it confusing to have individual global variables with no common name prefix. The @global_data struct effectively gives them all a namespace.

-static int audit_fd = -1;
-static AuditBackend enabled_backends;
+struct {
+   struct {
+        fd;
+   } auditd;
+   AuditBackend enabled_backends;
+} global_data = {
+    .auditd.fd = -1,
+};


And rename audit_fd to auditd_fd, to be consistent with BACKEND_AUDITD, "auditd"? Or the other way round... but consistent.


audit_field_init_str() and uint have a gboolean argument @backends, but you pass it an enum value. Also build_message() has an int typed backend argument.

_nm_audit_log_device_op() should also have the ifindex. I think that one is more deterministic then the interface name (you can rename an interface).
Similar to _nm_audit_log_connection_op(): can we add "id" too?





Ah, I see, from here the gulong type originates.

»···»···val = nm_auth_subject_get_unix_process_pid (subject);
»···»···if (val != G_MAXULONG)
»···»···»···pid = val;

»···»···val = nm_auth_subject_get_unix_process_uid (subject);
»···»···if (val != G_MAXULONG)
»···»···»···uid = val;


So, sorry, the original type of uid/pid was never uid_t/pid_t, but it is gulong. I think the point was to stick to the type that you got originally
and ensure you don't loose precision along the way.
Also, there are only two users of audit_field_init_uint() and both have originally gulong. Maybe you need instead audit_field_init_ulong()?




nm_audit_log():

»···if (NM_FLAGS_HAS (enabled_backends, BACKEND_LOG)) {
should be:
    level = success ? LOGL_INFO : LOGL_ERR;
    if (nm_logging_enabled (level, LOGD_AUDIT)) {



I think, there should also be a function nm_audit_enabled() which 
translates to:

   static inline gboolean
   nm_audit_enabled (gboolean success)
   {
       return nm_logging_enabled (success ? INFO: ERR, LOGD_AUDIT)
#if HAVE_LIBAUDIT
             || (NM_FLAGS_HAS (enabled_backends, BACKEND_AUDITD) && audit_fd >= 0)
#endif
             ;
   }


and check it both inside _nm_audit_log_device_op() and in the macros nm_audit_log_device_op() to avoid even evaluating the statement (like done for nm_log() macro).



Is a plain "gboolean success" enough? Don't we need an level enum, with some more granular severity levels? (asking, because I am not familiar with audit-logging). Can we reuse NMLogLevel or the syslog-levels?

(In reply to Thomas Haller from comment #8)
> The only question here is which is the oldest version we want to support (I
> don't know). Then I agree with whatever fix follows (one way or the other).
>
> But if unsure, I'd rather play it save and add the AC_SUBSTs.

All major distros released since 2011 seem to be using at least 0.24,
so after discussing on IRC I think we're going to remove those macros
if you agree.

> >> core: introduce audit support
> An alternative idea would be to have a singleton instance. I usually prefer
> that because it then it has more this feeling of individual objects that
> interact.

I repushed the branch and now it defines a singleton NMAuditManager GObject,
I like this approach.

> We only support backend "audit" (and nm-logging). To me nm-logging + journal
> is already great, having libaudit too is even better. But do you really
> expect to ever add another backend? What could that be?

I don't know, but probably we won't need any other backends, so maybe
the audit-backends= option isn't so useful :)

> Maybe instead of having a configuration option main.audit-backends, add
> instead a section [auditd] with (for now) the only option enabled=yes|no.
> Later we could add more options there -- especially for the auditd-backend.
> That is then consistent with our [logging] section.

Or maybe an [audit] section with the option enable-auditd=yes|no, so
that we can put all options related to audit in it, including the
auditd-specific ones, or other future backends?

> Please also explain the option in man/NM.conf.xml.in and add an
> commented-out example to contrib/rpm/fedora/NetworkManager.conf.
> Ah, you do that in the next commit. Maybe just squash them as they really
> belong together?

Ok.

> I think what you said yesterday, about logging the individual fields to
> journal is really great. But that would imply that you split build_message()
> in build_message_auditd() and one for nm-logging. Hence, AuditField would
> have to become public...

I found that journald already has support for collecting audit
messages from auditd and recording all the audit fields in the log. So if
you have journald logging backend and auditd support enabled, an entry
like the following will be saved to the journal for every NM audit
message:

  {
      "_UID" : "0",
      "_TRANSPORT" : "audit",
      "SYSLOG_FACILITY" : "32",
      "SYSLOG_IDENTIFIER" : "audit",
      "AUDIT_FIELD_RES" : "success",
      "_AUDIT_TYPE" : "1111",
      "AUDIT_FIELD_RESULT" : "success",
      "AUDIT_FIELD_EXE" : "/usr/sbin/NetworkManager",
      "AUDIT_FIELD_OP" : "connection-delete",
      "AUDIT_FIELD_UUID" : "367d636f-a8bb-49c6-b2ab-942f04c00532",
      "AUDIT_FIELD_NAME" : "eth0-static",
      "AUDIT_FIELD_PID" : "9355",
      "AUDIT_FIELD_UID" : "0",
      "_AUDIT_ID" : "779",
      "MESSAGE" : "<audit-1111> pid=8825 uid=0 msg='op=connection-delete uuid=367d636f-a8bb-49c6-b2ab-942f04c00532 name=\"eth0-static\" pid=9355 uid=0 result=success
  }

I think we don't need to reimplement this.

> Pushed minor fixups. Overall, looks great!!

Squashed them, thanks.

> audit_field_init_str() and uint have a gboolean argument @backends, but you
> pass it an enum value. Also build_message() has an int typed backend
> argument.

Fixed.

> _nm_audit_log_device_op() should also have the ifindex. I think that one is
> more deterministic then the interface name (you can rename an interface).
> Similar to _nm_audit_log_connection_op(): can we add "id" too?

Fixed.

> Ah, I see, from here the gulong type originates.
>
> »···»···val = nm_auth_subject_get_unix_process_pid (subject);
> »···»···if (val != G_MAXULONG)
> »···»···»···pid = val;
>
> »···»···val = nm_auth_subject_get_unix_process_uid (subject);
> »···»···if (val != G_MAXULONG)
> »···»···»···uid = val;
>
>
> So, sorry, the original type of uid/pid was never uid_t/pid_t, but it is
> gulong. I think the point was to stick to the type that you got originally
> and ensure you don't loose precision along the way.
> Also, there are only two users of audit_field_init_uint() and both have
> originally gulong. Maybe you need instead audit_field_init_ulong()?

An uint should be enough for both the uid and the pid fields, right?
Probably nm_auth_*() functions return a gulong because it can be set
to G_MAXULONG to signal the "missing/invalid" value, but we don't need
this as we can omit the field.

> nm_audit_log():
>
> »···if (NM_FLAGS_HAS (enabled_backends, BACKEND_LOG)) {
> should be:
>     level = success ? LOGL_INFO : LOGL_ERR;
>     if (nm_logging_enabled (level, LOGD_AUDIT)) {

Yes.

> I think, there should also be a function nm_audit_enabled() which
> translates to:

Fixed as well in bg/audit-bgo749364, thanks!

(In reply to Thomas Haller from comment #8)
> Is a plain "gboolean success" enough? Don't we need an level enum, with some
> more granular severity levels? (asking, because I am not familiar with
> audit-logging). Can we reuse NMLogLevel or the syslog-levels?

This is modeled after auditd APIs, which have a boolean result
parameter to indicate the outcome of the operation. IMO this is fine,
since we can't define different level of importance for audit
messages, we only have successful or failed operations.

> core: add audit support

Maybe the [audit] section should get merged into the [logging] section?  Unless we think there might be more stuff in the [audit] section in the future...

Other than that, the rest looks OK to me once you and thaller have agreed on the API.  Though I'm OK with simpler "success" type for now.

(In reply to Beniamino Galvani from comment #10)
> (In reply to Thomas Haller from comment #8)
> > Is a plain "gboolean success" enough? Don't we need an level enum, with some
> > more granular severity levels? (asking, because I am not familiar with
> > audit-logging). Can we reuse NMLogLevel or the syslog-levels?
> 
> This is modeled after auditd APIs, which have a boolean result
> parameter to indicate the outcome of the operation. IMO this is fine,
> since we can't define different level of importance for audit
> messages, we only have successful or failed operations.

Ok, sounds good then. 



#define AUDIT_LOGL_SUCCESS     LOGL_INFO
#define AUDIT_LOGL_FAIL        LOGL_ERR

an <err> logging for FAIL seems overly drastic. I think <err> should really indicate a serious issue -- maybe even be reserved for bugs.

Maybe just <warn>? Then, again, even <warn> seems too severe. For example, we would audit-log a FAIL message if a user has no permission to deactivate a connection. Seems quite drastic, that an unprivileged user can trigger <warn> messages. 

(I really like grepping through the log and find almost no <warn> messages)

Maybe the "success" argument of the audit-log, doesn't correspond to a NMLogLevel at all and we should do audit-logging in general with INFO level?

Similarly,

gboolean nm_audit_manager_audit_enabled (NMAuditManager *self, gboolean result);

maybe, whether audit is enabled, doesn't really depend on @result (but only on #if WITH_LIBAUDIT || nm_logging_enabled(INFO,AUDIT)).
I don't see why somebody would like to disable AUDIT_LOGL_SUCCESS, but enable AUDIT_LOGL_FAIL messages... they seem both equally interesting.






(In reply to Dan Williams from comment #11)
> > core: add audit support
> 
> Maybe the [audit] section should get merged into the [logging] section? 
> Unless we think there might be more stuff in the [audit] section in the
> future...

I slightly prefer that too, but I'd be fine with an [audit] section too.

Sorry about the bike shedding, but how about rename "enable-auditd"? NM doesn't "enable" it, it merely uses it. How about "use-auditd" or simply "auditd", or possibly "audit" without the 'd'?

I'd even go with:

[logging]
audit=yes|no



Pushed 4 fixup commits (if you choose to keep them), but IMO it's good.
ACK

(In reply to Thomas Haller from comment #12)

> Maybe the "success" argument of the audit-log, doesn't correspond to a
> NMLogLevel at all and we should do audit-logging in general with INFO level?

Yes, I agree, all messages should have LOGL_INFO.

(In reply to Thomas Haller from comment #13)
> I'd even go with:
> 
> [logging]
> audit=yes|no

Seems reasonable.

> Pushed 4 fixup commits (if you choose to keep them), but IMO it's good.
> ACK

Thanks, they all look good. I pushed some fixups to rename the keyfile option and uniform the log level; and an additional commit to add CAP_AUDIT_WRITE to the systemd service file.

Recent changes LGTM, lets merge this :)

Merged to master: 7c38b784632ecc490f7440a286dc81b452dc6213