chilcote / outset

Automatically process packages, profiles, and scripts during boot, login, or on demand.
572 stars 58 forks source link

Logout scripts option #35

Closed aysiu closed 8 years ago

aysiu commented 8 years ago

I've written (and done preliminary testing on my own machines) a modification that will allow for logout scripts as well.

com.github.outset.logout.plist goes into /Library/LaunchAgents

It calls the /usr/local/outset/share/logout.sh script, which in turn (at logout anyway) calls /usr/bin/python /usr/local/outset/outset --logout

chilcote commented 8 years ago

I'm not opposed to this on principle, but there are two glaring problem that needs to be addressed first:

  1. If the launchagent process crashes, it won't respawn.
  2. If the launchagent process crashes, it will immediately run the --logout jobs, without notice to the user.

This is potentially destructive, especially in the context of how I understand logout hooks to work in practice (e.g. "delete the user profile in a lab environment").

I think, since logout hooks still work, we should let this percolate and find a better method.

aysiu commented 8 years ago

Those are valid concerns, certainly. I'd love to hear input from others on how best to implement this. Logout hooks have theoretically been deprecated since 10.4, but Apple hasn't made them non-functional. So would a logout hook make you feel better about it?

Only one anecdotal data point, of course, but I've been using this method (not paired with Outset—just on its own) for the past 10 months on our machines and haven't seen any issues.

chilcote commented 8 years ago

What's the primary environment for this? Labs? I'll let others pipe in, but my main concern is one-to-one setups, with users who may not reboot or logout for weeks at a time.

chilcote commented 8 years ago

A third issue: This is run as a LaunchAgent, in the user context, so this isn't technically a solution to replace existing logout hook workflows, which run as root.

aysiu commented 8 years ago

I can speak only for myself, but I use it mainly for labs, which are set to log users out automatically (after five minutes of inactivity), and the scripts I run just clean up some user-specific files, so they don't need to be run as root. Others may find that less useful, possibly. I'm definitely open to writing a pull request that uses the official (but also officially deprecated) logout hook, if that'd be more useful to other folks.

I could also, if we can't come to a consensus, write a separate project that just does logout scripts. That way people can just use Outset for boots, logins, and ondemands, and then they could use a separate but similar tool for just logouts (accepting the risks)—as long as they had one folder to dump the scripts into.

clburlison commented 8 years ago

I can see the benefit and unfortunately am unable to a alternative solution however as this currently stands this wouldn't be an acceptable solution in my environment. The shear fact that if the process was killed the logout script(s) would be ran is a show stopped for me.

My only recommendation would be to use Apple's deprecated logout-hook method that doesn't have that same issue.

chilcote commented 8 years ago

I don't think it's necessary to create a wraparound for logout hooks since Apple already has a (deprecated) method for configuring that (my understanding is that there can only be one logout hook).

When we went back and forth over the on-demand additions, we stuck with the core tenet that the admin alone should define when scripts run. With this logout trap which may also be inadvertently triggered by a process termination, that control is taken from the admin.

Maybe this is better suited for a separate, parallel, project? "Offset", anyone?

aysiu commented 8 years ago

Yeah, I may do an Offset or Offshoot then based on your code. It would probably be a separate similar directory like /usr/local/offset/logout-every and /usr/local/offset/logout-once

And it seems the logout hook may be the way to go, despite being "deprecated."

chilcote commented 8 years ago

Has Apple ever indicated what they recommend instead of logout hooks? I submitted a radar just in case.

aysiu commented 8 years ago

No, they haven't. This is what their official documentation says:

Login and logout scripts are a deprecated technology. In most cases, you should use launchd jobs instead, as described in Creating Launch Daemons and Agents. Login and logout scripts are run as root, which presents a security risk.

Of course when you visit that link, there's absolutely zero about logout launch agents / daemons.

clburlison commented 8 years ago

Relevant: https://jamfnation.jamfsoftware.com/discussion.html?id=17668#responseChild104096 @bruienne is evidently using that exact same method. I'm still pretty against the idea however maybe it isn't as bad as I think it is?

