[devbox add] --allow-insecure should handle multiple, user-specified packages

[savil]

Summary

Changes

1. Modify devbox add's --allow-insecure flag to take a []string instead of bool.
2. Save allow_insecure field in the package in the devbox.json config
3. Set NIXPKGS_ALLOW_INSECURE=1 in the nix build during devbox add
4. Set the packages specified in allow_insecure in the generated flake's permittedInsecurePackages.

TODO:

• Handle existing projects with allow_insecure: true in their devbox.lock ([add] migrate allow_insecure from lockfile to config #1754)

Downside:
If a user erroneously specifies the wrong package name or names in --allow-insecure=<packages>, then a devbox add <pkg> --allow-insecure=<packages> outside the devbox shell environment will quietly work. However, the user will experience an error when running devbox shell (or similar) next time, and need to fix the erroneous --allow-insecure value.

• Fix: we could fix this by doing "recomputeState" when installing-with-insecure in "ensureStateIsUpToDate", but I haven't to avoid adding complexity for this unlikely scenario. I call it unlikely because the error message (see below) specifically tells the user what values to use with --allow-insecure=<packages>

Fixes #1337

How was it tested?

devbox add python@2.7 gives the following error now:

 devbox add python@2.7
Info: Adding package "python@2.7" to devbox.json
[1/1] python2
[1/1] python2: Fail

Error: Nix: Package  ‘python-2.7.18.7’ is insecure.

Known vulnerabilities:
Python 2.7 has reached its end of life after 2020-01-01. See https://www.python.org/doc/sunset-python-2/.

To override, use `devbox add <pkg> --allow-insecure=python-2.7.18.7`

Doing devbox add python@2.7 --allow-insecure=python-2.7.18.1 works with the devbox.json now having:

+    "python": {
+      "version":        "2.7",
+      "allow_insecure": ["python-2.7.18.7"],
+    },

Also for the package reported in #1337:

[savil]

Current dependencies on/for this PR:

• main
  • PR [devbox add] --allow-insecure should handle multiple, user-specified packages #1749 👈
    • PR [add] migrate allow_insecure from lockfile to config #1754

This stack of pull requests is managed by Graphite.

[mikeland73]

Currently reviewing, but a quick comment about the json change. cc: @Lagoja

Instead of the current json change, thoughts on:

"python": {
  "version":        "2.7",
  "allow_insecure" true,
},

"python": {
  "version":        "2.7",
  "dependencies": {
    "openssl": {
      "allow_insecure": true,
    }
  }
},

Motivation:

• Same format for top level package and dependencies
• allows for future input overrides (and any other configuration we want to apply to dependent packages).

The only downside is that this format doesn't make a distinction between direct dependencies and sub-depedencies, but I think that's way too complicated.

[savil]

@mikeland73 🤔 I'm a little concerned that it'll be introducing quite some complexity in this PR's implementation: especially around needing to distinguish the top-level package from dependency packages, and their properties. Can we cross that bridge when we need to later?

[gcurtis]

I'm liking the comprehensive tests.

[gcurtis]

I think you can remove the if check here.

Suggested change

	{{- if .AllowInsecure }}
	"{{ .StoreName }}"
	{{- if .HasAllowInsecure }}
	{{- range .AllowInsecure }}
	"{{ . }}"
	{{- end }}
	{{- range .AllowInsecure }}
	"{{ . }}"
	{{- end }}

[savil]

@mikeland73 landed the code to close the week out, but happy to discuss more and make any followup fixes

[mikeland73]

@savil I would run it by @Lagoja .

JSON changes are semi-1 way doors. This is OK but just wanted to make sure you are aware of:

• As currently implemented, this changes the previous behavior. Specifically, if the package is updated and the version changes, allow insecure version will no longer march. My suggested implementation keeps the same behavior. This is probably a bit safer, but wanted to call out that we're changing how it works.
• If we do add input overrides, we may want to add allow insecure to the individual packages themselves (in order to standardize fields between top level packages and dependencies). This may leave this new field as a bit of an awkward outlier (because it's a field that applies to both top level and also dependencies). Not enough to be a blocker, but it does feel a bit weird if we end up in that state.

[sentry-io]

Suspect Issues

This pull request was deployed and Sentry observed the following issues:

• ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
• ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
• ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
• ‼️ **Generic Error: <redacted *errors.withStack>: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue
• ‼️ **Generic Error: <redacted errors.withStack>: <redacted usererr.combined> go.jetpack.io/devbox/internal/boxcli/usererr in... View Issue

_{Did you find this useful? React with a 👍 or 👎}