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 Windows release binary and installer #1746

Merged
merged 4 commits into from
Dec 1, 2020
Merged

Conversation

imiric
Copy link
Contributor

@imiric imiric commented Nov 30, 2020

Closes #1034

I tested this in my fork and you can download the signed MSI from here. As explained in this comment, I chose to not use any 3rd-party extensions to avoid any security issues. As noted there, this won't remove the "Windows protected your PC" warning, but should eventually if we reach a certain "trust" level with Microsoft, which is gained by a lot of people choosing to run the app anyway.

Two secrets were added to the GitHub repo for this: WIN_SIGN_CERT and WIN_SIGN_PASS. Information about both can be found in the password manager.

Note that currently this only signs the binary in the MSI package and the MSI itself that are uploaded to Bintray. The binary that is uploaded to GitHub and available as release assets is not signed, as that would be more convoluted to setup, since signtool.exe is Windows-only and the build stage happens on Linux. I suppose we could overwrite the assets from Windows, but I didn't test this.

@imiric imiric requested review from mstoykov and na-- November 30, 2020 11:15
@@ -307,13 +307,23 @@ jobs:
Expand-Archive -Path ".\dist\k6-$env:VERSION-win64.zip" -DestinationPath .\packaging\
move .\packaging\k6-$env:VERSION-win64\k6.exe .\packaging\
rmdir .\packaging\k6-$env:VERSION-win64\
- name: Add signtool to PATH
run: echo "C:\Program Files (x86)\Windows Kits\10\bin\10.0.17763.0\x86" | Out-File -FilePath $env:GITHUB_PATH -Encoding utf8 -Append
Copy link
Contributor Author

@imiric imiric Nov 30, 2020

Choose a reason for hiding this comment

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

This static path is concerning if it ever changes, but I haven't found a way to determine it dynamically. :-/ I guess a find would work, but not sure if we should bother...

This GitHub Action also hardcodes it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I managed to get a more robust ${env:ProgramFiles(x86)}\Windows Kits\10\bin\x64 working (4353e64), and also use the x64 executable @na--.

mstoykov
mstoykov previously approved these changes Nov 30, 2020
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

Interesting that signtool is an x86 program, and we're using it to sign an x64 executable... 😕 🤷‍♂️

In any case, I have two requests... Can you please delete k6.pfx at the end of the task, just in case our VM is not properly cleaned up between builds? And also rename the CERTIFICATE secret to something more descriptive, WIN_SIGN_CERTIFICATE or something like that? It's not unlikely that we'll want to add other certificates in the CI process in the future.

@imiric
Copy link
Contributor Author

imiric commented Nov 30, 2020

@na-- I changed the secret to WIN_SIGN_CERT and added removal of the k6.pfx.

I haven't tested a full build yet, but I can do that tomorrow, or we can merge this as is since I'll test a full deployment before the next release anyway.

@imiric imiric requested review from na-- and mstoykov November 30, 2020 17:43
@codecov-io
Copy link

codecov-io commented Nov 30, 2020

Codecov Report

Merging #1746 (0ed867c) into master (40979fd) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1746   +/-   ##
=======================================
  Coverage   71.40%   71.40%           
=======================================
  Files         178      178           
  Lines       13751    13751           
=======================================
  Hits         9819     9819           
  Misses       3320     3320           
  Partials      612      612           
Flag Coverage Δ
ubuntu 71.38% <ø> (+0.02%) ⬆️
windows 69.94% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
core/engine.go 90.95% <0.00%> (-2.02%) ⬇️
js/runner.go 80.45% <0.00%> (-0.66%) ⬇️
stats/cloud/collector.go 80.44% <0.00%> (+0.63%) ⬆️
lib/executor/vu_handle.go 95.23% <0.00%> (+1.90%) ⬆️
lib/testutils/minirunner/minirunner.go 86.04% <0.00%> (+4.65%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 40979fd...0ed867c. Read the comment docs.

Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

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

LGTM

@na--
Copy link
Member

na-- commented Dec 1, 2020

Hmm @imiric I think that we should also upload the signed .msi package to the github releases as well... As a separate PR of course, but if we have the .msi in there as well, it will probably make it easier to implement the /latest link in k6.io that would redirect to it

@imiric
Copy link
Contributor Author

imiric commented Dec 1, 2020

@na-- Sure, makes sense. Can you create a new issue for that, since merging this will close #1034? Or we can keep that open and mention the upload. I'll likely do that while testing the full deployment, and fix the checksum issue as well...

@na--
Copy link
Member

na-- commented Dec 1, 2020

I'll create a new one, merge this and close the new one :shipit: 😅

@imiric imiric merged commit 7d3809a into master Dec 1, 2020
@imiric imiric deleted the feat/1034-windows-bin-sign branch December 1, 2020 09:15
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Sign release binaries and installers for Windows
4 participants