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

Conversation

mikeland73
Copy link
Contributor

@mikeland73 mikeland73 commented Jul 11, 2023

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):

  • When installing an insecure package, ask the user if they want to allow it, update the devbox.json, and install it.

How was it tested?

devbox add nodejs@16
devbox add nodejs@16 --allow-insecure
devbox run run_test
# edited lockfile to remove allow_insecure
devbox run run_test # error
devbox install # error
devbox shell # error

See examples/insecure

@gcurtis
Copy link
Collaborator

gcurtis commented Jul 11, 2023

@mikeland73 should I hold off on reviewing this for now?

@mikeland73 mikeland73 marked this pull request as draft July 11, 2023 18:28
@mikeland73 mikeland73 marked this pull request as ready for review July 12, 2023 00:26
@mikeland73 mikeland73 changed the title [allow-insecure] Allow insecure packages with devbox.json field [allow-insecure] Allow insecure packages with --allow-insecure flag Jul 12, 2023
// 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.

return err
}
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.

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.

@mikeland73 mikeland73 merged commit 95ee457 into main Jul 12, 2023
9 checks passed
@mikeland73 mikeland73 deleted the landau/insecure branch July 12, 2023 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants