libnm: cleanup handling of "connection.permissions" and improve validationth/connection-permissions-cleanup

Previously, both nm_setting_connection_add_permission() and the GObject
property setter would merely assert that the provided values are valid
(and otherwise don't do anything). That is bad for handling errors.

For example, we use the property setter to initialize the setting from
keyfile and GVariant (D-Bus). That means, if a user provides an invalid
permissions value, we would emit a g_critical() assertion failure, but
otherwise ignore the permissions. What we instead need to do is to
accept the value, and afterwards fail verification. That way, a proper error
message can be generated.

  $ mcli connection add type ethernet autoconnect no ifname bogus con-name x connection.permissions 'bogus:'

  (process:429514): libnm-CRITICAL **: 12:12:00.359: permission_new: assertion 'strchr (uname, ':') == NULL' failed

  (process:429514): libnm-CRITICAL **: 12:12:00.359: nm_setting_connection_add_permission: assertion 'p != NULL' failed
  Connection 'x' (2802d117-f84e-44d9-925b-bfe26fd85da1) successfully added.
  $ $  nmcli -f connection.permissions connection show x
  connection.permissions:                 --

While at it, also don't track the permissions in a GSList. Tracking one
permission in a GSList requires 3 allocations (one for the user string,
one for the Permission struct, and one for the GSList struct). Instead,
use a GArray. That is still not great, because GArray cannot be embedded
inside NMSettingConnectionPrivate, so tracking one permission also
requires 3 allocations (which is really a fault of GArray). So, GArray
is not better in the common case where there is only one permissions. But even
in the worst case (only one entry), GArray is no worse than GSList.

Also change the API of nm_setting_connection_add_permission().
Previously, the function would assert that the arguments are in
a certain form (strcmp (ptype, "user") == 0), but still document
the such behaviors like regular operation ("[returns] %FALSE if @ptype
or @pitem was invalid"). Don't assert against the function arguments.
Also, if you first set the user to "fo:o", then
nm_setting_connection_add_permission() would accept it -- only at
a later phase, the property setter would assert against such values.
Also, the function would return %FALSE both if the input value was
invalid (an error) and if the value already existed. I think the
function should not treat a duplicate entry like a badly formatted
input.
Now the function does much less asserting of the arguments, but will
return %FALSE only if the values are invalid. And it will silently ignore
duplicate entries.

8 files changed, 352 insertions, 252 deletions

diff --git a/clients/cli/devices.c b/clients/cli/devices.c
index 534596887c..e103ab93a4 100644
--- a/clients/cli/devices.c
+++ b/clients/cli/devices.c
@@ -3868,7 +3868,10 @@ do_device_wifi_connect(const NMCCommand *cmd, NmCli *nmc, int argc, const char *
 
             /* Connection will only be visible to this user when 'private' is specified */
             if (private)
-                nm_setting_connection_add_permission(s_con, "user", g_get_user_name(), NULL);
+                nm_setting_connection_add_permission(s_con,
+                                                     NM_SETTINGS_CONNECTION_PERMISSION_USER,
+                                                     g_get_user_name() ?: "",
+                                                     NULL);
         }
         if (bssid2_arr || hidden) {
             s_wifi = (NMSettingWireless *) nm_setting_wireless_new();
diff --git a/clients/common/nm-meta-setting-desc.c b/clients/common/nm-meta-setting-desc.c
index 938eae9813..1e61aed903 100644
--- a/clients/common/nm-meta-setting-desc.c
+++ b/clients/common/nm-meta-setting-desc.c
@@ -2504,12 +2504,14 @@ static gconstpointer _get_fcn_connection_permissions(ARGS_GET_FCN)
     for (i = 0; i < n; i++) {
         if (!nm_setting_connection_get_permission(s_con, i, &perm_type, &perm_item, NULL))
             continue;
+        if (!nm_streq(perm_type, NM_SETTINGS_CONNECTION_PERMISSION_USER))
+            continue;
 
         if (!perm)
             perm = g_string_new(NULL);
         else
             g_string_append_c(perm, ',');
-        g_string_append_printf(perm, "%s:%s", perm_type, perm_item);
+        g_string_append_printf(perm, NM_SETTINGS_CONNECTION_PERMISSION_USER_PREFIX "%s", perm_item);
     }
 
     NM_SET_OUT(out_is_default, !perm);
@@ -2591,19 +2593,13 @@ static const char *const *_complete_fcn_connection_type(ARGS_COMPLETE_FCN)
     return (const char *const *) (*out_to_free = result);
 }
 
-#define PERM_USER_PREFIX "user:"
-
 static const char *
 _sanitize_connection_permission_user(const char *perm)
 {
-    if (NM_STR_HAS_PREFIX(perm, PERM_USER_PREFIX))
-        perm += NM_STRLEN(PERM_USER_PREFIX);
-
-    if (perm[0] == '\0')
+    if (NM_STR_HAS_PREFIX(perm, NM_SETTINGS_CONNECTION_PERMISSION_USER_PREFIX))
+        perm += NM_STRLEN(NM_SETTINGS_CONNECTION_PERMISSION_USER_PREFIX);
+    if (!nm_settings_connection_validate_permission_user(perm, -1))
         return NULL;
-    if (!g_utf8_validate(perm, -1, NULL))
-        return NULL;
-
     return perm;
 }
 
@@ -2626,19 +2622,25 @@ static gboolean
 _multilist_set_fcn_connection_permissions(NMSetting *setting, const char *item)
 {
     item = _sanitize_connection_permission_user(item);
-    nm_setting_connection_add_permission(NM_SETTING_CONNECTION(setting), "user", item, NULL);
+    if (!item)
+        return FALSE;
+    if (!nm_setting_connection_add_permission(NM_SETTING_CONNECTION(setting),
+                                              NM_SETTINGS_CONNECTION_PERMISSION_USER,
+                                              item,
+                                              NULL))
+        nm_assert_not_reached();
     return TRUE;
 }
 
 static gboolean
 _multilist_remove_by_value_fcn_connection_permissions(NMSetting *setting, const char *item)
 {
-    const char *sanitized;
-
-    sanitized = _sanitize_connection_permission_user(item);
+    item = _sanitize_connection_permission_user(item);
+    if (!item)
+        return FALSE;
     nm_setting_connection_remove_permission_by_value(NM_SETTING_CONNECTION(setting),
-                                                     "user",
-                                                     sanitized ?: item,
+                                                     NM_SETTINGS_CONNECTION_PERMISSION_USER,
+                                                     item,
                                                      NULL);
     return TRUE;
 }
diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c
index 8ca41cf5cf..41df175e2c 100644
--- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c
+++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.c
@@ -314,3 +314,32 @@ nm_utils_validate_dhcp4_vendor_class_id(const char *vci, GError **error)
 
     return TRUE;
 }
+
+gboolean
+nm_settings_connection_validate_permission_user(const char *item, gssize len)
+{
+    gsize l;
+
+    if (!item)
+        return FALSE;
+
+    if (len < 0) {
+        nm_assert(len == -1);
+        l = strlen(item);
+    } else
+        l = (gsize) len;
+
+    if (l == 0)
+        return FALSE;
+
+    if (!g_utf8_validate(item, l, NULL))
+        return FALSE;
+
+    if (l >= 100)
+        return FALSE;
+
+    if (memchr(item, ':', l))
+        return FALSE;
+
+    return TRUE;
+}
diff --git a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
index eff10df0d8..f2f74873f3 100644
--- a/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
+++ b/libnm-core/nm-libnm-core-intern/nm-libnm-core-utils.h
@@ -142,4 +142,11 @@ const char *nm_utils_route_type2str(guint8 val, char *buf, gsize len);
 
 gboolean nm_utils_validate_dhcp4_vendor_class_id(const char *vci, GError **error);
 
+/*****************************************************************************/
+
+#define NM_SETTINGS_CONNECTION_PERMISSION_USER        "user"
+#define NM_SETTINGS_CONNECTION_PERMISSION_USER_PREFIX "user:"
+
+gboolean nm_settings_connection_validate_permission_user(const char *item, gssize len);
+
 #endif /* __NM_LIBNM_SHARED_UTILS_H__ */
diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c
index 9a2ab4988e..5fa6cb3506 100644
--- a/libnm-core/nm-setting-connection.c
+++ b/libnm-core/nm-setting-connection.c
@@ -31,13 +31,14 @@
 
 /*****************************************************************************/
 
-typedef enum {
-    PERM_TYPE_USER = 0,
+typedef enum _nm_packed {
+    PERM_TYPE_INVALID,
+    PERM_TYPE_USER,
 } PermType;
 
 typedef struct {
-    guint8 ptype;
-    char * item;
+    PermType ptype;
+    char *   item;
 } Permission;
 
 NM_GOBJECT_PROPERTIES_DEFINE(NMSettingConnection,
@@ -68,7 +69,7 @@ NM_GOBJECT_PROPERTIES_DEFINE(NMSettingConnection,
                              PROP_MUD_URL, );
 
 typedef struct {
-    GSList *permissions; /* list of Permission structs */
+    GArray *permissions;
     GSList *secondaries; /* secondary connections to activate with the base connection */
     char *  id;
     char *  uuid;
@@ -102,83 +103,85 @@ G_DEFINE_TYPE(NMSettingConnection, nm_setting_connection, NM_TYPE_SETTING)
 
 /*****************************************************************************/
 
-#define PERM_USER_PREFIX "user:"
+static void
+_permission_set_stale(Permission *permission, PermType ptype, char *item_take)
+{
+    nm_assert(permission);
+    nm_assert(NM_IN_SET(ptype, PERM_TYPE_INVALID, PERM_TYPE_USER));
+    nm_assert(ptype != PERM_TYPE_USER
+              || nm_settings_connection_validate_permission_user(item_take, -1));
+
+    /* we don't inspect (clear) permission before setting. It takes a
+     * stale instance. */
+    *permission = (Permission){
+        .ptype = ptype,
+        .item  = item_take,
+    };
+}
+
+static void
+_permission_clear_stale(Permission *permission)
+{
+    g_free(permission->item);
+    /* We leave the permission instance with dangling pointers.
+     * It is "stale". */
+}
 
-static Permission *
-permission_new_from_str(const char *str)
+static gboolean
+_permission_set_stale_parse(Permission *permission, const char *str)
 {
-    Permission *p;
+    const char *str0 = str;
     const char *last_colon;
-    size_t      ulen = 0, i;
+    gsize       ulen;
+
+    nm_assert(str);
 
-    g_return_val_if_fail(strncmp(str, PERM_USER_PREFIX, strlen(PERM_USER_PREFIX)) == 0, NULL);
-    str += strlen(PERM_USER_PREFIX);
+    if (!str)
+        goto invalid;
+
+    if (!NM_STR_HAS_PREFIX(str, NM_SETTINGS_CONNECTION_PERMISSION_USER_PREFIX))
+        goto invalid;
+
+    str += NM_STRLEN(NM_SETTINGS_CONNECTION_PERMISSION_USER_PREFIX);
 
     last_colon = strrchr(str, ':');
     if (last_colon) {
-        /* Ensure that somebody didn't pass "user::" */
-        g_return_val_if_fail(last_colon > str, NULL);
-
         /* Reject :[detail] for now */
-        g_return_val_if_fail(*(last_colon + 1) == '\0', NULL);
-
-        /* Make sure we don't include detail in the username */
+        if (last_colon[1] != '\0')
+            goto invalid;
         ulen = last_colon - str;
     } else
         ulen = strlen(str);
 
-    /* Sanity check the length of the username */
-    g_return_val_if_fail(ulen < 100, NULL);
-
-    /* Make sure there's no ':' in the username */
-    for (i = 0; i < ulen; i++)
-        g_return_val_if_fail(str[i] != ':', NULL);
-
-    /* And the username must be valid UTF-8 */
-    g_return_val_if_fail(g_utf8_validate(str, -1, NULL) == TRUE, NULL);
+    if (!nm_settings_connection_validate_permission_user(str, ulen))
+        goto invalid;
 
     /* Yay, valid... create the new permission */
-    p        = g_slice_new0(Permission);
-    p->ptype = PERM_TYPE_USER;
-    if (last_colon) {
-        p->item = g_malloc(ulen + 1);
-        memcpy(p->item, str, ulen);
-        p->item[ulen] = '\0';
-    } else
-        p->item = g_strdup(str);
-
-    return p;
-}
-
-static Permission *
-permission_new(const char *uname)
-{
-    Permission *p;
-
-    g_return_val_if_fail(uname, NULL);
-    g_return_val_if_fail(uname[0] != '\0', NULL);
-    g_return_val_if_fail(strchr(uname, ':') == NULL, NULL);
-    g_return_val_if_fail(g_utf8_validate(uname, -1, NULL) == TRUE, NULL);
+    if (permission)
+        _permission_set_stale(permission, PERM_TYPE_USER, g_strndup(str, ulen));
+    return TRUE;
 
-    /* Yay, valid... create the new permission */
-    p        = g_slice_new0(Permission);
-    p->ptype = PERM_TYPE_USER;
-    p->item  = g_strdup(uname);
-    return p;
+invalid:
+    if (permission)
+        _permission_set_stale(permission, PERM_TYPE_INVALID, g_strdup(str0));
+    return FALSE;
 }
 
 static char *
-permission_to_string(Permission *p)
-{
-    return g_strdup_printf(PERM_USER_PREFIX "%s:", p->item);
-}
-
-static void
-permission_free(Permission *p)
+_permission_to_string(Permission *permission)
 {
-    g_free(p->item);
-    memset(p, 0, sizeof(*p));
-    g_slice_free(Permission, p);
+    nm_assert(permission);
+
+    switch (permission->ptype) {
+    case PERM_TYPE_USER:
+        return g_strdup_printf(NM_SETTINGS_CONNECTION_PERMISSION_USER_PREFIX "%s:",
+                               permission->item);
+    case PERM_TYPE_INVALID:
+        nm_assert(permission->item);
+        return g_strdup(permission->item);
+    }
+    nm_assert_not_reached();
+    return g_strdup(permission->item ?: "");
 }
 
 /*****************************************************************************/
@@ -279,14 +282,15 @@ nm_setting_connection_get_num_permissions(NMSettingConnection *setting)
 {
     g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), 0);
 
-    return g_slist_length(NM_SETTING_CONNECTION_GET_PRIVATE(setting)->permissions);
+    return nm_g_array_len(NM_SETTING_CONNECTION_GET_PRIVATE(setting)->permissions);
 }
 
 /**
  * nm_setting_connection_get_permission:
  * @setting: the #NMSettingConnection
  * @idx: the zero-based index of the permissions entry
- * @out_ptype: on return, the permission type (at this time, always "user")
+ * @out_ptype: on return, the permission type. This is currently always "user",
+ *   unless the entry is invalid, in which case it returns "invalid".
  * @out_pitem: on return, the permission item (formatted according to @ptype, see
  * #NMSettingConnection:permissions for more detail
  * @out_detail: on return, the permission detail (at this time, always %NULL)
@@ -304,22 +308,29 @@ nm_setting_connection_get_permission(NMSettingConnection *setting,
                                      const char **        out_detail)
 {
     NMSettingConnectionPrivate *priv;
-    Permission *                p;
+    Permission *                permission;
 
     g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE);
 
     priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting);
 
-    g_return_val_if_fail(idx < g_slist_length(priv->permissions), FALSE);
-
-    p = g_slist_nth_data(priv->permissions, idx);
-    if (out_ptype)
-        *out_ptype = "user";
-    if (out_pitem)
-        *out_pitem = p->item;
-    if (out_detail)
-        *out_detail = NULL;
+    g_return_val_if_fail(idx < nm_g_array_len(priv->permissions), FALSE);
 
+    permission = &g_array_index(priv->permissions, Permission, idx);
+    switch (permission->ptype) {
+    case PERM_TYPE_USER:
+        NM_SET_OUT(out_ptype, NM_SETTINGS_CONNECTION_PERMISSION_USER);
+        NM_SET_OUT(out_pitem, permission->item);
+        NM_SET_OUT(out_detail, NULL);
+        return TRUE;
+    case PERM_TYPE_INVALID:
+        goto invalid;
+    }
+    nm_assert_not_reached();
+invalid:
+    NM_SET_OUT(out_ptype, "invalid");
+    NM_SET_OUT(out_pitem, permission->item);
+    NM_SET_OUT(out_detail, NULL);
     return TRUE;
 }
 
@@ -337,23 +348,22 @@ gboolean
 nm_setting_connection_permissions_user_allowed(NMSettingConnection *setting, const char *uname)
 {
     NMSettingConnectionPrivate *priv;
-    GSList *                    iter;
+    guint                       i;
 
     g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE);
     g_return_val_if_fail(uname != NULL, FALSE);
-    g_return_val_if_fail(*uname != '\0', FALSE);
 
     priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting);
 
-    /* If no permissions, visible to all */
-    if (priv->permissions == NULL)
+    if (nm_g_array_len(priv->permissions) == 0) {
+        /* If no permissions, visible to all */
         return TRUE;
+    }
 
-    /* Find the username in the permissions list */
-    for (iter = priv->permissions; iter; iter = g_slist_next(iter)) {
-        Permission *p = iter->data;
+    for (i = 0; i < priv->permissions->len; i++) {
+        const Permission *permission = &g_array_index(priv->permissions, Permission, i);
 
-        if (strcmp(uname, p->item) == 0)
+        if (permission->ptype == PERM_TYPE_USER && nm_streq(permission->item, uname))
             return TRUE;
     }
 
@@ -372,8 +382,10 @@ nm_setting_connection_permissions_user_allowed(NMSettingConnection *setting, con
  * #NMSettingConnection:permissions: for more details.
  *
  * Returns: %TRUE if the permission was unique and was successfully added to the
- * list, %FALSE if @ptype or @pitem was invalid or it the permission was already
- * present in the list
+ * list, %FALSE if @ptype or @pitem was invalid.
+ * If the permission was already present in the list, it will not be added
+ * a second time but %TRUE will be returned. Note that before 1.28, in this
+ * case %FALSE would be returned.
  */
 gboolean
 nm_setting_connection_add_permission(NMSettingConnection *setting,
@@ -382,30 +394,39 @@ nm_setting_connection_add_permission(NMSettingConnection *setting,
                                      const char *         detail)
 {
     NMSettingConnectionPrivate *priv;
-    Permission *                p;
-    GSList *                    iter;
+    guint                       i;
 
     g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE);
-    g_return_val_if_fail(ptype && ptype[0], FALSE);
-    g_return_val_if_fail(detail == NULL, FALSE);
+    g_return_val_if_fail(ptype, FALSE);
+    g_return_val_if_fail(pitem, FALSE);
 
-    /* Only "user" for now... */
-    g_return_val_if_fail(strcmp(ptype, "user") == 0, FALSE);
+    if (!nm_streq0(ptype, NM_SETTINGS_CONNECTION_PERMISSION_USER))
+        return FALSE;
+
+    if (!nm_settings_connection_validate_permission_user(pitem, -1))
+        return FALSE;
+
+    if (detail)
+        return FALSE;
 
     priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting);
 
-    /* No dupes */
-    for (iter = priv->permissions; iter; iter = g_slist_next(iter)) {
-        p = iter->data;
-        if (strcmp(pitem, p->item) == 0)
-            return FALSE;
+    if (!priv->permissions) {
+        priv->permissions = g_array_sized_new(FALSE, FALSE, sizeof(Permission), 1);
+        g_array_set_clear_func(priv->permissions, (GDestroyNotify) _permission_clear_stale);
     }
 
-    p = permission_new(pitem);
-    g_return_val_if_fail(p != NULL, FALSE);
-    priv->permissions = g_slist_append(priv->permissions, p);
-    _notify(setting, PROP_PERMISSIONS);
+    for (i = 0; i < priv->permissions->len; i++) {
+        const Permission *permission = &g_array_index(priv->permissions, Permission, i);
 
+        if (permission->ptype == PERM_TYPE_USER && nm_streq(permission->item, pitem))
+            return TRUE;
+    }
+
+    _permission_set_stale(nm_g_array_append_new(priv->permissions, Permission),
+                          PERM_TYPE_USER,
+                          g_strdup(pitem));
+    _notify(setting, PROP_PERMISSIONS);
     return TRUE;
 }
 
@@ -420,16 +441,15 @@ void
 nm_setting_connection_remove_permission(NMSettingConnection *setting, guint32 idx)
 {
     NMSettingConnectionPrivate *priv;
-    GSList *                    iter;
 
     g_return_if_fail(NM_IS_SETTING_CONNECTION(setting));
 
     priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting);
-    iter = g_slist_nth(priv->permissions, idx);
-    g_return_if_fail(iter != NULL);
 
-    permission_free((Permission *) iter->data);
-    priv->permissions = g_slist_delete_link(priv->permissions, iter);
+    g_return_if_fail(idx < nm_g_array_len(priv->permissions));
+
+    g_array_remove_index(priv->permissions, idx);
+
     _notify(setting, PROP_PERMISSIONS);
 }
 
@@ -453,25 +473,28 @@ nm_setting_connection_remove_permission_by_value(NMSettingConnection *setting,
                                                  const char *         detail)
 {
     NMSettingConnectionPrivate *priv;
-    Permission *                p;
-    GSList *                    iter;
+    guint                       i;
 
     g_return_val_if_fail(NM_IS_SETTING_CONNECTION(setting), FALSE);
-    g_return_val_if_fail(ptype && ptype[0], FALSE);
-    g_return_val_if_fail(detail == NULL, FALSE);
-    g_return_val_if_fail(pitem != NULL, FALSE);
+    g_return_val_if_fail(ptype, FALSE);
+    g_return_val_if_fail(pitem, FALSE);
+
+    if (!nm_streq0(ptype, NM_SETTINGS_CONNECTION_PERMISSION_USER))
+        return FALSE;
 
-    /* Only "user" for now... */
-    g_return_val_if_fail(strcmp(ptype, "user") == 0, FALSE);
+    if (detail)
+        return FALSE;
 
     priv = NM_SETTING_CONNECTION_GET_PRIVATE(setting);
-    for (iter = priv->permissions; iter; iter = g_slist_next(iter)) {
-        p = iter->data;
-        if (strcmp(pitem, p->item) == 0) {
-            permission_free((Permission *) iter->data);
-            priv->permissions = g_slist_delete_link(priv->permissions, iter);
-            _notify(setting, PROP_PERMISSIONS);
-            return TRUE;
+    if (priv->permissions) {
+        for (i = 0; i < priv->permissions->len; i++) {
+            const Permission *permission = &g_array_index(priv->permissions, Permission, i);
+
+            if (permission->ptype == PERM_TYPE_USER && nm_streq(permission->item, pitem)) {
+                g_array_remove_index(priv->permissions, i);
+                _notify(setting, PROP_PERMISSIONS);
+                return TRUE;
+            }
         }
     }
     return FALSE;
@@ -1298,6 +1321,27 @@ after_interface_name:
         }
     }
 
+    if (priv->permissions) {
+        guint i;
+
+        for (i = 0; i < priv->permissions->len; i++) {
+            const Permission *permissions = &g_array_index(priv->permissions, Permission, i);
+
+            if (permissions->ptype != PERM_TYPE_USER) {
+                g_set_error_literal(error,
+                                    NM_CONNECTION_ERROR,
+                                    NM_CONNECTION_ERROR_INVALID_PROPERTY,
+                                    _("invalid permissions not in format \"user:$UNAME[:]\""));
+                g_prefix_error(error,
+                               "%s.%s: ",
+                               nm_setting_get_name(setting),
+                               NM_SETTING_CONNECTION_PERMISSIONS);
+                return FALSE;
+            }
+            nm_assert(nm_settings_connection_validate_permission_user(permissions->item, -1));
+        }
+    }
+
     /* *** errors above here should be always fatal, below NORMALIZABLE_ERROR *** */
 
     if (!priv->uuid) {
@@ -1462,40 +1506,6 @@ compare_property(const NMSettInfoSetting *sett_info,
         ->compare_property(sett_info, property_idx, con_a, set_a, con_b, set_b, flags);
 }
 
-static GSList *
-perm_strv_to_permlist(char **strv)
-{
-    GSList *list = NULL;
-    int     i;
-
-    if (!strv)
-        return NULL;
-
-    for (i = 0; strv[i]; i++) {
-        Permission *p;
-
-        p = permission_new_from_str(strv[i]);
-        if (p)
-            list = g_slist_append(list, p);
-    }
-
-    return list;
-}
-
-static char **
-perm_permlist_to_strv(GSList *permlist)
-{
-    GPtrArray *strings;
-    GSList *   iter;
-
-    strings = g_ptr_array_new();
-    for (iter = permlist; iter; iter = g_slist_next(iter))
-        g_ptr_array_add(strings, permission_to_string((Permission *) iter->data));
-    g_ptr_array_add(strings, NULL);
-
-    return (char **) g_ptr_array_free(strings, FALSE);
-}
-
 /*****************************************************************************/
 
 static void
@@ -1521,8 +1531,20 @@ get_property(GObject *object, guint prop_id, GValue *value, GParamSpec *pspec)
         g_value_set_string(value, nm_setting_connection_get_connection_type(setting));
         break;
     case PROP_PERMISSIONS:
-        g_value_take_boxed(value, perm_permlist_to_strv(priv->permissions));
+    {
+        char **strv;
+        gsize  i, l;
+
+        l    = nm_g_array_len(priv->permissions);
+        strv = g_new(char *, l + 1u);
+
+        for (i = 0; i < l; i++)
+            strv[i] = _permission_to_string(&g_array_index(priv->permissions, Permission, i));
+        strv[i] = NULL;
+
+        g_value_take_boxed(value, strv);
         break;
+    }
     case PROP_AUTOCONNECT:
         g_value_set_boolean(value, nm_setting_connection_get_autoconnect(setting));
         break;
@@ -1613,9 +1635,25 @@ set_property(GObject *object, guint prop_id, const GValue *value, GParamSpec *ps
         priv->type = g_value_dup_string(value);
         break;
     case PROP_PERMISSIONS:
-        g_slist_free_full(priv->permissions, (GDestroyNotify) permission_free);
-        priv->permissions = perm_strv_to_permlist(g_value_get_boxed(value));
+    {
+        const char *const *strv;
+        guint              i;
+
+        nm_clear_pointer(&priv->permissions, g_array_unref);
+        strv = g_value_get_boxed(value);
+        if (strv && strv[0]) {
+            priv->permissions =
+                g_array_sized_new(FALSE, FALSE, sizeof(Permission), NM_PTRARRAY_LEN(strv));
+            g_array_set_clear_func(priv->permissions, (GDestroyNotify) _permission_clear_stale);
+
+            for (i = 0; strv[i]; i++) {
+                Permission *permission = nm_g_array_append_new(priv->permissions, Permission);
+
+                _permission_set_stale_parse(permission, strv[i]);
+            }
+        }
         break;
+    }
     case PROP_AUTOCONNECT:
         priv->autoconnect = g_value_get_boolean(value);
         break;
@@ -1729,7 +1767,7 @@ finalize(GObject *object)
     g_free(priv->master);
     g_free(priv->slave_type);
     g_free(priv->mud_url);
-    g_slist_free_full(priv->permissions, (GDestroyNotify) permission_free);
+    nm_clear_pointer(&priv->permissions, g_array_unref);
     g_slist_free_full(priv->secondaries, g_free);
 
     G_OBJECT_CLASS(nm_setting_connection_parent_class)->finalize(object);
diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c
index 7c10f02756..b66dd196c7 100644
--- a/libnm-core/tests/test-general.c
+++ b/libnm-core/tests/test-general.c
@@ -3208,7 +3208,7 @@ check_permission(NMSettingConnection *s_con, guint32 idx, const char *expected_u
     gboolean    success;
     const char *ptype = NULL, *pitem = NULL, *detail = NULL;
 
-    success = nm_setting_connection_get_permission(s_con, 0, &ptype, &pitem, &detail);
+    success = nm_setting_connection_get_permission(s_con, idx, &ptype, &pitem, &detail);
     g_assert(success);
 
     g_assert_cmpstr(ptype, ==, "user");
@@ -3233,50 +3233,43 @@ test_setting_connection_permissions_helpers(void)
     s_con = NM_SETTING_CONNECTION(nm_setting_connection_new());
 
     /* Ensure a bad [type] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(strcmp(ptype, "user") == 0));
     success = nm_setting_connection_add_permission(s_con, "foobar", "blah", NULL);
-    g_test_assert_expected_messages();
     g_assert(!success);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
 
     /* Ensure a bad [type] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(ptype && ptype[0]));
+    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(ptype));
     success = nm_setting_connection_add_permission(s_con, NULL, "blah", NULL);
     g_test_assert_expected_messages();
     g_assert(!success);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
 
     /* Ensure a bad [item] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(uname));
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(p != NULL));
+    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(pitem));
     success = nm_setting_connection_add_permission(s_con, "user", NULL, NULL);
     g_test_assert_expected_messages();
     g_assert(!success);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
 
     /* Ensure a bad [item] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(uname[0] != '\0'));
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(p != NULL));
     success = nm_setting_connection_add_permission(s_con, "user", "", NULL);
-    g_test_assert_expected_messages();
     g_assert(!success);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
 
     /* Ensure an [item] with ':' is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(strchr(uname, ':') == NULL));
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(p != NULL));
     success = nm_setting_connection_add_permission(s_con, "user", "ad:asdf", NULL);
-    g_test_assert_expected_messages();
     g_assert(!success);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
 
     /* Ensure a non-UTF-8 [item] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(g_utf8_validate(uname, -1, NULL) == TRUE));
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(p != NULL));
     success = nm_setting_connection_add_permission(s_con, "user", buf, NULL);
-    g_test_assert_expected_messages();
     g_assert(!success);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
 
     /* Ensure a non-NULL [detail] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(detail == NULL));
     success = nm_setting_connection_add_permission(s_con, "user", "dafasdf", "asdf");
-    g_test_assert_expected_messages();
     g_assert(!success);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
 
     /* Ensure a valid call results in success */
     success = nm_setting_connection_add_permission(s_con, "user", TEST_UNAME, NULL);
@@ -3337,74 +3330,92 @@ add_permission_property(NMSettingConnection *s_con,
 static void
 test_setting_connection_permissions_property(void)
 {
-    NMSettingConnection *s_con;
-    gboolean             success;
-    char                 buf[9] = {0x61, 0x62, 0x63, 0xff, 0xfe, 0xfd, 0x23, 0x01, 0x00};
+    gs_unref_object NMSettingConnection *s_con = NULL;
+    gboolean                             success;
+    char buf[9] = {0x61, 0x62, 0x63, 0xff, 0xfe, 0xfd, 0x23, 0x01, 0x00};
 
     s_con = NM_SETTING_CONNECTION(nm_setting_connection_new());
 
+#define _assert_permission_invalid_at_idx(s_con, idx, expected_item)                            \
+    G_STMT_START                                                                                \
+    {                                                                                           \
+        NMSettingConnection *_s_con = (s_con);                                                  \
+        guint                _idx   = (idx);                                                    \
+        const char *         _ptype;                                                            \
+        const char *         _pitem;                                                            \
+        const char *         _detail;                                                           \
+        const char **        _p_ptype  = nmtst_get_rand_bool() ? &_ptype : NULL;                \
+        const char **        _p_pitem  = nmtst_get_rand_bool() ? &_pitem : NULL;                \
+        const char **        _p_detail = nmtst_get_rand_bool() ? &_detail : NULL;               \
+                                                                                                \
+        g_assert_cmpint(_idx, <, nm_setting_connection_get_num_permissions(_s_con));            \
+        g_assert(                                                                               \
+            nm_setting_connection_get_permission(_s_con, _idx, _p_ptype, _p_pitem, _p_detail)); \
+        if (_p_ptype)                                                                           \
+            g_assert_cmpstr(_ptype, ==, "invalid");                                             \
+        if (_p_pitem) {                                                                         \
+            const char *_expected_item = (expected_item);                                       \
+                                                                                                \
+            if (!_expected_item)                                                                \
+                g_assert_cmpstr(_pitem, !=, NULL);                                              \
+            else                                                                                \
+                g_assert_cmpstr(_pitem, ==, _expected_item);                                    \
+        }                                                                                       \
+        if (_p_detail)                                                                          \
+            g_assert_cmpstr(_detail, ==, NULL);                                                 \
+    }                                                                                           \
+    G_STMT_END
+
     /* Ensure a bad [type] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(
-        NMTST_G_RETURN_MSG(strncmp(str, PERM_USER_PREFIX, strlen(PERM_USER_PREFIX)) == 0));
     add_permission_property(s_con, "foobar", "blah", -1, NULL);
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, "foobar:blah:");
 
     /* Ensure a bad [type] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(
-        NMTST_G_RETURN_MSG(strncmp(str, PERM_USER_PREFIX, strlen(PERM_USER_PREFIX)) == 0));
     add_permission_property(s_con, NULL, "blah", -1, NULL);
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, ":blah:");
 
     /* Ensure a bad [item] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(last_colon > str));
     add_permission_property(s_con, "user", NULL, -1, NULL);
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, "user::");
 
     /* Ensure a bad [item] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(last_colon > str));
     add_permission_property(s_con, "user", "", -1, NULL);
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, "user::");
 
     /* Ensure an [item] with ':' in the middle is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(str[i] != ':'));
     add_permission_property(s_con, "user", "ad:asdf", -1, NULL);
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, "user:ad:asdf:");
 
     /* Ensure an [item] with ':' at the end is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(str[i] != ':'));
     add_permission_property(s_con, "user", "adasdfaf:", -1, NULL);
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, "user:adasdfaf::");
 
     /* Ensure a non-UTF-8 [item] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(g_utf8_validate(str, -1, NULL) == TRUE));
     add_permission_property(s_con, "user", buf, (int) sizeof(buf), NULL);
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, NULL);
 
     /* Ensure a non-NULL [detail] is rejected */
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(*(last_colon + 1) == '\0'));
     add_permission_property(s_con, "user", "dafasdf", -1, "asdf");
-    g_test_assert_expected_messages();
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
+    _assert_permission_invalid_at_idx(s_con, 0, "user:dafasdf:asdf");
 
     /* Ensure a valid call results in success */
     success = nm_setting_connection_add_permission(s_con, "user", TEST_UNAME, NULL);
     g_assert(success);
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
-
-    check_permission(s_con, 0, TEST_UNAME);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 2);
+    _assert_permission_invalid_at_idx(s_con, 0, "user:dafasdf:asdf");
+    check_permission(s_con, 1, TEST_UNAME);
 
     /* Now remove that permission and ensure we have 0 permissions */
     nm_setting_connection_remove_permission(s_con, 0);
-    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 0);
-
-    g_object_unref(s_con);
+    g_assert_cmpint(nm_setting_connection_get_num_permissions(s_con), ==, 1);
 }
 
 static void
@@ -4780,7 +4791,7 @@ test_setting_connection_changed_signal(void)
     ASSERT_CHANGED(nm_setting_connection_add_permission(s_con, "user", "billsmith", NULL));
     ASSERT_CHANGED(nm_setting_connection_remove_permission(s_con, 0));
 
-    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(iter != NULL));
+    NMTST_EXPECT_LIBNM_CRITICAL(NMTST_G_RETURN_MSG(idx < nm_g_array_len(priv->permissions)));
     ASSERT_UNCHANGED(nm_setting_connection_remove_permission(s_con, 1));
     g_test_assert_expected_messages();
 
diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c
index 6e93f177ca..5aa9e12c98 100644
--- a/src/settings/nm-settings-connection.c
+++ b/src/settings/nm-settings-connection.c
@@ -359,10 +359,13 @@ nm_settings_connection_check_visibility(NMSettingsConnection *self,
         return TRUE;
 
     for (i = 0; i < num; i++) {
+        const char *ptype;
         const char *user;
         uid_t       uid;
 
-        if (!nm_setting_connection_get_permission(s_con, i, NULL, &user, NULL))
+        if (!nm_setting_connection_get_permission(s_con, i, &ptype, &user, NULL))
+            continue;
+        if (!nm_streq(ptype, NM_SETTINGS_CONNECTION_PERMISSION_USER))
             continue;
         if (!nm_utils_name_to_uid(user, &uid))
             continue;
@@ -407,6 +410,8 @@ nm_settings_connection_check_permission(NMSettingsConnection *self, const char *
     }
 
     for (i = 0; i < num; i++) {
+        const char *ptype;
+
         /* For each user get their secret agent and check if that agent has the
          * required permission.
          *
@@ -414,10 +419,13 @@ nm_settings_connection_check_permission(NMSettingsConnection *self, const char *
          * name or a PID but if the user isn't running an agent they won't have
          * either.
          */
-        if (nm_setting_connection_get_permission(s_con, i, NULL, &puser, NULL)) {
-            if (nm_agent_manager_has_agent_with_permission(priv->agent_mgr, puser, permission))
-                return TRUE;
-        }
+        if (!nm_setting_connection_get_permission(s_con, i, &ptype, &puser, NULL))
+            continue;
+        if (!nm_streq(ptype, NM_SETTINGS_CONNECTION_PERMISSION_USER))
+            continue;
+
+        if (nm_agent_manager_has_agent_with_permission(priv->agent_mgr, puser, permission))
+            return TRUE;
     }
 
     return FALSE;
diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
index 6d5ad3d5ff..42a392b8b0 100644
--- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
+++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-writer.c
@@ -2029,15 +2029,15 @@ write_dcb_setting(NMConnection *connection, shvarFile *ifcfg, GError **error)
 static void
 write_connection_setting(NMSettingConnection *s_con, shvarFile *ifcfg)
 {
-    guint32                  n, i;
-    GString *                str;
-    const char *             master, *master_iface = NULL, *type;
-    int                      vint;
-    gint32                   vint32;
-    NMSettingConnectionMdns  mdns;
-    NMSettingConnectionLlmnr llmnr;
-    guint32                  vuint32;
-    const char *             tmp, *mud_url;
+    guint32              n, i;
+    nm_auto_free_gstring GString *str = NULL;
+    const char *                  master, *master_iface = NULL, *type;
+    int                           vint;
+    gint32                        vint32;
+    NMSettingConnectionMdns       mdns;
+    NMSettingConnectionLlmnr      llmnr;
+    guint32                       vuint32;
+    const char *                  tmp, *mud_url;
 
     svSetValueStr(ifcfg, "NAME", nm_setting_connection_get_id(s_con));
     svSetValueStr(ifcfg, "UUID", nm_setting_connection_get_uuid(s_con));
@@ -2083,22 +2083,25 @@ write_connection_setting(NMSettingConnection *s_con, shvarFile *ifcfg)
     /* Permissions */
     n = nm_setting_connection_get_num_permissions(s_con);
     if (n > 0) {
-        str = g_string_sized_new(n * 20);
-
+        nm_gstring_prepare(&str);
         for (i = 0; i < n; i++) {
+            const char *ptype = NULL;
             const char *puser = NULL;
 
+            if (!nm_setting_connection_get_permission(s_con, i, &ptype, &puser, NULL))
+                continue;
+            if (!nm_streq(ptype, NM_SETTINGS_CONNECTION_PERMISSION_USER))
+                continue;
+
             /* Items separated by space for consistency with eg
              * IPV6ADDR_SECONDARIES and DOMAIN.
              */
             if (str->len)
                 g_string_append_c(str, ' ');
 
-            if (nm_setting_connection_get_permission(s_con, i, NULL, &puser, NULL))
-                g_string_append(str, puser);
+            g_string_append(str, puser);
         }
         svSetValueStr(ifcfg, "USERS", str->str);
-        g_string_free(str, TRUE);
     }
 
     svSetValueStr(ifcfg, "ZONE", nm_setting_connection_get_zone(s_con));
@@ -2160,22 +2163,21 @@ write_connection_setting(NMSettingConnection *s_con, shvarFile *ifcfg)
     /* secondary connection UUIDs */
     n = nm_setting_connection_get_num_secondaries(s_con);
     if (n > 0) {
-        str = g_string_sized_new(n * 37);
-
+        nm_gstring_prepare(&str);
         for (i = 0; i < n; i++) {
             const char *uuid;
 
             /* Items separated by space for consistency with eg
              * IPV6ADDR_SECONDARIES and DOMAIN.
              */
+            if (!(uuid = nm_setting_connection_get_secondary(s_con, i)))
+                continue;
+
             if (str->len)
                 g_string_append_c(str, ' ');
-
-            if ((uuid = nm_setting_connection_get_secondary(s_con, i)) != NULL)
-                g_string_append(str, uuid);
+            g_string_append(str, uuid);
         }
         svSetValueStr(ifcfg, "SECONDARY_UUIDS", str->str);
-        g_string_free(str, TRUE);
     }
 
     vuint32 = nm_setting_connection_get_gateway_ping_timeout(s_con);