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

[branch/v16] Invoke signing function for Windows Connect binary #44419

Merged
merged 3 commits into from
Jul 22, 2024

Conversation

fheinecke
Copy link
Contributor

@fheinecke fheinecke commented Jul 18, 2024

Backport #41963

changelog: Fixed Teleport Connect binaries not being signed correctly.

@@ -473,7 +490,7 @@ function Write-Version-Objects {
& $GoWinres simply --no-suffix --arch amd64 `
--file-description "Teleport tsh command-line client" `
--original-filename tsh.exe `
--copyright "Copyright (C) $Year Gravitational Inc." `
--copyright "Copyright (C) $Year Gravitational, Inc." `
Copy link
Collaborator

Choose a reason for hiding this comment

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

Best to run a tag build and then right click and go to properties on tsh.exe in a Windows environment. This version metadata is very picky and I seem to recall something about the comma breaking things.

--icon "$TeleportSourceDirectory\e\windowsauth\installer\teleport.ico" `
--product-name Teleport `
--product-version $TeleportVersion `
--file-version $TeleportVersion `
--out "$TeleportSourceDirectory\tool\tctl\resource.syso"

# generate windowsauth version info (note the --admin flag, as the installer must run as admin)
& $GoWinres simply --no-suffix --arch amd64 --admin `
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why was this removed?! This is necessary in order for the installer to properly request admin permissions.

Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

I'm not convinced this doesn't break things, can we verify the version metadata is correct before merging this?

@fheinecke
Copy link
Contributor Author

@zmb3 I screwed up my local backport commands and this should now be fixed, with a significantly smaller set of changes.

@fheinecke fheinecke requested a review from zmb3 July 19, 2024 17:19
@zmb3
Copy link
Collaborator

zmb3 commented Jul 19, 2024

Looks better but I'm now confused how this fixes anything. All I can see is that SignedBinaryPath is no longer mandatory. Is that correct?

@fheinecke
Copy link
Contributor Author

Yep that's pretty much it. Somehow when I did several other backports I managed to include only part of the original PR and it's breaking things due to this attribute.

@fheinecke
Copy link
Contributor Author

Having mandatory set makes the field mandatory (as the name implies), and we're calling this commandlet in some places without this parameter now.

@fheinecke fheinecke added this pull request to the merge queue Jul 22, 2024
@public-teleport-github-review-bot public-teleport-github-review-bot bot removed the request for review from ravicious July 22, 2024 08:12
Merged via the queue into branch/v16 with commit a8ea70b Jul 22, 2024
36 checks passed
@fheinecke fheinecke deleted the fred/win-signinig-fix-2-branch/v16 branch July 22, 2024 08:22
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.

3 participants