NetworkManager: Wireguard private key flag support #371

daniloegea · 2023-06-23T15:06:12Z

Description

Network Manager supports storing the Wireguard private key in an external keychain agent, like the kdewallet. In this cases, it will emit the following configuration in the keyfile: wireguard.private-key-flags=1 and omit the private key, which will be stored somewhere else.

The new key will look like this:

network:
  version: 2
  tunnels:
    wg-tunnel:
      renderer: NetworkManager
      addresses:
      - "10.20.30.1/24"
      mode: "wireguard"
      port: 51820
      keys:
        private-key-flags:
        - agent-owned

Adding this options is one way of solving the issue described here https://bugs.launchpad.net/ubuntu/+source/netplan.io/+bug/2024661

Checklist

Runs make check successfully.
Retains 100% code coverage (make check-coverage).
New/changed keys in YAML format are documented.
(Optional) Adds example YAML for new feature.
(Optional) Closes an open bug in Launchpad.

slyon · 2023-06-26T15:28:40Z

doc/netplan-yaml.md

+          private-key-flags:
+            - agent-owned


Regarding new schema:
This seems to be a rather NetworkManager specific settings, I feel like it should therefore live in the nm_backend_settings_handlers instead. Also, I wonder if we could/should make this a bit more generic, by introducing a generic "networkmanager.flags" setting, which might contain other (future) flags, then using the "validation" stage to verify those flags make sense for the given interface definition implicitly (c.f. wakeonwlan setting). E.g.:

