checkpoint: preserve in-memory state of connectionsbg/rollback-in-memory

If a connection is in-memory (i.e. has flag "unsaved"), after a
checkpoint and rollback it can be wrongly persisted to disk:

 - if the connection was modified and written to disk after the
   rollback, during the rollback we update it again with persist mode
   "keep", which keeps it on disk;

 - if the connection was deleted after the rollback, during the
   rollback we add it again with persist mode "to-disk".

Instead, remember whether the connection had the "unsaved" flag set
and try to restore the previous state.

However, this is not straightforward as there are 4 different possible
states for the settings connection: persistent; in-memory only;
in-memory shadowing a persistent file; in-memory shadowing a detached
persistent file (i.e. the deletion of the connection doesn't delete
the persistent file). Handle all those cases.

Fixes: 3e09aed2a09f ('checkpoint: add create, rollback and destroy D-Bus API')

2 files changed, 205 insertions, 39 deletions

diff --git a/NEWS b/NEWS
index ef0cee8f69..05f67f716e 100644
--- a/NEWS
+++ b/NEWS
@@ -15,6 +15,8 @@ USE AT YOUR OWN RISK.  NOT RECOMMENDED FOR PRODUCTION USE!
   visible in nmcli via "nmcli -f all device show $DEV".
 * Deprecated 802-11-wireless and 802-11-wired property 'mac-address-blacklist'
   and introduced the 'mac-address-denylist' property.
+* Properly restore in-memory connection profiles during the rollback
+  of a checkpoint.
 * Fix detection of 6 GHz band capability for WiFi devices
 * Allow IPv6 SLAAC and static IPv6 DNS server assignment for modem broadband
   when IPv6 device address was not explicitly passed on by ModemManager
diff --git a/src/core/nm-checkpoint.c b/src/core/nm-checkpoint.c
index cc5c189bf9..ffcf6e3aad 100644
--- a/src/core/nm-checkpoint.c
+++ b/src/core/nm-checkpoint.c
@@ -10,6 +10,7 @@
 #include "nm-active-connection.h"
 #include "nm-act-request.h"
 #include "libnm-core-aux-intern/nm-auth-subject.h"
+#include "libnm-core-intern/nm-keyfile-internal.h"
 #include "nm-core-utils.h"
 #include "nm-dbus-interface.h"
 #include "devices/nm-device.h"
@@ -17,6 +18,7 @@
 #include "nm-manager.h"
 #include "settings/nm-settings.h"
 #include "settings/nm-settings-connection.h"
+#include "settings/plugins/keyfile/nms-keyfile-storage.h"
 #include "nm-simple-connection.h"
 #include "nm-utils.h"
 
