-
-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Correctly set the security application group to support correct app sandboxing for MAS on OSX #371
Comments
|
@malept electorn-osx-sign only sign app, it seems our |
@develar @develar hmm, I think it's partially to do with https://github.com/electron-userland/electron-osx-sign. We'll see how the testing goes first I think? |
@sethlu And electron/electron@5d906c0 So... if I understand correctly, electron-packager must require Team ID and add
to Info.plist. electron-osx-sign must change entitlements according to docs. Right? |
@develar yup, I think if |
@sethlu Sorry, I am confused.. would it work and pass the MAS review if I follow the guide here? https://github.com/electron/electron/blob/master/docs/tutorial/mac-app-store-submission-guide.md |
@mmm117 As I haven't created a new app since earlier this year, I don't think I may do any tests with submission to iTC. Following the guide, the app should be able to pass the MAS review I think; more updates are available from electron/electron#5584. |
We got our app through the review after updating Electron to 1.1.2+ (which has some MAS related patches). So yeah, those instructions will let you get through the review assuming the app itself passes the apple guidelines. And until this lovely tool adds some way to inject the team-id into the Info.plist during packaging we use this command after packaging with
|
@slaskis Why don't we just edit the "info.plist" file directly? It seems to be simpler.. doesn't it? |
Well for me the process of testing the app sandbox was a bit tedious because it requires the app to be packaged and signed. It was also the first time I ever worked with the mac app sandbox thus it was a bunch of trial and error. So being able to run it in a single command kept me sane. And hopefully it will help someone else. Of course you can do it manually, but I guess you could say that about the electron-packager too, it's mostly convenience (and reliability to not miss any manual steps). |
@slaskis It can be done in single command? Including all steps here? https://github.com/electron/electron/blob/master/docs/tutorial/mac-app-store-submission-guide.md Could you share that command? I only know how to package the app in single command, but not the other steps.. |
@mmm117 Well, with previous versions (before the team id was introduced) we could, using # 1. Package the app
electron-packager path/to/my/app.app # [--everything same as before excluding osx-sign]
# 2. Modify the team id (with the following script or manually do it)
plutil -insert ElectronTeamID -string "TEAM_ID" path/to/my.app/Contents/Info.plist
# 3. Also... Create a custom entitlements file for the app as electron-osx-sign hasn't yet supported a template for the feature (and save it at path/to/my/entitlements.plist). Only need to do it once though.
# 4. Then sign the app with...
electron-osx-sign path/to/my/app.app --entitlements path/to/my/entitlements.plist
# 5. If wish to submit to MAS, use the following to create a flat package (nb: electron-osx-flat comes with electron-osx-sign). It should create a package at path/to/my/app.pkg
electron-osx-flat path/to/my/app.app A sample entitlements file with team id should be like (ref to https://github.com/electron/electron/blob/d6ab81c1da4630e88f8996dc79c3bb3dda264832/docs/tutorial/mac-app-store-submission-guide.md): <?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>com.apple.security.app-sandbox</key>
<true/>
<key>com.apple.security.application-groups</key>
<string>TEAM_ID.your.bundle.id</string>
</dict>
</plist> |
@malept I've just updated electron/osx-sign#44, which follows the requirement from the Electron docs. 😸 After merged, |
@sethlu Thanks. So do I need to create the following entitlement file "child.plist"?
|
@mmm117 No need I think, the |
@sethlu Hmmm. Just so I understand correctly, is the team ID only needed for signing? If so, since you're already using |
@malept I want to avoid double write, so, it is better to add this entry in the electron-packager (since it is required in any case). |
@develar avoiding writing to the plist twice just means you would pass the file pointer. |
@malept I think |
As @malept said, it is not electron-packager responsibility to implement sign — this logic goes to electron-osx-sign (and param team ID to https://github.com/electron-userland/electron-packager/blob/master/docs/api.md#osx-sign). In my PR I am going to pass parsed Info.plist to electron-osx-sign (electron-osx-sign will analyze/modify and save it). Anyway, it is better to discuss in the PR ;) I hope, I will find time tomorrow to do it. |
@develar Thanks for clarifying 😸 I was quite confused just now. (This addition to me's something sitting in between packaging and signing honestly.) |
Sorry, I haven't yet investigate what is team id is. I am slowpoke. If team id is a part of certificate common name You cannot get team id from the identity in all cases (it can be specified just as "Developer Name"), in this case we can use openssl to extract common name from cert — https://github.com/electron-userland/electron-builder/blob/master/src/codeSign.ts#L117 (as electron-builder does on CI server). Or maybe using some |
@develar yea I agree. It's possible that someone inputs just his/her name. What about introducing a routine check for the identity name? It matches the input against the keychain contents and finds the corresponding item with a full team id. |
Hi @malept @develar, with electron/osx-sign#44 soon merged, the entitlements file and Then, signing an app may be like: electron-osx-sign path/to/my.app --version=1.1.1
electron-osx-sign path/to/my.app --version=1.2.0 So far, the two commands above are equivalent to what shows below: electron-osx-sign path/to/my.app --pre-auto-entitlements-app-group as version >= 1.1.1 triggers Add: It's still fine to use: electron-osx-sign path/to/my.app ... Just the user needs to prepare the correct entitlements file with |
👍 for more automation. How much do beginners actually need to know about the signing process? |
So I think to sign everything by default, However, with Electron version v1.1.1, However, going to more customizations, someone may have already added The only time that someone needs looking at Apple docs is when adding their entitlements keys, but not necessarily Just to clarify: So literally all someone needs to know, when signing with the automation introduced, is the version of Electron used (which is put to
|
Oh @malept I just thought that it may be better to assume the user's handling the latest version of Electron... So for every version >= v1.1.1, it's fine to use |
@sethlu I like the idea that electron-packager sends the Electron version that it's going to package and electron-osx-sign will "just work". If you can make that work without too much effort on your end, that would be excellent. |
@malept I've now trying to have |
@malept Just updated the logic... So it goes like this now: Use the following for the latest version of Electron: electron-osx-sign path/to/my.app However, use the following for better compatibility with earlier versions: electron-osx-sign path/to/my.app --version=1.1.1 # for example It's nothing wrong to specify the additional version because it's just there for convenience. Now if someone wants to use the latest release of Electron, and, however, doesn't walk the full features provided by # EITHER
electron-osx-sign path/to/my.app --no-pre-auto-entitlements
# OR
electron-osx-sign path/to/my.app --pre-auto-entitlements=false So the
What do you think? This should help slightly with understanding the introduced feat here in a next release. And it's quite easy to set up as the command |
@sethlu I like your solution.
So, in the end, nothing is required to do on |
@develar great! And yes, there's nothing pending for updates on |
Pretty sure this is handled by |
As per a recent change to Electron. When building for MAS this string variable needs to be correctly set. Electron packager should set this string for us 😄
https://github.com/electron/electron/pull/5584/files#diff-2438340ff7dedbdf61d9a8373a7befb3R57
Although this can't be implemented yet as there is not a released version of electron with this new requirement I thought I would open this here to allow for feature/issue tracking
The text was updated successfully, but these errors were encountered: