RC npm: lockfiles or no lockfiles

[laurentsimon]

(Note: I will leave aside the npm-shrinkwrap.json since it's discussed separately in #10)

In this issue, I'd like to propose a change to the recommendation w.r.t lockfiles.
The OSSF (e.g., via the Scorecard's Pinned-Dependency checks) has been encouraging lockfiles, a.k.a. "hash dependency pinning".

The main advantages of lockfiles are the following:

• incidence response (Concerns about metrics from @ljharb scorecard#1826): engineers can quickly identify the list of hashes that made use of a malicious package
• avoids immediate (potentially malicious) package updates to prod

There has been feedback, e.g. by @ljharb in ossf/scorecard#1826 (comment) and #10 (comment) that lockfiles, in some scenarios, have disadvantages.

The main drawback highlighted by @ljharb is dev lock files. A dev lockfile is one that is used by the maintainer of a project, call it P. P relies on a set of dependencies: PPs. All tests may be passing for P, yet some bugs are reported by consumers because of a drift in PP versions.

As a results, consumers have a poorer experience, since they hit bugs (possibly in prod), need to file issues for P, etc. @ljharb describes it as it's [...] important to ensure that maintainers are notified as early as possible about issues in their dep graph..

Since the benefit of lockfiles is prod and their absence is motivated by catching bugs early, I'd like to suggest rephrasing the recommendation to (roughly):

We recommend using a lock file in privileged environments, such as:
- Production environments
- Other environments with access to sensitive data, such as secrets, PII, write/push permission to the repository, etc

The use of a lock file has the following benefits:
- In an incidence response, it helps identify vulnerable packages that have been used, using the sha512
- It reduces the chance of pulling in new-compromised packages directly into privileged environments.
- _others_from_current_draft_

In a non-privileged environment, developers need not use a lock file. 
This is useful when developers want to exercise a wide range of dependency versions in their tests, 
to anticipate breaking before consumers do. If you run CI via GitHub Actions, a non-privileged environment is one of the following:
- pull_request trigger
- Other triggers without secrets and permissions explicitly declared as read (the default permissions is write-all)

This is just a rough draft, but I'd like to hear your opinion whether it would address some of the concerns that were raised.

/cc @lirantal, @soldair

[jeffmendoza]

It might be good to add "developers' machines" to the first list. They can be a target in addition to the listed production/CD environments (think crypto-wallets, or any other private file on your machine).

I think this is a good reason to suggest that all repositories commit a lock file. You wouldn't want a project developer's machine being at risk with the initial clone and npm-install of a repository. You want the "at risk" time to be when you are intentionally updating dependencies.

I agree that a test without a lockfile for libraries is useful to replicate the downstream consumer's experience, as long as it is in a "sealed" environment, like those in the last list.

[ljharb]

It would already be at risk even with a lockfile, if the lockfile contains the malicious code. A lockfile’s purpose isn’t to prevent malicious packages, it’s to make installs deterministic/reproducible (which does help prevent malicious code, ofc).

I think it is a very unwise recommendation that all developer machines use lockfiles.

[lirantal]

Not entirely sure if me and Jordan are at odds in our opinions (which is fine :-)), but I'm making the following observations:

1. I agree with prod envs requiring a lockfile
2. It is my opinion that developers who collaborate on a project (Jordan's application definition) should also always use lockfiles in their workflows (local dev, CI, etc). I do want to lend that this isn't only for security reasons, but also related to development benefits (deterministic installs and such)
3. It seems like this paragraph diminishes the attack vector of devs and their environments (CI and such), where-as in reality, we're seeing these only rising. Codecov is a good example.

[laurentsimon]

2. It is my opinion that developers who collaborate on a project (Jordan's application definition) should also always use lockfiles in their workflows (local dev, CI, etc).

Thanks. So we could say that in general, a lockfile should be used to protect maintainers/collaborators as well.

To accommodate @ljharb's use case of "exercising a wide range of version dependencies" during tests, we can then say that "non-privileged" CI runs (pull_request on GH, or when permissions are only read and not secrets are available), developers may "ignore" the lock file, e.g.:

checkout-code
rm package-lock.json
npm install ...

Wdut?

3. It seems like this paragraph diminishes the attack vector of devs and their environments (CI and such), where-as in reality, we're seeing these only rising. Codecov is a good example.

which paragraph do you mean?
We're not saying it's not important, just that if you're confident the env is not privileged, you may ignore the lockfile. We should warn of the dangers (not always obvious what resources an environment has access to in general - do-able on GitHub)

Wdut?

[lirantal]

I agree with the leeway described where the lockfile is ignored in a non-privileged environment. This however still leaves users vulnerable to malicious packages (such as node-ipc, colors, event-stream), even if it's just their dev/local environment.

[laurentsimon]

not if you consider the dev environment as privileged and require the lockfile (I'm assuming their fork the repo). Unless the dependency is updated via a PR, it won't immediately be pulled in by developers, right?

If you're thinking of catching malware, I agree it won't do that. But I think that's a different problem.

Wdut?

[erezrokah]

Hello 👋 Again another interesting discussion as #10

It would already be at risk even with a lockfile, if the lockfile contains the malicious code. A lockfile’s purpose isn’t to prevent malicious packages, it’s to make installs deterministic/reproducible (which does help prevent malicious code, ofc).

I think it is a very unwise recommendation that all developer machines use lockfiles.

If a dependency is compromised and you don't have a lock file you instantly and implicitly get the malicious package by running npm i.

If a dependency is compromised for it to reach the lock file (assuming it's committed to source control) the following needs to happen:

1. You need to update the dependency and submit a PR for it (or push directly to main)
2. It often needs to pass CI tests
3. Other developers have to sync with main.

Also once the lock file is committed to source control it can be verified by automated tools like GitHub security alerts, etc.

I'd say the second flow reduces the risk of a malicious package reaching developers machines, especially if before updating dependencies you verify a number of "stable" days, see https://docs.renovatebot.com/configuration-options/#stabilitydays.

even if it's just their dev/local environment.

@lirantal I'd say dev/local environment are privileged and should use a lock file.

We're not saying it's not important, just that if you're confident the env is not privileged, you may ignore the lockfile. We should warn of the dangers (not always obvious what resources an environment has access to in general - do-able on GitHub)

I think that makes sense and you can see a use case for it in the AVA repo of installing without a lock file and running tests https://github.com/avajs/ava/blob/91f5254d7bfb6c658d0e89f48a852b92b70e31da/.github/workflows/ci.yml#L85.
The use case is detecting if an update to a dependency breaks AVA (relates to #10 as AVA doesn't use a shrink-wrap file).
I actually had it failing on a PR I did avajs/ava#2968 (comment)

[jeffmendoza]

Closing this as resolved with #25. If anyone wants to continue the discussion, we'll re-open and continue.