feat: Add option to specify login helper entitlements file #210

oNaiPs · 2019-10-15T04:27:50Z

When signing the electron app for App Store, the Login Helper is signed child inherited entitlements. Since the Login Helper is a standalone executable, it crashes on because it shouldn't have the com.apple.security.inherit.

This PR adds an option to specify a custom entitlement file for the login helper.

greenimpala · 2020-04-09T10:28:29Z

I can confirm this is an issue if you are trying to get a MAS app to start at login. @oNaiPs the fix you propose works (I am currently applying it with patch-package) for the time being.

electron-osx-sign+0.4.15.patch.zip

I think you have some linting issues (semicolons etc) from looking at the circleci failing build.
Hopefully if those are fixed somebody can merge this :)

sethlu · 2020-04-12T01:09:46Z

I'll can get this merged if the checks are passing 😸

Also I think it may be better to default the login helper entitlements to the default.entitlements.mas.plist/default.entitlements.darwin.plist instead of the child entitlements file... since the login helper doesn't launch as a child process so the key com.apple.security.inherit may be unnecessary?

Ref: https://github.com/electron/electron/blob/master/docs/tutorial/mac-app-store-submission-guide.md

greenimpala · 2020-04-27T09:14:15Z

@sethlu correct, the login helper should only have the sandbox entitlement to work correctly. If we add inherit (or anything else) - it breaks.

With that in mind, I am wondering whether it is overkill to provide a custom plist for login helper entitlements to fix this. Instead we could just change electron-osx-sign to sign any Library/LoginItems binary with default.entitlements.mas.plist?

On both darwin and mas

sethlu · 2020-05-04T07:54:31Z

@greenimpala I agree that it may be a better approach to default to default.entitlements.mas.plist, and a code signed login helper shouldn't harm apps that are distributed outside the Mac App Store without App Sandbox... right? 🤔

I've pushed some changes to this branch following this approach.

greenimpala · 2020-05-04T08:43:01Z

sign.js

            opts['entitlements-inherit'] = filePath
          }
