-
Notifications
You must be signed in to change notification settings - Fork 9
feat: publish to registry #45
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
Conversation
🦋 Changeset detectedLatest commit: 3fbc786 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
Things are nicely separated minimising risk.
The challenge with the publish-mcp
workflow is that although it does segregate permissions appropriately, an attacker would be able to access an oidc token which is essentially a (temporary) key to the vault, so ensuring the integrity of that binary is critical.
I've commented a way i think we can do that.
.github/workflows/publish-mcp.yml
Outdated
working-directory: packages/mcp-stdio | ||
run: | | ||
# Download MCP Publisher pinned to v1.2.3 | ||
curl -L "https://github.com/modelcontextprotocol/registry/releases/download/v1.2.3/mcp-publisher_1.2.3_$(uname -s | tr '[:upper:]' '[:lower:]')_$(uname -m | sed 's/x86_64/amd64/;s/aarch64/arm64/').tar.gz" | tar xz mcp-publisher |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these binaries checked into the repo? A release can be overwritten while a commit hash cannot, so a hash would be safer.
Github releases now have automatic sha256 checksums, so if we can pin down the exact build we want to use, we can get the checksum from here and include the actual checksum in this workflow, and confirm that it matches the binary before we execute anything.
That would give about as much security as we can reasonably have (a long as we are happy with the repo at a specific commit/ release) as sha256 collision attacks are essentially impossible.
on Linux:
sha256sum ./path/to/binary
on mac:
shasum -a 256 ./path/to/binary
The value provided by the above command needs to match the binary we copy paste from github.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will add the Sha check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re-reviewed.
Additionally, there is a risk of a mitm attack here. We should restrict the protocol and tls version:
--proto '=https' # restrict to https
--proto-redir '=https' # restrict redirects to https
--tlsv1.2. # restrict to modern tls version
We should also fail on non 2xx:
-fL # instead of just -L
Additionally we should download without streaming to tar until we have verified the checksum.
After we have verified the checksum we should extract only the expected file:
tar -xzf mcp-publisher-binary.tar.gz -C tmp
We should also reset permissions on the tar:
--no-same-owner
--no-same-permissions
Then we can add the permissions back when we install
install -m 0755 tmp/mcp-publisher
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh that sounds complex...will try later and ask you for a new review 🧡
Longterm, we should encourage the maintainers to generate github attestations for releases which would offer a little more security. But this is a close second. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can merge this in and tweak it later. The risk is pretty low right now, despite checking a variety of checksums against a single binary. We can harden that up in another pass.
Partially address #42
Something to figure out:
mcp-publisher
...they suggest fetching the latest, but that feels like an absurd safety hazard. Should we find a way to automate the update of that?Similarly, we should technically find a way to keep the release version in the package.json in sync with the version inserver.json
...can changesets do that? Another option would be to write a custom step in therelease.yml
workflow that pushes the change in the open PR, but I currently have no idea how to do it.