With old versions of openconnect we need to extract the port# from the
initial URL and then append it to the hostname we eventually get back.
Using strrchr(gw, ':') isn't going to work right with IPv6 literals,
ad we should also be dropping any path element.
So switch to using an int for the port instead of a string, and import a
cut-down variant of openconnect's internal_parse_url() which does
*largely* the same thing with strrchr() but is saved by using the 'end'
value returned from strtol() and insisting that the port is the very
end of the host part of the URL.
Rather than letting openconnect run, and whine that there's no gateway,
and making the user scroll up past the openconnect usage information,
give them an explicit error.
Pull a bunch of stuff into nm_vpn_openconnect_authenticate_helper() that
both callers were doing for themselves, and make its API a bit simpler.
It's given the NMSettingVpn and the GPtrArray of secrets, and it simply
succeeds or fails.
Ideally, we wouldn't have this hard-coded in NetworkManager itself; we
would invoke a tool to do it for us, like the GUI auth-dialog, which
can live in the NetworkManager-openconnect repository and be kept up
to date as new options are added.
To start with though, let's bring it into sync. We don't add new options
that often, and this will cover the majority of use cases.
Since OpenConnect 8.20, 'openconnect --authenticate' will return the
full gateway URL, including the hostname and the path. This allows
servers behind SNI-based proxies to work. To ensure we end up at the
same IP address even behind round-robin DNS, there is a separate
--resolve argument.
Update nmcli/nmtui to use this, as NetworkManager-openconnect does.
Shift some of the logic into the nm_vpn_openconnect_authenticate_helper()
function instead of duplicating it in the callers.
Also, pass the correct protocol in rather than only supporting Cisco
AnyConnect.
G_TYPE_CHECK_INSTANCE_CAST() can trigger a "-Wcast-align":
src/core/devices/nm-device-macvlan.c: In function 'parent_changed_notify':
/usr/include/glib-2.0/gobject/gtype.h:2421:42: error: cast increases required alignment of target type [-Werror=cast-align]
2421 | # define _G_TYPE_CIC(ip, gt, ct) ((ct*) ip)
| ^
/usr/include/glib-2.0/gobject/gtype.h:501:66: note: in expansion of macro '_G_TYPE_CIC'
501 | #define G_TYPE_CHECK_INSTANCE_CAST(instance, g_type, c_type) (_G_TYPE_CIC ((instance), (g_type), c_type))
| ^~~~~~~~~~~
src/core/devices/nm-device-macvlan.h:13:6: note: in expansion of macro 'G_TYPE_CHECK_INSTANCE_CAST'
13 | (G_TYPE_CHECK_INSTANCE_CAST((obj), NM_TYPE_DEVICE_MACVLAN, NMDeviceMacvlan))
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
Avoid that by using _NM_G_TYPE_CHECK_INSTANCE_CAST().
This can only be done for our internal usages. The public headers
of libnm are not changed.
This is the version shipped in Fedora 37. As Fedora 37 is now out, the
core developers switch to it. Our gitlab-ci will also use that as base
image for the check-{patch.tree} tests and to generate the pages. There
is a need that everybody agrees on which clang-format version to use,
and that version should be the one of the currently used Fedora release.
Also update the used Fedora image in "contrib/scripts/nm-code-format-container.sh"
script.
The gitlab-ci still needs update in the following commit. The change
in isolation will break the "check-tree" test.
NM_STR_BUF_INIT() and nm_str_buf_init() were pretty much redundant. Drop one of
them.
Usually our pattern is that we don't have functions that return structs.
But NM_STR_BUF_INIT() returns a struct, because it's convenient to use
with
nm_auto_str_buf NMStrBuf strbuf = NM_STR_BUF_INIT(...);
So use that variant instead.
Don't log the failure to spawn the auth dialog. This is polluting the
terminal when using nmcli when activating an OpenVPN profile if
/usr/libexec/nm-openvpn-auth-dialog is not available. Since nmcli can
still ask for the credentials, the missing auth dialog does not block
the activation, so the "warning" level is too much. Since it is a
library, any output to the terminal is bad, therefore remove the
logging.
Signed-off-by: Till Maas <opensource@till.name>
We use clang-format for automatic formatting of our source files.
Since clang-format is actively maintained software, the actual
formatting depends on the used version of clang-format. That is
unfortunate and painful, but really unavoidable unless clang-format
would be strictly bug-compatible.
So the version that we must use is from the current Fedora release, which
is also tested by our gitlab-ci. Previously, we were using Fedora 34 with
clang-tools-extra-12.0.1-1.fc34.x86_64.
As Fedora 35 comes along, we need to update our formatting as Fedora 35
comes with version "13.0.0~rc1-1.fc35".
An alternative would be to freeze on version 12, but that has different
problems (like, it's cumbersome to rebuild clang 12 on Fedora 35 and it
would be cumbersome for our developers which are on Fedora 35 to use a
clang that they cannot easily install).
The (differently painful) solution is to reformat from time to time, as we
switch to a new Fedora (and thus clang) version.
Usually we would expect that such a reformatting brings minor changes.
But this time, the changes are huge. That is mentioned in the release
notes [1] as
Makes PointerAligment: Right working with AlignConsecutiveDeclarations. (Fixes https://llvm.org/PR27353)
[1] https://releases.llvm.org/13.0.0/tools/clang/docs/ReleaseNotes.html#clang-format
In NetworkManager, a profile cannot have "ipvx.dns" or "ipvx.dns-search"
while the corresponding IP method is disabled. Together with the oddity
that in NetworkManager DNS settings are separate per IPv4 and IPv6, this
causes problems:
$ cat wg0.conf
[Interface]
PrivateKey = CBXpiLxQ98TLISJ2cypEFtQb/djzYzENyy0jzhWa/UA=
Address = 192.168.1.100
DNS = 10.11.12.13, foobar.de
[Peer]
PublicKey = Wus1sBzZiQkyxr6ZitUFNvfYD7KJkwTsWlcxvJ/4SHI=
Endpoint = 1.2.3.4:51827
AllowedIPs = 0.0.0.0/0
$ nmcli connection import type wireguard file wg0.conf
Error: failed to import 'wg0.conf': Failed to create WireGuard connection: ipv6.dns-search: this property is not allowed for 'method=disabled'.
Fixes: 3ab082ed96 ('cli: support dns-search for import of WireGuard profiles')
This is actually trying *too* hard to prevent DNS leaks, breaking normal
expected use of split DNS. Let systemd-resolved handle sending our DNS
queries to the right place instead.
It's true that NetworkManager is trying to emulate the behavior of
wg-quick here, and wg-quick uses 'resolvconf -x' to attempt to set
"exclusive" DNS. But with systemd-resolved this is implemented by
setting a ~. routing domain for the Wireguard interface. That is a
*really* big hammer already, since Domain=~. overrides +DefaultRoute,
ensuring most DNS queries can only go to other interfaces with Domain=~.
NetworkManager follows systemd-resolved's recommended convention by only
applying Domain=~. to other "privacy VPNs" since 1.26.6. Setting DNS
priority only prevents *domain-specific* "leaks", which are almost
always desired. For example, it prevents using both the Wireguard VPN
and a corporate VPN at the same time.
Note that all of the justification behind !688 applies here as well.
See-also: https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/688https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/585https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/merge_requests/901
This is confusing Coverity:
Error: RESOURCE_LEAK (CWE-772): [#def249] [important]
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:810: alloc_fn: Storage is returned from allocation function "g_string_free".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:810: var_assign: Assigning: "auth_dialog_request_str" = storage returned from "g_string_free(auth_dialog_request, 0)".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:822: noescape: Resource "auth_dialog_request_str" is not freed or pointed-to in "g_output_stream_write_async".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:822: noescape: Resource "auth_dialog_request_str" is not freed or pointed-to in "g_output_stream_write_async".
NetworkManager-1.31.5/src/libnmc-base/nm-secret-agent-simple.c:838: leaked_storage: Variable "auth_dialog_request_str" going out of scope leaks the storage it points to.
# 836| data);
# 837|
# 838|-> return TRUE;
# 839| }
# 840|
Maybe this works better to avoid the warning. At least, it also
documents it better to the reader.
Found by Coverity:
Error: RESOURCE_LEAK (CWE-772): [#def271] [important]
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: alloc_fn: Storage is returned from allocation function "nm_utils_ssid_to_utf8".
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: var_assign: Assigning: "ssid_utf8" = storage returned from "nm_utils_ssid_to_utf8(g_bytes_get_data(ssid, NULL), g_bytes_get_size(ssid))".
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:877: noescape: Resource "ssid_utf8" is not freed or pointed-to in "g_strdup_printf".
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:882: leaked_storage: Variable "ssid_utf8" going out of scope leaks the storage it points to.
# 880|
# 881| if (!add_wireless_secrets(request, secrets))
# 882|-> goto out_fail;
# 883| } else if (nm_connection_is_type(request->connection, NM_SETTING_WIRED_SETTING_NAME)) {
# 884| title = _("Wired 802.1X authentication");
Error: RESOURCE_LEAK (CWE-772): [#def272] [important]
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: alloc_fn: Storage is returned from allocation function "nm_utils_ssid_to_utf8".
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:874: var_assign: Assigning: "ssid_utf8" = storage returned from "nm_utils_ssid_to_utf8(g_bytes_get_data(ssid, NULL), g_bytes_get_size(ssid))".
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:877: noescape: Resource "ssid_utf8" is not freed or pointed-to in "g_strdup_printf".
NetworkManager-1.31.3/src/libnmc-base/nm-secret-agent-simple.c:883: leaked_storage: Variable "ssid_utf8" going out of scope leaks the storage it points to.
# 881| if (!add_wireless_secrets(request, secrets))
# 882| goto out_fail;
# 883|-> } else if (nm_connection_is_type(request->connection, NM_SETTING_WIRED_SETTING_NAME)) {
# 884| title = _("Wired 802.1X authentication");
# 885| msg = g_strdup_printf(_("Secrets are required to access the wired network %s"),
Fixes: 3fbabde4c3 ('libnm-core: replace GByteArray with pointer + length in some APIs')
- use strstrip() to remove leading and trailing whitespace
- use _nm_utils_ascii_str_to_int64() for parsing numeric values
like -1, 0 and 1. In particular, this now also allows passing
the numeric values.
- also accept "default" as valid value for NM_TERNARY_DEFAULT.
With this change, nmc_string_to_ternary() can also parse everything that
we commonly and currently parse with _nm_utils_enum_from_str_full()
and NM_TYPE_TERNARY. This will allow to configure ternary values in
a more flexible way.
- use strstrip() to remove leading and trailing whitespace
- use _nm_utils_ascii_str_to_int64() for parsing numeric values
like 0 and 1. The difference is small, for one, it also accepts
hex numbers like 0x1. More interestingly, it uses our common
number parsing function, and we will later do the same for
parsing ternaries.
g_set_error(error, 1, 0, ...) is not right. "1" is not a valid GQuark,
we should initialize proper error instances.
Use nm_utils_error_set() for that.
Also, the code previously hacked the numeric value "1" to indicate
ambiguous text. Add and use a new error code NM_UTILS_ERROR_AMBIGUOUS
for that.
With a const argument, we can make variables static const,
which means the linker loads the memory as read only.
Also, use NM_CAST_STRV_CC() macro, which casts the argument
accordingly.
