chilcote / outset

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

LaunchDaemon Modifications #51

Closed franton closed 6 years ago

franton commented 7 years ago

Suggestion: Append the following code into each of your launchd files.

` EnvironmentVariables

PATH /usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin ` That way the full standard macOS path will be available to the scripts being run, otherwise you end up having to call everything with full file path. Any dependencies in the commands run in those scripts also tend to fail.
franton commented 7 years ago

Sorry for the spacing. Github keeps mangling it!

chilcote commented 7 years ago

I'm not opposed to this but to play devil's advocate: using full paths is good script hygiene, isn't it?

In any case, I don't really have the time to try this out. If you want to submit a PR with a test plan, feel free.

erikng commented 7 years ago

I’m somewhat opposed to this just for the fact that I’ll need to update the launchdaemons (which to do safely requires a reboot) and could potentially cause issues with other people’s scripts/binary calls.

I think if you are calling a binary, you should specify its full path. Someone could put a $bad version of your binary in /usr/local, which would trump your version. Explicitly passing the path to the binary would resolve your issue and not lead you to potential security issues in the future.

Thanks, Erik Gomez


From: Joseph Chilcote notifications@github.com Sent: Wednesday, October 18, 2017 6:30:16 PM To: chilcote/outset Cc: Subscribed Subject: Re: [chilcote/outset] LaunchDaemon Modifications (#51)

I'm not opposed to this but to play devil's advocate: using full paths is good script hygiene, isn't it?

In any case, I don't really have the time to try this out. If you want to submit a PR with a test plan, feel free.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/chilcote/outset/issues/51#issuecomment-337756943, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFa_GInqwyHQ6-6clT64MtDfCQAMgch4ks5stooIgaJpZM4P9UYZ.

chilcote commented 6 years ago

I'm gonna assume that we don't want to add these paths to the launchd plists. Silence is golden, so I'll close this unless I hear otherwise.

franton commented 6 years ago

Well I find it pretty handy, despite protests from others and I'm custom modifying the outset i'm pushing with no ill effects. I leave the choice to yourself.

chilcote commented 6 years ago

Forgive my ignorance, but I'm unclear what this is accomplishing. I tested to see what is in $PATH in a login-every script, and it's identical to what you want explicitly added to the launchd plists:

$ cat /usr/local/outset/login-every/foo.sh 
#!/bin/bash
echo $PATH >> /tmp/foo.txt
$ /usr/local/outset/outset --login
Processing /usr/local/outset/login-every/foo.sh
$ cat /tmp/foo.txt 
/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
franton commented 6 years ago

Really? Because my tests are showing that anything privileged isn’t getting path variable at all without the mods.

gregneagle commented 6 years ago

If you want a specific PATH in your script, define it. That's the only safe approach.

franton commented 6 years ago

I don’t want a specific path. I want the standard OS path available in an environment run through launchd.

I don’t get this at all. It’s not a security flaw to specify that: the areas I’m after are covered by SIP. I’m not after anything weird, or that you wouldn’t get running a standard terminal window.

What’s the beef?

chilcote commented 6 years ago

Yeah looks like the login-privileged launchd doesn't include /usr/local/bin in the path. shrug. I guess because of the way it's being loaded as OnDemand, but who knows. In any case, I dunno, I'm disinclined to make any sorts of assumptions about $PATH, and leave that up to the admin.

$ cat /usr/local/outset/login-privileged-every/foo.sh 
#!/bin/bash
echo $PATH >> /tmp/foo.txt
$ /usr/local/outset/outset --login
$ cat /tmp/foo.txt 
/usr/bin:/bin:/usr/sbin:/sbin
$ cat /usr/local/outset/boot-every/foo.sh 
#!/bin/bash
echo $PATH >> /tmp/foo.txt
$ sudo /usr/local/outset/outset --boot
Processing /usr/local/outset/boot-every/foo.sh
Boot processing complete
$ cat /tmp/foo.txt 
/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin
gregneagle commented 6 years ago

There's no beef. There's no need. If your script expects a specific PATH (even if you think of it as a standard PATH) then you should define it. The fact that launchd jobs launched as root don't have that PATH is a clue that the "standard" you think you want isn't so standard after all.

Even if @chilcote were to add this, there are lots of other tools/environments/mechanisms where you can't rely on a script having a specific PATH. Therefore if you want to rely on a specific path, define it in your script. Or full-path all executables. Those are the only generically safe approaches.