keyfile: don't require verified profile in nm_keyfile_write()

Previously, only the daemon was writing keyfiles, and it ensures
that they are always valid.

As we now have this function as public API of libnm, we should drop this
restriction and write the profile the best we can. Granted, an invalid
profile may not be expressed in keyfile format, and the result is
undefined. But make the best of it.

2 files changed, 126 insertions, 51 deletions

diff --git a/src/libnm-core-impl/nm-keyfile.c b/src/libnm-core-impl/nm-keyfile.c
index 08539beaad..00fb8a335d 100644
--- a/src/libnm-core-impl/nm-keyfile.c
+++ b/src/libnm-core-impl/nm-keyfile.c
@@ -168,13 +168,13 @@ _nm_printf(5, 6) static void _read_handle_warn(KeyfileReaderInfo    *info,
 
 /*****************************************************************************/
 
-_nm_unused _nm_printf(6, 7) static void _write_handle_warn(KeyfileWriterInfo    *info,
-                                                           NMSetting            *setting,
-                                                           const char           *kf_key,
-                                                           const char           *cur_property,
-                                                           NMKeyfileWarnSeverity severity,
-                                                           const char           *fmt,
-                                                           ...)
+_nm_printf(6, 7) static void _write_handle_warn(KeyfileWriterInfo    *info,
+                                                NMSetting            *setting,
+                                                const char           *kf_key,
+                                                const char           *cur_property,
+                                                NMKeyfileWarnSeverity severity,
+                                                const char           *fmt,
+                                                ...)
 {
     NMKeyfileHandlerData handler_data;
 
@@ -3929,14 +3929,8 @@ write_setting_value(KeyfileWriterInfo        *info,
 
     if (!pip) {
         if (!setting_info) {
-            /* the setting type is unknown. That is highly unexpected
-             * (and as this is currently only called from NetworkManager
-             * daemon, not possible).
-             *
-             * Still, handle it gracefully, because later keyfile writer will become
-             * public API of libnm, where @setting is (untrusted) user input.
-             *
-             * Gracefully here just means: ignore the setting. */
+            /* the setting type is unknown. Handle this gracefully by
+             * ignoring the setting. */
             return;
         }
         if (!property_info->param_spec)
@@ -4154,8 +4148,10 @@ _write_setting_wireguard(NMSetting *setting, KeyfileWriterInfo *info)
  * @user_data: argument for @handler.
  * @error: the #GError in case writing fails.
  *
- * @connection must verify as a valid profile according to
- * nm_connection_verify().
+ * @connection should verify as a valid profile according to
+ * nm_connection_verify(). If it does not verify, the keyfile may
+ * be incomplete and the parser may not be able to fully recreate
+ * the original profile.
  *
  * Returns: (transfer full): a new #GKeyFile or %NULL on error.
  *
@@ -4169,7 +4165,6 @@ nm_keyfile_write(NMConnection         *connection,
                  GError              **error)
 {
     nm_auto_unref_keyfile GKeyFile *keyfile = NULL;
-    GError                         *local   = NULL;
     KeyfileWriterInfo               info;
     NMSetting                     **settings;
     int                             i;
@@ -4179,28 +4174,6 @@ nm_keyfile_write(NMConnection         *connection,
     g_return_val_if_fail(!error || !*error, NULL);
     g_return_val_if_fail(handler_flags == NM_KEYFILE_HANDLER_FLAGS_NONE, NULL);
 
-    /* Technically, we might not require that a profile is valid in
-     * order to serialize it. Like also nm_keyfile_read() does not
-     * ensure that the read profile validates.
-     *
-     * However, if the profile does not validate, then there might be
-     * unexpected edge cases when we try to serialize it. Edge cases
-     * that might result in dangerous crash.
-     *
-     * So, for now we require valid profiles. */
-    if (!nm_connection_verify(connection, error ? &local : NULL)) {
-        if (error) {
-            g_set_error(error,
-                        NM_CONNECTION_ERROR,
-                        NM_CONNECTION_ERROR_FAILED,
-                        _("the profile is not valid: %s"),
-                        local->message);
-            g_error_free(local);
-        } else
-            nm_assert(!local);
-        return NULL;
-    }
-
     keyfile = g_key_file_new();
 
     info = (KeyfileWriterInfo){
@@ -4254,11 +4227,16 @@ nm_keyfile_write(NMConnection         *connection,
                                               key,
                                               (guint64) g_variant_get_uint32(v));
                     } else {
-                        /* BUG: The variant type is not implemented. Since the connection
-                         * verifies, this can only mean we either wrongly didn't reject
-                         * the connection as invalid, or we didn't properly implement the
-                         * variant type. */
-                        nm_assert_not_reached();
+                        if (!write_handle_warn(&info,
+                                               setting,
+                                               NULL,
+                                               key,
+                                               NM_KEYFILE_WARN_SEVERITY_WARN,
+                                               _("unsupported option \"%s.%s\" of variant type %s"),
+                                               setting_name,
+                                               key,
+                                               g_variant_get_type_string(v)))
+                            goto out_with_info_error;
                         continue;
                     }
                 }
diff --git a/src/libnm-core-impl/tests/test-keyfile.c b/src/libnm-core-impl/tests/test-keyfile.c
index 9bd13ffc30..c163c4293b 100644
--- a/src/libnm-core-impl/tests/test-keyfile.c
+++ b/src/libnm-core-impl/tests/test-keyfile.c
@@ -5,16 +5,18 @@
 
 #include "libnm-core-impl/nm-default-libnm-core.h"
 
-#include "libnm-glib-aux/nm-json-aux.h"
-#include "libnm-core-intern/nm-keyfile-utils.h"
+#include "libnm-base/nm-ethtool-utils-base.h"
 #include "libnm-core-intern/nm-keyfile-internal.h"
-#include "nm-simple-connection.h"
-#include "nm-setting-connection.h"
-#include "nm-setting-wired.h"
+#include "libnm-core-intern/nm-keyfile-utils.h"
+#include "libnm-glib-aux/nm-json-aux.h"
 #include "nm-setting-8021x.h"
+#include "nm-setting-connection.h"
+#include "nm-setting-ethtool.h"
+#include "nm-setting-proxy.h"
 #include "nm-setting-team.h"
 #include "nm-setting-user.h"
-#include "nm-setting-proxy.h"
+#include "nm-setting-wired.h"
+#include "nm-simple-connection.h"
 
 #include "libnm-glib-aux/nm-test-utils.h"
 
@@ -887,6 +889,100 @@ test_bridge_port_vlans(void)
 
 /*****************************************************************************/
 
+typedef struct {
+    bool  expect;
+    guint n_calls;
+} InvalidOptionWriteData;
+
+static gboolean
+_invalid_option_write_handler(NMConnection         *connection,
+                              GKeyFile             *keyfile,
+                              NMKeyfileHandlerType  handler_type,
+                              NMKeyfileHandlerData *handler_data,
+                              void                 *user_data)
+{
+    InvalidOptionWriteData *data = user_data;
+    const char             *message;
+    NMKeyfileWarnSeverity   severity;
+
+    g_assert(data);
+    g_assert(data->expect);
+
+    g_assert(data->n_calls == 0);
+    data->n_calls++;
+
+    switch (handler_type) {
+    case NM_KEYFILE_HANDLER_TYPE_WARN:
+        nm_keyfile_handler_data_warn_get(handler_data, &message, &severity);
+        g_assert(message && strstr(message, "ethtool.bogus"));
+        break;
+    default:
+        g_assert_not_reached();
+    }
+
+    return TRUE;
+}
+
+static void
+test_invalid_option(void)
+{
+    gs_unref_object NMConnection   *con = NULL;
+    NMSetting                      *s_ethtool;
+    nm_auto_unref_keyfile GKeyFile *kf    = NULL;
+    gs_free_error GError           *error = NULL;
+    InvalidOptionWriteData          data;
+
+    con = nmtst_create_minimal_connection("test invalid option",
+                                          NULL,
+                                          NM_SETTING_WIRED_SETTING_NAME,
+                                          NULL);
+
+    s_ethtool = nm_setting_ethtool_new();
+
+    nm_connection_add_setting(con, s_ethtool);
+
+    nm_setting_option_set_boolean(s_ethtool, NM_ETHTOOL_OPTNAME_PAUSE_RX, TRUE);
+
+    data = (InvalidOptionWriteData){};
+    kf   = nm_keyfile_write(con,
+                          NM_KEYFILE_HANDLER_FLAGS_NONE,
+                          _invalid_option_write_handler,
+                          &data,
+                          nmtst_get_rand_bool() ? &error : NULL);
+    nmtst_assert_success(kf, error);
+    nm_clear_pointer(&kf, g_key_file_unref);
+
+    nmtst_connection_normalize(con);
+
+    nmtst_assert_connection_verifies_without_normalization(con);
+
+    data = (InvalidOptionWriteData){};
+    kf   = nm_keyfile_write(con,
+                          NM_KEYFILE_HANDLER_FLAGS_NONE,
+                          _invalid_option_write_handler,
+                          &data,
+                          nmtst_get_rand_bool() ? &error : NULL);
+    nmtst_assert_success(kf, error);
+    nm_clear_pointer(&kf, g_key_file_unref);
+
+    nm_setting_option_set(s_ethtool, "bogus", g_variant_new_int64(0));
+
+    data = (InvalidOptionWriteData){
+        .expect = TRUE,
+    };
+    kf = nm_keyfile_write(con,
+                          NM_KEYFILE_HANDLER_FLAGS_NONE,
+                          _invalid_option_write_handler,
+                          &data,
+                          nmtst_get_rand_bool() ? &error : NULL);
+    nmtst_assert_success(kf, error);
+    nm_clear_pointer(&kf, g_key_file_unref);
+
+    g_assert_cmpint(data.n_calls, ==, 1);
+}
+
+/*****************************************************************************/
+
 NMTST_DEFINE();
 
 int
@@ -904,6 +1000,7 @@ main(int argc, char **argv)
     g_test_add_func("/core/keyfile/test_vpn/1", test_vpn_1);
     g_test_add_func("/core/keyfile/bridge/vlans", test_bridge_vlans);
     g_test_add_func("/core/keyfile/bridge-port/vlans", test_bridge_port_vlans);
+    g_test_add_func("/core/keyfile/invalid-option", test_invalid_option);
 
     return g_test_run();
 }