Https imports #36328
Https imports #36328
Conversation
bmeck
added
wip
|
@nodejs/modules-active-members I couldn't find a sane way to refactor the |
|
Interesting, would there be a way to statically know what an app uses? Right now Sqreen parses the content of the diverse |
|
@vdeturckheim due to |
|
@bmeck true, I often kind of think that |
This may a new motivation to revive #35524. I think that should fix the issue (but it's potentially disruptive). |
|
I think this is really cool and it would be cool if this loader (optionally?) cached resources (according to HTTP headers, like browsers do). I think this is what you mean by "Offline cache" :] |
|
@jkrems an alternative is to have a resource cache in our default loader rather than doing everything at time of request, we can drop the resource once it ref counts down to 0 or some other metric. |
|
@mcollina I'm not entirely opposed to flagging this (such as requiring it to be in a policy file listing), but if this leads to users consistently enabling them by default it would be good to understand why HTTPS (not HTTP) is insecure from your perspective so we can mitigate any issues. |
|
@mcollina... as this is still a draft and being actively discussed, I'm not sure the "-1"/Request Changes is helpful yet. @bmeck ... the security concerns on this really come down to the general risk of running untrusted code downloaded over arbitrary internet connections. For instance, I could fairly easily write a script that returns one bit of javascript during development but replaces that with a malicious script when accessed from a production server, without the developer ever knowing that there's been a change. Putting this behind a flag for now while we figure out the threat model and various mitigations definitely makes sense and would be right thing to do. |
|
@jasnell I've stated a security model document, though due to the lack of safety in using core's HTTPS a lot will inevitably be labeled as out of scope of this feature and must be secured by other means like policies. I would note that event-stream did the same kind of replacement workflow on files locally and do not consider this attack vector novel by adding HTTPS. Node's mitigation for that attack surface is to use a policy and the same would be true here. |
|
It is insecure because there is no signature of the original file. Anybody that can take control of a domain name can take control of a server. As a community we had some significant issues regarding security, from left-pad to event-stream. Adding this functionality would make us 10x more vulnerable to these kind of a attack because there would be no central entity that could intervene. |
|
@mcollina can you clarify how policies aren't that intervention/mitigation? |
The plan at the top highlight the fact that this will be enabled by default and it needs consensus to make users disable this. I do not agree with that approach, hence my comment. I'm happy to change my mind if it can be made safe to do so - however I do not think it is possible at all to make this safe. |
|
@bmeck .. for discussion.. from your checklist:
Initially I would mark this explicitly opt-in. Assuming we flip that default later, there should definitely be a way to explicitly opt-out on the process level.
Definitely a much larger discussion and one that we need to have with the package manager folks at the table. Not only would we need to figure out the caching semantics, we need to figure out the semantics around what happens if a require('https...') script does a require('some-local-script'). What if those things end up being different versions? I think what we need to do is take a moment to draw up a list of What Ifs and see if we can easily answer those.
I'm going to sound like a broken record but as I've been saying for years now we 100% need this. Adding https imports would only make that need more pronounced.
Blocklist / Allow list of origins at the very least. Cookies are a much more difficult matter. My knee jerk reaction is that we should not support cookies.
This I'm less convinced about and need to think through more. My knee jerk reaction is that not supporting redirects in some way is very anti-web and therefore a bad thing but that's the Standards Wonk side of me speaking. Will stew on this one.
I think I may be able to take this over for you but it would realistically be a 2021 project.
I think we can safely just say no to http modules.
This mechanism should allow for locally aliasing of https modules anyway, making it so that even if require('https...') is used, if there is a local alias established for it, the local one is used. That should address at least the immediate issue. However, the point here still stands: we'll need to provide observability into what is happening during load. We do have keylog support already built in to core. We'll want to make sure we have command line options to allow keylogging for Node.js' own module loader connections. |
|
@mcollina we need to understand why you consider it unsafe given current features and mitigations for these workflows that already exist. I don't think a blanket statement that it is unsafe and implication that it cannot be safe is helpful. |
|
@bmeck ... I'm going to take a bit more time to dig through your write ups and code and think it through before responding much more as I don't want to just churn the conversation here. Hopefully others will do the same. I'll definitely take on the MIME module work tho if that would make things easier for you. |
|
The attack vector is the same of left-pad and event-stream. There are plenty of write ups around on what happened. The mitigations that npm put in place are not feasible without a central/federated authority or strong cryptography. I don't have time right now for a long write-up, I'll try to get back as soon as I can. What would make this safe is giving authors a way to cryptographically sign the published files, and let developers validate those files against said keys that are fetched off-band. Overall I would recommend developing this as a module/loader and then propose its addition to Node.js. |
I don't understand this. I am vehemently against code signing on the developer if it is done improperly and have met over the years with a few certificate authorities about the complexity of doing this. This reduces down to the same workflow as TLS where you pull the keys off the trusted CA which is out of band by nature then verify. What advantage do we get here? Even if they upload a personal key such as using PGP the revocation mechanism and hijacking or repurposing under a different key are all possible.
What is "this"? I'd note that a variety of the mitigations are not possible in user land loaders (such as policy integration) and loader have repeatedly been stalled out by Node's process so I doubt they will move forward any time soon. I spent a lot of time on loaders and trying to move them forward and don't think they are worth the push back from the consensus model we use in Node core. |
|
Hey all, so I brought up some points recently on Twitter related to this proposal, so I thought I'd re-articulate them here. Keep in mind I've never contributed to Node.js, this is just my opinion as a long-time user and a contributor to Deno. TL;DR:Importing from npm isn't inherently more secure than importing from a raw URL, so let's work to make the whole ecosystem more secure, regardless of how packages are imported. URL Imports Aren't Inherently Less SecureFirst I want to address the biggest concern that always comes up when talking about URL imports: "It's insecure." The argument is usually summed up similarly to what @jasnell said earlier in this thread: "I could fairly easily write a script that returns one bit of javascript during development but replaces that with a malicious script when accessed from a production server, [...]" It's true, but it implies that the current way of importing is somehow more secure, and that it is more secure by virtue of not explicitely using a URL. However, packages hosted on npm are vulnerable to this problem as well. Not even a month ago, there was another report of malicious code being distributed in an npm package. We've become accustomed to assume that central registries are somehow more secure, but it's simply not the case. Packages are still fetched from a remote source and malicious code can still make its way into npm. Once we accept that reality, URL imports become much less scary (or central registries become much more scary, depending on how you look at it). The concerns raised here are all very valid; We just have to remember that they are issues that also affect the current import model, and so I don't it's fair to block this proposal on the grounds that it's "less secure". I would even argue that URL imports have the potential to be more secure than traditional imports through npm, because it leverages two important aspects of the web: Authority and Visibility. It gives end-users the possibility to verify that they are imported from a trusted domain name and that that domain name has a valid SSL certificates that verifies that it is indeed that domain. It also allows to verify the whois information associated with that domain, potentially being much more explicit about when a domain or a package changes ownership. (As an aside: wouldn't you rather import express directly from https://expressjs.org than https://npmjs.org ?) Those are all things that end users can implement themselves and include in their CI/CD pipelines and compliance tools. They've been the backbone of the internet for a long time. Browsers trust them. Users trust them. Why couldn't we? Feedback On The Concerns AboveThe other part of @jasnell comment - "[...] without the developer ever knowing that there's been a change." is very important; I think if URL imports are to go forward, they should be restricted to tagged versions, no "latest" allowed. deno.land/x uses the Local caching and interop with Similarly, I agree with @jasnell with regards to the cookies support; I don't think it makes much sense here. I'm less certain about redirects, they can be helpful for resolving versions when using tags like I don't see a reason to not include an allow/block function for specific origins, though I think we should also add a recommendation in the docs for operators to include those in firewall rules (hey if we're gonna take advantage of web mechanisms, let's go all the way!) Now for a big one: integrity checks. Honestly, I don't know how much we can do there. Ultimately we can include checksum verification to make sure that the content the user receives is the same one the server said it was sending and that it wasn't tempered with. Beyond that, I don't there's much else that can be done. As I said earlier, it's already possible for legitimate users to upload malicious code to a legitimate platform. The best anyone can do here is certify that the content wasn't messed with by a man-in-the-middle. |
They can be but the way the web is specified leads to issues if you do. Let us imagine:
On the web |
|
Per integrity checks that many people keep bringing up please read https://nodejs.org/dist/latest/docs/api/policy.html as they do already cover most of these concerns. However, like @wperron is bringing up, they assert integrity not if the resource is hostile or not. |
|
how about gopher imports? I think https imports are not great for security. I also think npm is not great for security. I'm fine with node supporting both. If one wants to argue that we should only be adding "secure" module systems moving forward, it would probably be a better use of time to discuss some sort of decentralised immutable protocol or something. |
|
Restricting https imports to some top-level One ecosystem concern I have here is npm packages starting to ship imports to URLs in them. This risks an npm ecosystem that becomes dynamically attackable from these https vectors. It would be nice to explicitly disable support for that somehow in the resolution mechanism. Eg turning this feature off in the module graph once file-based package resolutions have been hit. |
Can you clarify this idea? |
GeoffreyBooth
added
commit-queue-squash
nodejs-github-bot
added
commit-queue-failed
Commit Queue failed- Loading data for nodejs/node/pull/36328
✔ Done loading data for nodejs/node/pull/36328
----------------------------------- PR info ------------------------------------
Title Https imports (#36328)
Author Bradley Farias (@bmeck)
Branch bmeck:https-import -> nodejs:master
Labels semver-minor, esm, author ready, commit-queue-squash
Commits 12
- esm: support https remotely and http locally under flag
- fixup: rename `esm/fetch` → `esm/fetchModule`
- fixup: explain purpose of new http(s) agent instead of the global agent
- fixup: correct file name to conform to snake_case
- fixup: add missing modules to bootstrap test
- fixup: use require('buffer')
- fixup: new linting rules
- fixup: make test check for interfaces rather than assume ipv6 is avai…
- fixup: lint
- fixup: dtrace bootstrap check should only happen if dtrace exists
- fixup: ci
- fixup: ci
Committers 2
- Geoffrey Booth
- Bradley Farias
PR-URL: https://github.com/nodejs/node/pull/36328
Reviewed-By: Matteo Collina
Reviewed-By: Geoffrey Booth
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/36328
Reviewed-By: Matteo Collina
Reviewed-By: Geoffrey Booth
--------------------------------------------------------------------------------
ℹ This PR was created on Mon, 30 Nov 2020 16:47:19 GMT
✔ Approvals: 2
✔ - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/36328#pullrequestreview-867134230
✔ - Geoffrey Booth (@GeoffreyBooth): https://github.com/nodejs/node/pull/36328#pullrequestreview-878270674
✔ Last GitHub CI successful
ℹ Last Full PR CI on 2022-02-10T02:50:30Z: https://ci.nodejs.org/job/node-test-pull-request/42467/
- Querying data for job/node-test-pull-request/42467/
✔ Last Jenkins CI successful
--------------------------------------------------------------------------------
✔ No git cherry-pick in progress
✔ No git am in progress
✔ No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
* branch master -> FETCH_HEAD
✔ origin/master is now up-to-date
- Downloading patch for 36328
From https://github.com/nodejs/node
* branch refs/pull/36328/merge -> FETCH_HEAD
✔ Fetched commits as 52b1904a0dd4..cdbba252057d
--------------------------------------------------------------------------------
Auto-merging doc/api/errors.md
Auto-merging doc/api/esm.md
Auto-merging lib/internal/errors.js
Auto-merging lib/repl.js
[master c3f50181bb] esm: support https remotely and http locally under flag
Author: Bradley Farias
Date: Mon Nov 30 10:03:30 2020 -0600
31 files changed, 935 insertions(+), 132 deletions(-)
create mode 100644 lib/internal/modules/esm/fetch.js
create mode 100644 lib/internal/modules/esm/formats.js
create mode 100644 test/es-module/test-http-imports.mjs
[master 01543495ff] fixup: rename `esm/fetch` → `esm/fetchModule`
Author: Jacob Smith <[email protected]>
Date: Thu Feb 3 21:57:41 2022 +0100
5 files changed, 11 insertions(+), 11 deletions(-)
rename lib/internal/modules/esm/{fetch.js => fetchModule.js} (99%)
[master a79f3a4b13] fixup: explain purpose of new http(s) agent instead of the global agent
Author: Jacob Smith <[email protected]>
Date: Fri Feb 4 17:36:39 2022 +0100
1 file changed, 11 insertions(+), 13 deletions(-)
[master 7b180db04f] fixup: correct file name to conform to snake_case
Author: Jacob Smith <[email protected]>
Date: Fri Feb 4 17:56:16 2022 +0100
5 files changed, 4 insertions(+), 4 deletions(-)
rename lib/internal/modules/esm/{fetchModule.js => fetch_module.js} (100%)
[master 0453e4a154] fixup: add missing modules to bootstrap test
Author: Jacob Smith <[email protected]>
Date: Fri Feb 4 17:56:40 2022 +0100
1 file changed, 24 insertions(+), 15 deletions(-)
[master 8deca8052e] fixup: use require('buffer')
Author: Geoffrey Booth
Date: Sun Feb 6 18:27:04 2022 -0800
1 file changed, 1 insertion(+), 2 deletions(-)
[master 791a60ce3b] fixup: new linting rules
Author: Geoffrey Booth
Date: Sun Feb 6 18:50:57 2022 -0800
2 files changed, 9 insertions(+), 3 deletions(-)
[master b42683c098] fixup: make test check for interfaces rather than assume ipv6 is available
Author: Bradley Farias
Date: Tue Feb 8 09:14:27 2022 -0600
1 file changed, 30 insertions(+), 6 deletions(-)
[master d03922ba5d] fixup: lint
Author: Bradley Farias
Date: Tue Feb 8 09:27:39 2022 -0600
1 file changed, 4 insertions(+), 4 deletions(-)
[master 6692dadbef] fixup: dtrace bootstrap check should only happen if dtrace exists
Author: Bradley Farias
Date: Tue Feb 8 12:06:40 2022 -0600
1 file changed, 6 insertions(+), 1 deletion(-)
[master 1c671c49f0] fixup: ci
Author: Bradley Farias
Date: Wed Feb 9 09:26:16 2022 -0600
3 files changed, 8 insertions(+), 3 deletions(-)
[master 27383e3984] fixup: ci
Author: Bradley Farias
Date: Wed Feb 9 13:51:30 2022 -0600
1 file changed, 4 insertions(+), 2 deletions(-)
✔ Patches applied
There are 12 commits in the PR. Attempting to fixup everything into first commit.
[master 9cbf06a743] esm: support https remotely and http locally under flag
Author: Bradley Farias
Date: Mon Nov 30 10:03:30 2020 -0600
33 files changed, 1000 insertions(+), 149 deletions(-)
create mode 100644 lib/internal/modules/esm/fetch_module.js
create mode 100644 lib/internal/modules/esm/formats.js
create mode 100644 test/es-module/test-http-imports.mjs
--------------------------------- New Message ----------------------------------
esm: support https remotely and http locally under flag
https://github.com/nodejs/node/actions/runs/1821782609 |
Co-authored-by: Jacob Smith <[email protected]> Co-authored-by: James M Snell <[email protected]> Co-authored-by: Jordan Harband <[email protected]> Co-authored-by: James Sumners <[email protected]> PR-URL: nodejs#36328 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Geoffrey Booth <[email protected]>
|
Landed in ceadb47 |
|
Thanks @Trott ! Congrats @JakobJingleheimer ! |
|
I'm incredibly excited for this, thanks for adding it! Any word on what will be the first version of Node this will land in and in which I can expect the CLI flag to work? Thanks! |
|
Would there be a good way to check an https import resource with a checksum? |
|
@mrozbarry you can use policies for integrity checks https://nodejs.org/dist/latest-v17.x/docs/api/policy.html |
|
Unrelated to my previous comment, what if domains could be allowed/white-listed in a package.json? That could at least prevent imports from importing from some unknown domain. Maybe on first run, before cache, node could prompt |
|
@mrozbarry that is possible in a policy |
It would be great if node could automatically handle generating these integrity checksums for new https resources if they don't exist already. I'm going to assume that most js scripts in the wild don't have their own checksums. Also, is there a good standard in place for servers that don't want someone just importing their scripts? I'm assuming CORS won't really cover that, considering know doesn't necessarily have to have a concept of it's own server. |
|
@mrozbarry please open new issues, this is a merged PR |
|
I suggest locking as resolved. |
GeoffreyBooth
added
notable-change
| req.destroy(); | ||
| res.destroy(); | ||
| } | ||
| if (res.statusCode >= 300 && res.statusCode <= 303) { |
There was a problem hiding this comment.
We should probably think of adding support for HTTP status code 307 and 308 at some point.
EDIT: sorry for the noise, I didn't see the issue was locked.
This comment was marked as outdated.
This comment was marked as outdated.
danielleadams
added
backport-requested-v16.x
and removed
backport-requested-v16.x
labels
This allows for HTTPS (not HTTP) imports to work with the ESM module loader. This PR is only an initial PR for incremental progress behind a flag.
Supported concerns
--experimental-https-modulesflaghttp:node:builtins - banned anything excepthttp:andhttps:deps.Punted concerns
policiesmimelanded anytime soon due to pushbacksDocumentation
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes