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

fix(windows): re-enable signature check 🔥 #9695

Merged
merged 6 commits into from
Oct 9, 2023

Conversation

mcdurdin
Copy link
Member

@mcdurdin mcdurdin commented Oct 6, 2023

Fixes #9692.

Signature checking was skipped because we missed a ".virtual" to force nmake to build the test and test_i3633 targets. This opened up a small cascade of related formatting issues on Makefiles, and the fact that the test_i3633 (has there ever been a more poorly named project?) Makefile did not even work.

Refactored significantly, added same tests to Developer Makefile, and also now verifying the .msi and installer executable.

We can improve this further but I'd like to get this in to avoid further critical issues with code signing given the current broken signing configuration.

@keymanapp-test-bot skip

Fixes #9692.

Signature checking was skipped because we missed a ".virtual" to force
nmake to build the test and test_i3633 targets. This opened up a small
cascade of related formatting issues on Makefiles, and the fact that the
test_i3633 (has there ever been a more poorly named project?) Makefile
did not even work.

Refactored significantly, added same tests to Developer Makefile, and
also now verifying the .msi and installer executable.

We can improve this further but I'd like to get this in to avoid further
critical issues with code signing given the current broken signing
configuration.
@keymanapp-test-bot
Copy link

keymanapp-test-bot bot commented Oct 6, 2023

@mcdurdin mcdurdin marked this pull request as ready for review October 7, 2023 00:36
$(SIGCHECK) $(ROOT)\bin\inst\* >> sig1
$(VERIFY_SIGNATURES) < sig1

# prereq may not be needed?
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this comment? I think the prereq is needed to build the verify_signatures binary

Copy link
Member Author

Choose a reason for hiding this comment

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

The prereq may already have been built, but it's not a big deal because it builds quickly anyway.

Copy link
Contributor

@rc-swag rc-swag left a comment

Choose a reason for hiding this comment

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

LGTM - just the one comment about a comment in the makefiles.

@mcdurdin mcdurdin merged commit 6b4497b into master Oct 9, 2023
@mcdurdin mcdurdin deleted the fix/windows/9692-re-enable-signature-check branch October 9, 2023 05:09
@keyman-server
Copy link
Collaborator

Changes in this pull request will be available for download in Keyman version 17.0.187-alpha

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

Successfully merging this pull request may close these issues.

bug(developer,windows): 'Unknown Publisher' message displayed on the screen during the installation
3 participants