One problem is that older libnm-glib will assert against invalid connections: https://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=8c27a370ff70430225bd6a2408e32e332f872f2e

So, when upgrading NetworkManager, you can easily hit an assertion.
But that was really a bug in libnm-glib and not much we can do about it.

Note, that usually you'd also upgrade libnm-glib and don't hit any problem. But it's still ugly that older libnm-glib version will not work with newer NM.

this rabbit hole turns out to run deep.

First patches at th/invalid-connections-bgo760907

(In reply to Thomas Haller from comment #2)
> this rabbit hole turns out to run deep.
> 
> First patches at th/invalid-connections-bgo760907

ok, th/invalid-connections-bgo760907 gets it now right for libnm and NetworkManager core. 

Some of the later patches are potentially change in behavior that breaks existing applications. Please review.

libnm-util/libnm-glib is still todo...

> libnm-core: allow strict and relaxed error behavior for _nm_setting_new_from_dbus()

I would say by default parsing is strict, and the flags relax the behavior, rather than have two flags.  I don't think we need an explicit STRICT flag.

> libnm-core: add _nm_simple_connection_new_from_dbus() function

It doesn't seem like anything uses PARSE_FLAGS_VERIFY, so let's drop it until something needs it?

> libnm: don't normalize connection for nm_device_get_applied_connection()

The API docs should be updated to indicate that the returned connection may not actually validate if libnm is older than the NM version.

> libnm: be more accepting for invalid connections from NetworkManager

Same here; relevant API docs (for nm_client_get_connections() and the NMRemoteConnection docs) should be updated to indicate that the connection may not verify if the libnm version is older than the NM version, and what a user might need to do about that.

> core: be more relaxed parsing connection in AddAndActivateConnection

Here I think we are OK with incomplete connections (since they will be filled in by NM) but we *do* want to reject badly formatted ones with stuff like:

1) invalid/unknown permissions types (eg we want to call validate_permissions_type())
2) we want to reject unknown setting types
3) we want to reject duplicate settings

and ideally reject these things earlier rather than later.  But none of those happen with BEST_EFFORT; there are some classes of errors that aren't acceptable here.

(In reply to Dan Williams from comment #4)
> > libnm-core: allow strict and relaxed error behavior for _nm_setting_new_from_dbus()
> 
> I would say by default parsing is strict, and the flags relax the behavior,
> rather than have two flags.  I don't think we need an explicit STRICT flag.

The old behavior was a mixture of strict and best-behavior. To preserve the old behavior, a third way is needed.

Note that for example nm_simple_connection_new_from_dbus() and nm_connection_replace_settings() which are public API do not change behavior in this branch (apart from no longer printing a g_warning()).

The intent at this point is not to improve the public API (which would be a follow-up task).


Once all callers strictly use STRICT or BEST_EFFORT, those two flags can be merged.


> > libnm-core: add _nm_simple_connection_new_from_dbus() function
> 
> It doesn't seem like anything uses PARSE_FLAGS_VERIFY, so let's drop it
> until something needs it?

Ok.



> > libnm: don't normalize connection for nm_device_get_applied_connection()
> 
> The API docs should be updated to indicate that the returned connection may
> not actually validate if libnm is older than the NM version.
>
> > libnm: be more accepting for invalid connections from NetworkManager
> 
> Same here; relevant API docs (for nm_client_get_connections() and the
> NMRemoteConnection docs) should be updated to indicate that the connection
> may not verify if the libnm version is older than the NM version, and what a
> user might need to do about that.

Note that already before this branch, nm_client_get_connections() will happily return invalid connections. That doesn't change.

(ok, it changed for nm_device_get_applied_connection(), which would try to normalize the connection. But that is new API and now only behaves like the others). 


Anyway, new commit:
  "libnm: add code comments to hint that NMConnection might not validate"



> > core: be more relaxed parsing connection in AddAndActivateConnection
> 
> Here I think we are OK with incomplete connections (since they will be
> filled in by NM) but we *do* want to reject badly formatted ones with stuff
> like:
> 
> 1) invalid/unknown permissions types (eg we want to call
> validate_permissions_type())
> 2) we want to reject unknown setting types
> 3) we want to reject duplicate settings
> 
> and ideally reject these things earlier rather than later.  But none of
> those happen with BEST_EFFORT; there are some classes of errors that aren't
> acceptable here.