@@ -29,11 +31,14 @@ typedef struct {
     NMDevice          *device;
     NMConnection      *applied_connection;
     NMConnection      *settings_connection;
+    NMConnection      *settings_connection_shadowed;
     guint64            ac_version_id;
     NMDeviceState      state;
     bool               is_software : 1;
     bool               realized : 1;
     bool               activation_lifetime_bound_to_profile_visibility : 1;
+    bool               settings_connection_is_unsaved : 1;
+    bool               settings_connection_is_shadowed_owned : 1;
     NMUnmanFlagOp      unmanaged_explicit;
     NMActivationReason activation_reason;
     gulong             dev_exported_change_id;
@@ -150,37 +155,111 @@ nm_checkpoint_includes_devices_of(NMCheckpoint *self, NMCheckpoint *cp_for_devic
     return NULL;
 }
 
+static NMConnection *
+parse_connection_from_shadowed_file(const char *path, GError **error)
+{
+    nm_auto_unref_keyfile GKeyFile *keyfile  = NULL;
+    gs_free char                   *base_dir = NULL;
+    char                           *sep;
+
+    keyfile = g_key_file_new();
+    if (!g_key_file_load_from_file(keyfile, path, G_KEY_FILE_NONE, error))
+        return NULL;
+
+    sep      = strrchr(path, '/');
+    base_dir = g_strndup(path, sep - path);
+
+    return nm_keyfile_read(keyfile, base_dir, NM_KEYFILE_HANDLER_FLAGS_NONE, NULL, NULL, error);
+}
+
 static NMSettingsConnection *
-find_settings_connection(NMCheckpoint     *self,
-                         DeviceCheckpoint *dev_checkpoint,
-                         gboolean         *need_update,
-                         gboolean         *need_activation)
+find_settings_connection(NMCheckpoint                    *self,
+                         DeviceCheckpoint                *dev_checkpoint,
+                         gboolean                        *need_update,
+                         gboolean                        *need_update_shadowed,
+                         gboolean                        *need_activation,
+                         NMSettingsConnectionPersistMode *persist_mode)
 {
     NMCheckpointPrivate  *priv = NM_CHECKPOINT_GET_PRIVATE(self);
     NMActiveConnection   *active;
     NMSettingsConnection *sett_conn;
+    const char           *shadowed_file;
+    NMConnection         *shadowed_connection = NULL;
     const char           *uuid, *ac_uuid;
     const CList          *tmp_clist;
-
-    *need_activation = FALSE;
-    *need_update     = FALSE;
+    gboolean              sett_conn_unsaved;
+    NMSettingsStorage    *storage;
+
+    *need_activation      = FALSE;
+    *need_update          = FALSE;
+    *need_update_shadowed = FALSE;
+
+    /* With regard to storage, there are 4 different possible states for the settings
+     * connection: 1) persistent; 2) in-memory only; 3) in-memory shadowing a persistent
+     * file; 4) in-memory shadowing a detached persistent file (i.e. the deletion of
+     * the connection doesn't delete the persistent file).
+     */
+    if (dev_checkpoint->settings_connection_is_unsaved) {
+        if (dev_checkpoint->settings_connection_shadowed) {
+            if (dev_checkpoint->settings_connection_is_shadowed_owned)
+                *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY;
+            else
+                *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_DETACHED;
+        } else
+            *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_IN_MEMORY_ONLY;
+    } else {
+        *persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK;
+    }
 
     uuid      = nm_connection_get_uuid(dev_checkpoint->settings_connection);
     sett_conn = nm_settings_get_connection_by_uuid(NM_SETTINGS_GET, uuid);
 
-    if (!sett_conn)
-        return NULL;
-
-    /* Now check if the connection changed, ... */
-    if (!nm_connection_compare(dev_checkpoint->settings_connection,
-                               nm_settings_connection_get_connection(sett_conn),
-                               NM_SETTING_COMPARE_FLAG_EXACT)) {
+    /* Check if the connection changed */
+    if (sett_conn
+        && !nm_connection_compare(dev_checkpoint->settings_connection,
+                                  nm_settings_connection_get_connection(sett_conn),
+                                  NM_SETTING_COMPARE_FLAG_IGNORE_TIMESTAMP)) {
         _LOGT("rollback: settings connection %s changed", uuid);
         *need_update     = TRUE;
         *need_activation = TRUE;
     }
 
-    /* ... is active, ... */
+    storage       = sett_conn ? nm_settings_connection_get_storage(sett_conn) : NULL;
+    shadowed_file = storage ? nm_settings_storage_get_shadowed_storage(storage, NULL) : NULL;
+    shadowed_connection =
+        shadowed_file ? parse_connection_from_shadowed_file(shadowed_file, NULL) : NULL;
+
+    if (dev_checkpoint->settings_connection_shadowed) {
+        if (!shadowed_connection
+            || !nm_connection_compare(dev_checkpoint->settings_connection_shadowed,
+                                      shadowed_connection,
+                                      NM_SETTING_COMPARE_FLAG_IGNORE_TIMESTAMP)) {
+            _LOGT("rollback: shadowed connection changed for %s", uuid);
+            *need_update_shadowed = TRUE;
+            *need_update          = TRUE;
+        }
+    } else {
+        if (shadowed_connection) {
+            _LOGT("rollback: shadowed connection changed for %s", uuid);
+            *need_update = TRUE;
+        }
+    }
+
+    if (!sett_conn)
+        return NULL;
+
+    /* Check if the connection unsaved flag changed */
+    sett_conn_unsaved = NM_FLAGS_HAS(nm_settings_connection_get_flags(sett_conn),
+                                     NM_SETTINGS_CONNECTION_INT_FLAGS_UNSAVED);
+    if (sett_conn_unsaved != dev_checkpoint->settings_connection_is_unsaved) {
+        _LOGT("rollback: storage changed for settings connection %s: unsaved (%d -> %d)",
+              uuid,
+              dev_checkpoint->settings_connection_is_unsaved,
+              sett_conn_unsaved);
+        *need_update = TRUE;
+    }
+
+    /* Check if the active state changed */
     nm_manager_for_each_active_connection (priv->manager, active, tmp_clist) {
         ac_uuid =
             nm_settings_connection_get_uuid(nm_active_connection_get_settings_connection(active));
@@ -196,7 +275,7 @@ find_settings_connection(NMCheckpoint     *self,
         return sett_conn;
     }
 
-    /* ... or if the connection was reactivated/reapplied */
+    /* Check if the connection was reactivated/reapplied */
     if (nm_active_connection_version_id_get(active) != dev_checkpoint->ac_version_id) {
         _LOGT("rollback: active connection version id of %s changed", uuid);
         *need_activation = TRUE;
@@ -212,12 +291,19 @@ restore_and_activate_connection(NMCheckpoint *self, DeviceCheckpoint *dev_checkp
     NMSettingsConnection           *connection;
     gs_unref_object NMAuthSubject  *subject     = NULL;
     GError                         *local_error = NULL;
-    gboolean                        need_update, need_activation;
+    gboolean                        need_update;
+    gboolean                        need_update_shadowed;
+    gboolean                        need_activation;
     NMSettingsConnectionPersistMode persist_mode;
     NMSettingsConnectionIntFlags    sett_flags;
     NMSettingsConnectionIntFlags    sett_mask;
 
-    connection = find_settings_connection(self, dev_checkpoint, &need_update, &need_activation);
+    connection = find_settings_connection(self,
+                                          dev_checkpoint,
+                                          &need_update,
+                                          &need_update_shadowed,
+                                          &need_activation,
+                                          &persist_mode);
 
     /* FIXME: we need to ensure to re-create/update the profile for the
      *   same settings plugin. E.g. if it was a keyfile in /run or /etc,
@@ -229,9 +315,26 @@ restore_and_activate_connection(NMCheckpoint *self, DeviceCheckpoint *dev_checkp
     sett_mask  = NM_SETTINGS_CONNECTION_INT_FLAGS_NONE;
 
     if (connection) {
+        if (need_update_shadowed) {
+            _LOGD("rollback: updating shadowed file for connection %s",
+                  nm_connection_get_uuid(dev_checkpoint->settings_connection));
+            nm_settings_connection_update(
+                connection,
+                NULL,
+                dev_checkpoint->settings_connection_shadowed,
+                NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK,
+                sett_flags,
+                sett_mask,
+                NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS
+                    | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET,
+                "checkpoint-rollback",
+                NULL);
+        }
+
         if (need_update) {
-            _LOGD("rollback: updating connection %s", nm_settings_connection_get_uuid(connection));
-            persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_KEEP;
+            _LOGD("rollback: updating connection %s with persist mode \"%s\"",
+                  nm_connection_get_uuid(dev_checkpoint->settings_connection),
+                  nm_settings_connection_persist_mode_to_string(persist_mode));
             nm_settings_connection_update(
                 connection,
                 NULL,
@@ -246,21 +349,54 @@ restore_and_activate_connection(NMCheckpoint *self, DeviceCheckpoint *dev_checkp
         }
     } else {
         /* The connection was deleted, recreate it */
-        _LOGD("rollback: adding connection %s again",
-              nm_connection_get_uuid(dev_checkpoint->settings_connection));
-
-        persist_mode = NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK;
-        if (!nm_settings_add_connection(NM_SETTINGS_GET,
-                                        NULL,
-                                        dev_checkpoint->settings_connection,
-                                        persist_mode,
-                                        NM_SETTINGS_CONNECTION_ADD_REASON_NONE,
-                                        sett_flags,
-                                        &connection,
-                                        &local_error)) {
-            _LOGD("rollback: connection add failure: %s", local_error->message);
-            g_clear_error(&local_error);
-            return FALSE;
+        if (need_update_shadowed) {
+            _LOGD("rollback: adding back shadowed file for connection %s",
+                  nm_connection_get_uuid(dev_checkpoint->settings_connection));
+
+            if (!nm_settings_add_connection(NM_SETTINGS_GET,
+                                            NULL,
+                                            dev_checkpoint->settings_connection_shadowed,
+                                            NM_SETTINGS_CONNECTION_PERSIST_MODE_TO_DISK,
+                                            NM_SETTINGS_CONNECTION_ADD_REASON_NONE,
+                                            sett_flags,
+                                            &connection,
+                                            &local_error)) {
+                _LOGD("rollback: connection add failure: %s", local_error->message);
+                g_clear_error(&local_error);
+                return FALSE;
+            }
+
+            _LOGD("rollback: updating connection %s with persist mode \"%s\"",
+                  nm_connection_get_uuid(dev_checkpoint->settings_connection),
+                  nm_settings_connection_persist_mode_to_string(persist_mode));
+
+            nm_settings_connection_update(
+                connection,
+                NULL,
+                dev_checkpoint->settings_connection,
+                persist_mode,
+                sett_flags,
+                sett_mask,
+                NM_SETTINGS_CONNECTION_UPDATE_REASON_RESET_SYSTEM_SECRETS
+                    | NM_SETTINGS_CONNECTION_UPDATE_REASON_UPDATE_NON_SECRET,
+                "checkpoint-rollback",
+                NULL);
+        } else {
+            _LOGD("rollback: adding back connection %s with persist mode \"%s\"",
+                  nm_connection_get_uuid(dev_checkpoint->settings_connection),
+                  nm_settings_connection_persist_mode_to_string(persist_mode));
+            if (!nm_settings_add_connection(NM_SETTINGS_GET,
+                                            NULL,
+                                            dev_checkpoint->settings_connection,
+                                            persist_mode,
+                                            NM_SETTINGS_CONNECTION_ADD_REASON_NONE,
+                                            sett_flags,
+                                            &connection,
+                                            &local_error)) {
+                _LOGD("rollback: connection add failure: %s", local_error->message);
+                g_clear_error(&local_error);
+                return FALSE;
+            }
         }
         need_activation = TRUE;
     }
@@ -362,11 +498,15 @@ nm_checkpoint_rollback(NMCheckpoint *self)
     while (g_hash_table_iter_next(&iter, (gpointer *) &device, (gpointer *) &dev_checkpoint)) {
         guint32 result = NM_ROLLBACK_RESULT_OK;
 
-        _LOGD("rollback: restoring device %s (state %d, realized %d, explicitly unmanaged %d)",
+        _LOGD("rollback: restoring device %s (state %d, realized %d, explicitly unmanaged %d, "
+              "connection-unsaved %d, connection-shadowed %d, connection-shadowed-owned %d)",
               dev_checkpoint->original_dev_name,
               (int) dev_checkpoint->state,
               dev_checkpoint->realized,
-              dev_checkpoint->unmanaged_explicit);
+              dev_checkpoint->unmanaged_explicit,
+              dev_checkpoint->settings_connection_is_unsaved,
+              !!dev_checkpoint->settings_connection_shadowed,
+              dev_checkpoint->settings_connection_is_shadowed_owned);
 
         if (nm_device_is_real(device)) {
             if (!dev_checkpoint->realized) {
@@ -518,6 +658,7 @@ device_checkpoint_destroy(gpointer data)
     g_clear_object(&dev_checkpoint->applied_connection);
     g_clear_object(&dev_checkpoint->settings_connection);
     g_clear_object(&dev_checkpoint->device);
+    g_clear_object(&dev_checkpoint->settings_connection_shadowed);
     g_free(dev_checkpoint->original_dev_path);
     g_free(dev_checkpoint->original_dev_name);
 
@@ -555,7 +696,7 @@ _dev_exported_changed(NMDBusObject *obj, NMCheckpoint *checkpoint)
 }
 
 static DeviceCheckpoint *
-device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device)
+device_checkpoint_create(NMCheckpoint *self, NMDevice *device)
 {
     DeviceCheckpoint     *dev_checkpoint;
     NMConnection         *applied_connection;
@@ -579,7 +720,7 @@ device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device)
     dev_checkpoint->dev_exported_change_id = g_signal_connect(device,
                                                               NM_DBUS_OBJECT_EXPORTED_CHANGED,
                                                               G_CALLBACK(_dev_exported_changed),
-                                                              checkpoint);
+                                                              self);
 
     if (nm_device_get_unmanaged_mask(device, NM_UNMANAGED_USER_EXPLICIT)) {
         dev_checkpoint->unmanaged_explicit =
@@ -589,6 +730,11 @@ device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device)
 
     act_request = nm_device_get_act_request(device);
     if (act_request) {
+        NMSettingsStorage    *storage;
+        gboolean              shadowed_owned = FALSE;
+        const char           *shadowed_file;
+        gs_free_error GError *error = NULL;
+
         settings_connection = nm_act_request_get_settings_connection(act_request);
         applied_connection  = nm_act_request_get_applied_connection(act_request);
 
@@ -602,6 +748,24 @@ device_checkpoint_create(NMCheckpoint *checkpoint, NMDevice *device)
         dev_checkpoint->activation_lifetime_bound_to_profile_visibility =
             NM_FLAGS_HAS(nm_active_connection_get_state_flags(NM_ACTIVE_CONNECTION(act_request)),
                          NM_ACTIVATION_STATE_FLAG_LIFETIME_BOUND_TO_PROFILE_VISIBILITY);
+
+        dev_checkpoint->settings_connection_is_unsaved =
+            NM_FLAGS_HAS(nm_settings_connection_get_flags(settings_connection),
+                         NM_SETTINGS_CONNECTION_INT_FLAGS_UNSAVED);
+
+        storage = nm_settings_connection_get_storage(settings_connection);
+        shadowed_file =
+            storage ? nm_settings_storage_get_shadowed_storage(storage, &shadowed_owned) : NULL;
+        if (shadowed_file) {
+            dev_checkpoint->settings_connection_is_shadowed_owned = shadowed_owned;
+            dev_checkpoint->settings_connection_shadowed =
+                parse_connection_from_shadowed_file(shadowed_file, &error);
+            if (!dev_checkpoint->settings_connection_shadowed) {
+                _LOGW("error reading shadowed connection file for %s: %s",
+                      nm_device_get_iface(device),
+                      error->message);
+            }
+        }
     }
 
     return dev_checkpoint;