network: renderer: NetworkManager tunnels: wg0: mode: wireguard networkmanager: flags: [private-key-agent-owned] port: 5182 peers: - keys: public: rlbInAj0qV69CysWPQY7KEBnKxpYCpaWqOs/dLevdWc= allowed-ips: [0.0.0.0/0, "2001:fe:ad:de:ad:be:ef:1/24"] keepalive: 23 endpoint: 1.2.3.4:5 ``` What do you think?

That works for me. It could be moved to backend_settings, but it's used in the middle of the netdef struct, so adding a new variable there will change the struct layout. Is that fine?

On a second thought, I think having a generic flags key might be tricky to maintain. We'll have to have our own encoding. What if a flag for a given context have the same value as another flag from a different context in Network Manager?

I think it's fine to move the private-key-flags to the backend specific data though.

daniloegea · 2023-06-30T14:17:44Z

I moved the new key to the networkmanager section and rebased.

slyon

Sorry for the back and forth.
After some discussion with the Netplan team, we feel like this kind of flags should rather be close to the actual definition of the (private-)key. Before we continue with the review of the implementation details, I'd like to get an agreement on the new parts of the YAML schema.

What do you think about the following suggestion? We could also do private-key-flags: [agent-owned], but I think a generic keays.flags setting gives us more flexibility to expand for other use cases and key toggles in the future. We'd need an internal translation function from Netplan-keys.flags to NM's wireguard.private-key-flags, though.

Can you already think of any other (potential) use case, where such flags could be needed? What would it be and do you think this schema could cover that case?

 network:
   renderer: NetworkManager
   tunnels:
     wg0:
       mode: wireguard
       port: 5182
       peers:
         - keys:
             public: rlbInAj0qV69CysWPQY7KEBnKxpYCpaWqOs/dLevdWc=
             flags: [private-key-agent-owned]
           allowed-ips: [0.0.0.0/0, "2001:fe:ad:de:ad:be:ef:1/24"]
           keepalive: 23
           endpoint: 1.2.3.4:5

daniloegea · 2023-07-12T10:20:57Z

Having the flags in the peers.keys path is not accurate. Those flags are about the private key used by the local connection, not its peers.

After checking nm-settings(5), I've found many more places where the same set of flags (NMSettingSecretFlags) are used apart from wireguard.private-key-flags (I missed that before):
802-1x.ca-cert-password-flags, 802-1x.client-cert-password-flags, 802-1x.password-flags, 802-1x.password-raw-flags, 802-1x.phase2-ca-cert-password-flags, 802-1x.phase2-client-cert-password-flags, 802-1x.phase2-private-key-password-flags, 802-1x.pin-flags, 802-1x.private-key-password-flags, adsl.password-flags, cdma.password-flags, gsm.password-flags, gsm.pin-flags, macsec.mka-cak-flags, pppoe.password-flags, 802-11-wireless-security.leap-password-flags, leap-password-flags.psk-flags and leap-password-flags.wep-key-flags.

For reference, these are the flags:

/**
 * NMSettingSecretFlags:
 * @NM_SETTING_SECRET_FLAG_NONE: the system is responsible for providing and
 * storing this secret (default)
 * @NM_SETTING_SECRET_FLAG_AGENT_OWNED: a user secret agent is responsible
 * for providing and storing this secret; when it is required agents will be
 * asked to retrieve it
 * @NM_SETTING_SECRET_FLAG_NOT_SAVED: this secret should not be saved, but
 * should be requested from the user each time it is needed
 * @NM_SETTING_SECRET_FLAG_NOT_REQUIRED: in situations where it cannot be
 * automatically determined that the secret is required (some VPNs and PPP
 * providers don't require all secrets) this flag indicates that the specific
 * secret is not required
 *
 * These flags indicate specific behavior related to handling of a secret.  Each
 * secret has a corresponding set of these flags which indicate how the secret
 * is to be stored and/or requested when it is needed.
 *
 **/
typedef enum /*< flags >*/ {
    NM_SETTING_SECRET_FLAG_NONE         = 0x00000000,
    NM_SETTING_SECRET_FLAG_AGENT_OWNED  = 0x00000001,
    NM_SETTING_SECRET_FLAG_NOT_SAVED    = 0x00000002,
    NM_SETTING_SECRET_FLAG_NOT_REQUIRED = 0x00000004

    /* NOTE: if adding flags, update nm-core-internal.h as well */
} NMSettingSecretFlags;

That said, I think it makes sense to have a more generic key for setting these flags. Maybe something like networkmanager.secret-flags so we could use it to set flags for all those types of connections I listed above.

slyon · 2023-07-12T15:27:28Z

That said, I think it makes sense to have a more generic key for setting these flags. Maybe something like networkmanager.secret-flags so we could use it to set flags for all those types of connections I listed above.

Thanks for looking up the details! So that would be back to my original suggestion of networkmanager.(secret-)flags and be pretty much a 1:1 representation of the setting in NetworkManager, but it would still keep the secret-flags far away from the actual secrets in Netplan's YAML schema. I wonder if we can do better than that?

We don't need to keep a 1:1 representation of Netplan <-> NetworkManager settings. Can we think of any (hypothetical/future) usecase where such flags could be used for another backend renderer, like networkd or OVS? If so that would be an argument against moving it into the networkmanager. backend settings.

I feel like those secret-flags should be tightly bundled with the secrets itself, which are bundled under the key[s] setting in Netplan. You're right that it shouldn't be in peers.keys.flags, as this is about the private key for the local connection. Rather, it should be in tunnels.wg0.keys.flags, which uses the same datastructure/schema as peers.keys.

For example

 network:
   renderer: NetworkManager
   tunnels:
     wg0:
       mode: wireguard
       port: 5182
       key:
         flags: [agent-owned]
       peers:
         - keys:
             public: rlbInAj0qV69CysWPQY7KEBnKxpYCpaWqOs/dLevdWc=
           allowed-ips: [0.0.0.0/0, "2001:fe:ad:de:ad:be:ef:1/24"]
           keepalive: 23
           endpoint: 1.2.3.4:5

daniloegea · 2023-07-12T17:10:38Z

tunnels.interface.keys.flags sounds like a good place to add it. Should we call it private-key-flags though? There are 3 possible keys one can use inside keys: input, output and private. Maybe we should just explicitly correlate the flags with the private key.

slyon · 2023-07-13T10:42:42Z

Should we call it private-key-flags though?

That works for me. It's close to the actual secret (if any) and could also be used to define (future) flags for other backends. So let's try to get @vorlonofportland review of this schema proposal:

 network:
   renderer: NetworkManager
   tunnels:
     wg0:
       mode: wireguard
       key:
         private-key-flags: [agent-owned]
       peers:
         [...]

vorlonofportland · 2023-07-20T00:56:29Z

This looks good to me, thanks.

slyon · 2023-07-20T07:32:17Z

This looks good to me, thanks.

Thanks! I've added the "schema ok" tag and will do a review of the implementation details next.

slyon · 2023-07-20T12:22:49Z

I wonder if we can use the same schema for WPA-PSK credentials stored in an agent?
See https://discourse.ubuntu.com/t/call-for-testing-networkmanager-yaml-settings/32420/8 (point 3) and https://developer-old.gnome.org/NetworkManager/stable/settings-802-11-wireless-security.html => psk-flags

slyon

Thanks, this is looking good overall. The docs & implementation need to be adopted to the agreed upon YAML schema. And I wonder if we could use the same data-structure for wifi-security.psk-flags, too?

I added some remarks inline.

src/names.c

src/abi.h

slyon · 2023-07-20T12:32:04Z

src/nm.c

@@ -344,6 +344,9 @@ write_wireguard_params(const NetplanNetDefinition* def, GKeyFile *kf, GError** e
            g_key_file_set_string(kf, "wireguard", "private-key", def->tunnel.private_key);
    }

+    if (def->backend_settings.private_key_flags != NETPLAN_PRIVATE_KEY_FLAG_NONE)
+        g_key_file_set_uint64(kf, "wireguard", "private-key-flags", def->backend_settings.private_key_flags);


thought: I wonder if the same data-structure could be used for [wifi-security].psk-flags?

If only wireguard and the the psk password will have flags we could use the same variable, as the netdef will never be wifi and wireguard at the same time. But if we want to extend it to all the other secrets we will need multiple variables...

The case for wifi é covered by the passthrough data, only the wireguard one was causing issues due to how NM parses the keyfile and how we were generating it. Do we want to extend it to wifi right now?

We should probably use separate fields/data-structures internally, as you suggested above. But stick to a similar (user-facing) YAML structure. Also, let's postpone the psk-flags to another PR, so we can get this one landed.

src/validation.c

src/parse.c

The enum basically shadows NM's NMSettingSecretFlags. It will be used to store the Wireguard's private key flag if defined. The main reason for that is that the private key might not be present in the keyfile. It might the owned by an external agent that handles credentials. This is Network Manager specific. See LP: #2024661.

The new property is a YAML sequence that might have the values "agent-owned", "not-saved" and "not-required". It's documented in the section "Secret flag types" in nm-settings-nmcli(5). The new key is placed in network.tunnels.<interface>.keys.

It will only fail validation if the private key is not defined and there are no key flags defined.

slyon

Thanks, lgtm!

daniloegea added the schema change This PR includes a change to netplan's YAML schema, which needs sign-off label Jun 23, 2023

slyon reviewed Jun 26, 2023

View reviewed changes

daniloegea force-pushed the wireguard_agent_owned_private_key branch 2 times, most recently from df190e1 to 791f296 Compare June 30, 2023 11:54

daniloegea requested a review from slyon June 30, 2023 14:17

slyon requested changes Jul 10, 2023

View reviewed changes

slyon added the schema ok label Jul 20, 2023

slyon self-requested a review July 20, 2023 07:32

slyon requested changes Jul 20, 2023

View reviewed changes

daniloegea force-pushed the wireguard_agent_owned_private_key branch 2 times, most recently from 3f92645 to d5e52e3 Compare July 24, 2023 12:56

daniloegea added 2 commits July 24, 2023 14:05

daniloegea force-pushed the wireguard_agent_owned_private_key branch from d5e52e3 to 818ee43 Compare July 24, 2023 13:12

daniloegea added 3 commits July 24, 2023 14:33

validation: use the new field to validate WG connections

5272ac4

It will only fail validation if the private key is not defined and there are no key flags defined.

tests: add tests for the new wireguard property

b59dd91

docs: add the new private-key-flags key to the documentation

e8c1c4c

daniloegea force-pushed the wireguard_agent_owned_private_key branch from 818ee43 to e8c1c4c Compare July 24, 2023 13:34

daniloegea requested a review from slyon July 24, 2023 14:16

slyon approved these changes Jul 24, 2023

View reviewed changes

daniloegea merged commit 6ade72a into canonical:main Jul 24, 2023
13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NetworkManager: Wireguard private key flag support #371

NetworkManager: Wireguard private key flag support #371

daniloegea commented Jun 23, 2023 •

edited

Loading

slyon Jun 26, 2023

daniloegea Jun 28, 2023

daniloegea Jun 30, 2023

daniloegea commented Jun 30, 2023

slyon left a comment

daniloegea commented Jul 12, 2023

slyon commented Jul 12, 2023 •

edited

Loading

daniloegea commented Jul 12, 2023

slyon commented Jul 13, 2023

vorlonofportland commented Jul 20, 2023

slyon commented Jul 20, 2023

slyon commented Jul 20, 2023

slyon left a comment

slyon Jul 20, 2023

daniloegea Jul 21, 2023

slyon Jul 24, 2023

slyon left a comment

NetworkManager: Wireguard private key flag support #371

NetworkManager: Wireguard private key flag support #371

Conversation

daniloegea commented Jun 23, 2023 • edited Loading

Description

Checklist

slyon Jun 26, 2023

Choose a reason for hiding this comment

daniloegea Jun 28, 2023

Choose a reason for hiding this comment

daniloegea Jun 30, 2023

Choose a reason for hiding this comment

daniloegea commented Jun 30, 2023

slyon left a comment

Choose a reason for hiding this comment

daniloegea commented Jul 12, 2023

slyon commented Jul 12, 2023 • edited Loading

daniloegea commented Jul 12, 2023

slyon commented Jul 13, 2023

vorlonofportland commented Jul 20, 2023

slyon commented Jul 20, 2023

slyon commented Jul 20, 2023

slyon left a comment

Choose a reason for hiding this comment

slyon Jul 20, 2023

Choose a reason for hiding this comment

daniloegea Jul 21, 2023

Choose a reason for hiding this comment

slyon Jul 24, 2023

Choose a reason for hiding this comment

slyon left a comment

Choose a reason for hiding this comment

daniloegea commented Jun 23, 2023 •

edited

Loading

slyon commented Jul 12, 2023 •

edited

Loading