Note: This is a beta release of Red Hat Bugzilla 5.0. The data contained within is a snapshot of the live data so any changes you make will not be reflected in the production Bugzilla. Also email is disabled so feel free to test any aspect of the site that you want. File any problems you find or give feedback here.
Bug 1691619 - fix libnm and NetworkManager's handling of team configuration (strict JSON parsing)
Summary: fix libnm and NetworkManager's handling of team configuration (strict JSON pa...
Keywords:
Status: NEW
Alias: None
Product: Red Hat Enterprise Linux 8
Classification: Red Hat
Component: NetworkManager
Version: 8.0
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: rc
: 8.0
Assignee: sushil kulkarni
QA Contact: Desktop QE
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-03-22 05:50 UTC by Thomas Haller
Modified: 2019-03-22 09:58 UTC (History)
7 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: Bug
Target Upstream Version:


Attachments (Terms of Use)

Description Thomas Haller 2019-03-22 05:50:54 UTC
Originally, NetworkManager (on D-Bus and libnm) only supported a plain "team.config" setting for team connection profiles. This was the JSON setting, that was opaque and not understandable to NM.

This "API" of having opaque JSON was cumbersome, and there was the goal to add a more convenient, typed API.

---

How it should be:


NM has a very strict understanding of what a team configuration is. A supported team configuration in NetworkManager consists of a well-known set of parameters, that are strictly verified to have value values and these values make sense together. It should start from a small set of supported configurations, and gradually support more settings -- always being very strict about what is valid and what not. Let's call this structured setting NMTeamConfig.

Converting a valid NMTeamConfig to JSON is always possible -- and uniquely so. Meaning, one particular NMTeamConfig has a normalized, preferred JSON representation: the "normalized" JSON.

We also need to strictly parse arbitrary JSON into NMTeamConfig. This parsing must be strict, there must be only JSON elements that we understand, that are valid, and nothing else. At the very least, we must be able to parse a "normalized" JSON back. Optionally, slight variations may be accepted (like, ignoring the order of dictionary keys). But that's not necessary. It's only for convenience that we may also accept some sets of non-normalized JSON (that are well understood). The stricter the better.


Now it's simple:

(1) if the user uses new-style API to configure a NMTeamConfig, the configuration must validate. As always, nm_connection_verify() rejects invalid configurations. A valid configuration can be converted to a normalized JSON config.

(2) if the user sets "team.config" directly, then we try to strictly parse it as NMTeamConfig. If we fail, then new-style NMTeamConfig is un-initialized and we keep using the opaque JSON string (that we were unable to understand). We cannot fail nm_connection_verify() in this case to not break backwards compatibility.
If the JSON can be strictly parsed as NMTeamConfig, then it's good too. Then new-style API is also available. Now, "team.config" may be in normalized form or not. It doesn't matter, we can keep the unnormalized form that the user wrote by hand (e.g. if it has additional whitespaces).

(3) on D-Bus or from keyfile, if only "team.config" is present, then we do (2). If only new-style parameters are present, we do (1). If both are present, then the new-style parameters must be valid, but also "team.config" must be parsable to the exact saem NMTeamConfig. Again, "team.config" may be in non-normalized form or not, but it must be valid.

(4) when libnm or NetworkManager serialize a team config to D-Bus or keyfile, then it either only serializes the unparsable JSON or it serializes the valid JSON together with the valid new-style NMTeamConfig.


---

How it is instead:


libnm/NetworkManager has not a strong sense of what makes a valid (new-style) configuration. The checks in verify() [1] are not sufficiently strict.

Also, libnm does not strictly parse the JSON. Instead, it picks individual fields on a one-by-one basis [2].

Also, libnm does not generate one entire JSON configuration out of the settings. Instead, it sets individual fields [3] inside the existing JSON, without consideration for the context.

The mistake that libnm makes is trying to merge JSON config that is not fully parsed/validate with additional settings on a one by one basis. When using new-style API, then the settings MUST validate and nothing else.

This needs to be fixed. We may avoid breaking backwards compatibility in many cases because when we accepted something in the past that we would no longer accept, we can just still generate the same JSON (that we do not consider as valid new-style NMTeamConfig).

NMSettingTeam must keep track whether of the preferred style ("team.config" or new-style NMTeamConfig). Basically: what API did the user use last. Regardless of the preferred style, if the "team.config" JSON or the new-style NMTeamConfig is valid/parsable, then the alternative style can be generated without loss. If it's invalid, then we may still generate something on a best effort basis (though, it would not validate).


Sending over DBUS:

- When libnm client-side generates a GVariant, it only sends the JSON. That is always the best choice. If the preferred style is new-style's NMTeamConfig and the configuration is invalid, then we still want to try as good as possible to craft a JSON. NM server side would either understand it (unlikely), or it would accept it as opaque old-style value.

- When libnm server side sends a message to the clients, then it includes both JSON and new-style fields. Since NM only keeps connections that verify, it means it either includes new-style and JSON with identical meaning; or it only sends the non-validating old-style JSON.

- when libnm client side parses fields, it always makes the best of it and never rejects profiles right away (it doesn't call nm_connection_verify()). If the data parses as valid new-style, then the preferred style is new-style. Otherwise, the preferred style is old-style.

- when libnm server side parses a GVariant that only contains "team.config" JSON, it accepts it (either as parsable new-style or as non-parsable blob). When the message on D-Bus only new-style fields (either alone or along "team.config" JSON), they must validate or are rejected.




[1] https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/libnm-core/nm-setting-team.c?id=72bdeebd73f05c831084202f6b983ee3f9b830d6#n1192
[2] https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/libnm-core/nm-setting-team.c?id=72bdeebd73f05c831084202f6b983ee3f9b830d6#n1349
[3] https://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/libnm-core/nm-setting-team.c?id=72bdeebd73f05c831084202f6b983ee3f9b830d6#n1595

Comment 1 Thomas Haller 2019-03-22 09:58:39 UTC
See also https://github.com/NetworkManager/NetworkManager/pull/318


Note You need to log in before you can comment on or make changes to this bug.