Skip to content
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

feat: Add functionality to create a "signtool.exe" #7

Merged
merged 4 commits into from
Feb 4, 2024

Conversation

felixrieseberg
Copy link
Member

This PR adds functionality to have @electron/windows-sign create a signtool.exe that signs any given file with the same options passed to @electron/windows-sign in the first place. That's helpful for tools like Squirrel - which will only sign with signtool.exe. It's a bit of a clever hack to make @electron/windows-sign compatible with any tool that'll let users call signtool.exe.

Here's how it works:

  • On the user's machine, we use Node's "single executable binary" feature to create a custom version of node.exe that contains signing options. We'll call that newly minted binary signtool.exe.
  • When our signtool.exe is executed, it spawns user's node.exe, requires @electron/windows-sign, and signs the file originally passed to it with the options already embedded in the binary.
  • Since we create the signtool.exe on the local machine, users can use this tool with confidence - there is no mysterious "signtool.exe" that'd be able to exfiltrate secrets.

I don't expect users to actually use this feature directly. It'll be consumed by @electron/windows-installer, for which I'll open another PR.

Copy link
Member

@erikian erikian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just a few comments, but otherwise this is seriously nice 💯

src/sea.ts Outdated
Comment on lines 32 to 75
const SEA_MAIN_SCRIPT = `
const bin = "%PATH_TO_BIN%";
const script = "%PATH_TO_SCRIPT%";
const options = JSON.parse(\`
%WINDOWS_SIGN_OPTIONS%
\`.trim())

const { spawnSync } = require('child_process');

function main() {
console.log("@electron/windows-sign sea");
console.log({ bin, script });

try {
const spawn = spawnSync(
bin,
[ script, JSON.stringify(options), JSON.stringify(process.argv.slice(1)) ],
{ stdio: ['inherit', 'inherit', 'pipe'] }
);

const stderr = spawn.stderr.toString().trim();

if (stderr) {
throw new Error(stderr);
}
} catch (error) {
process.exitCode = 1;
throw new Error(error);
}
}

main();
`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about turning SEA_MAIN_SCRIPT and SEA_RECEIVER_SCRIPT into actual JS code somehow? I think that would make maintenance a little easier since it would enable editor highlighting and whatnot. I can think of two options:

  1. moving the code to a separate .js file, then reading the file contents, replacing the variables and writing the result to the appropriate destination file;
  2. creating a function to hold the code and writing a stringified version of that function to the appropriate file using an IIFE, like...
function seaMainScript () {
  const bin = '%PATH_TO_BIN%';
  const script = '%PATH_TO_SCRIPT%';
  const options = JSON.parse('%WINDOWS_SIGN_OPTIONS%'.trim());

  const { spawnSync } = require('child_process');

  function main () {
    console.log('@electron/windows-sign sea');
    console.log({
      bin,
      script
    });

    try {
      const spawn = spawnSync(
        bin,
        [script, JSON.stringify(options), JSON.stringify(process.argv.slice(1))],
        { stdio: ['inherit', 'inherit', 'pipe'] }
      );

      const stderr = spawn.stderr.toString().trim();

      if (stderr) {
        throw new Error(stderr);
      }
    } catch (error) {
      process.exitCode = 1;
      // @ts-ignore
      throw new Error(error);
    }
  }

  main();
}

// ...

const script = `(${seaMainScript.toString()
  .replace('%PATH_TO_BIN%', escapeMaybe(binPath))
  .replace('%PATH_TO_SCRIPT%', escapeMaybe(receiverPath))
  .replace('%WINDOWS_SIGN_OPTIONS%', JSON.stringify(options.windowsSign))})()`
;

...or even modifying the function to accept parameters instead of using placeholders and passing them when invoking the IIFE:

function seaMainScript (bin, script, options) {
  // ...
}

// ...

const script = `(${
  seaMainScript.toString()})(
  ${escapeMaybe(binPath)},
  ${escapeMaybe(receiverPath)},
  ${JSON.stringify(options.windowsSign)}
)`;

I've used 2. before, the "invocation"/templating part looks disgusting but at least the main function code gets editor goodies 😅 Anyway, I'm not sure this is something worth changing as the code is relatively short, so feel free to keep it like it is!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a great idea, thank you! I tried to get it to work properly - but it's hard to make prettier & co really understand what's going on, as the scripts here even import the module itself.

I'll try to revisit this later, since I do agree with ya. It's a little annoying that the editor can't tell me about my typos in the string.

src/sea.ts Outdated

if (!cloned.path) {
cloned.path = path.join(os.homedir(), '.electron', 'windows-sign', 'sea.exe');
await fs.mkdirp(cloned.path);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question: don't know if it makes a difference, but it looks like fs.ensureFile would be a better fit since it's the path to a file, not to a folder?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

return true;
}

throw new Error(`Your Node.js version (${process.version}) does not support Single Executable Applications. Please upgrade your version of Node.js.`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you mentioned this will be consumed by electron-winstaller: that module is currently on Node 8, so we might need to update it (and potentially Forge, which is on Node 16 and has electron-winstaller as an optional dep - not sure how we handle these) to 18 first.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a little tough to do with backwards compat - since only a small number of people will actually need to use the sea ability (apps with EV certificates and custom codesigning pipelines in the cloud), I think my recommendation would be:

  • Keep @electron/windows-installer at Node 8. Or upgrade it, but maybe only to ~16 or so.
  • Document that windowsSign options inside @electron/windows-installer requires at least Node 18.

@felixrieseberg felixrieseberg merged commit 8b23eaa into main Feb 4, 2024
3 checks passed
@felixrieseberg felixrieseberg deleted the felixr-sea branch February 4, 2024 21:14
Copy link

🎉 This PR is included in version 1.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants