Invoke signing function for Windows Connect binary

[fheinecke]

The Windows Teleport Connect binary is not currently signed, only the installer. This PR invokes the Powershell signing function that we use to sign all other Windows binaries.

changelog: Teleport Connect binaries for Windows are now signed.

[gzdunek]

I think we have to do some changes:

1. Signing is invoked a few times, because electron-builder signs a few executables. We can add "signingHashAlgorithms": ["sha256"] to avoid calling sign with sha1 option (which we ignore anyway). This will reduce the number of calls by half.
2. I think I was wrong when it comes to resultOuputPath, I see it is undefined (except of the last invocation https://github.com/gravitational/teleport.e/actions/runs/9211610782/job/25341406104). Idk what does this option mean, perhaps we should do Invoke-SignBinary -UnsignedBinaryPath "${customSign.path}" -SignedBinaryPath "${customSign.path}", so the file is written in the same place.
3. Signing should be done only on the CI, locally it throws The term 'wsl-ubuntu-command' is not recognized as the name of a cmdlet, should we add an env variable to control this? 🤔

[fheinecke]

1. Signing is invoked a few times, because electron-builder signs a few executables. We can add "signingHashAlgorithms": ["sha256"] to avoid calling sign with sha1 option (which we ignore anyway). This will reduce the number of calls by half.

Won't this potentially impact customers? If so, we might want to leave this as is.

2. I think I was wrong when it comes to resultOuputPath, I see it is undefined (except of the last invocation https://github.com/gravitational/teleport.e/actions/runs/9211610782/job/25341406104). Idk what does this option mean, perhaps we should do Invoke-SignBinary -UnsignedBinaryPath "${customSign.path}" -SignedBinaryPath "${customSign.path}", so the file is written in the same place.

Do you know where I can find docs on the signing function/the object used for signing?

If possible, I'd prefer to not output to the same file. This might affect how we sign other Windows binaries.

3. Signing should be done only on the CI, locally it throws The term 'wsl-ubuntu-command' is not recognized as the name of a cmdlet, should we add an env variable to control this? 🤔

Probably. Do I just need to do if (process.env["SOME VAR"]) { return; }?

[gzdunek]

Won't this potentially impact customers? If so, we might want to leave this as is.

It shouldn't, we don't support Windows 7 anyway.

Do you know where I can find docs on the signing function/the object used for signing?
If possible, I'd prefer to not output to the same file

There is no documentation for these options 🙈, that's all I could find: https://www.electron.build/configuration/win.html#how-do-delegate-code-signing
https://www.electron.build/tutorials/code-signing-windows-apps-on-unix.html#integrate-signing-with-electron-builder (take a look at the last example, they pass path to that JSign tool).

I looked into the source code, and it seems to me that you have to create the resultOutputPath manually. It is then used to rename the signed file again into the original file path. I believe it is for situations where the signing tool doesn't support signing in-place. I think it should be used like this:

    sign: customSign => {
      customSign.resultOutputPath = `signed-${customSign.path}`;
      ...
            `Invoke-SignBinary -UnsignedBinaryPath "${customSign.path}" -SignedBinaryPath "${customSign.resultOutputPath}"`,
        ],
        { stdio: 'inherit' }
      );
    }

BUT, if our tool is fine with having the same input and output path then I'd keep using path.

Probably. Do I just need to do if (process.env["SOME VAR"]) { return; }?

Yeah, although it should rather be opt-in, not opt-out:

  if (!process.env.SOME_VAR) {
    console.warn(
      'missing $SOME_VAR: signing will be skipped'
    );
    return;
  }

[fheinecke]

I think I've fixed the above. Please take a look. If the signing code is reasonable, then I'll run a test build prior to merge.

[gzdunek]

I still think that we should set "signingHashAlgorithms": ["sha256"]. These signing algorithms are passed to the sign function, so the signing tool can be invoked with different algorithms (customSign.hash). We don't read that value at all, so we end up invoking the tool with exactly the same parameters twice (and I believe the second invocation will be signing the already signed binary).

[gzdunek]

The code looks good (I left some minor comments).
I think we can run the test build after addressing them.

[gzdunek]

Suggested change

	if (process.env.CI != "true") {
	if (!process.env.CI) {

[fheinecke]

The variable will be a string rather than a bool. It can also be set to other values (such as "false") in some cases. Is this implicit type conversion a safe approach?

[gzdunek]

Okay, I thought it's a boolean.
We may only want to replace != with !== (strict equality check) to make sure that no type conversion happens.

[fheinecke]

@gzdunek do you have an easy way to check the binary signature? If so, would you mind verifying this PR? I've built a binary, tag v17.0.0-dev.fred-win-fix-1.2. Binary should be in S3.

I don't have a windows box easily available, but I could stand one up later this week to test.

[gzdunek]

Yes, I have a Windows VM, I will test it later today!

[gzdunek]

I tested the build and unfortunately it is still not signed. In the build logs I see:

Invoke-SignBinary : The term 'New-Guid' is not recognized as the name of a cmdlet, function, script file, or operable 
  program. Check the spelling of the name, or if a path was included, verify that the path is correct and try again.
  At line:1 char:119
  + ... /build.ps1; Invoke-SignBinary -UnsignedBinaryPath "C:\a\teleport.e\te ...
  +                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      + CategoryInfo          : ObjectNotFound: (New-Guid:String) [Invoke-SignBinary], CommandNotFoundException
      + FullyQualifiedErrorId : CommandNotFoundException,Invoke-SignBinary

so I think there is a problem with running the PS script?

[gzdunek]

I tested the 17.0.0-dev.fred-win-2.7 build [download] and the app is finally signed (as well as the uninstaller).

But I think we have to do some cleanup in this PR: revert the tag build stuff, and most of the changes in build.ps1, no? We don't need that logging.

[gzdunek]

@fheinecke I let myself remove the PR from the queue, in case you want to address the comment I left :)

[public-teleport-github-review-bot]

@fheinecke See the table below for backport results.

Branch	Result
branch/v13	Failed
branch/v14	Failed
branch/v15	Failed
branch/v16	Failed