Consolidate OSX signing params

[malept]

Per discussion in #285, I feel that there are too many OS X signing options, and that it would be better to group them together. This has the added benefit of leaving in room for a potential future Windows signing module to be integrated (#32).

CC: @sethlu

[malept]

For the moment, I want to get a consensus on what the API/CLI params should look like, so I did the docs first.

@positlabs per #285 (comment), I don't know what bundle-identifier is. That doesn't seem like the same as identity?

[sethlu]

@malept Do we then feed the osx-sign object into electron-osx-sign? That could work quite nicely when put together.
Also, we could try the same mechanism for modifying the various plists' items in OS X builds?

[malept]

Do we then feed the osx-sign object into electron-osx-sign?

That was the plan, yes.

Also, we could try the same mechanism for modifying the various plists' items in OS X builds?

That sounds good too, but that should wait until after 6.0.0 is released.

[positlabs]

@malept, sorry, I meant identity, not bundle-identifier.

I kind of started working on this. Here is my progress, in case it helps: https://github.com/positlabs/electron-packager/blob/master/mac.js#L162-L166

There is some extra logic for handling identifier. I think ideally we would copy the whole osx-sign object and pass it to electron-osx-sign without modification. Maybe this warrants an update to osx-electron-sign?

[malept]

I think ideally we would copy the whole osx-sign object and pass it to electron-osx-sign without modification. Maybe this warrants an update to osx-electron-sign?

👍 Keeping the logic in electron-packager minimal is a goal of the project.

[sethlu]

On https://github.com/positlabs/electron-packager/blob/master/mac.js#L166, I think we may go with
identity: signOpts.identity || true,
identity: signOpts.identity,
so when signing from CLI, we could simply use --osx-sign to trigger auto discovery of settings? Now we may need --osx-sign.identity instead.
Previously this line both triggers code-signing and checks whether opts.sign to be a flag or not. As we now have opts['osx-sign'] instead as a flag signaling the code-sign, I think those lines may need changing to:

identity: signOpts.identity || undefined,
entitlements: signOpts['entitlements'] || undefined,
'entitlements-inherit': signOpts['entitlements-inherit'] || undefined,
binaries: signOpts.binaries || undefined

Then --osx-sign could enable code-signing of packaged app or --osx-sign.identity, as well, enables signing but specifies the cert used at the same time? opts['osx-sign'] may be either a bool or an object.

[positlabs]

Do you think this would work? I want to use the whole osx-sign object so it can serve as a passthrough to electron-osx-sign

      var signOpts = opts['osx-sign']

      // --osx-sign from CLI or osx-sign: true from API 
      if(signOpts === true){
        signOpts = {
          identity: true
        }
      }

      // assign additional options from main opts
      if(typeof signOpts === Object){
        signOpts.app = finalAppPath
        signOpts.platform = opts.platform
      }

[malept]

I'm not a fan of having parameters be more than one type, but we've already got a precedent with tmpdir. As with tmpdir, there would need to be special handling of the argument in cli.js (because minimist can't handle an argument that's more than one type).

[malept]

I updated the description of osx-sign in both the CLI and API docs. Let me know if those changes don't accurately reflect the discussion.

[positlabs]

Looks good. Should we add docs for binaries and verbose?

[malept]

...yeah, why not. It's not too much effort. I really want to feature freeze master so 6.0.0 can get out the door.

[malept]

@positlabs do you mind writing the docs here (or in a branch of this branch)? I won't be able to get to it until tomorrow at the earliest.

[positlabs]

Sure, I can do that now

[positlabs]

I can't figure out how to grab this branch. Maybe I have to be an org member? Anyway, here's a snippet from the electron-osx-sign docs.

`binaries` - *Array*

  Path to additional binaries that will be signed along with built-ins of Electron. Default to null.

`verbose` - *Boolean*

  Verbose flag, to display logs. 

[malept]

This is how I'd do it:

git clone https://github.com/electron-userland/electron-packager
git checkout osx-sign-single-param

[malept]

I would prefer it if we have an abbreviated version electron-osx-sign docs (don't say what the defaults are, for example) and make it obvious that for more information, you need to look at the other module's docs. I think I already did that in the readme.

[positlabs]

Huh, deleting the repo and re-cloning did the trick.

For the sake of brevity, and the fact that these options are documented elsewhere, I think we should only add docs for binaries.

[sethlu]

I'm thinking if we need to here distinguish darwin and mas as the signing identities are different for the two if both specified from osx-sign (and I'll have a think on how to put it more naturally if to have these two settings separated).
Also, once we have the params consolidated, I could have these pages on https://github.com/electron-userland/electron-osx-sign/wiki updated.

[sethlu]

Here's my interpretation of this current discussion: (Please correct me if any's misunderstood.)

• If to codesign an app with electron-packager, then a flag --osx-sign should be expected. (However, we are not taking into account that a mas distribution needs signing before going submitted.)
• If to codesign an app with electron-packager, however, with custom identity, then (without an explicit --osx-sign flag) --osx-sign.identity will call codesign with the identity plugged in.

On the verbose flag, I think it could go with --verbose, so opts.verbose; with a verbose flag given for electron-packager (if it eventually is going to have something like it), the same should apply for electron-osx-sign.

[malept]

