-
Notifications
You must be signed in to change notification settings - Fork 249
[allow-insecure] Allow insecure packages with --allow-insecure flag #1265
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
Conversation
@mikeland73 should I hold off on reviewing this for now? |
internal/impl/packages.go
Outdated
// save at the end when everything has succeeded. | ||
p, err := d.lockfile.Resolve(pkg.Raw) | ||
if err != nil { | ||
return err |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
internal/impl/packages.go
Outdated
return err | ||
} | ||
if d.allowInsecureAdds { | ||
p.AllowInsecure = true |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
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") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭
There was a problem hiding this comment.
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.
Summary
This allows installing insecure packages using the
--allow-insecure
flag:devbox add nodejs@16 --allow-insecure
This saves the allow insecure state to lock file.
If user tries to do add/shell/run/install and there are insecure pacakges that are not in marked in lock file, they will see an error indicating they should use flag.
I used flag (instead of prompt) to limit the size of this already massive PR. TODO (in follow up):
How was it tested?
See examples/insecure