aysiu commented 8 years ago

As I mentioned before, I've been using it for about 10 months with absolutely zero issues. I think @chilcote brings up some good points in theory, but the method works just fine.

aysiu commented 8 years ago

I've created a first working draft of Offset (thanks, @chilcote for the name suggestion). It is heavily, heavily based on code from Offset and uses the LogoutHook instead of the launch agent: https://github.com/aysiu/offset

chilcote commented 8 years ago

I'll take a look, thanks. Again, I'm not against this PR. Let's try it out and see? If the docs are solid, then maybe that will be sufficient to keep people from shooting themselves in the foot. After all, we don't have to put anything in the logout directories :)

aysiu commented 8 years ago

Thanks. Let me know what you think is best. I'd love to get the PR into Outset (maybe even with a huge disclaimer about the remote possibility of premature script execution), but if you think Offset may be a viable and recommended implementation, I'm also more than happy to maintain that.

chilcote commented 8 years ago

Let's let it sit for a week or two for testing. Hopefully anyone out there following these issues and pull requests will test it out (hint hint) and chime in on whether they think it's a good idea.

aysiu commented 8 years ago

That sounds like a good plan.

I do think this is a worthwhile endeavor—just a matter of whether it's integrated into Outset with a launch agent, integrated with a Logout Hook, or not integrated at all and its own separate project.

A consensus doesn't have to be reached, but it would be really nice to have a standard recommended way to go for Mac Admins who find Outset useful (and who may find a logout scripts folder also useful).

gregneagle commented 8 years ago

One alternative to a logouthook I've used successfully is a LaunchAgent that runs in the loginwindow context.

User logs out, and when the machine returns to the loginwindow, the loginwindow LaunchAgent runs (as root, even).

It doesn't run at the exact same time as the loginhook did, and does not have direct access to the name of the user who is logging out, but with some additional effort you can determine who just logged out: defaults read /Library/Preferences/com.apple.loginwindow lastUserName (one might want to double-check that the value of defaults read /Library/Preferences/com.apple.loginwindow lastUser is loggedOut...)

One additional thing you might need to handle: the script will also be called at boot when the loginwindow is displayed before the first user logs in.

gregneagle commented 8 years ago

One more comment: yes, the proposed implementation for user-level logout scripts is a bit of a hack, and the scripts could be accidentally triggered outside of an actual logout, but: the admin should be aware of these possible issues. If it bothers the admin, they can choose not to use this feature of Outset and use a different method to run things at logout if needed. (logouthooks, loginwindow LaunchAgent, etc)

chilcote commented 8 years ago

Yes, as long as it's documented with the usual caveats, it's up to the admin whether or not to use the provided rope to hang him/herself.

aysiu commented 8 years ago

So a launch agent would be the way to go?

The current pull request I made uses a launch agent that runs a script at logout (or if the launch agent gets prematurely killed).

Greg, do you mind sharing your launch agent that runs at the login window instead of at logout?

gregneagle commented 8 years ago
<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
  <key>Label</key>
  <string>com.foo.bar</string>
  <key>LimitLoadToSessionType</key>
  <array>
    <string>LoginWindow</string>
  </array>
  <key>ProgramArguments</key>
  <array>
    <string>/usr/local/bin/foo_bar</string>
  </array>
  <key>RunAtLoad</key>
  <true/>
  </dict>
</plist>
aysiu commented 8 years ago

Thanks, Greg.

aysiu commented 8 years ago

Okay, I just made some major changes based on what we discussed. I'd love it if people could test:

  1. The .plist Greg shared (modified slightly for path to script and argument) should go in /Library/LaunchAgents
  2. /usr/local/outset/outset will check to see the output of lastUser to make sure it is loggedOut before running anything. I double-checked and whether you did a fresh boot up from shutdown or just a live reboot, the lastUser will be Restart instead of loggedIn or loggedOut
  3. I also updated the README file to explain a bit about how the logout scripts work.

Would love some testers...

gregneagle commented 8 years ago

