Fix 8021x eap method parsing (LP: #2016625)

[daniloegea]

Description

See LP: #2016625 for more details.

I'm also including a little tool that was useful during investigation and tests.

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]

Nice. This looks like a good solution to handle the common case. While managing the NM vs Netplan incompatibilities via passthrough.

Just one little inline comment.

[slyon]

thought: What happens if 802-1x.eap=garbage;. I guess in that case we would clear the passthrough but produce an potentially invalid YAML... We should probably handle that else case here.

[daniloegea]

If it's just "garbage;" nothing will happen, because the string doesn't match any of the conditions and there is nothing after the ";". But if it's "garbage;moregarbage;" for example, it will end up in the passthrough section only. The YAML will be fine but the resulting nmconnection will have this 802-1x.eap=garbage;moregarbage;.

[daniloegea]

I mean, we will generate a broken keyfile if the keyfile used as input was also broken.

[slyon]

I think the problem is that if we don't set any value in auth->eap_method the generator in nm.c will fallback to PSK mode instead of WPA-EAP, dropping the "identity", "password", "ca-cert", "phase2-auth", ... fields and adding "psk" instead. Which will corrupt the connection data.

What do you think about a workaround like this, using a handled helper variable and setting it to an arbitrary value (NETPLAN_AUTH_EAP_TLS) that will be overwritten by passthrough (it's an edge case anyway..). Maybe also emitting a warning about it?

        gboolean handled = FALSE;
        if (g_strcmp0(first_method, "tls") == 0) {
            auth->eap_method = NETPLAN_AUTH_EAP_TLS;
            handled = TRUE;
        } else if (g_strcmp0(first_method, "peap") == 0) {
            auth->eap_method = NETPLAN_AUTH_EAP_PEAP;
            handled = TRUE;
        } else if (g_strcmp0(first_method, "ttls") == 0) {
            auth->eap_method = NETPLAN_AUTH_EAP_TTLS;
            handled = TRUE;
        } else {
            // FIXME: This is a workaround to keep the nm.c generator stick with WPA-EAP,
            // instead of falling back to PSK. EAP method will be a passthrough override.
            auth->eap_method = NETPLAN_AUTH_EAP_TLS;
            handled = FALSE;
        }
        /* If "method" (which is a list separated by ";") has more than one value,
         * we keep the key so it will also be written as a passthrough key.
         * That's required because Network Manager accepts multiple methods
         * but Netplan accepts only one.
         *
         * TODO: eap_method needs to be fixed to store multiple methods.
         */
        if (handled && (split[1] == NULL || !g_strcmp0(split[1], "")))
            _kf_clear_key(kf, "802-1x", "eap");

[daniloegea]

But imagine this, the user has this in their nmconnection file:

[802-1x]
eap=blah

In this case we fallback to the else statement, we will default to eap-tls, emit a YAML with auth.method: tls and add 802-1x.eap=blah to passthrough.

When we call the generator, it will find the auth.method: tls in the YAML but it will end up being overridden with the value from the passthrough eap=blah. All the other required fields will be emitted thanks to the method: tls but it's still a broken nmconnection.

Your suggestion would allow users to use other values supported by NM but not supported by Netplan though (from nm-settings(5): "leap", "md5", "tls", "peap", "ttls", "pwd", and "fast"). But the right way to do that would be explicitly supporting these values as part of our grammar.

nmcli will not allow me to add a connection like that without the eap method, so if the user tries to use an nmconnection file without that field, we should probably error out instead of using a default value for it. But by doing that we would be validating NM keyfiles and do we want to implement keyfiles parsing/validation?

[slyon]

The remark mentioned above is an edge case, which we don't expect to hit. I'll unblock this PR by giving my approval. I'll leave it up to you if you want to follow up with the recommendation I made above, or keep it as-is and do the merge.

[daniloegea]

I left a comment above with reasons why I believe we shouldn't do that. I'll go ahead and merge it but we can go back to it next week in case I'm missing something.