However, we are not taking into account that a mas distribution needs signing before going submitted.

We should address this before this gets merged.

Re: the verbose flag: I think your DEBUG change will be sufficient, I don't think we need to pass that in explicitly.

[positlabs]

I have added a few lines here, but I'm unsure of how to merge into this PR.

(However, we are not taking into account that a mas distribution needs signing before going submitted.)

@sethlu I think we can allow mas builds to be unsigned in case the user wants to implement their own solution

[malept]

@positlabs I'll merge it tomorrow. Looks good so far. Do you want to put your initial implementation in there as well?

[sethlu]

Re: the verbose flag: I think your DEBUG change will be sufficient, I don't think we need to pass that in explicitly.

@malept nice. I should be able to have electron/osx-sign#24 merged so DEBUG could be supported in a later release.

@sethlu I think we can allow mas builds to be unsigned in case the user wants to implement their own solution

@positlabs I don' know, because the entitlements specified should be different as well. 😕

[malept]

Would there be any reason to allow the user to override platform or app if those options are included in osx-sign? Should we throw a warning/error if they attempt to? Or should we add logic to allow it? I vote we warn. If they want to really fiddle with the options, they can use electron-osx-sign directly.

A warning would be sufficient.

---

So I think what I want to do now is let @positlabs work on their branch (based off of mine), and when they're done, I'll merge that into this branch in a separate PR, then rebase + merge the whole thing into master.

Yep, I know it's pretty convoluted.

[positlabs]

@malept, thanks. I'll add that warning.

Back to @sethlu's comment on mas distributions...

(However, we are not taking into account that a mas distribution needs signing before going submitted.)

Should this be handled in a separate PR? I'm not sure how to address it.

[malept]

I think it's just another warning.

[positlabs]

I made a noob mistake and assumed npm install would pull in my local version of electron-packager even though version hadn't changed. My previous test was invalid.

osx-sign is borking on binaries because they are not done copying before it runs. I'm copying a binaries folder with the asar-unpack-dir option. Is there a way we can run this copy op before signing?

I added some warnings, and my code is here https://github.com/positlabs/electron-packager

[malept]

I assume you mean https://github.com/positlabs/electron-packager/tree/osx-sign-single-param

You may want to run $(npm bin)/standard.

Is there a way we can run this copy op before signing?

I don't see why not, from a conceptual standpoint. Try it and see if any tests fail.

[positlabs]

@Malet, yep, on that branch.

I've realized that the issue is that the files are actually in the temp build directory, and not the final path.

One solution would be to resolve the binaries paths to the temp dir. This adds more logic, but I'm not sure of any other way.

@sethlu do you have any ideas?

[sethlu]

@positlabs On the asar-unpack-dir option, I'd just recommend not to use the asar option so all unpacked binaries that come with one app could be completely signed individually and ready for shipping.
However, as you mentioned that the binaries are packed in asar packages. I can't really tell if those packed executables are permitted to run in a sandbox environment. (I'm not sure if codesign alters the internal structure of executables/binaries, or just stores a hash for testing data integrity.)

Extra Unpacking on Some APIs
Most fs APIs can read a file or get a file’s information from asar archives without unpacking, but for some APIs that rely on passing the real file path to underlying system calls, Electron will extract the needed file into a temporary file and pass the path of the temporary file to the APIs to make them work. This adds a little overhead for those APIs.
From: http://electron.atom.io/docs/v0.36.3/tutorial/application-packaging/#extra-unpacking-on-some-apis

If those executables are unpacked to a temp dir, those binaries unsigned, it could be possible that they work without sandbox. I'll have a check on this now.

[positlabs]

I swear I remember needing to sign these binaries for some reason, but maybe not. Using unsigned binaries in asar-unpacked doesn't seem to affect the .pkg or the .app during runtime, but I will try submitting a test build.

I'm using a lib, node-fluent-ffmpeg, which uses child_process.spawn(), which requires the binaries to live in the asar-unpacked dir according to this.

I suppose I could go without asar, but I think this is an issue worth solving. I just submitted a test build with unsigned asar-unpacked binaries, and no errors came up in the upload process. I think that means the code signing was successful.

Is there ever a reason to sign extra binaries?

[malept]

@positlabs is the binaries signing option the only thing holding up your branch? If so, I'd like to defer that to another PR.

[positlabs]

@malept, yeah that's all. It just doesn't work because of temp dir path. I'm not even sure if it's really needed, but @sethlu might know more.

I can add a warning against it, and comment it out of the docs. I agree it shouldn't hold up v6 any more.

https://github.com/positlabs/electron-packager/tree/osx-sign-single-param

[malept]

I can add a warning against it, and comment it out of the docs. I agree it shouldn't hold up v6 any more.

👍 Thanks for doing that. I'm going to (probably tomorrow) merge your branch into mine, squash your commits, and do some documentation fixes before merging this PR into master, and finally release v6.0.0.

[malept]

In addition to documentation tweaks, I had to revert some syntax/API that didn't exist in Node 0.12, and added some testcases.

[sethlu]

Just found https://github.com/electron/electron/blob/master/docs/tutorial/application-packaging.md#adding-unpacked-files-in-asar-archive. @positlabs I'll have a check later to see if some binaries present in the asar.unpacked dir will be auto-signed.