GNOME Bugzilla – Bug 686997
build in all session tracking modules (systemd, consolekit and none) and choose one at runtime
Last modified: 2016-02-23 13:55:52 UTC
I was inspired to write this down by a patch by Luke Shumaker submitted to NM mailing list and a discussion thread on Abclinuxu.cz: * https://mail.gnome.org/archives/networkmanager-list/2012-October/msg00064.html * https://www.abclinuxu.cz/blog/JKR/2012/10/totalni-znechuceni-jmenem-systemd/diskuse#28 (in Czech) Currently, we have three session tracking modules selected at build time with --with-session-tracking=systemd|ck|none. I don't have a precise idea how this should work, but at least: 1) NetworkManager should be able to detect session manager at start 2) It should be possible to configure NetworkManager to disable session management and use 'none' instead 3) NetworkManager should also be able to reject all non-root requests for server use cases Anyone is welcome to add his concerns and ideas. Luke: As you already submitted some patches, I would like to know if you might be willing to send patches in line with this or not.
(In reply to comment #0) > 1) NetworkManager should be able to detect session manager at start For systemd, you can check if org.freedesktop.login1 is on the bus. Presumably there's a different interface you could check for for ConsoleKit. I guess since this only matters at startup, we don't need to do the check asynchronously... So, just add GInitable support to the three session manager implementations, and try to create+initialize each one, and stop when you get one that works. also: 0) either/both implementation can be disabled at compile time. (In particular, the systemd implementation requires a library, so it won't be compilable on non-systemd distros anyway.)
> For systemd, you can check if org.freedesktop.login1 is on the bus. Presumably > there's a different interface you could check for for ConsoleKit. Sounds reasonable. > I guess since this only matters at startup, we don't need to do the check > asynchronously... Agreed. > So, just add GInitable support to the three session manager > implementations, and try to create+initialize each one, and stop when you get > one that works. This could work.
Summary of an IRC discussion: The --without-session-tracking is insecure in a way that it grants any user the same privileges as with an active local session. This may be confusing and may constitute a security problem when the administrator is not aware of it. To maintain backwards compatibility I propose obsoleting session-tracking=none and adding two new settings instead. Their names are yet to be defined but my first thought was 'permissive' and 'restrictive'. The permissive one could also be called 'dummy'. After a release, 'restrictive' could be made the default behavior for --without-session-tracking. In the case of moving session tracking option to /etc/NetworkManager.conf, it would work exactly the same. Example: [security] session-tracking=dummy The idea behind this change is that removal of session tracking should not negatively impact security. Session tracking is here to *grant* privileges, not to *deny* them. From the desktop perspective, nobody really cared.
Consolidating bugs... Pacho Ramos [reporter] 2013-05-11 09:57:00 UTC Created an attachment (id=243839) [details] [review] consolekit/logind runtime detection for 0.9.8.x From downstream: https://bugs.gentoo.org/show_bug.cgi?id=468930 This patch makes possible to move the logind/consolekit detection to runtime (from build time), that way, people running other init (like upstart or openrc) can still use logind provided by systemd package if desired Link to attachment: https://bug700118.bugzilla-attachments.gnome.org/attachment.cgi?id=243839 More patches can be attached to this bug report.
*** Bug 700118 has been marked as a duplicate of this bug. ***
Hi Pacho, the patch needs a couple of updates: * configure.ac and Makefile.am changes won't apply to NetworkManager master. * SESSION_TRACKING_CK should be used to allow building without CK support * nm-session-tracking-null is trivial enough to be also merged in nm-session-tracking * 'git log --grep session' should be used to check if you missed out any fixes since you weren't patching the master Please look also at the suggestions already present in this bug report and please comment which ones you are addressing (or willing to address) in your patches. Of course we don't have to do everything at once and someone can take over later.
Created attachment 248911 [details] [review] merge systemd and consolekit implementations into one, move detection to runtime
Created attachment 248912 [details] [review] merge the null implementation as well
The only thing these two patches don't address is: 3) NetworkManager should also be able to reject all non-root requests for server use cases Currently, the null implementation just returns positive answers to any request.
Rebased on the current git master and published as 'pavlix/session' branch.
Please review 'pavlix/session'. Feel free to fixup and push under your own namespace or to 'master' directly.
> core: runtime detect logind and ConsoleKit - License on nm-session-monitor-systemd.c should be GPL; really the license header text shouldn't get changed at all, except matthias should get added to the copyright list. - No good reason to remove the gtkdoc stuff at the top either, it should stay - No good reason to rename CHANGED to CHANGED_SIGNAL, it's only ever used internally with g_signal_emit(), so having _SIGNAL would be redundant: g_signal_emit (obj, signals[CHANGED_SIGNAL], ...) - No good reason to change the /******/ to /-------/, or to remove them - No good reason to rename 'finalize' to nm_session_monitor_finalize() - No good reason to remove the code documentation from nm_session_monitor_user_has_session() or nm_session_monitor_uid_has_session() or nm_session_monitor_user_active() or nm_session_monitor_uid_active() - duplicated #define of LOGIND_RUNNING in src/nm-session-monitor.h, plus that #define should go into the source file instead of the header since nothing except the source file uses it. - Some style issues when calling "if (LOGIND_RUNNING)"; first, the () are not necessary as LOGIND_RUNNING is not a function (nor does the macro take an argument), and second, the { should be on the same line as the if() statement. > core: merge the nm-session-monitor-null.c implementation - Some added whitespace in nm_session_monitor_init() - nm_session_monitor_finalize() needs to chain up to parent GObject instead of just calling 'return' - No reason to not pass the struct offset when calling g_signal_new() in the class_init function So basically, mostly minor cleanups, the logic appears sound but lets do another round and then a re-review. Thanks Pavel and Fabio!
(In reply to comment #12) > > core: runtime detect logind and ConsoleKit > > - License on nm-session-monitor-systemd.c should be GPL; The file you mention is removed by the *first* patch. You most probably mean nm-session-monitor.c, changing the header for that one. > really the license header text shouldn't get changed at all, It didn't. It was a verbatim copy of nm-session-monitor-systemd.c's header. It would be best to avoid such headers entirely, if they're not maintained anyway. > except matthias should get added to the copyright list. Done. > - No good reason to remove the gtkdoc stuff at the top either, it should stay Added gtkdoc stuff from nm-session-monitor-ck.c. > - No good reason to rename CHANGED to CHANGED_SIGNAL, it's only ever used > internally with g_signal_emit(), so having _SIGNAL would be redundant: Changed to match -ck instead of -systemd. > g_signal_emit (obj, signals[CHANGED_SIGNAL], ...) > > - No good reason to change the /******/ to /-------/, or to remove them > > - No good reason to rename 'finalize' to nm_session_monitor_finalize() > > - No good reason to remove the code documentation from Should have been fixed before the code got to NetworkManager master :). > nm_session_monitor_user_has_session() or nm_session_monitor_uid_has_session() > or nm_session_monitor_user_active() or nm_session_monitor_uid_active() Done as part of gtkdoc. > - duplicated #define of LOGIND_RUNNING in src/nm-session-monitor.h, Thanks. > plus that > #define should go into the source file instead of the header since nothing > except the source file uses it. Done. > - Some style issues when calling "if (LOGIND_RUNNING)"; first, the () are not > necessary as LOGIND_RUNNING is not a function (nor does the macro take an > argument), and second, the { should be on the same line as the if() statement. Turned LOGIND_RUNNING into an ordinary macro, fixed the {. > > core: merge the nm-session-monitor-null.c implementation > > - Some added whitespace in nm_session_monitor_init() Fixed. Also simplified 'error' handling (in the first patch). > - nm_session_monitor_finalize() needs to chain up to parent GObject instead of > just calling 'return' Actually, NULL tracking doesn't require the method overridden at all. Fixed. Improved SESSION_TRACKING_NULL usage. > - No reason to not pass the struct offset when calling g_signal_new() in the > class_init function Granted. > So basically, mostly minor cleanups, the logic appears sound but lets do > another round and then a re-review. Thanks Pavel and Fabio! Thanks. Ready for next round.
In the first patch, the whitespace in nm_session_monitor_class_init() changed and is now incorrect. Second, the LOGIND_RUNNING macro should be removed, since it's only relevant in nm_session_monitor_init() to detect whether logind is running. From then on, it should be sufficent to use NMSessionMonitor's priv->sd_source as the indicator of whether logind is running or not in nm_session_monitor_user_has_session() and the rest of those functions. Variable declarations should be at the top of the function, eg in nm_session_monitor_user_has_session() for "Session *s" which should be moved above the SYSTEMD block. Pretty sure that won't compile if systemd support is enabled: nm-session-monitor.c: In function 'nm_session_monitor_user_has_session': nm-session-monitor.c:522:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement] Session *s; For nm-session-monitor.h, is the <unistd.h> include used anywhere in the header file itself? If not, then it should either be removed if not used at all, or moved to nm-session-monitor.c if it's used there. --- Second patch: I would not bother with #else in nm_session_monitor_user_has_session() and the rest, just #endif is fine after the return. Same for the rest of those functions.
Two more quick requests: 1) put the bug # into both patches commit messages like we typically do, both in the short message and as a full link at the bottom of the commit message 2) change the first patch's "logind" to "systemd-logind" Thanks!
Just a note about future intentions... I would like to eventually remove the NULL conditional and create a NetworkManager.conf option that could look like: [main] session=auto|root|anyone We could possible have values for 'systemd' and 'consolekit' but I'm happy with just using 'auto' instead.
Created attachment 273498 [details] [review] A single patch implementing the feature Here's a brand new patch, somewhat based on the former two patches included in the bugzilla as well. The code is refactored a bit and I believe you will like it. I expect some feedback but it now seems to be pretty much ready for inclusion.
Created attachment 273499 [details] The complete nm-session-monitor.c for convenience I'm adding the source file for convenience, as larger scale patches are not very readable.
(In reply to comment #17) > Created an attachment (id=273498) [details] [review] > A single patch implementing the feature > > Here's a brand new patch, somewhat based on the former two patches included in > the bugzilla as well. The code is refactored a bit and I believe you will like > it. I expect some feedback but it now seems to be pretty much ready for > inclusion. This comment seems relevant to detect systemd: https://bugzilla.gnome.org/show_bug.cgi?id=703040#c7
The relevant check to detect whether systemd is PID 1 [0] (it's trivial to re-implement in case you don't want to link against libsystemd(-daemon) To check if systemd-logind is available, see the proposal from Martin Pitt, which as been applied to a couple of GNOME projects already. [0] http://www.freedesktop.org/software/systemd/man/sd_booted.html http://cgit.freedesktop.org/systemd/systemd/commit/?id=66e41181 [1] https://mail.gnome.org/archives/desktop-devel-list/2013-March/msg00092.html
The patch still has Fabio Erculiani as author (but the commit message says "Based on patches by Fabio Erculiani") For the configure changes, it would perhaps be better to let --with-session-tracking take a comma-separate list of modules? (Among other things, automatic backward compatibility.) And I'd keep "none" rather than "fake" as the name of the no-session-tracking mode; the important aspect of it is that you get no session tracking, not that we happen to implement that functionality via a fake session monitor class. In nm-session-monitor, I feel like we should not initialize the ConsoleKit monitor if initializing the systemd monitor succeeds. (Eg, there's no point in registering a GFileMonitor for CKDB_PATH if we're going to be using systemd.) (In reply to comment #20) > The relevant check to detect whether systemd is PID 1 [0] (it's trivial to > re-implement in case you don't want to link against libsystemd(-daemon) We have to link against libsystemd-login anyway, so linking against libsystemd-daemon as well shouldn't be a problem.
(In reply to comment #20) > To check if systemd-logind is available, see the proposal from Martin Pitt, > which as been applied to a couple of GNOME projects already. I believe that in this case the best way would be to indeed talk to systemd-logind if available and to consolekit otherwise. Checking for systemd itself would mostly work but this patch is really about systemd-logind. Where can I find Martin's proposal? (In reply to comment #21) > The patch still has Fabio Erculiani as author (but the commit message says > "Based on patches by Fabio Erculiani") TODO > For the configure changes, it would perhaps be better to let > --with-session-tracking take a comma-separate list of modules? That will force source based distributions to assemble the list and NM's configure.ac to parse it back. I would prefer individual options in this case as they are much easier to work with. > And I'd keep "none" rather than "fake" as the name of the > no-session-tracking mode There are two distinct modes that don't rely on actual session tracking. One is the insecure one I call 'fake' because it *pretends* a session exists and is active. I would rather keep 'none' for the case where no session tracking is performed nor pretended. > ; the important aspect of it > is that you get no session tracking, not that we happen to implement that > functionality via a fake session monitor class. The none/fake distinction here is about the result, not implementation. While I expect 'none' to perform no session tracking at all and to answer FALSE to any session request, 'fake' is the exact opposite and always pretends there is a session. The name of the class follows from the name of the option, not the other way round. > In nm-session-monitor, I feel like we should not initialize the ConsoleKit > monitor if initializing the systemd monitor succeeds. That sounds reasonable and may actually make things simpler. TODO > (In reply to comment #20) > > The relevant check to detect whether systemd is PID 1 [0] (it's trivial to > > re-implement in case you don't want to link against libsystemd(-daemon) > > We have to link against libsystemd-login anyway, so linking against > libsystemd-daemon as well shouldn't be a problem. I don't object linking to any systemd library but in this case I would indeed prefer detecting systemd-logind in particular. Thanks for the feedback. Will send updates but of course any help is welcome.
So the patches here essentially rename the "null" session tracking to "fake", and add a new "none" tracking with different behavior. What's the point of "none" and why do we need it? The "none" tracker will simply cause all connections to be invisible for all users, which doesn't seem very useful to me.
(In reply to comment #23) > So the patches here essentially rename the "null" session tracking to "fake", > and add a new "none" tracking with different behavior. > > What's the point of "none" and why do we need it? > > The "none" tracker will simply cause all connections to be invisible for all > users, which doesn't seem very useful to me. I think, "all users, except root". So maybe a better name would be "root-only" then "none" and "fake" (previously "null") should be named "allow-all" (sorry for the bikeshedding)
(In reply to comment #24) > (In reply to comment #23) > > So the patches here essentially rename the "null" session tracking to "fake", > > and add a new "none" tracking with different behavior. > > > > What's the point of "none" and why do we need it? > > > > The "none" tracker will simply cause all connections to be invisible for all > > users, which doesn't seem very useful to me. > > I think, "all users, except root". Yeah, though we've already two solutions for that: (1) permissions=user:root and (2) only giving 'root' the PolicyKit permissions and denying them to every other user. So I don't know why we'd need a root-only session tracker.
(In reply to comment #25) > (In reply to comment #24) > > (In reply to comment #23) > > > So the patches here essentially rename the "null" session tracking to "fake", > > > and add a new "none" tracking with different behavior. > > > > > > What's the point of "none" and why do we need it? > > > > > > The "none" tracker will simply cause all connections to be invisible for all > > > users, which doesn't seem very useful to me. > > > > I think, "all users, except root". > > Yeah, though we've already two solutions for that: (1) permissions=user:root > and (2) only giving 'root' the PolicyKit permissions and denying them to every > other user. So I don't know why we'd need a root-only session tracker. But this solution only works if you compile with session tracking, either systemd or consolekit, right? What if you disable both?
AFAIS, does the current patch not really detect the session tracking at runtime and uses it. For example, if you configure SESSION_TRACKING_SYSTEMD, it will always set monitor->sd_source and from then on using systemd. I think, this should be more sophisticated -- especially, when you can compile time enable both together.
PolicyKit has nothing to do with the session tracking stuff, so it's not relevant here. You can build NM with session-tracking=none and policy-kit=yes, they are independent. Session tracking affects two things only: 1) connection visibility; and we already have permissions=user:root for that, which works no matter what session tracking you have. If you don't want non-root users to see your connections, ensure they have the correct permissions set on them via "permissions=". Users cannot change connections without the right PolicyKit permissions. 2) secret agent registration; agents are currently required to have a session, otherwise they are denied. So whatever "null" tracking we have should return TRUE for "has_session()" so that secret agents are still allowed to register, and they will still be validated with PolicyKit. Agents are *never* asked for secrets for connections which are not visible to the agent's user with "permissions=" So there is no need for different "fake" and "root" session tracking. Make sure that connections have the right permissions set on them instead. Now, if somebody wants to discuss how to do permissions without PolicyKit, that's a completely separate topic than session tracking, and we should do that in another bug. But these changes would apply to nm-manager-auth.c, and have nothing to do with session-tracking.
Pavel, would you mind reworking the patch to remove the 'root' session tracking functionality (leaving the NULL eg "everything has a session") functionality? We'd like to merge this for 0.9.10 and get that out the door by the end of May. Thanks!
(In reply to comment #29) > Pavel, would you mind reworking the patch to remove the 'root' session tracking > functionality (leaving the NULL eg "everything has a session") functionality? Isn't this unexpected privilege escalation when all users have management rights for NetworkManager? After all this actually means providing a fake active session for any user.
I believe this is was a bad idea from security perspective in the first place. In my opinion, the session tracking code should never provide fake positive answers about (active) sessions unless explicitly requested. In this case I would prefer an explicit way rather than implicit. Using '--session-tracking-fake' sounds explicit enough to me. If you insist on having fake session tracking by default and are willing to accept the risk of providing fake positive answers about sessions when the operator doesn't expect it, I'll modify the patch accordingly.
Trying to audit where the session management data is used: $ grep nm_session_monitor_u `find -iname '*.c' -and -not -iname 'nm-session-monitor*'` ./src/nm-manager-auth.c: if (!nm_session_monitor_uid_has_session (smon, uid, &user, &local)) { ./src/settings/nm-agent-manager.c: && !nm_session_monitor_uid_has_session (nm_session_monitor_get (), ./src/settings/nm-agent-manager.c: a_active = nm_session_monitor_uid_active (nm_session_monitor_get (), ./src/settings/nm-agent-manager.c: b_active = nm_session_monitor_uid_active (nm_session_monitor_get (), ./src/settings/nm-settings-connection.c: if (nm_session_monitor_user_has_session (priv->session_monitor, puser, NULL, NULL)) {
(In reply to comment #32) > ./src/settings/nm-agent-manager.c: a_active = > nm_session_monitor_uid_active (nm_session_monitor_get (), > ./src/settings/nm-agent-manager.c: b_active = > nm_session_monitor_uid_active (nm_session_monitor_get (), These look pretty safe as they are just comparing the agents to prefer the one with an active session. It doesn't seem to have security implications.
(In reply to comment #32) > ./src/nm-manager-auth.c: if (!nm_session_monitor_uid_has_session (smon, > uid, &user, &local)) { > ./src/settings/nm-agent-manager.c: && > !nm_session_monitor_uid_has_session (nm_session_monitor_get (), > ./src/settings/nm-settings-connection.c: if > (nm_session_monitor_user_has_session (priv->session_monitor, puser, NULL, > NULL)) { All of the remaining three seem to check whether a user has a physical session or not. This looks like a security feature to avoid changing NM configuration from non-physical sessions like SSH. They don't block physical requests from not-active sessions, though. I would like to know whether this is an expected security feature and what is the rationale. Therefore the active session based security is not based on nm-session-manager at all so making all sessions active seems to be safe. Do I understand correctly that active session based security is handled by policykit? The first two lines above also translate the uid to name which can be done using getpwuid just as well.
Created attachment 277050 [details] [review] fixup patch to remove the distinction between fake and none session tracking This is how a fixup patch could look like.
Created attachment 277051 [details] [review] fixup patch to use consolekit only if properly initialized This patch is (also) based on the assumption that session tracking is not used for security purposes and uses consolekit only if it's available.
Created attachment 277055 [details] [review] a patch to illustrate the question about security implications This illustrative patch removes the code that seems to try to use session tracking for security.
The above patches from comment 17, comment 35, comment 36, and comment 37 did not apply cleanly. For convenience, I pushed all 4 of them to the branch: th/review/pavlix/bgo686997_session
(In reply to comment #38) > The above patches from comment 17, comment 35, comment 36, and comment 37 did > not apply cleanly. > > For convenience, I pushed all 4 of them to the branch: > > th/review/pavlix/bgo686997_session Pushed some fixes on to of the branch, mainly trivial renaming.
Thanks! I like all of those changes. I may post some more patches in this area. In the meantime, feel free to collapse all the fixups including those that aren§t marked so. I think it's practical to treat the session monitor implementation as new code (only based on the original code) and collapse *any* updates into it.
Created attachment 277146 [details] [review] a patch to simplify nm-session-monitor singleton API Also published in 'pavlix/session'.
(In reply to comment #41) > Created an attachment (id=277146) [details] [review] > a patch to simplify nm-session-monitor singleton API > > Also published in 'pavlix/session'. $ grep nm_session_ `find -iname '*.c' -and -not -iname 'nm-session-monitor.c'` ./src/nm-manager-auth.c: if (!nm_session_monitor_uid_to_user (uid, &user, &local) || !user) { ./src/settings/nm-agent-manager.c: a_active = nm_session_monitor_uid_active (nm_secret_agent_get_owner_uid (a), ./src/settings/nm-agent-manager.c: b_active = nm_session_monitor_uid_active (nm_secret_agent_get_owner_uid (b), ./src/settings/nm-settings-connection.c: priv->session_changed_id = nm_session_monitor_connect (session_changed_cb, self); ./src/settings/nm-settings-connection.c: nm_session_monitor_disconnect (priv->session_changed_id); ./src/main.c: session_monitor = nm_session_monitor_get ();
(In reply to comment #40) > Thanks! I like all of those changes. I may post some more patches in this area. > In the meantime, feel free to collapse all the fixups including those that > aren§t marked so. I think it's practical to treat the session monitor > implementation as new code (only based on the original code) and collapse *any* > updates into it. IMO, I would not collapse them, because the first commit ("core: runtime detect systemd-logind and fallback to ConsoleKit") mainly moves code around, while the following commits add some cleanup. Squashing the (trivial) renamings, makes it harder to review the patch. I repushed "th/review/pavlix/bgo686997_session" (no-ff, including your latest commit). I think, we should later merge it like this (after squashing the two fixup! commits).
(In reply to comment #43) > (In reply to comment #40) > > Thanks! I like all of those changes. I may post some more patches in this area. > > In the meantime, feel free to collapse all the fixups including those that > > aren§t marked so. I think it's practical to treat the session monitor > > implementation as new code (only based on the original code) and collapse *any* > > updates into it. > > IMO, I would not collapse them, because the first commit ("core: runtime detect > systemd-logind and fallback to ConsoleKit") mainly moves code around, I think it already includes substantial modifications, but it's up to you, sir. > I repushed "th/review/pavlix/bgo686997_session" (no-ff, including your latest > commit). I think, we should later merge it like this Sure. > (after squashing the two fixup! commits). Those are the patches I was talking about primarily. I see three fixup patches, though. > trivial: whitespace fix > core: remove assert from nm_session_monitor_get() in NMSessionMonitor Those are trivial. > core: clear pointers in finalize and free methods in NMSessionMonitor Clearing pointers is unnecessary. Finalize is only called once. By the way, do you know about g_clear_pointer? The function prefixing and data prefixing (creating separate structures) could IMO be done in a single cleanup commit as well, but it's up to you.
(In reply to comment #34) > (In reply to comment #32) > > ./src/nm-manager-auth.c: if (!nm_session_monitor_uid_has_session (smon, > > uid, &user, &local)) { > > ./src/settings/nm-agent-manager.c: && > > !nm_session_monitor_uid_has_session (nm_session_monitor_get (), > > ./src/settings/nm-settings-connection.c: if > > (nm_session_monitor_user_has_session (priv->session_monitor, puser, NULL, > > NULL)) { > > All of the remaining three seem to check whether a user has a physical session > or not. This looks like a security feature to avoid changing NM configuration > from non-physical sessions like SSH. They don't block physical requests from > not-active sessions, though. I would like to know whether this is an expected > security feature and what is the rationale. > > Therefore the active session based security is not based on nm-session-manager > at all so making all sessions active seems to be safe. Do I understand > correctly that active session based security is handled by policykit? > > The first two lines above also translate the uid to name which can be done > using getpwuid just as well. Session tracking is used in the agent code to prefer active sessions, since requesting secrets from a user session may trigger interactive mode, and clearly inactive sessions are not supposed to be interactive, so secrets would just fail. Agents which have no session are currently rejected because agents are not supposed to be spawned by random processes that have no lifetime tied to an actual user login (GUI, ssh, VNC, etc). Session tracking is used in nm-settings-connection.c to gate visibility of the connection for that user, for some reason I cannot remember. We might even remove this restriction now (see below). Session tracking is also used in various places (via nm_auth_uid_in_acl()) to gate access to connections from the D-Bus API, like ensuring that a user can only touch connections they have permissions to (from NMSettingConnection::permissions). That includes things like activation/deactivation/disconnection/add/delete/update/etc. All these things ensure the Unix user is present in the NMSettingConnection::permissions property, but the code also checks whether that user has a session or not. Almost all these things are *also* protected by PolicyKit permissions too though. So I'm really not sure what benefit is gained by ensuring that the requesting Unix user has a session or not. The only cases that I think are relevant here are thinks like a process spawned from a cron job or system daemon that su's to the Unix user and touches connections, since the cronjob won't have a login session. My overall point here is that I think perhaps NM should actually do *less* session tracking than it does now. What do you think? Are there instances where security is increased by denying access from non-session processes, given that we already enforce NMSettingConnection::permissions checks and PolicyKit checks? (I see you've done this in 3881ca9f2ecc01131ccb9c7b40c19a465205c101 already...)
Note that 3881ca9f2ecc01131ccb9c7b40c19a465205c101 now breaks the purpose of nm_settings_connection_recheck_visibility(). This function originally ensured that if a connection had valid NMSettingConnection::permissions and no user listed in the permissions had a session, the connection would be marked invisible. This does exactly one thing: it prevents autoconnect for the connection, because no user that "owns" the connection is logged in, so the connection shouldn't be started automatically. 3881ca9f2ecc01131ccb9c7b40c19a465205c101 will now cause any restricted connection to be available for auto-connect, regardless of whether or not the user(s) in ::permissions are logged in. Which seems kinda wrong... Obviously if you have no session tracking, you have to assume that even restricted connections are visible all the time (and thus all users have a session) because there's no way to tell when somebody logs in or logs out. Which was one reason the "fake" session management said everyone had a session all the time.
So there are two questions we have to answer: 1) is it a security risk to allow non-root, non-login-session processes (like spawned from cron jobs or system daemons) to update/modify/delete connections and to start/stop/disconnect network devices? 2) is it a problem to autoconnect restricted connections where no user with permissions for that connection is currently logged in? ------ Anyway, I've pushed some additional cleanups and simplifications to the branch, and one fix for an accidental change on Thomas' rename patch. What do you think? Goodbye LoC!
(In reply to comment #45) > My overall point here is that I think perhaps NM should actually do *less* > session tracking than it does now. +1 I would be happy if we could only use the session tracking in places where it has some actual purpose and if we could have the purpose documented in the source code. > where security is increased by denying access from non-session processes, given > that we already enforce NMSettingConnection::permissions checks and PolicyKit > checks? After reading the relevant parts of the source code, and with permissions and PolicyKit in mind, I don't think there are any security consequences of getting around the logind/consolekit checks, which of course, you said at the beginning. My concern regarding session security now is to make it clear in the source code (and comments) that there are no security expectations on session tracking code. > (I see you've done this in 3881ca9f2ecc01131ccb9c7b40c19a465205c101 already...) (In reply to comment #47) > So there are two questions we have to answer: > > 1) is it a security risk to allow non-root, non-login-session processes (like > spawned from cron jobs or system daemons) to update/modify/delete connections > and to start/stop/disconnect network devices? Status changes from a non-privileged non-active sessions might be considered a security/functional problem (unless the administrator says otherwise). Preparing a configuration that would be used later in an active session should be OK. > 2) is it a problem to autoconnect restricted connections where no user with > permissions for that connection is currently logged in? I would say NetworkManager should only (auto)activate user connections when the user has an active session and should terminate them when the active connection is closed. But that would mean to actually use session tracking for security purposes, which I guess we wanted to avoid. > Anyway, I've pushed some additional cleanups and simplifications to the branch, > and one fix for an accidental change on Thomas' rename patch. What do you > think? Goodbye LoC! I'm not sure whether we now have all requested modifications together. Please talk to Thomas before merging as he had his own branch on this. I typically prefer to avoid pushing to branches in user namespaces as each developer can have different ways of managing his own branches. For now I think you can push just anything that you agree on and we can improve it later, on the way to 1.0.
Is there anything here that can still go in for 0.9.10 or is it all bumped to 1.0?
(In reply to comment #48) > (In reply to comment #47) > > So there are two questions we have to answer: > > > > 1) is it a security risk to allow non-root, non-login-session processes (like > > spawned from cron jobs or system daemons) to update/modify/delete connections > > and to start/stop/disconnect network devices? > > Status changes from a non-privileged non-active sessions might be considered a > security/functional problem (unless the administrator says otherwise). > Preparing a configuration that would be used later in an active session should > be OK. The security here is already handled by PolicyKit permissions. If you want to restrict access to these functions (through org.freedesktop.NetworkManager.enable-disable-network or org.freedesktop.NetworkManager.settings.modify.system) then you can do that via PolicyKit and the allow_active/allow_inactive/allow_all stuff. I don't think NM should really duplicate this kind of stuff which POlicyKit already does. Even finer-grained control can be achievied through .pkla files or the Javascript stuff that PolicyKit does. How to do this is less than clear though, and I think (as you've alluded to) we really do need to write up the expectations and sample procedures. > > 2) is it a problem to autoconnect restricted connections where no user with > > permissions for that connection is currently logged in? > > I would say NetworkManager should only (auto)activate user connections when the > user has an active session and should terminate them when the active connection > is closed. Currently NM does what you say here, as long as the connection is permission restricted (eg, permissions=user:dcbw). Permissions in the connection and enabling session tracking enables the 'connection visibility' stuff which listens to session changes and auto-activate/deactivate in response.\ This leads to some interesting corner cases, like if a non-session client starts the connection, then a user with that non-session client's UID logs in (visibility flips from FALSE -> TRUE, but nothing happens because the connection is active) and then the user logs out (flips from TRUE -> FALSE due to no session, disconnected by the policy). I guess we could fix that by tracking whether the client that requested the activation was part of a session or not. That's a problem for later though. > But that would mean to actually use session tracking for security purposes, > which I guess we wanted to avoid. I think we should just not autoconnect connections that are restricted when the user doesn't have a session. Clients that care (and have that user's UID) could still manually start the connection if they cared. I did another review of all the users of session tracking in NM git master and came up with the following: --------- Two main reasons for session tracking: 1) agents - prefer agents in an active session 2) autoconnect - don't autoconnect restricted connections when user isn't logged in; disconnect restricted connections when user logs out nm_session_monitor_user_has_session() - _connection_recheck_visibility - autoconnect related so still required nm_session_monitor_uid_has_session() - agent manager - agents must currently belong to sessions - nm_auth_uid_in_acl() - this is the "big one" *** nm_auth_uid_in_acl() *** device_auth_request_cb(): impl_device_request_scan() impl_device_disconnect() impl_device_delete() - all these are things that I think a non-session client should be able to do, and they are already all protected by PK and (for disconnect) connection permissions _internal_activate_device(): _internal_activate_generic() _internal_activation_auth_done() nm_manager_activate_connection() (P) auto_activate_device() - already invisible if no session, thus no autoactivate (P) activate_secondary_connections() - already does PK and perm using subject or original activatio request (M) ensure_master_active_connection() - already does PK and perm using subject or original activatio request _activation_auth_done() impl_manager_activate_connection() - already does PK and perm activation_add_done() _add_and_activate_auth_done() - already does PK and perm - I think activating connections should also be available to non-session clients, and they are still PK and connection permissions protected nm_manager_activate_connection(): - see above, this is just here because it calls nm_auth_uid_in_acl() again validate_activation_request(): - see above (same as nm_manager_activate_connection()) it does nm_auth_uid_in_acl() again impl_manager_deactivate_connection(): - same rationale as Device.Disconnect() auth_start(): impl_settings_connection_get_settings() impl_settings_connection_update_helper() impl_settings_connection_delete() impl_settings_connection_get_secrets() - These are already protected by PK and connection permissions impl_settings_connection_update_helper(): - see above for auth_start() this function redundantly calls nm_auth_uid_in_acl() impl_settings_get_connection_by_uuid(): - No reason non-session clients shouldn't be able to get the connection path from a UUID, this is also already protected by connection permissions nm_settings_add_connection_dbus(): - same rationale as AddAndActivateConnection(); non-session clients should be able to do this, of course restricted with PolicyKit and connection permissions --------- So in summary, I don't think anything above should be restricted from non-session clients (eg OpenLMI or Cockpit) as long as permissions are set up correctly by the admin. And to help the admin do that, we really do need to add some documentation on this stuff. Thoughts?
I have rebased pavlix/session onto master and resolved conflicts. I pushed the code into jk/bgo686997-session-tracking. Hopefully, I have not introduced any errors. If I understand it correctly, the code is (more or less) complete. Dan (dcbw), I think it makes sense that you take over, check and merge it, if you do not mind. You inspected the code as last and understand it better anyway.
Since the libsystemd >= 209 library patches, there's a configure.ac conflict which I think you can solve by taking the original code from this branch, and then using the following double-level PKG_CHECK_MODULES: if test "$with_systemd_logind" = "yes"; then PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd >= 209],, [PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd-login >= 183])]) AC_SUBST(SYSTEMD_LOGIN_CFLAGS) AC_SUBST(SYSTEMD_LOGIN_LIBS) from systemd 209 and later all the separate libraries got combined into libsystemd, it seems.
> core: runtime detect systemd-logind and fallback to ConsoleKit (bgo #686997) I don't think we can apply the nm-settings-connection.c bits of this patch because that will change current autoconnect behavior. It would allow *all* connections to be autoconnected even when the user is not logged in, which isn't what we want. So we need to remove that hunk from this patch, and then also add back the nm_session_monitor_user_has_session() call that gets removed in "simplify NMSessionMonitor API". > simplify NMSessionMonitor API this commit should get renamed to something like "core: simplify session monitor API" I pushed a fixup branch (which squashed your original branch's fixups) to dcbw/jk/bgo686997-session-tracking. Do the fixups I added there look OK? We should test that a user-locked autoconnect connection doesn't connect before the user is logged in, but does after the user logs in. Probably easiest to take one of your WiFi connections and set USER=jklimes and see if that makes things work like we expect.
(In reply to comment #52) > Since the libsystemd >= 209 library patches, there's a configure.ac conflict > which I think you can solve by taking the original code from this branch, and > then using the following double-level PKG_CHECK_MODULES: > > if test "$with_systemd_logind" = "yes"; then > PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd >= 209],, > [PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd-login >= > 183])]) > AC_SUBST(SYSTEMD_LOGIN_CFLAGS) > AC_SUBST(SYSTEMD_LOGIN_LIBS) > > from systemd 209 and later all the separate libraries got combined into > libsystemd, it seems. Yes, it looks OK. For the record, the systemd changes in version 209 are here http://lists.freedesktop.org/archives/systemd-devel/2014-February/017146.html (In reply to comment #53) > > core: runtime detect systemd-logind and fallback to ConsoleKit (bgo #686997) > > I don't think we can apply the nm-settings-connection.c bits of this patch > because that will change current autoconnect behavior. It would allow *all* > connections to be autoconnected even when the user is not logged in, which > isn't what we want. > > So we need to remove that hunk from this patch, and then also add back the > nm_session_monitor_user_has_session() call that gets removed in "simplify > NMSessionMonitor API". > > > simplify NMSessionMonitor API > > this commit should get renamed to something like "core: simplify session > monitor API" > > > I pushed a fixup branch (which squashed your original branch's fixups) to > dcbw/jk/bgo686997-session-tracking. Do the fixups I added there look OK? We > should test that a user-locked autoconnect connection doesn't connect before > the user is logged in, but does after the user logs in. Probably easiest to > take one of your WiFi connections and set USER=jklimes and see if that makes > things work like we expect. Your fixes look good to me. Auto-connecting works. When I connect a user restricted connection and logout, it is disconnected. After login in again it is reconnected again. (The ifcfg variable is USERS, not USER)
Ok, so based on that testing I think the branch is good to go
Feel free to grab dcbw/jk/bgo686997-session-tracking, squash what's needed, add any further fixups to it and then merge.
I rebased dcbw/jk/bgo686997-session-tracking onto git master and squashed fixups. Back for re-review.
It looks good to me, except I would not call these commits "trivial". trivial: remove useless if() trivial: fix some whitespace and simplify signal setup
I tested the branch again. All seems good to me. Only, I think we can lower required systemd version for session tracking. logind is available in systemd 30 and higher (http://en.wikipedia.org/wiki/Systemd#logind). diff --git a/configure.ac b/configure.ac index ebbdeb6..9f48d5a 100644 --- a/configure.ac +++ b/configure.ac @@ -349,7 +349,7 @@ AS_IF([test "$with_session_tracking" = "none"], [with_consolekit="no" with_syste session_tracking= if test "$with_systemd_logind" = "yes"; then PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd >= 209],, - [PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd-login >= 183])]) + [PKG_CHECK_MODULES(SYSTEMD_LOGIN, [libsystemd-login >= 30])]) AC_SUBST(SYSTEMD_LOGIN_CFLAGS) AC_SUBST(SYSTEMD_LOGIN_LIBS) AC_DEFINE([SESSION_TRACKING_SYSTEMD], 1, [Define to 1 if libsystemd-login is available])
This is not going to hit 1.0, but should land soon after that, once we do one final audit of any security implications.
This is a pity especially for the Gentoo users that NetworkManager 1.0 will not be fully compatible with their possible setups.
Also it's becoming a huge patchset, so I can't even apply it easily to my system t just get the fix immediately.
Created attachment 291878 [details] [review] the current patchset squashed into one diff for convenience
Created attachment 293296 [details] [review] non-working result of a quick rebase So I see that my work on this has been effectively destroyed by changes on git master and I'm starting over again. In this situation, I hope to receive help from core developers on getting the patch back into shape.
Created attachment 293372 [details] [review] patch adapted to the current codebase I updated the patch to work with the current code base. I plan to do some minor simplifications but it's more or less ready to go. Please review. A version split into multiple patches is in git `pavlix/session` branch and I consider the first two patches trivial and immediately applicable.
Slightly updated and further split. Two trivial patches, two patches to evaluate and the final session tracking patch. All to be found in `pavlix/session`.
Updated 'pavlix/session'. Split thoroughly. Moved all policy changes after the main patch for this ticket so that we can first implement the feature and then decide about the specific details. Please review.
http://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=pavlix/session
Incorporated IRC feedback from Thomas. Simplified the systemd code as well. Minor changes in consolekit code. Pushed, please review.
Part 1 of this branch was merged to master as: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=0de60b300ed6cc0fc94c742396112b1274989371 session: merge nm-session-monitor-* modules http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=eb2eda4408667395ffb73ec94fc5a02f311b66f5 main: don't initialize the session monitor http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=4a9eaa190fbfd82e4f285e4fde0c0940a0a50a1c session: switch code to nm_session_monitor_{,dis}connect() http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=d42f2c11b79cf46701d046a418d39d03f3809582 auth: remove session monitor argument http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=5fb31ba5d1c54ae9bf723e20fb1e4a83cf2a0496 session: switch code to nm_session_monitor_session_exists() http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=335bbc63357cc963628ad97f465433d1936fe0ba session: merge nm-session-utils into nm-session-manager http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=b52d21a68c196ac77ef107abb135920699899474 consolekit: fix memory leak on error
The feature is now in master, so the bug report could theoretically be closed. Before that happens, please look at the patches that resulted as a side effect of the feature. Let me know which of them should be applied, which should be ignored and which should be modified. commit 0e4e76bfcf6f49f59abf1f3124071857fd600021 Author: Pavel Šimerda <psimerda@redhat.com> Date: Sat Dec 27 20:01:28 2014 +0100 EXPERIMENTAL: settings: avoid dependency on an existing user session Existence of an inactive session doesn't imply that a connection should be visible. Only an active session carries some significance. diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 25459c2..06a8cd5 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -268,7 +268,7 @@ nm_settings_connection_recheck_visibility (NMSettingsConnection *self) continue; if (!nm_session_monitor_user_to_uid (user, &uid)) continue; - if (!nm_session_monitor_session_exists (uid, FALSE)) + if (!nm_session_monitor_session_exists (uid, TRUE)) continue; set_visible (self, TRUE); commit d233cd36e2784fca749c385ab7aefbc296a15177 Author: Pavel Šimerda <psimerda@redhat.com> Date: Fri Jan 2 18:19:07 2015 +0100 EXPERIMENTAL: auth: avoid depending on existing user session Existence of a user session doesn't carry any security information. diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index fa50a7b..12924e7 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -439,13 +439,6 @@ nm_auth_is_subject_in_acl (NMConnection *connection, if (0 == uid) return TRUE; - /* Reject the request if the request comes from no session at all */ - if (!nm_session_monitor_session_exists (uid, FALSE)) { - if (out_error_desc) - *out_error_desc = g_strdup_printf ("No session found for uid %lu", uid); - return FALSE; - } - if (!nm_session_monitor_uid_to_user (uid, &user)) { if (out_error_desc) *out_error_desc = g_strdup_printf ("Could not determine username for uid %lu", uid); commit 8f05c5f93d2db14bff6e90943f75b8c6ae6729d4 Author: Pavel Šimerda <psimerda@redhat.com> Date: Sat Dec 27 19:55:25 2014 +0100 EXPERIMENTAL: agent: avoid dependency on an existing user session We can register an agent whether there is a (possibly inactive) session for the user or not. Agent in an active session is already preferred. diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 8ebb690..d35b1c5 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -286,14 +286,6 @@ impl_agent_manager_register_with_capabilities (NMAgentManager *self, } sender_uid = nm_auth_subject_get_unix_process_uid (subject); - if ( 0 != sender_uid - && !nm_session_monitor_session_exists (sender_uid, FALSE)) { - error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, - NM_AGENT_MANAGER_ERROR_PERMISSION_DENIED, - "Session not found"); - goto done; - } - /* Validate the identifier */ if (!validate_identifier (identifier, &error)) goto done;
(In reply to comment #71) > The feature is now in master, so the bug report could theoretically be closed. > Before that happens, please look at the patches that resulted as a side effect > of the feature. Let me know which of them should be applied, which should be > ignored and which should be modified. The 3 patches look reasonable to me. (I'm not 100% sure though)
(In reply to comment #72) > The 3 patches look reasonable to me. (I'm not 100% sure though) Let's wait for dcbw and then push and close.
Bump.
(In reply to comment #71) > diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c > index 8ebb690..d35b1c5 100644 > --- a/src/settings/nm-agent-manager.c > +++ b/src/settings/nm-agent-manager.c > @@ -286,14 +286,6 @@ impl_agent_manager_register_with_capabilities > (NMAgentManager *self, > } > sender_uid = nm_auth_subject_get_unix_process_uid (subject); > > - if ( 0 != sender_uid > - && !nm_session_monitor_session_exists (sender_uid, FALSE)) { > - error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, > - > NM_AGENT_MANAGER_ERROR_PERMISSION_DENIED, > - "Session not found"); > - goto done; > - } > - > /* Validate the identifier */ > if (!validate_identifier (identifier, &error)) > goto done; The second patch (above) is an important piece of the puzzle. When NM is compiled without session tracking, I don't see VPN connections in a user-started nm-applet.
Improved commit messages of the first two patches. Changed the third patch to avoid the check entirely. The first two patches are currently necessary to allow NetworkManager configuration from an ordinary users when session tracking fails or is not enabled at build time. I think this is the best way to achieve the goal as it removes checks that have no actual value. There is no security distinction between a user session checked using only UID and one with the UID checked for presence of a logind or consolekit tracked session. Note that all three checks to be removed are just checking for presence of a logind/consolekit session for the UID. Just that. Not an active session. And not the session of an actual process. From 1447512d7717cabcc643add45f1553d63dc748d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20=C5=A0imerda?= <psimerda@redhat.com> Date: Sat, 27 Dec 2014 19:55:25 +0100 Subject: [PATCH 1/3] agent: don't enforce user session Agent registration 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 register a secret agent, 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 checking for presence or absence of a user session tracked by logind has no value in this context. Acked-By: Thomas Haller <thaller@redhat.com> --- src/settings/nm-agent-manager.c | 8 -------- 1 file changed, 8 deletions(-) diff --git a/src/settings/nm-agent-manager.c b/src/settings/nm-agent-manager.c index 8ebb690..d35b1c5 100644 --- a/src/settings/nm-agent-manager.c +++ b/src/settings/nm-agent-manager.c @@ -286,14 +286,6 @@ impl_agent_manager_register_with_capabilities (NMAgentManager *self, } sender_uid = nm_auth_subject_get_unix_process_uid (subject); - if ( 0 != sender_uid - && !nm_session_monitor_session_exists (sender_uid, FALSE)) { - error = g_error_new_literal (NM_AGENT_MANAGER_ERROR, - NM_AGENT_MANAGER_ERROR_PERMISSION_DENIED, - "Session not found"); - goto done; - } - /* Validate the identifier */ if (!validate_identifier (identifier, &error)) goto done; -- 2.0.5 From 3acafa5ae50b8d26f367a2daa53779ec6a3c5de1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20=C5=A0imerda?= <psimerda@redhat.com> Date: Fri, 2 Jan 2015 18:19:07 +0100 Subject: [PATCH 2/3] auth: don't enforce user session 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> --- src/nm-auth-utils.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/nm-auth-utils.c b/src/nm-auth-utils.c index fa50a7b..12924e7 100644 --- a/src/nm-auth-utils.c +++ b/src/nm-auth-utils.c @@ -439,13 +439,6 @@ nm_auth_is_subject_in_acl (NMConnection *connection, if (0 == uid) return TRUE; - /* Reject the request if the request comes from no session at all */ - if (!nm_session_monitor_session_exists (uid, FALSE)) { - if (out_error_desc) - *out_error_desc = g_strdup_printf ("No session found for uid %lu", uid); - return FALSE; - } - if (!nm_session_monitor_uid_to_user (uid, &user)) { if (out_error_desc) *out_error_desc = g_strdup_printf ("Could not determine username for uid %lu", uid); -- 2.0.5 From 86fe0b87bb61001eb985cf157385e80439255e46 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pavel=20=C5=A0imerda?= <psimerda@redhat.com> Date: Sat, 27 Dec 2014 20:01:28 +0100 Subject: [PATCH 3/3] settings: don't enforce user session Connection visibility should not be affected by the absence of a user session tracked by logind or consolekit. Note: I haven't noticed any impact of the user session check on nm-applet. --- src/settings/nm-settings-connection.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/settings/nm-settings-connection.c b/src/settings/nm-settings-connection.c index 25459c2..398d2ed 100644 --- a/src/settings/nm-settings-connection.c +++ b/src/settings/nm-settings-connection.c @@ -268,8 +268,6 @@ nm_settings_connection_recheck_visibility (NMSettingsConnection *self) continue; if (!nm_session_monitor_user_to_uid (user, &uid)) continue; - if (!nm_session_monitor_session_exists (uid, FALSE)) - continue; set_visible (self, TRUE); return; -- 2.0.5
Also pushed as 'pavlix/session'. Please review.
The last patch will break user-specific autoconnect connections. For example, if you have a user-specific wifi connection, without that piece the connection will be available to activate even if the user is not logged into the system. Which will lead to the connection being active even though it should not be.
The first two patches seem reasonable though.
Pavel committed the first two patches: 75221bbc1b13db30941060dad213be56f1b30e7c agent: don't enforce user session cd5d5655bab8d66e63782e57e2c99b57602417c6 auth: don't enforce user session
Seems like the remaining patch "813e0a4 settings: don't enforce user session" was NACK'ed. Is there anything left to do on this bug then? Maybe attach the patch here for later reference, and close this BZ as "RESOLVED"?
Created attachment 303583 [details] [review] [patch] settings: don't enforce user session Attach the last unmerged patch from pavlix/session branch for later reference. This patch was NACK'ed in comment 78.
Created attachment 321960 [details] [review] [patch] jk/bgo686997-session-tracking Attaching obsolete branch jk/bgo686997-session-tracking from Jirka. Applies on commit 4cc3c6ab71cc9d89c3345752bd26262d60f636e4