+          if (!opts['entitlements-loginhelper']) {
+            // Default to App Sandbox enabled
+            filePath = path.join(__dirname, 'default.entitlements.mas.plist')


Should we keep current behavior here and sign non-mas builds with the empty default.entitlements.darwin.inherit.plist? We know this is currently working and I wouldn't like to assume adding sandbox will be harmless.

That's a good point 🤔

Since the apps that are not going to be shipped on the Mac App Store can still be sandboxed, we may need to take into consideration too if app sandbox is enabled for the Electron app and then selectively apply sandbox for the login helper by default too?

If users aren't enabling app sandbox on their Electron apps for distribution outside the Mac App Store we can fallback to empty entitlements?

Isn't that then up to the user to provide a custom entitlements-loginhelper? If they are sandboxing their app then they must have already passed entitlements or entitlements-inherit and be familiar with the custom plist API.

[Edit] Or at this point sign with opts.entitlements. Which would either be a custom plist or the empty default.entitlements.darwin.plist.

That sounds like a good idea 👌
So it'll either be the same entitlements to sign the app bundle or a specifically specified entitlements for the login helper.

jack-arms · 2020-05-27T20:47:49Z

Hey @sethlu, if I understand this thread correctly the only change before this is ready to merge is

if (!opts['entitlements-loginhelper']) {
  filePath = opts.entitlements;
  ...
  opts['entitlements-loginhelper'] = filePath
}

Do you know approximately when you can make that change? I'm eager to have launching on startup work for the Mac store, and also pipe this functionality through to electron-builder as well.

Port of changes from electron/osx-sign#210, plus changes to macPackager

sethlu · 2020-06-08T09:24:45Z

@greenimpala @jack-arms Sorry about the delay!

I've pushed some changes to delay assigning the default value for opts['entitlements-loginhelper'] and it should now be the same entitlements file as what's used to sign the app bundle without user specification.

If you have some time to give it a try that'll be awesome 🙌
If it looks good we can get it out in a few days 😺

greenimpala · 2020-06-08T11:04:57Z

@sethlu Tried the branch and the behavior is working as expected:

1. Without entitlements-loginhelper and entitlements it signed LoginHelper with default.entitlements.mas.inherit.plist. (Breaks MAS).
1. With entitlements-loginhelper it signed LoginHelper with entitlements-loginhelper. (MAS works!).
1. Without entitlements-loginhelper it signed LoginHelper with entitlements. (Breaks MAS in my case as I have inherit).

Perhaps one more addition — if platform == mas AND !entitlements-loginhelper — sign with default.entitlements.mas.plist instead. This would fix 1 and 3.

sethlu · 2020-06-08T20:38:41Z

@greenimpala can you double check if you're running the latest from this branch?

For MAS the following is the current expected behavior:

Without entitlements-loginhelper and entitlements it should sign LoginHelper with the default entitlements file default.entitlements.mas.plist (or a locally modified version when pre-auto-entitlements opted in by default) which doesn't inherit sandbox
With entitlements-loginhelper it should sign LoginHelper with what's specified
Without entitlements-login but with user-specified entitlements it should sign LoginHelper with entitlements (or a locally modified version when pre-auto-entitlements opted in by default) which doesn't inherit sandbox

I think your last paragraph is similar to what we discussed in #210 (comment)?

greenimpala · 2020-06-09T10:48:42Z

Tested again (ef288c3) to double check. I had made a mistake with the your last scenario and can confirm it works as expected.

Without any entitlements it signed with the auto-modified default.entitlements.mas.plist! 🎉

sethlu · 2020-06-11T09:36:06Z

@greenimpala great, I’m merging this now! Will publish a new release on npm tomorrow 😀
@oNaiPs thanks for starting on the PR!

sethlu · 2020-06-12T02:16:07Z

@greenimpala Release 0.4.17 is available now 🚀

greenimpala · 2020-06-12T17:26:26Z

Great job @sethlu! 🎉

* Initial commit Port of changes from electron/osx-sign#210, plus changes to macPackager * Update logic in sign.js

sethlu added the enhancement label Nov 27, 2019

oNaiPs and others added 3 commits May 4, 2020 00:17

Add option to specify login helper entitlement

10f714f

fix: Use default mas entitlements for login helper

1b64c58

On both darwin and mas

fix: Style

51ecaee

greenimpala reviewed May 4, 2020

View reviewed changes

jack-arms added a commit to jack-arms/electron-builder that referenced this pull request Jun 1, 2020

Initial commit

3be2504

Port of changes from electron/osx-sign#210, plus changes to macPackager

jack-arms mentioned this pull request Jun 1, 2020

Add loginhelper entitlement option electron-userland/electron-builder#5023

Merged

Update login helper entitlements default value

ef288c3

sethlu changed the title ~~Add option to specify login helper entitlement~~ feat: Add option to specify login helper entitlements file Jun 11, 2020

sethlu merged commit 114e817 into electron:master Jun 11, 2020

develar pushed a commit to electron-userland/electron-builder that referenced this pull request Jul 9, 2020

feat(mac): add loginhelper entitlement option (#5023)

7109e88

* Initial commit Port of changes from electron/osx-sign#210, plus changes to macPackager * Update logic in sign.js

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add option to specify login helper entitlements file #210

feat: Add option to specify login helper entitlements file #210

oNaiPs commented Oct 15, 2019

greenimpala commented Apr 9, 2020

sethlu commented Apr 12, 2020 •

edited

Loading

greenimpala commented Apr 27, 2020

sethlu commented May 4, 2020 •

edited

Loading

greenimpala May 4, 2020 •

edited

Loading

sethlu May 5, 2020

greenimpala May 12, 2020 •

edited

Loading

sethlu May 23, 2020

jack-arms commented May 27, 2020

sethlu commented Jun 8, 2020

greenimpala commented Jun 8, 2020 •

edited

Loading

sethlu commented Jun 8, 2020

greenimpala commented Jun 9, 2020

sethlu commented Jun 11, 2020 •

edited

Loading

sethlu commented Jun 12, 2020

greenimpala commented Jun 12, 2020

feat: Add option to specify login helper entitlements file #210

feat: Add option to specify login helper entitlements file #210

Conversation

oNaiPs commented Oct 15, 2019

greenimpala commented Apr 9, 2020

sethlu commented Apr 12, 2020 • edited Loading

greenimpala commented Apr 27, 2020

sethlu commented May 4, 2020 • edited Loading

greenimpala May 4, 2020 • edited Loading

Choose a reason for hiding this comment

sethlu May 5, 2020

Choose a reason for hiding this comment

greenimpala May 12, 2020 • edited Loading

Choose a reason for hiding this comment

sethlu May 23, 2020

Choose a reason for hiding this comment

jack-arms commented May 27, 2020

sethlu commented Jun 8, 2020

greenimpala commented Jun 8, 2020 • edited Loading

sethlu commented Jun 8, 2020

greenimpala commented Jun 9, 2020

sethlu commented Jun 11, 2020 • edited Loading

sethlu commented Jun 12, 2020

greenimpala commented Jun 12, 2020

sethlu commented Apr 12, 2020 •

edited

Loading

sethlu commented May 4, 2020 •

edited

Loading

greenimpala May 4, 2020 •

edited

Loading

greenimpala May 12, 2020 •

edited

Loading

greenimpala commented Jun 8, 2020 •

edited

Loading

sethlu commented Jun 11, 2020 •

edited

Loading