Skip to content

Commit d6d7283

Browse files
authored
build: pin versions of Nimble packages (#466)
This commit pins each of our Nimble package dependencies in the `.nimble` file to the ref of its latest tag at the time of this commit. Before this commit, `nimble build` downloaded the latest tag of each package when that package was not installed, which is a violation of best practices for reliability and security in the CI context - a buggy or malicious update to one of our dependencies could affect a configlet release or cause CI to fail. However, there are good reasons that we didn't pin the refs like this earlier: - The `.nimble` file is not designed for pinning to exact versions [1], and doing so produces some awkward behaviors. - The recommended place to pin versions will be in a lockfile, but the version of Nimble that's distributed with Nim doesn't support lockfiles yet [2]. See also the discussion in The Cargo Book [3] about version pinning differences between libraries and binaries. - The production security issue can be mitigated by ensuring that configlet releases are only built with known-good versions of Nimble package dependencies [4]. - The reliability issue occurred only once in practice: a `jsony` release caused failing CI in configlet PRs recently [5]. Our Nimble package dependencies rarely had an upstream release; only `jsony` is currently in an active development phase, and that's a relatively recent addition to our dependencies. Transient network problems in CI (and outright outages in GitHub Actions or Azure), have caused more problems in the configlet repo by at least an order of magnitude. Furthermore, trying to pin exact dependency versions in a `.nimble` file has at least these awkward behaviors: - The package version used during `nimble build` can depend on the declaration order in the `.nimble` file. - A "wrong" ordering can cause `nimble build` to use a different version of a package than the "pinned" ref [6]. - Even if we order the configlet dependencies carefully, the latest tag for `isaac` is downloaded even if the "pinned" commit ref version is already installed, and so `nimble test` ends up using a different `isaac` version to the one that `nimble build` uses. Therefore if `isaac` gets a new release, the UUID tests would use a different `isaac` version to that used in the configlet release. - The "pinning" does not verify at build-time that the state of the package directory is that of the commit ref. That is, any change to the package directory since clone-time is not warned about or reverted at build-time. - Manually running `nim c src/configlet.nim` can use different package versions to those specified in the `.nimble` file (and so the resulting binary can behave differently to one that `nimble build` produces). - It won't work for packages with more complex dependency graphs. In general, these issues can cause hard-to-diagnose bugs. But in our case, because configlet's dependency graph is simple, at least the ordering problem is restricted to `isaac` - which hasn't been updated since 2017 anyway. However, we do want a consistent environment locally and in CI, and writing refs in the `.nimble` file is the simplest way to achieve that. The alternatives are more painful: adding an extra build script, or adding a git submodule (or subtree, or subrepo) for each dependency. So let's just write commit refs in the `.nimble` file for now - a stop-gap measure until we can use a Nimble version that supports lockfiles. It solves the biggest problem (buggy or malicious package updates can affect configlet releases or CI), and the complications seem manageable if they're truly limited to those stated above. Note also that this commit does not pin the Nim version in the `.nimble` file, so that we support a configlet developer who uses the `devel` Nim compiler locally (currently at version 1.7.1). However, the configlet release pipeline _does_ pin the Nim version (at 1.6.0). [1] https://github.com/nim-lang/nimble/blob/0a23c44cd8d6/readme.markdown#dependencies The Nimble documentation explicitly discourages it, saying: > Specifying a concrete version as a dependency is not a good idea because your package may end up depending on two different versions of the same package. If this happens, Nimble will refuse to install the package. (but see also [3] below). [2] The lockfile implementation was merged in the Nimble repo, and the Nim team hoped that Nim 1.6.0 would ship with a lockfile-supporting Nimble. However, although that happened for 1.6.0 rc1 (2021-09-07), it was deemed unsuitable for 1.6.0 (2021-10-19). [3] https://doc.rust-lang.org/cargo/faq.html#why-do-binaries-have-cargolock-in-version-control-but-not-libraries > Why do binaries have `Cargo.lock` in version control, but not libraries? > The purpose of a `Cargo.lock` lockfile is to describe the state of the world at the time of a successful build. Cargo uses the lockfile to provide deterministic builds on different times and different systems, by ensuring that the exact same dependencies and versions are used as when the `Cargo.lock` file was originally generated. > This property is most desirable from applications and packages which are at the very end of the dependency chain (binaries). As a result, it is recommended that all binaries check in their `Cargo.lock`. > For libraries the situation is somewhat different. A library is not only used by the library developers, but also any downstream consumers of the library. Users dependent on the library will not inspect the library’s `Cargo.lock` (even if it exists). This is precisely because a library should _not_ be deterministically recompiled for all users of the library. > If a library ends up being used transitively by several dependencies, it’s likely that just a single copy of the library is desired (based on semver compatibility). If Cargo used all of the dependencies' `Cargo.lock` files, then multiple copies of the library could be used, and perhaps even a version conflict. > In other words, libraries specify SemVer requirements for their dependencies but cannot see the full picture. Only end products like binaries have a full picture to decide what versions of dependencies should be used. [4] Someone who builds configlet locally was theoretically vulnerable to a buggy or malicious package update if that package was not already installed - but this is true of most packaging ecosystems. [5] For more details, see the upstream fix: treeform/jsony@a46972034d94 [6] To actually use the pinned `isaac` version, it turns out that we must specify `isaac-#45a5cbbd54ff59ba3ed94242620c818b9aad1b5b` _after_ `uuids`, which has `isaac` as a dependency. This is necessary so that when running $ nimble build --verbose the path to `isaac-#45a5cbbd54ff59ba3ed94242620c818b9aad1b5b` comes at the end, and therefore takes precedence: Executing /usr/bin/nim c \ --colors:on \ --noNimblePath \ -d:NimblePkgVersion=4.0.0 \ --path:'/home/foo/.nimble/pkgs/cligen-#b962cf8bc0be847cbc1b4f77952775de765e9689' \ --path:'/home/foo/.nimble/pkgs/jsony-#eb63a326b7f16537764c090f8859eb2451ad8d4d' \ --path:'/home/foo/.nimble/pkgs/parsetoml-#9cdeb3f65fd10302da157db8a8bac5c42f055249' \ --path:'/home/foo/.nimble/pkgs/uuids-#8cb8720b567c6bcb261bd1c0f7491bdb5209ad06' \ --path:/home/foo/.nimble/pkgs/isaac-0.1.3 \ --path:'/home/foo/.nimble/pkgs/isaac-#45a5cbbd54ff59ba3ed94242620c818b9aad1b5b' \ -o:/home/foo/configlet/configlet \ /home/foo/configlet/src/configlet.nim If `isaac` is instead pinned _before_ `uuids` in the `.nimble` file, the path to `isaac-0.1.3` takes precedence. That is, `nimble build` uses the latest `isaac` tag even if that does not correspond to the `isaac` ref in the `.nimble` file. This commit otherwise orders the dependencies alphabetically - `uuids` is our only dependency that depends on another Nimble package. Closes: #387
1 parent 34e864e commit d6d7283

1 file changed

Lines changed: 12 additions & 4 deletions

File tree

configlet.nimble

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -8,10 +8,18 @@ bin = @["configlet"]
88

99
# Dependencies
1010
requires "nim >= 1.6.0"
11-
requires "parsetoml"
12-
requires "cligen"
13-
requires "uuids >= 0.1.11"
14-
requires "jsony >= 1.0.4"
11+
requires "cligen#b962cf8bc0be847cbc1b4f77952775de765e9689" # 1.5.19 (2021-09-13)
12+
requires "jsony#eb63a326b7f16537764c090f8859eb2451ad8d4d" # 1.1.1 (2021-11-16)
13+
requires "parsetoml#9cdeb3f65fd10302da157db8a8bac5c42f055249" # 0.6.0 (2021-06-07)
14+
requires "uuids#8cb8720b567c6bcb261bd1c0f7491bdb5209ad06" # 0.1.11 (2021-01-15)
15+
# To make Nimble use the pinned `isaac` version, we must pin `isaac` after `uuids`
16+
# (which has `isaac` as a dependency).
17+
# Nimble still clones the latest `isaac` tag if there is no tag-versioned one
18+
# on-disk (e.g. at ~/.nimble/pkgs/isaac-0.1.3), and adds it to the path when
19+
# building, but (due to writing it later) the pinned version takes precedence.
20+
# Nimble will support lock files in the future, which should provide more robust
21+
# version pinning.
22+
requires "isaac#45a5cbbd54ff59ba3ed94242620c818b9aad1b5b" # 0.1.3 (2017-11-16)
1523

1624
task test, "Runs the test suite":
1725
exec "nim r ./tests/all_tests.nim"

0 commit comments

Comments
 (0)