GNOME Bugzilla – Bug 784471
[review] lr/connectivity-resolved: use systemd-resolved for connectivity checking
Last modified: 2020-11-12 14:31:39 UTC
We'd like to do per-device lookups and always use DNS for resolving, if possible. The glibc resolver can do neither. https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/connectivity-resolved
+resolved_proxy_created (GObject *object, GAsyncResult *res, gpointer user_data) +{ + NMConnectivity *self = NM_CONNECTIVITY (object); The first argument is the GDBusProxy, you should take @user_data as self. Also , I believe the cast macro can't be used before verifying that the operation wasn't canceled, because @self could be invalid if dispose() was called on it.
+ g_dbus_proxy_call (priv->resolve, + "ResolveHostname", indentation + g_variant_new ("(isit)", ifindex, cb_data->host, AF_UNSPEC, SD_RESOLVED_DNS), "t" wants a 64bit integer. SD_RESOLVED_DNS must be cast (or otherwise ensured that it's the right type). While at it, also cast AF_UNSPEC for clarify. host_and_port_from_uri() does not check for the URI scheme. You cannot assume that every scheme follows the same "scheme://host:port/" pattern of http URIs. I think you should also reject '@' in host. Of course, specifying any URIs beside http(s) is not supported anyway, so you can just check that the scheme is http(s). Also, the port number should be rejected if it isn't all numeric. The current parsing accepts many URIs that are invalid: ftp://user@host:677/ http://host:// http://host:XXX/ The parsing should rather be strict in what it accepts, better a false reject, then a false allow.
(In reply to Beniamino Galvani from comment #1) > +resolved_proxy_created (GObject *object, GAsyncResult *res, gpointer > user_data) > +{ > + NMConnectivity *self = NM_CONNECTIVITY (object); > > The first argument is the GDBusProxy, you should take @user_data as > self. Also , I believe the cast macro can't be used before verifying > that the operation wasn't canceled, because @self could be invalid if > dispose() was called on it. Fixed. (In reply to Thomas Haller from comment #2) > + g_dbus_proxy_call (priv->resolve, > + "ResolveHostname", > > > indentation Fixed. > + g_variant_new ("(isit)", ifindex, > cb_data->host, AF_UNSPEC, SD_RESOLVED_DNS), > > "t" wants a 64bit integer. SD_RESOLVED_DNS must be cast (or otherwise > ensured that it's the right type). > While at it, also cast AF_UNSPEC for clarify. Fixed. > host_and_port_from_uri() does not check for the URI scheme. You cannot > assume that every scheme follows the same "scheme://host:port/" pattern of > http URIs. > I think you should also reject '@' in host. > Of course, specifying any URIs beside http(s) is not supported anyway, so > you can just check that the scheme is http(s). > > Also, the port number should be rejected if it isn't all numeric. > > The current parsing accepts many URIs that are invalid: > > ftp://user@host:677/ > http://host:// > http://host:XXX/ > > The parsing should rather be strict in what it accepts, better a false > reject, then a false allow. Why all this? I mean, I'd very much prefer not implementing a complete URI parser in the connectivity checker. The string comes from a trusted source. Why would you use a nonsense one? Just to shoot yourself in your foot for no reason at all? In fact, nothing really terrible happens if someone enters something that we don't understand; just the systemd-resolved based resolving wouldn't be utilized.
(In reply to Lubomir Rintel from comment #3) > Why all this? > > I mean, I'd very much prefer not implementing a complete URI parser in the > connectivity checker. The string comes from a trusted source. Why would you > use a nonsense one? Just to shoot yourself in your foot for no reason at > all? In fact, nothing really terrible happens if someone enters something > that we don't understand; just the systemd-resolved based resolving wouldn't > be utilized. if somebody enters something that we don't understand, we should not try to resolve it. The string is user configuration, it should be strictly validated. You should not implement a complete URI parser. You should parse a very limited subset, and reject the rest. ^[hH][tT][tT][pP][sS]?://[a-z.A-Z0-9]+(:[0-9]+)?/.*$ and check that the hostname is not already a valid IP address. Reject everything else.
(In reply to Thomas Haller from comment #4) > (In reply to Lubomir Rintel from comment #3) > > ^[hH][tT][tT][pP][sS]?://[a-z.A-Z0-9]+(:[0-9]+)?/.*$ > > and check that the hostname is not already a valid IP address. Reject > everything else. Assuming you meant ^[hH][tT][tT][pP][sS]?://[a-z\.A-Z0-9]+(:[0-9]+)?/.*$ This is going to reject valid http URLs: http://hello.world http://hello-world/ http://žriebä/ http://xn--rieb-ooa33g/ Also, it's going to accept invalid URIs: http://xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ http://hello.world:1234567890/ Also, a regular expression is not too helpful; open-coding it is non-trivial and linking to a regex library a non-starter. Nevertheless, what I'm trying to say is that a half-functional attempt at pretending we grok the URIs is worse than not attempting at all. If the URI is invalid, then CURL is going to complain -- it is only important that we accept all good URIs; which we do.
(In reply to Lubomir Rintel from comment #5) > (In reply to Thomas Haller from comment #4) > > (In reply to Lubomir Rintel from comment #3) > > > > ^[hH][tT][tT][pP][sS]?://[a-z.A-Z0-9]+(:[0-9]+)?/.*$ > > > > and check that the hostname is not already a valid IP address. Reject > > everything else. > > Assuming you meant ^[hH][tT][tT][pP][sS]?://[a-z\.A-Z0-9]+(:[0-9]+)?/.*$ > > This is going to reject valid http URLs: > > http://hello.world > http://hello-world/ > http://žriebä/ > http://xn--rieb-ooa33g/ the regex was a quick example, not the final solution (also, I wasn't suggest to use g_regex_*)). Anyway, there should be a unit-test for the parsing, so that we can add there tests for what we accept and what we don't.
(In reply to Thomas Haller from comment #6) > (In reply to Lubomir Rintel from comment #5) > > (In reply to Thomas Haller from comment #4) > > > (In reply to Lubomir Rintel from comment #3) > > > > > > ^[hH][tT][tT][pP][sS]?://[a-z.A-Z0-9]+(:[0-9]+)?/.*$ > > > > > > and check that the hostname is not already a valid IP address. Reject > > > everything else. > > > > Assuming you meant ^[hH][tT][tT][pP][sS]?://[a-z\.A-Z0-9]+(:[0-9]+)?/.*$ > > > > This is going to reject valid http URLs: > > > > http://hello.world > > http://hello-world/ > > http://žriebä/ > > http://xn--rieb-ooa33g/ > > the regex was a quick example, not the final solution (also, I wasn't > suggest to use g_regex_*)). > > Anyway, there should be a unit-test for the parsing, so that we can add > there tests for what we accept and what we don't. What I'm trying to say that it's inplausible. It's just bits wasted. That would need a very good reason where there is none.
bugzilla.gnome.org is being shut down in favor of a GitLab instance. We are closing all old bug reports and feature requests in GNOME Bugzilla which have not seen updates for a long time. If you still use NetworkManager and if you still see this bug / want this feature in a recent and supported version of NetworkManager, then please feel free to report it at https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/ Thank you for creating this report and we are sorry it could not be implemented (workforce and time is unfortunately limited).