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

[allow-insecure] Allow insecure packages with --allow-insecure flag #1265

Merged
merged 8 commits into from
Jul 12, 2023
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion devbox.json
Original file line number Diff line number Diff line change
Expand Up @@ -22,4 +22,4 @@
"nixpkgs": {
"commit": "3364b5b117f65fe1ce65a3cdd5612a078a3b31e3"
}
}
}
15 changes: 15 additions & 0 deletions examples/insecure/devbox.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
{
"packages": [
"nodejs@16"
],
"shell": {
"init_hook": [
"echo 'Welcome to devbox!' > /dev/null"
],
"scripts": {
"run_test": [
"node --version"
]
}
}
}
12 changes: 12 additions & 0 deletions examples/insecure/devbox.lock
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"lockfile_version": "1",
"packages": {
"nodejs@16": {
"allow_insecure": true,
"last_modified": "2023-06-29T16:20:38Z",
"resolved": "github:NixOS/nixpkgs/3c614fbc76fc152f3e1bc4b2263da6d90adf80fb#nodejs_16",
"source": "devbox-search",
"version": "16.20.1"
}
}
}
12 changes: 9 additions & 3 deletions internal/boxcli/add.go
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@ import (
const toSearchForPackages = "To search for packages, use the `devbox search` command"

type addCmdFlags struct {
config configFlags
config configFlags
allowInsecure bool
}

func addCmd() *cobra.Command {
Expand Down Expand Up @@ -47,13 +48,18 @@ func addCmd() *cobra.Command {
}

flags.config.register(command)
command.Flags().BoolVar(
&flags.allowInsecure, "allow-insecure", false,
"Allow adding packages marked as insecure.")

return command
}

func addCmdFunc(cmd *cobra.Command, args []string, flags addCmdFlags) error {
box, err := devbox.Open(&devopt.Opts{
Dir: flags.config.path,
Writer: cmd.ErrOrStderr(),
Dir: flags.config.path,
Writer: cmd.ErrOrStderr(),
AllowInsecureAdds: flags.allowInsecure,
})
if err != nil {
return errors.WithStack(err)
Expand Down
20 changes: 20 additions & 0 deletions internal/devpkg/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -542,3 +542,23 @@ func (p *Package) ContentAddressedPath() (string, error) {
}
return localPath, err
}

func (p *Package) AllowInsecure() bool {
return p.lockfile.Get(p.Raw).IsAllowInsecure()
}

// StoreName returns the last section of the store path. Example:
// /nix/store/abc123-foo-1.0.0 -> foo-1.0.0
// Warning, this is probably slowish. If you need to call this multiple times,
// consider caching the result.
func (p *Package) StoreName() (string, error) {
u, err := p.urlForInstall()
if err != nil {
return "", err
}
name, err := nix.EvalPackageName(u)
if err != nil {
return "", err
}
return name, nil
}
31 changes: 19 additions & 12 deletions internal/impl/devbox.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,13 @@ const (
)

type Devbox struct {
cfg *devconfig.Config
lockfile *lock.File
nix nix.Nixer
projectDir string
pluginManager *plugin.Manager
pure bool
cfg *devconfig.Config
lockfile *lock.File
nix nix.Nixer
projectDir string
pluginManager *plugin.Manager
pure bool
allowInsecureAdds bool

// Possible TODO: hardcode this to stderr. Allowing the caller to specify the
// writer is error prone. Since it is almost always stderr, we should default
Expand All @@ -81,18 +82,24 @@ func Open(opts *devopt.Opts) (*Devbox, error) {
}

box := &Devbox{
cfg: cfg,
nix: &nix.Nix{},
projectDir: projectDir,
pluginManager: plugin.NewManager(),
writer: opts.Writer,
pure: opts.Pure,
cfg: cfg,
nix: &nix.Nix{},
projectDir: projectDir,
pluginManager: plugin.NewManager(),
writer: opts.Writer,
pure: opts.Pure,
allowInsecureAdds: opts.AllowInsecureAdds,
}

lock, err := lock.GetFile(box)
if err != nil {
return nil, err
}
// if lockfile has any allow insecure, we need to set the env var to ensure
// all nix commands work.
if opts.AllowInsecureAdds || lock.HasAllowInsecurePackages() {
nix.AllowInsecurePackages()
}
box.pluginManager.ApplyOptions(
plugin.WithDevbox(box),
plugin.WithLockfile(lock),
Expand Down
9 changes: 5 additions & 4 deletions internal/impl/devopt/devboxopts.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,9 @@ import (
)

type Opts struct {
Dir string
Pure bool
IgnoreWarnings bool
Writer io.Writer
AllowInsecureAdds bool
Dir string
Pure bool
IgnoreWarnings bool
Writer io.Writer
}
20 changes: 15 additions & 5 deletions internal/impl/packages.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,19 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames ...string) error {

// Only add packages that are not already in config. If same canonical exists,
// replace it.
pkgs := []*devpkg.Package{}
for _, pkg := range devpkg.PackageFromStrings(lo.Uniq(pkgsNames), d.lockfile) {
pkgs := devpkg.PackageFromStrings(lo.Uniq(pkgsNames), d.lockfile)
for _, pkg := range pkgs {
// Resolving here ensures we allow insecure before running ensurePackagesAreInstalled
// which will call print-dev-env. Resolving does not save the lockfile, we
// save at the end when everything has succeeded.
p, err := d.lockfile.Resolve(pkg.Raw)
if err != nil {
return err
Copy link
Collaborator

Choose a reason for hiding this comment

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

Will this error for the fallthrough case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch. Will test and fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This works, because in the fallthrough case resolve works fine. It actually enters non-fallthrough packages twice (and then cleans it up when lock.Tidy() is called). I'm gonna see if I can make this better.

}
if d.allowInsecureAdds {
p.AllowInsecure = true
Copy link
Collaborator

Choose a reason for hiding this comment

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

So this will add allow insecure field to all the packages in the add command, even though some of them may not need allow insecure?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. The idea is that the flag affects all packages being added in that command. e.g.

devbox add curl go --allow-insecure will mark both of them as allow insecure. If user only wants it to apply to one of them, they should add one at a time.

I think this is a reasonable tradeoff, otherwise we probably end up needing a new cli command e.g. devbox allow-insecure <pkg> or something like that.

}

// If exact versioned package is already in the config, skip.
if slices.Contains(d.cfg.Packages, pkg.Versioned()) {
continue
Expand All @@ -64,6 +75,7 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames ...string) error {
if err == nil && ok {
d.cfg.Packages = append(d.cfg.Packages, pkg.Versioned())
} else {
// TODO (landau): use nix.Search to check if this package exists
// fallthrough and treat package as a legacy package.
d.cfg.Packages = append(d.cfg.Packages, pkg.Raw)
}
Expand All @@ -88,9 +100,7 @@ func (d *Devbox) Add(ctx context.Context, pkgsNames ...string) error {
}
}

if err := d.lockfile.Add(
lo.Map(pkgs, func(pkg *devpkg.Package, _ int) string { return pkg.Raw })...,
); err != nil {
if err := d.lockfile.Save(); err != nil {
return err
}

Expand Down
9 changes: 9 additions & 0 deletions internal/lock/lockfile.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,15 @@ func (l *File) Get(pkg string) *Package {
return entry
}

func (l *File) HasAllowInsecurePackages() bool {
for _, pkg := range l.Packages {
if pkg.AllowInsecure {
return true
}
}
return false
}

// This probably belongs in input.go but can't add it there because it will
// create a circular dependency. We could move Input into own package.
func IsLegacyPackage(pkg string) bool {
Expand Down
8 changes: 8 additions & 0 deletions internal/lock/package.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const (
)

type Package struct {
AllowInsecure bool `json:"allow_insecure,omitempty"`
LastModified string `json:"last_modified,omitempty"`
PluginVersion string `json:"plugin_version,omitempty"`
Resolved string `json:"resolved,omitempty"`
Expand All @@ -35,3 +36,10 @@ func (p *Package) GetSource() string {
}
return p.Source
}

func (p *Package) IsAllowInsecure() bool {
if p == nil {
return false
}
return p.AllowInsecure
}
9 changes: 6 additions & 3 deletions internal/nix/command.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package nix

import (
"os"
"os/exec"
)

Expand All @@ -12,6 +11,10 @@ func command(args ...string) *exec.Cmd {
return cmd
}

func allowUnfreeEnv() []string {
return append(os.Environ(), "NIXPKGS_ALLOW_UNFREE=1")
func allowUnfreeEnv(curEnv []string) []string {
return append(curEnv, "NIXPKGS_ALLOW_UNFREE=1")
}

func allowInsecureEnv(curEnv []string) []string {
return append(curEnv, "NIXPKGS_ALLOW_INSECURE=1")
}
56 changes: 56 additions & 0 deletions internal/nix/eval.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package nix

import (
"encoding/json"
"os"
"strconv"
)

func EvalPackageName(path string) (string, error) {
cmd := command("eval", "--raw", path+".name")
out, err := cmd.Output()
if err != nil {
return "", err
}
return string(out), nil
}

// PackageIsInsecure is a fun little nix eval that maybe works.
func PackageIsInsecure(path string) bool {
cmd := command("eval", path+".meta.insecure")
out, err := cmd.Output()
if err != nil {
// We can't know for sure, but probably not.
return false
}
var insecure bool
if err := json.Unmarshal(out, &insecure); err != nil {
// We can't know for sure, but probably not.
return false
}
return insecure
}

func PackageKnownVulnerabilities(path string) []string {
cmd := command("eval", path+".meta.knownVulnerabilities")
out, err := cmd.Output()
if err != nil {
// We can't know for sure, but probably not.
return nil
}
var vulnerabilities []string
if err := json.Unmarshal(out, &vulnerabilities); err != nil {
// We can't know for sure, but probably not.
return nil
}
return vulnerabilities
}

func AllowInsecurePackages() {
os.Setenv("NIXPKGS_ALLOW_INSECURE", "1")
}

func IsInsecureAllowed() bool {
allowed, _ := strconv.ParseBool(os.Getenv("NIXPKGS_ALLOW_INSECURE"))
return allowed
}
23 changes: 22 additions & 1 deletion internal/nix/nix.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,13 @@ import (
"os"
"os/exec"
"path/filepath"
"regexp"
"runtime/trace"
"strings"

"github.com/pkg/errors"
"go.jetpack.io/devbox/internal/boxcli/featureflag"
"go.jetpack.io/devbox/internal/boxcli/usererr"

"go.jetpack.io/devbox/internal/debug"
)
Expand Down Expand Up @@ -69,7 +71,9 @@ func (*Nix) PrintDevEnv(ctx context.Context, args *PrintDevEnvArgs) (*PrintDevEn
cmd.Args = append(cmd.Args, "--json")
debug.Log("Running print-dev-env cmd: %s\n", cmd)
data, err = cmd.Output()
if err != nil {
if insecure, insecureErr := isExitErrorInsecurePackage(err); insecure {
return nil, insecureErr
} else if err != nil {
return nil, errors.Wrapf(err, "Command: %s", cmd)
}

Expand Down Expand Up @@ -120,6 +124,7 @@ func System() (string, error) {
// For Savil to debug "remove nixpkgs" feature. The Search api lacks x86-darwin info.
// So, I need to fake that I am x86-linux and inspect the output in generated devbox.lock
// and flake.nix files.
// This is also used by unit tests.
override := os.Getenv("__DEVBOX_NIX_SYSTEM")
if override != "" {
return override, nil
Expand All @@ -144,3 +149,19 @@ func System() (string, error) {
func ProfileBinPath(projectDir string) string {
return filepath.Join(projectDir, ProfilePath, "bin")
}

func isExitErrorInsecurePackage(err error) (bool, error) {
var exitErr *exec.ExitError
if errors.As(err, &exitErr) && exitErr.ExitCode() == 1 {
if strings.Contains(string(exitErr.Stderr), "is marked as insecure") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

😭

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this is not the best. An alternative is to loop though all packages and check if any of them are insecure, that would make errors much slower so it's a tradeoff.

re := regexp.MustCompile(`Package ([^ ]+)`)
match := re.FindStringSubmatch(string(exitErr.Stderr))
return true, usererr.New(
"Package %s is insecure. \n\n"+
"To override use `devbox add <pkg> --allow-insecure`",
match[0],
)
}
}
return false, nil
}
Loading