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

opensnitch: Add at v1.6.7 #4783

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

opensnitch: Add at v1.6.7 #4783

wants to merge 2 commits into from

Conversation

uni-dos
Copy link
Contributor

@uni-dos uni-dos commented Jan 10, 2025

Summary

  • adds the package.yml to include opensnitch.

Test Plan

  • Once installed, open the gui and see outgoing traffic.
  • There is a conflict with wireguard and opensnitch. Need ebf module.

Checklist

  • Package was built and tested against unstable
  • This change could gainfully be listed in the weekly sync notes once merged

Resolves #289

@uni-dos uni-dos force-pushed the opensnitch branch 9 times, most recently from c78cdb0 to e231295 Compare January 13, 2025 21:36
@uni-dos uni-dos marked this pull request as ready for review January 13, 2025 21:37
@uni-dos
Copy link
Contributor Author

uni-dos commented Jan 15, 2025

I did a quick ripgrep for /etc/opensnitchd and updated the locations. Hopefully that was all of them.

@EbonJaeger
Copy link
Member

Almost there. Just changing the locations isn't enough; we want users to be able to copy the default configs to /etc, modify them, and have them be loaded. So, we need to check if a file exists in the /etc tree, and use that if there is, otherwise use the /usr/share/defaults/etc path. Does that make sense?

@uni-dos
Copy link
Contributor Author

uni-dos commented Jan 15, 2025

Ah got it. I am not familiar with go but Ill give it a shot

**Summary**
- add python-qt-material a dependecy of opensnitch
@uni-dos
Copy link
Contributor Author

uni-dos commented Jan 15, 2025

Not my best work but I think it's done. /etc/rules is created when running opensnitch since these are user preferences. Not sure if the directory will be created each time though.

@uni-dos uni-dos requested a review from EbonJaeger January 15, 2025 16:09
@EbonJaeger EbonJaeger added the Topic: Sync Notes This PR/Issue can be highlighted in sync notes label Jan 17, 2025
@EbonJaeger
Copy link
Member

Hm I should have clicked Comment instead of Request Changes. Oops.

@uni-dos
Copy link
Contributor Author

uni-dos commented Jan 22, 2025

About the /etc/rules. I did not make sense to me that the rules directory should be in /usr/ because these will be created by the user and not Solus itself.

@uni-dos
Copy link
Contributor Author

uni-dos commented Jan 22, 2025

An issue I just experienced is that the /etc/opensnitchd/rules is not being created

Copy link
Member

@EbonJaeger EbonJaeger left a comment

Choose a reason for hiding this comment

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

I think I might see why.

packages/o/opensnitch/files/update-config.patch Outdated Show resolved Hide resolved
**Summary**
- adds opensnitch a firewall inspired by Little Snitch
Copy link
Member

@silkeh silkeh left a comment

Choose a reason for hiding this comment

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

Great work! I had a few (admittedly minor) comments on the systemd and Go patches.

if args.socket == None:
# default
- args.socket = "unix:///tmp/osui.sock"
+ args.socket = "unix:///run/user/1000/opensnitch/osui.sock"
Copy link
Member

Choose a reason for hiding this comment

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

I think (haven't checked) this should use $XDG_RUNTIME_DIR instead of hardcoding the UID.

Comment on lines +31 to +38
- c.file = "/etc/opensnitchd/system-fw.json"
+ _, fileErr := os.Stat("/etc/opensnitchd/system-fw.json")
+
+ if fileErr == nil {
+ c.file = "/etc/opensnitchd/system-fw.json"
+ } else {
+ c.file = "/usr/share/defaults/etc/opensnitchd/system-fw.json"
+ }
Copy link
Member

Choose a reason for hiding this comment

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

This should probably only catch fs.ErrNotExist to make things like permission issues more obvious. That also allows you to invert the if-statement making the patch cleaner:

Suggested change
- c.file = "/etc/opensnitchd/system-fw.json"
+ _, fileErr := os.Stat("/etc/opensnitchd/system-fw.json")
+
+ if fileErr == nil {
+ c.file = "/etc/opensnitchd/system-fw.json"
+ } else {
+ c.file = "/usr/share/defaults/etc/opensnitchd/system-fw.json"
+ }
c.file = "/etc/opensnitchd/system-fw.json"
+
+ if _, err := os.Stat(c.file); errors.Is(err, fs.ErrNotExist) {
+ c.file = "/usr/share/defaults/etc/opensnitchd/system-fw.json"
+ }
+

+
+func userConfigExists() (string) {
+ _, err := os.Stat("/etc/opensnitchd/default-config.json")
+ if err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Same:

Suggested change
+ if err == nil {
+ if !errors.Is(fs.ErrNotExist) {


+func userConfigExists() (string) {
+ _, err := os.Stat("/etc/opensnitchd/default-config.json")
+ if err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ if err == nil {
+ if !errors.Is(err, fs.ErrNotExist) {

logUTC = true
logMicro = false
rulesPath = "/etc/opensnitchd/rules/"
- configFile = "/etc/opensnitchd/default-config.json"
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply change this line to configFile = userConfigExists()?

Comment on lines +77 to +78
+ os.MkdirAll("/etc/opensnitchd/rules", 755)
+
Copy link
Member

@silkeh silkeh Jan 27, 2025

Choose a reason for hiding this comment

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

I would remove this in favour of the systemd unit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Topic: Sync Notes This PR/Issue can be highlighted in sync notes
Projects
Status: Triage
Development

Successfully merging this pull request may close these issues.

OpenSnitch (T6067)
3 participants