The number of user-datas attached to the NMAuthChain is generally fixed
and small.
For example, in current code impl_manager_get_permissions() will be the
instance that ends up with the largest number of data items. It
performs zero calls of nm_auth_chain_set_data() but 16 times
nm_auth_chain_add_call(). So currently the maximum number is 16.
With such a fixed, small number of elements it is expected to be more
efficient to just track the elements in a CList instead of a GHashTable.
- consistently name the ChainData variable "chain_data"
- return the ChainData element from _get_data(). This way it
also can be used by nm_auth_chain_steal_data(), which needs
the ChainData element.
NMAuthChain is not ref-counted.
You may call nm_auth_chain_destroy() once before the callback
gets invoked. This destroys the auth-chain instance right away.
You may call nm_auth_chain_destroy() once from inside the callback.
This basically has no effect but is allowed for convenince.
All this does is remembering that destroy was called and asserts that
destroy gets called at most once.
After the callback returns, the auth-chain will always be destroyed.
That means, generally there is no need to call nm_auth_chain_destroy()
from inside the callback.
Remove that code, and refactor some code to return early (where it makes
sense).
NMAuthChain is not really ref-counted, there is no need for that additional
complexity.
But it is graceful towards calling nm_auth_chain_destroy() from inside the
callback. The caller may do so.
But we don't need a "ref_count" to track that. Two flags suffice: one to
say whether destroy was called and one to indicate that we are in the
process of finishing (to delay deallocating the NMAuthChain struct).
We already had the "done" flag that we used to indicate that the chain
is finished. So, we just need one more flag instead.
From the files under "shared/nm-utils" we build an internal library
that provides glib-based helper utilities.
Move the files of that basic library to a new subdirectory
"shared/nm-glib-aux" and rename the helper library "libnm-core-base.la"
to "libnm-glib-aux.la".
Reasons:
- the name "utils" is overused in our code-base. Everything's an
"utils". Give this thing a more distinct name.
- there were additional files under "shared/nm-utils", which are not
part of this internal library "libnm-utils-base.la". All the files
that are part of this library should be together in the same
directory, but files that are not, should not be there.
- the new name should better convey what this library is and what is isn't:
it's a set of utilities and helper functions that extend glib with
funcitonality that we commonly need.
There are still some files left under "shared/nm-utils". They have less
a unifying propose to be in their own directory, so I leave them there
for now. But at least they are separate from "shared/nm-glib-aux",
which has a very clear purpose.
(cherry picked from commit 80db06f768)
It makes sense to use NMAuthChain also when not attaching any user-data to
the chain. The main reason would be, the ability to schedule multiple permission
checks in parallel, and wait for them to complete together.
Only allocate the hash-table, when we really need it.
For one, we already do <trace> level logging inside NMAuthManager.
So, at trace level we have everything.
If a request fails, it's not up to NMAuthChain to log a warning.
This makes NMAuthCallResult not only usable from within a NMAuthChain.
It makes sense to just call nm-auth-manager directly, but then we need
a way to convert the more detailed result into an NMAuthCallResult
value.
NMAuthChain's nm_auth_chain_add_call() used to add special handling for
the NMAuthSubject. This handling really belongs to NMAuthManager for two
reasons:
- NMAuthManager already goes through the effort of scheduling an idle
handler to handle the case where no GDBusProxy is present. It can
just as well handle the special cases where polkit-auth is disabled
or when we have internal requests.
- by NMAuthChain doing special handling, it makes it more complicated
to call nm_auth_manager_check_authorization() directly. Previously,
the NMAuthChain had additional logic, which means you either were
forced to create an NMAuthChain, or you had to reimplement special
handling like nm_auth_chain_add_call().
Supporting PolicyKit required no additional library, just extra code
to handle the D-Bus calls. For that, there was a compile time option
to even stip out that code. Note, that you could (and still can)
configure the system not to use policy-kit. The point was to reduce
the binary size in case you don't need it.
Remove this. I guess, we we aim for such aggressive optimization of
the binary size, we should instead make all device types disablable
at configuration time. We don't do that either and other low hanging
fruits, because it's better to always enable features, unless they
require external dependencies.
Also, the next commit will make more use of NMAuthManager. So, having
it disabled at compile time, makes even less sense.
Don't use the GAsyncResult pattern for internal API of auth-manager. Instead,
use a simpler API that has a more strict API and simpler use.
- return a call-id handle when scheduling the authorization request.
The request is always scheduled asynchronsously and thus call-id
is never %NULL.
- the call-id can be used to cancel the request. It can be used exactly
once, and only before the callback is invoked.
- the async keeps the auth-manager alive. It needs to do so, because
when cancelling the request we might not yet be done: instead we
might still need to issue a CancelCheckAuthorization call (which
we need to handle as well).
- the callback is always invoked exactly once.
Currently NMAuthManager's API effectivly is only called by NMAuthChain.
The point of this is to make NMAuthManager's API more consumable, and
thus let users use it directly (instead of using the NMAuthChain layer).
As well known, we don't do a good job during shutdown of NetworkManager
to release all resources and cancel pending requests. This rework also
makes it possible to actually get this right. See the comment in
nm_auth_manager_force_shutdown(). But yes, it is still a bit complicated
to do a controlled shutdown, because we cannot just synchronously
complete. We need to issue CancelCheckAuthorization D-Bus calls, and
give these requests time to complete. The new API introduced by this patch
would make that easier.
NMAuthChain schedules (possibly) multiple authentication requests.
When they all complete, it will once invoke the result-callback.
There is no need to schedule this result-callback on another idle-handler,
because nm_auth_manager_polkit_authority_check_authorization() should guarantee
to invoke the callback never-synchronously and on a clean call-stack (to avoid
problems with re-entrancy). At that point, NMAuthChain does not need to
delay this further.
NMAuthChain is not really ref-counted. True, we have an internal ref-counter
to ensure that the instance stays alive while the callback is invoked. However,
the user cannot take additional references as there is no nm_auth_chain_ref().
When the user wants to get rid of the auth-chain, with the current API it
is important that the callback won't be called after that point. From the
name nm_auth_chain_unref(), it sounds like that there could be multiple references
to the auth-chain, and merely unreferencing the object might not guarantee that
the callback is canceled. However, that is luckily not the case, because
there is no real ref-counting involved here.
Just rename the destroy function to make this clearer.
- instead of allocating memory separately for the @tag (key)
and ChainData (data), store the tag also inside ChainData.
- instead of adding two separate key and value items to GHashTable,
use g_hash_table_add(), which is optimized for this case.
auth_call_complete() had two callers: once from the idle handler, and
once from pk_call_cb(). The conditions are slightly different, split
the function in two. For one, this allows to unset the obsolete
call_idle_id.
Next we will use siphash24() instead of the glib version g_direct_hash() or
g_str_hash(). Hence, the "nm-utils/nm-hash-utils.h" header becomes very
fundamental and will be needed basically everywhere.
Instead of requiring the users to include them, let it be included via
"nm-default.h" header.
Replace the usage of g_str_hash() with our own nm_str_hash().
GLib's g_str_hash() uses djb2 hashing function, just like we
do at the moment. The only difference is, that we use a diffrent
seed value.
Note, that we initialize the hash seed with random data (by calling
getrandom() or reading /dev/urandom). That is a change compared to
before.
This change of the hashing function and accessing the random pool
might be undesired for libnm/libnm-core. Hence, the change is not
done there as it possibly changes behavior for public API. Maybe
we should do that later though.
At this point, there isn't much of a change. This patch becomes
interesting, if we decide to use a different hashing algorithm.
src/nm-auth-utils.c:343:6: error: 'is_authorized' may be used uninitialized in this function [-Werror=maybe-uninitialized]
if (is_authorized) {
^
src/nm-auth-utils.c:320:11: note: 'is_authorized' was declared here
gboolean is_authorized, is_challenge;
^
src/nm-auth-utils.c:346:13: error: 'is_challenge' may be used uninitialized in this function [-Werror=maybe-uninitialized]
} else if (is_challenge) {
^
src/nm-auth-utils.c:320:26: note: 'is_challenge' was declared here
gboolean is_authorized, is_challenge;
^
(cherry picked from commit 24ab2a4945)
It could be that the client is just newer and it's just too harsh to
fail the whole request. Leave the unknown permission in unknown and
possibly proceed filling in the rest.
Fix the following build error:
nm-auth-utils.c: In function ‘nm_auth_chain_add_call’:
nm-auth-utils.c:402:46: error: ‘DBUS_GERROR’ undeclared (first use in this function)
call->chain->error = g_error_new_literal (DBUS_GERROR,
Fixes: 1cf35cb26b
GError codes are only unique per domain, so logging the code without
also indicating the domain is not helpful. And anyway, if the error
messages are not distinctive enough to tell the whole story then we
should fix the error messages.
Based-on-patch-by: Dan Winship <danw@gnome.org>
- All internal source files (except "examples", which are not internal)
should include "config.h" first. As also all internal source
files should include "nm-default.h", let "config.h" be included
by "nm-default.h" and include "nm-default.h" as first in every
source file.
We already wanted to include "nm-default.h" before other headers
because it might contains some fixes (like "nm-glib.h" compatibility)
that is required first.
- After including "nm-default.h", we optinally allow for including the
corresponding header file for the source file at hand. The idea
is to ensure that each header file is self contained.
- Don't include "config.h" or "nm-default.h" in any header file
(except "nm-sd-adapt.h"). Public headers anyway must not include
these headers, and internal headers are never included after
"nm-default.h", as of the first previous point.
- Include all internal headers with quotes instead of angle brackets.
In practice it doesn't matter, because in our public headers we must
include other headers with angle brackets. As we use our public
headers also to compile our interal source files, effectively the
result must be the same. Still do it for consistency.
- Except for <config.h> itself. Include it with angle brackets as suggested by
https://www.gnu.org/software/autoconf/manual/autoconf.html#Configuration-Headers
- move nm_auth_chain_check_done() and nm_auth_chain_remove_call()
into the only caller auth_call_complete().
- take a ref of the "context" argument.
- in nm_auth_chain_add_call(), assert that we didn't yet invoke the
done-callback. The auth-chain should not be reusued.
- use slice allocator for ChainData, AuthCall and NMAuthChain
Port remaining bits to gdbus and remove stray dbus-glib references
Drop the dbus-glib version check from configure, since nothing depends
on new dbus-glib any more.
Move nm-dbus-glib-types.h and nm-gvaluearray-compat.h from include/ to
libnm-util/ since they are now only used by libnm-util and libnm-glib.
Move D-Bus export/unexport handling into NMExportedObject and remove
type-specific export/get_path methods (export paths are now specified
at the class level, and NMExportedObject handles the counters for all
exported types automatically).
Since all exportable objects now use the same get_path() method, we
can also add some helper methods to simplify get_property()
implementations for object-path and object-path-array properties.
Rather than randomly including one or more of <glib.h>,
<glib-object.h>, and <gio/gio.h> everywhere (and forgetting to include
"nm-glib-compat.h" most of the time), rename nm-glib-compat.h to
nm-glib.h, include <gio/gio.h> from there, and then change all .c
files in NM to include "nm-glib.h" rather than including the glib
headers directly.
(Public headers files still have to include the real glib headers,
since nm-glib.h isn't installed...)
Also, remove glib includes from header files that are already
including a base object header file (which must itself already include
the glib headers).
==5177== 6 (+6) bytes in 1 (+1) blocks are definitely lost in loss record 118 of 6,581
==5177== at 0x4C29BCF: malloc (in /usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==5177== by 0x7F4A6F5: g_malloc (gmem.c:97)
==5177== by 0x7F6301E: g_strdup (gstrfuncs.c:356)
==5177== by 0x4AD902: nm_auth_chain_set_data (nm-auth-utils.c:194)
==5177== by 0x50919E: impl_agent_manager_register_with_capabilities (nm-agent-manager.c:323)
==5177== by 0x62649BE: invoke_object_method (dbus-gobject.c:1899)
==5177== by 0x62649BE: object_registration_message (dbus-gobject.c:2161)
==5177== by 0x649D5CE: _dbus_object_tree_dispatch_and_unlock (dbus-object-tree.c:1018)
==5177== by 0x648F193: dbus_connection_dispatch (dbus-connection.c:4718)
==5177== by 0x6261DB4: message_queue_dispatch (dbus-gmain.c:90)
==5177== by 0x7F44AEA: g_main_dispatch (gmain.c:3111)
==5177== by 0x7F44AEA: g_main_context_dispatch (gmain.c:3710)
==5177== by 0x7F44E87: g_main_context_iterate.isra.29 (gmain.c:3781)
==5177== by 0x7F451B1: g_main_loop_run (gmain.c:3975)
Access to connection configuration should not be blocked by absence of a
user session tracked using logind or consolekit. Access control based on
UID is sufficient.
This patch ensures that the user can always access connections even if
he doesn't have a session tracked by logind or consolekit and even when
NetworkManager is not built with logind or consolekit support.
Please note that presence or absence of a session tracked by logind or
consolekit doesn't carry any security information.
Acked-By: Thomas Haller <thaller@redhat.com>
Acked-By: Dan Williams <dcbw@redhat.com>
There's no need to call `nm_session_monitor_get()` individually for each
call to `nm_auth_is_subject_in_acl()`.
Acked-By: Thomas Haller <thaller@redhat.com>
config.h should be included from every .c file, and it should be
included before any other include. Fix that.
(As a side effect of how I did this, this also changes us to
consistently use "config.h" rather than <config.h>. To the extent that
it matters [which is not much], quotes are more correct anyway, since
we're talking about a file in our own build tree, not a system
include.)
Let the user completly disable polkit authentication by
building NM with configure option '--enable-polkit=disabled'.
In that case, configuring 'main.auth-polkit=yes' will fail all
authentication requests (except root-requests, which are always granted).
This reduces the size of the NetworkManager binary by some 26KB (16KB
stripped).
Signed-off-by: Thomas Haller <thaller@redhat.com>
This makes NetworkManager independent of <polkit/polkit.h>
development headers and libpolkit-gobject-1.so library.
Instead communicate directly with polkit using its DBUS
interface.
PolicyKit support is now always compiled in. You can control
polkit authorization with the configuration option
[main]
auth-polkit=yes|no
If the configure option is omitted, a build time default
value is used. This default value can be set with the
configure option --enable-polkit.
This commit adds a new class NMAuthManager that reimplements the
relevant DBUS client parts. It takes source code from the polkit
library.
https://bugzilla.gnome.org/show_bug.cgi?id=734146
Signed-off-by: Thomas Haller <thaller@redhat.com>