Which is exactly what STRICT does. Changed, and updated code comment.



Repushed.

> libnm-core: allow strict and relaxed error behavior for _nm_setting_new_from_dbus()

+               setting = _nm_setting_new_from_dbus (type, setting_dict, new_settings, NM_SETTING_PARSE_FLAGS_NONE, &local);

		if (!setting) {
+                       if (NM_FLAGS_HAS (parse_flags, NM_SETTING_PARSE_FLAGS_BEST_EFFORT))
+                               continue;
+                       g_propagate_error (error, local);
			g_slist_free_full (settings, g_object_unref);
			return FALSE;
		}

Since all paths returning NULL in _nm_setting_new_from_dbus() check
now for flags, maybe we can pass @parse_flags instead of FLAGS_NONE
and remove the 'if (NM_FLAGS_HAS (parse_flags, BEST_EFFORT)' below?

> core: be more strict when parsing connection in AddAndActivateConnection
> core: be strict about connection argument in D-Bus methods

As the commit messages say these potentially break applications, in my
opinion we should not do it.

(In reply to Beniamino Galvani from comment #6)
> > libnm-core: allow strict and relaxed error behavior for _nm_setting_new_from_dbus()
> 
> +               setting = _nm_setting_new_from_dbus (type, setting_dict,
> new_settings, NM_SETTING_PARSE_FLAGS_NONE, &local);
> 
> 		if (!setting) {
> +                       if (NM_FLAGS_HAS (parse_flags,
> NM_SETTING_PARSE_FLAGS_BEST_EFFORT))
> +                               continue;
> +                       g_propagate_error (error, local);
> 			g_slist_free_full (settings, g_object_unref);
> 			return FALSE;
> 		}
> 
> Since all paths returning NULL in _nm_setting_new_from_dbus() check
> now for flags, maybe we can pass @parse_flags instead of FLAGS_NONE
> and remove the 'if (NM_FLAGS_HAS (parse_flags, BEST_EFFORT)' below?

Good catch. Of course, we must pass @parse_flags and not NONE.

And you are right, that indeed settings will not be NULL when parse_flags are BEST_EFFORT. But I'd prefer to keep this extra "if" because otherwise we make assumptions on _nm_setting_new_from_dbus that are not obviously true.

How about the fixup?


> > core: be more strict when parsing connection in AddAndActivateConnection
> > core: be strict about connection argument in D-Bus methods
> 
> As the commit messages say these potentially break applications, in my
> opinion we should not do it.

Maybe, we might now reject messages that were accepted previously. But note, that these messages were always invalid, so... maybe we can change behavior in this regard?
@dcbw, what do you think?


Repushed.

(In reply to Thomas Haller from comment #7)

> How about the fixup?

Looks right.

Pushed more patches, now also adding relaxed behavior to libnm-glib.

This would be it...

New patches look good to me.

Regarding the strict validation of connections, I think this is
something that should eventually be done, because ignoring unknown
properties can be dangerous. For example, we recently introduced the
8021x.domain-suffix-match property, NM versions that don't understand
it should not accept connections with that property, otherwise they
will allow connection to insecure authenticators.

(In reply to Beniamino Galvani from comment #10)
> New patches look good to me.
> 
> Regarding the strict validation of connections, I think this is
> something that should eventually be done, because ignoring unknown
> properties can be dangerous. For example, we recently introduced the
> 8021x.domain-suffix-match property, NM versions that don't understand
> it should not accept connections with that property, otherwise they
> will allow connection to insecure authenticators.

I agree. 

Note that we want to support older clients against newer server version -- not the other way around.
So using a newer library version that supports 8021x.domain-suffix-match is strictly speaking not supported anyway -- of course, we should try to make do as much as possible, and being strict about unknown properties is also right in this case.

merged to master:
https://cgit.freedesktop.org/cgit/?url=NetworkManager/NetworkManager/commit/&id=23136ecf89f279479337ead355b7ff5e80465a0b