https://github.com/aysiu/outset/blob/f13f4326072a24b08221605cadfaa44047a69ef9/outset#L363

I'd think that would always print "None" or "loginwindow" as the valid of the "console_user"... Is that what would be expected?

aysiu commented 8 years ago

I don't know. That was in the original code. I just wrapped my own if / else around it.

gregneagle commented 8 years ago

But your context is now different. You are proposing the change -- you should follow through with the issues.

aysiu commented 8 years ago

Okay. Just did a fix on that and even tested it by adding an ignored user and removing the ignored user. Should be good now.

chilcote commented 8 years ago

Catching up. So the new thought is to run the launchagent in the loginwindow context? Any concerns with the scripts running at boot? For example, what if someone installs the logout-once scripts onto an image (I know I know, but they will). When the system loads, those scripts will run at the login window, and then won't run when the user logs out at a later time.

Another question: what happens when you reboot instead of logging out? Will this LaunchAgent be triggered?

I'll try and test this weekend.

gregneagle commented 8 years ago

https://github.com/chilcote/outset/pull/35#issuecomment-192516528

"/usr/local/outset/outset will check to see the output of lastUser to make sure it is loggedOut before running anything. I double-checked and whether you did a fresh boot up from shutdown or just a live reboot, the lastUser will be Restart instead of loggedIn or loggedOut"

gregneagle commented 8 years ago

(note that I'm not advocating this approach at all, just explaining what's been proposed so far)

chilcote commented 8 years ago

Ah.... clever. If that works as described, that would be a better approach than the sig traps. Nice.

gregneagle commented 8 years ago

With the huge differences that the script runs after the logout is complete and that it runs as root.

logouthooks run before the logout is entirely complete and they run in the context of the user that is logging out. Same goes for the launch agent script sig trap approach.

aysiu commented 8 years ago

@gregneagle, I like your login screen Launch Agent. It definitely addresses @chilcote 's original concerns about the launch agent potentially being killed before logout and running the scripts without prompting the user. Running as root, as opposed to in the user context, may just be the least worst tradeoff.

Maybe we can just say in the documentation to run the scripts with care, since they will run as root?

chilcote commented 8 years ago

"Logouthooks run before the logout is entirely complete and they run in the context of the user that is logging out. Same goes for the launch agent script sig trap approach."

Don't logouthooks run as root?

gregneagle commented 8 years ago

"Don't logouthooks run as root?"

Yes, they do. Getting all the alternatives confused.

LaunchAgent-sig-trap: user loginhook: root loginwindow LaunchAgent: root

chilcote commented 8 years ago

OK after playing around with this, I still have reservations. Mainly, similarly to the previous PR, it's not really a "replacement" for logouthooks since the scripts won't trigger if the user reboots instead of explicitly logging out. Also, I wasn't able to get the logout-once scripts to work, and I think it's because you have once=True instead of delete_items=True in the args.logout section.

However, I'm also still hesitant because with this, we would be adding complexity to outset to not-quite match what can already be done with a logouthook. More complexity to get little or less benefit isn't a goal of outset.

I think we should wait until Apple gives guidance on what they plan to replace logouthooks with before we start trying to replace it ourselves. To this end, I have a radar open with Apple, and I encourage everyone to duplicate it. Or, better yet, write a better bug report than I'm capable of doing and I'll dupe that :)

http://openradar.appspot.com/radar?id=5047810739142656

aysiu commented 8 years ago

I appreciate you viewing this as adding more complexity to Outset with little benefit. There's quite a bit of benefit for my school to have multiple logout scripts instead of one logout hook, so I'll likely bring this over to Offset and develop that a bit further.

chilcote commented 8 years ago

Thanks @aysiu. Remind me to put a link to Offset in the readme.

aysiu commented 8 years ago

Let me do some more development/testing on Offset first before you put it in the readme... but, yes, that'd be awesome, and I'll remind you.

aysiu commented 8 years ago

No rush on linking, but I have a first working version up, so you can link to Offset if you'd like: https://github.com/aysiu/offset

Still would like more testers than just me--maybe @majorsl, who suggested the logout feature originally.