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

Sign release binaries and installers for Windows #1034

Closed
na-- opened this issue May 30, 2019 · 4 comments · Fixed by #1746
Closed

Sign release binaries and installers for Windows #1034

na-- opened this issue May 30, 2019 · 4 comments · Fixed by #1746
Labels
ci evaluation needed proposal needs to be validated or tested before fully implementing it in k6 ux
Milestone

Comments

@na--
Copy link
Member

na-- commented May 30, 2019

As illustrated by this recent issue, we might benefit from signing our binary releases with a valid key for Windows.

To be honest, I'm not sure if that would prevent or even decrease future antivirus false-positive results, but even if we just make that "Do you want this app from an unknown publisher to make changes to your PC" warning dialog less scary, it's probably worth it.

Some resources on the topic:

Potentially connected issues:

  • Use Azure Pipelines for CI Use Azure Pipelines or GitHub Actions for CI #959:
    Since it's a Microsoft service, it's worth investigating whether there is some sort of a better way to do binary signing there. I'm somewhat worried about just uploading our future code-signing key to a random CI service, so maybe they have some sort of a key enclave or managed signing or something that adds a bit of security and transparency to the process.
  • Chocolatey installation (this new PR Add chocolatey gallery installation method #1027 and the current windows installation instructions)
    Worth investigating if chocolatey, being a package manager, would actually benefit from this whole binary code-signing business. In the Linux world, it's not the binaries that are signed, but the different rpm/deb/apk/pacman/etc. packages...
@na-- na-- added ci evaluation needed proposal needs to be validated or tested before fully implementing it in k6 ux labels May 30, 2019
@majkinetor
Copy link

majkinetor commented Aug 7, 2019

Worth investigating if chocolatey, being a package manager, would actually benefit from this whole binary code-signing business

It wouldn't at this moment AFAIK. Furthermore, Chocolatey Gallery has integration with Virus Total so packages are as safe as it can get because they are not going to be approved if there are more then couple of false positives on entirety of 70+ antivirus engines.

In the case of latest version there is one false positive result:

image

Previous version has 2 results.

This is the list of AV's used, there is no Windows Defender tho.

@imiric
Copy link
Contributor

imiric commented Nov 26, 2020

After a couple of days of diving into the Windows certificate world 😅, I finally managed to produce a signed binary and installer. 🎉

You can see the CI changes and download the signed MSI from here.

I opted to not use any third-party GH Actions for this because of security reasons:

  • this one uses MS' signtool.exe but doesn't allow specifying a password, presumably because it expects a blank one... plus it's written in JS...
  • And the other uses osslsigncode, which has a noble goal of open sourcing this functionality and allowing it to be cross-platform, but we should probably use the suggested MS tool for this to avoid any potential issues, and the windows-latest GHA image already comes bundled with signtool.exe, so we don't need the cross-platform support.

Anyway, here's how it looks like in Windows:
2020-11-26-125319_1572x657_scrot

However, having a signed installer doesn't get rid of the "Windows protected your PC" warning dialog... 😭

Now it shows the same screen just with the correct publisher information, whereas before it showed "Unknown publisher":
2020-11-26-130232_676x633_scrot

This SO answer and this linked one suggest that in order to remove this warning a publisher can either "gain" trust with SmartScreen over time, presumably by a lot of users choosing to ignore and run this signed installer anyway, but I don't see any guarantees of when or if that will eventually happen... Or, we can outright "buy trust" with an EV certificate, which the current one that Robin provided is not. These are usually much more expensive than regular certs and it's a plain scam if you ask me, but perhaps it might be worth it as a shortcut to get rid of this warning.

@robingustafsson @na-- @mstoykov What do you think?

@imiric
Copy link
Contributor

imiric commented Nov 26, 2020

Here's an idea of the time and number of installs it takes for SmartScreen to gain trust in a publisher without an EV cert:

For me, it took 16 days and some 500 to 2000 installs before SmartScreen trusted my brand-new certificate.

And the other answer for that question mentions that even an EV cert is not guaranteed to get rid of this warning, but it probably speeds things up.

Given this, I'm inclined to suggest that we start using this non-EV cert from v0.30.0 and just wait for MS to trust it. I reckon we can hit ~2K installs in a couple of weeks.

@na--
Copy link
Member Author

na-- commented Nov 26, 2020

Hmm not sure if we should buy into the MS EV certificate racket. Even this basic signing gets rid of the other "unknown publisher" warning, and puts Load Impact AB in the blue dialog in the last screenshot, so it's definitely better than nothing and may be good enough for now...

Regarding signtool.exe - I think you made the right decision. osslsigncode seems interesting and I'm glad it exists, but since we're already running on a windows VM for other reasons, we have no need to use it.

imiric pushed a commit that referenced this issue Nov 30, 2020
imiric pushed a commit that referenced this issue Dec 1, 2020
* Sign Windows release binary and installer

Closes #1034

* Use more generic signtool path

Resolves #1746 (comment)

* Update change in secret name

Resolves #1746 (review)

* Remove PFX file after signing

Resolves #1746 (review)
@na-- na-- added this to the v0.30.0 milestone Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci evaluation needed proposal needs to be validated or tested before fully implementing it in k6 ux
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants