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

add support for cargo toml/non-toml files #4286

Merged
merged 4 commits into from
May 20, 2021
Merged

add support for cargo toml/non-toml files #4286

merged 4 commits into from
May 20, 2021

Conversation

glitsj16
Copy link
Collaborator

This is a follow-up for #4284. See cargo book and discussion for details.

@reinerh
Copy link
Collaborator

reinerh commented May 19, 2021

why are we blacklisting individual files in .cargo instead of just blacklisting the whole directory?

@rusty-snake
Copy link
Collaborator

I think it has something to do with ~/.cargo/bin and/or ~/.cargo/env. blacklist ${HOME}/.cargo will show a error if you source $HOME/.cargo/env in your .bashrc and cargo install TOOL tools aren't available in a firejailed bash.

Is it worth it?

@reinerh
Copy link
Collaborator

reinerh commented May 19, 2021

I don't know if changing it now is worth the effort, or if having problems with cargo install is acceptable.

@glitsj16
Copy link
Collaborator Author

I don't know if changing it now is worth the effort, or if having problems with cargo install is acceptable.

Regarding the first part - changing things now is worth the effort - I'd say it's not a big effort at all. We just need to have some sort of consensus on the need for change IMO. And @rusty-snake's argument that sourcing $HOME/.cargo/env in ~/.bashrc will produce an error makes me feel happy having avoided that from happening.

The second part - if having problems with cargo install is acceptable - seems more straightforward IMO. It makes sense to me to regard the install subcommand basic functionality that one would expect to be supported in a firejailed cargo setup.

I understand the remark, but taking in the above I honestly feel blacklisting individual files instead of the whole directory is the saner, more user-friendly thing to do in this case. Just voicing an opinion here. In that regard I won't 'force' this discussion in one way or another by committing this PR myself. We can wait and see if other people chime in after having had the time to play/test this cargo profile more thoroughly.

I must admit I only use cargo build in PKGBUILD's being on Arch Linux. I haven't seen any repo or AUR package that uses a cargo install command on Arch Linux. The native makepkg routine deals with installing files by its own logic. I'm too unfamiliar with other distro's packaging tools to judge any potential ramifications when we decide to not support cargo install by default.

@rusty-snake
Copy link
Collaborator

To be clear about source $HOME/.cargo/env. Nothing is broken, you just get a bash: $HOME/.cargo.env: Permission denied if you run firejail bash. This affects nothing outside of firejail and inside firejail you can noblacklist it (e.g. for IDEs).

blacklist ${HOME}/.cargo does not break cargo install TOOL¹. The tools installed by it can't be used inside firejail w/o noblacklist.

¹ if you run cargo in firejail and don't have a noblacklist ${HOME}/.cargo every cargo command does not work (of course).

@rusty-snake
Copy link
Collaborator

Globbing could be used as compromise.

noblacklist ${HOME}/.cargo/*
blacklist ${HOME}/.cargo/*

@glitsj16
Copy link
Collaborator Author

Globbing could be used as compromise.

That's actually quite elegant! I like it. Which brings me to posing the following question to @rusty-snake. If I understand all this, we can drop this PR if you're willing to bring in these globbing changes for #4284. Not trying to ditch the work, just trying to not disrupt the chain of PR's (is there such a thing lol) etcetera. What do you think? @reinerh Would you be OK with the globbing solution?

@rusty-snake
Copy link
Collaborator

#4284 is merged. Doing it in this PR would be best (IMHO).

disable-common.inc (what do we have in disable-programs.inc?):

noblacklist ${HOME}/.cargo/bin
noblacklist ${HOME}/.cargo/env
blacklist ${HOME}/.cargo/*

allow-common-devel.inc:

noblacklist ${HOME}/.cargo/*

@glitsj16
Copy link
Collaborator Author

@rusty-snake Fair enough, I'll make the needed changes. disable-programs.inc and allow-common-devel.inc are done. I need some more time for disable-common.inc after a food-break, pizza delivery just rang my bell heh!

@rusty-snake
Copy link
Collaborator

IMHO we can leave disable-common as is.

@glitsj16
Copy link
Collaborator Author

IMHO we can leave disable-common as is.

@rusty-snake Great, in that case the work is done. Let's wait for @reinerh view on these latest changes before merging. Thanks for your input, always a pleasure working together.

@reinerh
Copy link
Collaborator

reinerh commented May 20, 2021

Looks good to me! :-)

@glitsj16 glitsj16 merged commit 8882d65 into netblue30:master May 20, 2021
@glitsj16 glitsj16 deleted the cargo branch May 20, 2021 21:52
@matu3ba matu3ba mentioned this pull request Oct 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants