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

Add GnuPG (gpg) verification of checksum file #736

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

madarche
Copy link
Contributor

This is an acknowledged partial implementation: to finalize it I'm
waiting for #664 to be merged. All comments are welcome still.

It's partial because it's only done for Node.js recent archives. But it
may still be useful. At least it works for me :-)

This is an acknowledged partial implementation: to finalize it I'm
waiting for nvm-sh#664 to be merged. All comments are welcome still.

It's partial because it's only done for Node.js recent archives. But it
may still be useful. At least it works for me :-)
nvm_checksum() {
local NVM_CHECKSUM
if nvm_has "sha1sum" && ! nvm_is_alias "sha1sum"; then
Copy link
Member

Choose a reason for hiding this comment

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

nvm must always support sha-1 checking for older versions of node.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ljharb How useful is checksum checking without checking the signatures? (ref)

Copy link
Member

Choose a reason for hiding this comment

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

It ensures no transfer corruption, which is exponentially more likely than someone attempting to middleman the download.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Nobody's disputing that it's possible. We're talking about likely. Which it's not.

@ljharb
Copy link
Member

ljharb commented Apr 19, 2015

This seems like it removes proper sha checking entirely for many uses, and makes gpg required to replicate that feature.

I don't want to add a dependency, especially another implicit dependency, to an external tool - POSIX only, please.

@ljharb ljharb added installing node: checksums This relates to checksum checking of downloaded node archives. feature requests I want a new feature in nvm! needs followup We need some info or action from whoever filed this issue/PR. labels Apr 19, 2015
@madarche
Copy link
Contributor Author

As written it is partial because it is in wait of #664. I didn't want to do a full featured PR that would have to be rewritten again once #664 is merged (since it brings changes on the checksum side).

I know alright that nvm must always support sha-1 checking for older versions of node, and that the final implementation needs to support the sha checking without any regression in general. And GnuPG support will only be an additional feature for users who have it installed.

So no worry :-) see this PR as just a first step, hopefully I'll work on it again soon :-) In the meanwhile it is working for limited use cases and might help some people.

@jbergstroem
Copy link
Contributor

Perhaps check for gpg or gpg2 before making any assumptions about which to use?

ljharb added a commit that referenced this pull request Jan 24, 2016
[New] Added support for sha256 checksums on `io.js` / merged `node`.

Fixes #664, relates to #736 and #687.
@ypid
Copy link
Contributor

ypid commented Feb 11, 2017

@madarche Nice work! Any progress with it? Would be nice to ship this by default 😉

ypid added a commit to ypid/asdf-nodejs that referenced this pull request Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/) for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Also note that this PR also updates the base download URL from "http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that before this PR (or asdf-vm#16 which is not merged), binaries where downloaded over plain legacy HTTP! (those binaries where later executed by the user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this pull request Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/) for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Also note that this PR also updates the base download URL from "http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that before this PR (or asdf-vm#16 which is not merged), binaries where downloaded over plain legacy HTTP! (those binaries where later executed by the user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this pull request Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Also note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this pull request Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
@ypid
Copy link
Contributor

ypid commented Feb 12, 2017

For reference, I implemented this feature for asdf, refer to Check signatures/checksums to ensure authenticity. You are free/encouraged to use this example code (under the terms of the MIT License) also in this project.

Checking signatures is one of the basic steps a package/version manager should do (ref). Would be nice to see further work on this PR. Keep up the good work!

ypid added a commit to ypid/asdf-nodejs that referenced this pull request Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
ypid added a commit to ypid/asdf-nodejs that referenced this pull request Feb 12, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
@madarche
Copy link
Contributor Author

madarche commented Feb 13, 2017

Hello all. I still intend to work again on this PR following all @ljharb's advices. The problem is just that I've too much work. I need to find some time.

And FYI I personally use my nvm fork everyday to be sure of the downloaded Node.js binaries. So this fork is still functional for those who can't wait to have this feature in the official nvm.

@ljharb
Copy link
Member

ljharb commented Feb 13, 2017

@madarche using it every day, have you even once had any of the checksum verifications fail? I'd love to hear about that.

@madarche
Copy link
Contributor Author

@ljharb I've never ever had the checksum verification failed. But that way I feel safe and I can safely use Node.js + nvm on important systems. We also take good care on NPM packages checking+upgrading. NPM packages are not signed for now, but it's not a reason to not check Node.js signature if it's available. A chain is only as strong as its weakest link.

Cheers

@madarche
Copy link
Contributor Author

@ypid I'll read your code. Thanks. And I've noted that you've mentioned that your code is MIT-licensed and thus can safely be reused in nvm as is.

ETA for me back on this PR: 2 weeks

ypid added a commit to ypid/asdf-nodejs that referenced this pull request Feb 20, 2017
Please refer to [Verifying Node.js Binaries](https://blog.continuation.io/verifying-node-js-binaries/)
for why this is important.

Related to: asdf-vm/asdf#158
Mitigates: nodejs/node#9859
Mitigates: nodejs/node#6821

Implementing this feature required some rework of the `install` script
which is included in this PR. The following other PR are
superseded/included in this one:

Closes: asdf-vm#15
Closes: asdf-vm#16
Closes: asdf-vm#19

Note that this PR also updates the base download URL from
"http://nodejs.org/dist" to "https://nodejs.org/dist" meaning that
before this PR (or asdf-vm#16 which is not merged), binaries where downloaded
over plain legacy HTTP! (those binaries where later executed by the
user). This is really bad and is fairly easy to exploit!

Related to: nvm-sh/nvm#736
Related to: nvm-sh/nvm#793
@fishcharlie
Copy link

@madarche You said the ETA for you bak on this PR was 2 weeks in February. Would love to see Node.js binary verification in NVM along with details on how to setup in the Readme.

Not too experienced with this repo (other than a user standpoint) but would be happy to try to help in anyway possible.

@ljharb
Copy link
Member

ljharb commented Sep 29, 2021

@madarche any chance you're interested in completing this PR?

@ljharb ljharb marked this pull request as draft September 29, 2021 06:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature requests I want a new feature in nvm! installing node: checksums This relates to checksum checking of downloaded node archives. needs followup We need some info or action from whoever filed this issue/PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants