Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

chore: enable tunnel binary verification#36

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
deansheather merged 7 commits intomainfromdean/enable-bin-verification
Mar 7, 2025

Conversation

deansheather
Copy link
Member

@deansheatherdeansheather commentedMar 1, 2025
edited
Loading

  • Enables assembly version verification
  • Enables authenticode verification
  • Adds local machine registry config options to enable/disable either of these

TODO:

  • Installer should set these registry values to the default

Closes#41
Closes#45

@deansheatherdeansheather marked this pull request as ready for reviewMarch 6, 2025 04:45
}

public Task ValidateAsync(string path, CancellationToken ct = default)
{
var info = FileVersionInfo.GetVersionInfo(path);
if (string.IsNullOrEmpty(info.ProductVersion))
throw new Exception("File ProductVersion is empty or null, was the binary compiled correctly?");
if (info.ProductVersion != _expectedAssemblyVersion)
if (!Version.TryParse(info.ProductVersion, out var productVersion))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Instead of getting this as a string and then parsing it, you should be able to directly access these numbers asinfo.ProductMajorPart,info.ProductMinorPart,info.ProductBuildPart andinfo.ProductPrivatePart (tee-hee 🍆 )

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

These values seem to be set even if the version couldn't be fully parsed, e.g. invalid version1-2-3-4 gets parsed as(1, 0, 0, 0) in these values. I think it's better to just parse the string version, which will fail immediately if it's not the correct format

Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently, there are 4 different "versions" that can be stored in a file. There is a file version and a product version and each can be stored in binary as 4 16-bit fields, and as a string.

https://learn.microsoft.com/en-us/windows/win32/menurc/versioninfo-resource

What you are examining here is the product version string. Things likeinfo.ProductMajorPart examine the product version binary. I'm guessing that go-winres attempts to parse the version string, and if successful, also sets the binary values. Is this the tool we use for the real go builds? If so, then I agree it's appropriate to use the product version string and parse it like this.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yeah we use go-winres in the real builds

@"[INSTALLFOLDER]coder-desktop-service.log"));
@"[INSTALLFOLDER]coder-desktop-service.log"),
new RegValue(RegistryHive, RegistryKey, "Manager:TunnelBinarySignatureSigner", "Coder Technologies Inc."),
new RegValue(RegistryHive, RegistryKey, "Manager:TunnelBinaryAllowVersionMismatch", "false"));
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I'm understanding this correctly, theManager: prefix corresponds to the section name (ManagerConfigSection) we bind in Program.cs:

        builder.Services.AddOptions<ManagerConfig>()            .Bind(builder.Configuration.GetSection(ManagerConfigSection))

But that means we've got this string key coupling between these two sections of the code, which feels fragile if we were to change the name. Is there a way they could be based on the same constant?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

I don't think we can without creating a new library package or adding this value to a package where it doesn't really belong likeVpn.

These values should never be changed anyways without some consideration for backwards compatibility. I can add comments to both of these places saying that it's not to be changed.

@deansheatherdeansheather merged commit7fc6398 intomainMar 7, 2025
3 checks passed
@deansheatherdeansheather deleted the dean/enable-bin-verification branchMarch 7, 2025 07:14
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@spikecurtisspikecurtisspikecurtis approved these changes

@ethanndicksonethanndicksonAwaiting requested review from ethanndickson

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check the Authenticode certificate for extended validation for any downloaded coder binaries. Enable validation for the VPN binary
2 participants
@deansheather@spikecurtis

[8]ページ先頭

©2009-2025 Movatter.jp