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

Make mutexes private #890

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

mjholub
Copy link
Contributor

@mjholub mjholub commented Mar 26, 2023

see: uber-go/guide#127
in terms of stylistic changes this also combines the IPv4 and IPv6 errors in an unified struct.

@gustavo-iniguez-goya
Copy link
Collaborator

gustavo-iniguez-goya commented Mar 27, 2023

Hi @154pinkchairs ,

Thank you for these improvements! Making the mutexes private and the FirewallError type would be really nice to have.

Could you split the PR into smaller ones? "Make mutexes private" has nothing to do with adding a new error type, it'll make things easier to analyze and debug.

On the other hand, could you add only the changes relevant to the PR?
For example returning here prevents monitoring the file system-fw.json for changes:

https://github.com/evilsocket/opensnitch/pull/890/files#diff-c01f644586c786f0bdee74c46336aeb7d402fa28faef803bb48cdad9aa7c9f62L166-R181

Testing it out:
execute the daemon -> see that the error is logged -> modify the file system-fw.json -> notice that it's not reloaded.
then comment out the return;
execute the daemon -> see that the error is logged -> modify the file system-fw.json -> notice that it's reloaded .

Also, the new json tags doesn't correspond with the existent system-fw.json configuration fields, which on the one hand would break users' configurations, and on the other hand prevents from adding system firewall rules.

For example renaming "Version" to "versions" prevents from adding rules:

image

Other examples:
TargetParameters string json:"target_parameters"
Enabled bool json:"enabled" -> making it not capitalize disables GUI's ability to enable/disable it.
SystemRules []*chainsList json:"chains_list" -> error:

image

And here another change not relevant to the PR:
https://github.com/evilsocket/opensnitch/pull/890/files#diff-0c26094505ea9f96de01bd8aeb1abee5455726a6269ddfcceb939434ae6977dfR132

@mjholub
Copy link
Contributor Author

mjholub commented Apr 1, 2023

I have updated my PR to adjust to the changes requested.
The error wrapping moved to:
#899
I've also checked out config/config.go to master for this PR and fixed the json tags in #889 as it affects the json schema more.

@gustavo-iniguez-goya
Copy link
Collaborator

thank you @154pinkchairs , looks better now!

There's only one minor problem, that's probably why these mutexes were embedded instead of being members of the structs.
There's a leak re-loading the fw configuration (for example when changing the input policy drop <-> allow, go build -race -o opensnitchd .)

WARNING: DATA RACE
Write at 0x00c000cb8b30 by goroutine 80:

(...)

  github.com/evilsocket/opensnitch/daemon/firewall/config.(*Config).loadConfiguration()
      /home/devuan/pr889/daemon/firewall/config/config.go:201 +0x112
  github.com/evilsocket/opensnitch/daemon/firewall/config.(*Config).SaveConfiguration()
      /home/devuan/pr889/daemon/firewall/config/config.go:218 +0x1b3
  github.com/evilsocket/opensnitch/daemon/firewall/nftables.(*Nft).SaveConfiguration()
      <autogenerated>:1 +0x4f

Previous read at 0x00c000cb8b30 by goroutine 59:
  github.com/evilsocket/opensnitch/daemon/firewall/nftables.(*Nft).AddSystemRules()
      /home/devuan/pr889/daemon/firewall/nftables/system.go:92 +0x537
  github.com/evilsocket/opensnitch/daemon/firewall/nftables.(*Nft).reloadConfCallback()
      /home/devuan/pr889/daemon/firewall/nftables/monitor.go:56 +0xd5

I've only found 2 ways of getting rid of it: 1) by embedding the lock as it was before, 2) by creating a new SystemConfig struct:

s := SystemConfig{}
if err := json.Unmarshal(rawConfig, &s); err != nil {
...
}
c.SysConfig = s

But unfortunately, doing it in this way, it does not delete all the rules when changing the policies.
I'll investigate it a litle bit further

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.

2 participants