|
Description
Jan Alexander Steffens (heftig)
2015-11-07 16:07:44 UTC
Created attachment 119464 [details] [review] [PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses The default policy already disallows calls on system buses. Since any bus with a service helper cleans the environment anyway, there's no point in allowing this to be called. Created attachment 119465 [details] [review] [PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd If we use systemd activation, forward all UpdateActivationEnvironment requests to org.freedesktop.systemd1.Manager.SetEnvironment, in order to ensure variables needed by D-Bus services are available when these services are launched by systemd. Since UpdateActivationEnvironment is not available on the system bus, this only applies to user buses. Created attachment 119466 [details] [review] [PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding Created attachment 119467 [details] [review] [PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID Attempting to call SetEnvironment on systemd causes it to inquire about the caller's connection UID and PID. If this check fails, the call is rejected. Comment on attachment 119464 [details] [review] [PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses Review of attachment 119464 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1011,4 @@ > } > #endif > > + { Opening an artificial block (where we don't already have an "if", "while", etc.) just as a scope for variables is unusual style: we normally only do this inside #ifdefs, if at all. I'd normally declare BusContext at the top of the function with the rest of the local variables (retval, etc.), and assign to it just before use. But this is fine too. Comment on attachment 119465 [details] [review] [PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd Review of attachment 119465 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +991,5 @@ > + _DBUS_ASSERT_ERROR_IS_CLEAR (error); > + > + service_name = dbus_message_get_destination (message); > + > + _dbus_assert (service_name != NULL); I think this deserves a pseudo-doc-comment, something like /* * message must already have a non-NULL destination. */ above the function definition. @@ +1011,5 @@ > + return FALSE; > + } > + > + if (!bus_activation_activate_service (activation, connection, transaction, > + TRUE, message, service_name, error)) Hmm. I think it's incorrect to pass a non-NULL connection here: conceptually, systemd is being activated by dbus-daemon itself, not by the caller of UpdateActivationEnvironment. (In practice it doesn't matter much in the auto_activation=TRUE code path, but I'd prefer to be consistent, in case it does matter later.) I'd prefer this function to take the BusContext as its first argument. The caller can get it from the DBusConnection - it's essentially a container for global singletons, and has methods to get the BusActivation and BusRegistry. If there is some reason why you *need* the DBusConnection here, it should be named something like "initiator" to distinguish it from any other connections that might be floating around, like the connection to systemd. The dbus-daemon code has a bad habit of using "connection" as a variable name where it should really be using "sender" or "destination" or something. @@ +1189,4 @@ > > if (!bus_activation_set_environment_variable (activation, > key, value, error)) > + { In future please don't re-indent unless you have to change that line anyway... but this is small, and it's changing from "wrong style" to "correct style", so it's OK. @@ +1202,5 @@ > + > + /* SetEnvironment wants an array of KEY=VALUE strings */ > + if (!_dbus_string_init (&envline) || > + !_dbus_string_append_printf (&envline, "%s=%s", key, value) || > + (s = _dbus_string_get_data (&envline)) == NULL || If init'ing the string succeeded, which it did, then _dbus_string_get_data() can't actually return NULL, so there's no need for this check. I prefer to treat assignment as a statement (like it is in Python) rather than using it as an expression: if (!init (...) || !append_printf (...)) { /* ... recover */ } s = _dbus_string_get_data (&envline); if (!append_basic (&systemd_iter, &s)) { /* recover */ } Comment on attachment 119466 [details] [review] [PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding Review of attachment 119466 [details] [review]: ----------------------------------------------------------------- ::: test/sd-activation.c @@ +65,5 @@ > g_assert_cmpint (dbus_message_get_reply_serial (m), ==, 0); \ > } while (0) > > +#define assert_method_call(m, sender, path, iface, \ > + method, signature) \ Did you copy this from the monitor test? If not, please do. We should assert about the destination, not just the sender, and I think the version in the monitor test already does. (g_assert_cmpstr accepts NULL, so it's fine to have a nullable destination parameter if needed.) Comment on attachment 119467 [details] [review] [PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID Review of attachment 119467 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1754,5 @@ > reply = dbus_message_new_method_return (message); > if (reply == NULL) > goto oom; > > + if (found != BUS_DRIVER_FOUND_PEER || Would benefit from a comment something like /* We don't know how to find "ADT audit session data" * for the dbus-daemon itself (or even whether that would be * meaningful) */ @@ +1815,5 @@ > > + if (found == BUS_DRIVER_FOUND_PEER) > + context = bus_connection_get_selinux_id (conn); > + else > + context = NULL; Would benefit from a comment /* Ideally we would also know how to get the security context * of the dbus-daemon itself, but someone who knows SELinux * will need to provide and test that patch */ @@ +1880,5 @@ > + if ((found == BUS_DRIVER_FOUND_PEER && > + dbus_connection_get_unix_process_id (conn, &ulong_val) && > + ulong_val <= _DBUS_UINT32_MAX) || > + (found == BUS_DRIVER_FOUND_SELF && > + (ulong_val = _dbus_getpid ()) != DBUS_PID_UNSET)) I'd prefer to break this up, treating assignments as expressions not statements. Maybe something like /* ... an "if/else if" or "switch" to set ulong_val */ /* we can't represent > 32-bit pids; ... */ if (ulong_val <= _DBUS_UINT32_MAX && ulong_val != DBUS_PID_UNSET) { /* ... add it */ } @@ +1892,5 @@ > + if ((found == BUS_DRIVER_FOUND_PEER && > + dbus_connection_get_unix_user (conn, &ulong_val) && > + ulong_val <= _DBUS_UINT32_MAX) || > + (found == BUS_DRIVER_FOUND_SELF && > + (ulong_val = _dbus_getuid ()) != DBUS_UID_UNSET)) Similarly, I'd prefer to break this up a bit. @@ +1899,5 @@ > goto oom; > } > > + if (found == BUS_DRIVER_FOUND_PEER && > + dbus_connection_get_windows_user (conn, &s)) Comment /* ideally this would know how to get the dbus-daemon's Windows SID */ (I might implement this myself later - it's testable with Wine.) @@ +1922,5 @@ > dbus_free (s); > } > > + if (found == BUS_DRIVER_FOUND_PEER && > + _dbus_connection_get_linux_security_label (conn, &s)) /* ideally this would know how to get the dbus-daemon's label too */ ::: dbus/dbus-sysdeps-win.c @@ +2138,5 @@ > return GetCurrentProcessId (); > } > > +/** Gets our UID > + * @returns process UID "Gets our Unix uid. On Windows, this just returns DBUS_UID_UNSET." (or something) ::: dbus/dbus-sysdeps.h @@ +641,5 @@ > DBUS_PRIVATE_EXPORT > dbus_pid_t _dbus_getpid (void); > > +DBUS_PRIVATE_EXPORT > +dbus_uid_t _dbus_getuid (void); Don't you also need to remove it from dbus-sysdeps-unix.h? (In reply to Simon McVittie from comment #7) Thanks for your reviews. I'll post adjusted patches soon. > > +#define assert_method_call(m, sender, path, iface, \ > > + method, signature) \ > > Did you copy this from the monitor test? If not, please do. We should assert > about the destination, not just the sender, and I think the version in the > monitor test already does. IIRC I removed it because the destination was a unique name. Should I assert on that name? (In reply to Simon McVittie from comment #8) > /* Ideally we would also know how to get the security context > * of the dbus-daemon itself, but someone who knows SELinux > * will need to provide and test that patch */ > /* ideally this would know how to get the dbus-daemon's Windows SID */ > /* ideally this would know how to get the dbus-daemon's label too */ Should these be FIXMEs? (In reply to Jan Alexander Steffens (heftig) from comment #9) > > We should assert > > about the destination, not just the sender, and I think the version in the > > monitor test already does. > > IIRC I removed it because the destination was a unique name. Should I assert > on that name? Yes please. You can get the unique name from dbus_bus_get_unique_name(). In the monitor test, they're all cached in the Fixture to make these assertions easier. > (In reply to Simon McVittie from comment #8) > > /* Ideally we would also know how to get the security context (etc.) > > Should these be FIXMEs? If you like - there's no strong policy either way. I don't think writing FIXME necessarily makes it any more or less likely that they will be fixed, particularly the ones that require LSM knowledge. Created attachment 119657 [details] [review] [PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses Adjusted: - Removed scope used only for variable Created attachment 119658 [details] [review] bus_driver_handle_update_activation_environment: Forward to systemd Adjusted: - bus_driver_send_or_activate now takes only a BusContext instead of a DBusConnection and a BusTransaction; a transaction is created internally - Building the systemd message no longer uses assignment in a conditional Created attachment 119659 [details] [review] [PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd Created attachment 119660 [details] [review] [PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding Adjusted: - Copy complete asserts from the monitor test, now matching destinations This also highlighted how bus_transaction_send_from_driver overwrites the destination with the unique name; the activation leaves it alone. Created attachment 119661 [details] [review] [PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID Adjusted: - FIXMEs added for missing information about the org.freedesktop.DBus "connection" - Restructured code to avoid assigning in conditions Comment on attachment 119657 [details] [review] [PATCH 1/4] bus_driver_handle_update_activation_environment: Error on system buses Review of attachment 119657 [details] [review]: ----------------------------------------------------------------- Makes sense. I'll probably apply this next week. Comment on attachment 119659 [details] [review] [PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd Review of attachment 119659 [details] [review]: ----------------------------------------------------------------- ::: bus/driver.c @@ +1002,5 @@ > + > + service = bus_registry_lookup (bus_context_get_registry (context), > + &service_string); > + > + transaction = bus_transaction_new (context); The existing transaction was previously a parameter, and I think it should stay a parameter, instead of you providing a new one. The purpose of the transactions is that, as much as possible, a method call should either entirely fail with OOM or entirely succeed. In this case, if there is enough memory to send SetEnvironment to systemd *and* update the environment *and* send a success reply back to the caller, we should do all of those; but if there is insufficient memory, we should not do any of them. The transaction methods that say they send a message do not actually send it: "send" in the method name is shorthand for "queue the message to be sent on commit, pre-allocating all the memory that you will need". Comment on attachment 119660 [details] [review] [PATCH 3/4] test/sd-activation: Test UpdateActivationEnvironment forwarding Review of attachment 119660 [details] [review]: ----------------------------------------------------------------- Makes sense (but I haven't reviewed in great detail, I'll do so next week) Comment on attachment 119661 [details] [review] [PATCH 4/4] bus-driver: Support returning org.freedesktop.DBus UID and PID Review of attachment 119661 [details] [review]: ----------------------------------------------------------------- Looks good from a brief review. I'll check this more thoroughly next week. Created attachment 119683 [details] [review] [PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd Adjusted: - bus_driver_send_or_activate takes the BusTransaction from the caller again; the context is taken from the transaction Comment on attachment 119683 [details] [review] [PATCH 2/4] bus_driver_handle_update_activation_environment: Forward to systemd Review of attachment 119683 [details] [review]: ----------------------------------------------------------------- Thanks, this looks good now. I'll check through all the patches again before merging, but hopefully they're suitable for 1.10.4. Fixed in 1.10.4. Thanks! (I changed the error for inability to determine the pid back to DBUS_ERROR_UNIX_PROCESS_ID_UNKNOWN, to be consistent with how it was in the past.) |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.