Skip to content

Add binary signature verification #558

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

code-asher
Copy link
Member

@code-asher code-asher commented Jul 23, 2025

I extracted the download function (since I needed to reuse it to download signatures) to a separate commit so it is easier to review the signature additions separately, if that is of interest.

This downloads the detached signature from Coder if available or releases.coder.com if not, then verifies the binary using that detached signature and the bundled public key. The check is performed only when the binary is first download.

  • Add option to disable
  • Skip headers when requesting releases.coder.com
  • Tests (for signing, no tests for the UI changes, ideally we get something set up to do that at some point, although we could mock the VS Code APIs in the meantime...)

@code-asher code-asher force-pushed the asher/binary-verification branch 2 times, most recently from 753e955 to 3e18934 Compare July 23, 2025 20:45
The main thing here is to pass in an Axios client instead of the SDK
client since this does not need to make API calls and we will need to
pass a separate client without headers when downloading external
signatures.

Otherwise the structure remains the same.  Some variables are renamed
due to being in a new context and some strings messages are simplified.
A tiny refactor since I will need to get a third config option.
@code-asher code-asher force-pushed the asher/binary-verification branch 2 times, most recently from 33b14b9 to 18c3c88 Compare July 23, 2025 21:59
@code-asher code-asher force-pushed the asher/binary-verification branch from 18c3c88 to 860b1aa Compare July 23, 2025 22:11
@code-asher code-asher requested a review from aslilac July 23, 2025 22:15
They are not needed, and the packaging step will error that it looks
like you are trying to package secrets due to the test key fixtures.
Copy link
Member

@aslilac aslilac 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 gonna need you to sign a waiver indicating that you know I'm not a cryptography expert and will not be held responsible for any glaring security issues before you merge this 😝 but it looks good!

aGN86JBOmwpU87sfFxLI7HdI02DkvlxYYK3vYlA6zEyWaeLZ3VNr6tHcQmOnFe8Q
26gcS0AQcxQZrcWTCZ8DJYF+RnXjSVRmHV/3YDts4JyMKcD6QX8s/3aaldk=
=dLmT
-----END PGP PUBLIC KEY BLOCK-----
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-----END PGP PUBLIC KEY BLOCK-----
-----END PGP PUBLIC KEY BLOCK-----

all of the test files have trailing new lines but this one does not

case VerificationErrorCode.Invalid:
return "Signature does not match";
default:
return "Failed to read signature";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return "Failed to read signature";
return "Failed to verify signature";

since this is the default, it might not necessarily be about reading I guess?

// Download the binary to a temporary file.
await fs.mkdir(path.dirname(binPath), { recursive: true });
const tempFile =
binPath + ".temp-" + Math.random().toString(36).substring(8);
Copy link
Member

Choose a reason for hiding this comment

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

this is such a silly way to get a couple random characters lol. I love it.

): Promise<number> {
const baseUrl = client.defaults.baseURL;

const controller = new AbortController();
Copy link
Member

Choose a reason for hiding this comment

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

I've known about this api for years and literally never seen someone use it in production. 🥹 so proud!!

Comment on lines +84 to +91
return new VerificationError(VerificationErrorCode.Invalid, error);
}
} catch (e) {
const error = `Failed to read signature or binary: ${errToStr(e)}.`;
logger?.warn(error);
return new VerificationError(VerificationErrorCode.Read, error);
}
return true;
Copy link
Member

Choose a reason for hiding this comment

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

I feel weird about returning errors in js. if these were thrown instead we could do something like...

try {
  await verifySignature(...);
  // more happy path here
} catch (error) {
  if (error instanceof VerificationError) {
    // unhappy path here
  }
}

which is maybe a bit noisier than what you're currently doing, but is also a bit more "when in rome", y'know?

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.

2 participants