From 8740755f642d503e09da6d2c4482801ff0412104 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gustavo=20I=C3=B1iguez=20Goia?= Date: Sat, 15 Jul 2023 20:32:42 +0200 Subject: [PATCH] sys fw: report errors to the GUI after reloading - Send errors to the server (GUI) if there's any error when reloading the system fw rules (far from being perfect/optimal, needs a rewrite). - Don't load the configuration after saving it, let the watcher reload it on write change to avoid double reload/duplicated errors. --- daemon/firewall/common/common.go | 34 ++++++++++++++++- daemon/firewall/config/config.go | 2 - daemon/firewall/iptables/iptables.go | 1 + daemon/firewall/nftables/nftables.go | 1 + daemon/firewall/nftables/rules.go | 8 ++-- daemon/firewall/nftables/system.go | 2 +- daemon/firewall/rules.go | 13 +++++++ daemon/ui/notifications.go | 55 +++++++++++++++++++++------- 8 files changed, 95 insertions(+), 21 deletions(-) diff --git a/daemon/firewall/common/common.go b/daemon/firewall/common/common.go index 5c8e5851ab..67e3afffa3 100644 --- a/daemon/firewall/common/common.go +++ b/daemon/firewall/common/common.go @@ -26,8 +26,9 @@ type ( // iptables and nftables. Common struct { RulesChecker *time.Ticker - stopChecker chan bool + ErrChan chan string QueueNum uint16 + stopChecker chan bool Running bool Intercepting bool FwEnabled bool @@ -35,6 +36,37 @@ type ( } ) +// ErrorsChan returns the channel where the errors are sent to. +func (c *Common) ErrorsChan() <-chan string { + return c.ErrChan +} + +// ErrChanEmpty checks if the errors channel is empty. +func (c *Common) ErrChanEmpty() bool { + return len(c.ErrChan) == 0 +} + +// SendError sends an error to the channel of errors. +func (c *Common) SendError(err string) { + log.Warning("%s", err) + + if len(c.ErrChan) >= cap(c.ErrChan) { + log.Debug("fw errors channel full, emptying errChan") + for e := range c.ErrChan { + log.Warning("%s", e) + if c.ErrChanEmpty() { + break + } + } + return + } + select { + case c.ErrChan <- err: + case <-time.After(100 * time.Millisecond): + log.Warning("SendError() channel locked? REVIEW") + } +} + // SetQueueNum sets the queue number used by the firewall. // It's the queue where all intercepted connections will be sent. func (c *Common) SetQueueNum(qNum *int) { diff --git a/daemon/firewall/config/config.go b/daemon/firewall/config/config.go index 3152b9403d..e25a63a509 100644 --- a/daemon/firewall/config/config.go +++ b/daemon/firewall/config/config.go @@ -215,8 +215,6 @@ func (c *Config) SaveConfiguration(rawConfig string) error { return err } - c.loadConfiguration([]byte(rawConfig)) - if err = os.Chmod(c.file, 0600); err != nil { log.Warning("unable to set system-fw.json permissions: %s", err) } diff --git a/daemon/firewall/iptables/iptables.go b/daemon/firewall/iptables/iptables.go index 3a7cd067dc..d4b9dcc8b5 100644 --- a/daemon/firewall/iptables/iptables.go +++ b/daemon/firewall/iptables/iptables.go @@ -101,6 +101,7 @@ func (ipt *Iptables) Init(qNum *int) { return } ipt.SetQueueNum(qNum) + ipt.ErrChan = make(chan string, 100) // In order to clean up any existing firewall rule before start, // we need to load the fw configuration first to know what rules diff --git a/daemon/firewall/nftables/nftables.go b/daemon/firewall/nftables/nftables.go index 7f650aea9c..257add89b0 100644 --- a/daemon/firewall/nftables/nftables.go +++ b/daemon/firewall/nftables/nftables.go @@ -75,6 +75,7 @@ func (n *Nft) Init(qNum *int) { if n.IsRunning() { return } + n.ErrChan = make(chan string, 100) InitMapsStore() n.SetQueueNum(qNum) n.Conn = NewNft() diff --git a/daemon/firewall/nftables/rules.go b/daemon/firewall/nftables/rules.go index 5edf1a1189..9fe75e1f2a 100644 --- a/daemon/firewall/nftables/rules.go +++ b/daemon/firewall/nftables/rules.go @@ -151,7 +151,7 @@ func (n *Nft) InsertRule(chain, table, family string, position uint64, exprs *[] } n.Conn.InsertRule(rule) if !n.Commit() { - return fmt.Errorf("%s rule not added", logTag) + return fmt.Errorf("rule not added") } return nil @@ -161,13 +161,13 @@ func (n *Nft) InsertRule(chain, table, family string, position uint64, exprs *[] func (n *Nft) AddRule(chain, table, family string, position uint64, key string, exprs *[]expr.Any) (*nftables.Rule, error) { tbl := n.GetTable(table, family) if tbl == nil { - return nil, fmt.Errorf("%s addRule, Error getting table: %s, %s", logTag, table, family) + return nil, fmt.Errorf("getting %s table: %s, %s", logTag, table, family) } chainKey := getChainKey(chain, tbl) chn, chok := sysChains.Load(chainKey) if !chok { - return nil, fmt.Errorf("%s addRule, Error getting table: %s, %s", logTag, table, family) + return nil, fmt.Errorf("getting table: %s, %s", table, family) } rule := &nftables.Rule{ @@ -179,7 +179,7 @@ func (n *Nft) AddRule(chain, table, family string, position uint64, key string, } n.Conn.AddRule(rule) if !n.Commit() { - return nil, fmt.Errorf("%s Error adding rule", logTag) + return nil, fmt.Errorf("adding %s rule", logTag) } return rule, nil diff --git a/daemon/firewall/nftables/system.go b/daemon/firewall/nftables/system.go index d9a4f8b92a..08c08a2e06 100644 --- a/daemon/firewall/nftables/system.go +++ b/daemon/firewall/nftables/system.go @@ -125,7 +125,7 @@ func (n *Nft) AddSystemRules(reload, backupExistingChains bool) { } if chain.Rules[i].Enabled { if err4, _ := n.AddSystemRule(chain.Rules[i], chain); err4 != nil { - log.Warning("rule not added: %s", err4) + n.SendError(fmt.Sprintf("%s (%s)", err4, chain.Rules[i].UUID)) } } } diff --git a/daemon/firewall/rules.go b/daemon/firewall/rules.go index 5e45bd9054..f7644797f8 100644 --- a/daemon/firewall/rules.go +++ b/daemon/firewall/rules.go @@ -31,6 +31,9 @@ type Firewall interface { Serialize() (*protocol.SysFirewall, error) Deserialize(sysfw *protocol.SysFirewall) ([]byte, error) + + ErrorsChan() <-chan string + ErrChanEmpty() bool } var ( @@ -78,6 +81,16 @@ func IsRunning() bool { return fw != nil && fw.IsRunning() } +// ErrorsChan returns the channel where the errors are sent to. +func ErrorsChan() <-chan string { + return fw.ErrorsChan() +} + +// ErrChanEmpty checks if the errors channel is empty. +func ErrChanEmpty() bool { + return fw.ErrChanEmpty() +} + // CleanRules deletes the rules we added. func CleanRules(logErrors bool) { if fw == nil { diff --git a/daemon/ui/notifications.go b/daemon/ui/notifications.go index f6073c64d9..cf7d34b5cf 100644 --- a/daemon/ui/notifications.go +++ b/daemon/ui/notifications.go @@ -194,6 +194,47 @@ func (c *Client) handleActionStopMonitorProcess(stream protocol.UI_Notifications c.sendNotificationReply(stream, notification.Id, "", nil) } +func (c *Client) handleActionReloadFw(stream protocol.UI_NotificationsClient, notification *protocol.Notification) { + log.Info("[notification] reloading firewall") + + sysfw, err := firewall.Deserialize(notification.SysFirewall) + if err != nil { + log.Warning("firewall.Deserialize() error: %s", err) + c.sendNotificationReply(stream, notification.Id, "", fmt.Errorf("Error reloading firewall, invalid rules")) + return + } + if err := firewall.SaveConfiguration(sysfw); err != nil { + c.sendNotificationReply(stream, notification.Id, "", fmt.Errorf("Error saving system firewall rules: %s", err)) + return + } + // TODO: + // - add new API endpoints to delete, add or change rules atomically. + // - a global goroutine where errors can be sent to the server (GUI). + go func(c *Client) { + var errors string + for { + select { + case fwerr := <-firewall.ErrorsChan(): + errors = fmt.Sprint(errors, fwerr, ",") + if firewall.ErrChanEmpty() { + goto ExitWithError + } + + // FIXME: can this operation last longer than 2s? if there're more than.. 100...10000 rules? + case <-time.After(2 * time.Second): + log.Debug("[notification] reload firewall. timeout fired, no errors?") + c.sendNotificationReply(stream, notification.Id, "", nil) + goto Exit + + } + } + ExitWithError: + c.sendNotificationReply(stream, notification.Id, "", fmt.Errorf("%s", errors)) + Exit: + }(c) + +} + func (c *Client) handleNotification(stream protocol.UI_NotificationsClient, notification *protocol.Notification) { switch { case notification.Type == protocol.Action_MONITOR_PROCESS: @@ -224,19 +265,7 @@ func (c *Client) handleNotification(stream protocol.UI_NotificationsClient, noti c.sendNotificationReply(stream, notification.Id, "", nil) case notification.Type == protocol.Action_RELOAD_FW_RULES: - log.Info("[notification] reloading firewall") - - sysfw, err := firewall.Deserialize(notification.SysFirewall) - if err != nil { - log.Warning("firewall.Deserialize() error: %s", err) - c.sendNotificationReply(stream, notification.Id, "", fmt.Errorf("Error reloading firewall, invalid rules")) - return - } - if err := firewall.SaveConfiguration(sysfw); err != nil { - c.sendNotificationReply(stream, notification.Id, "", fmt.Errorf("Error saving system firewall rules: %s", err)) - return - } - c.sendNotificationReply(stream, notification.Id, "", nil) + c.handleActionReloadFw(stream, notification) // ENABLE_RULE just replaces the rule on disk case notification.Type == protocol.Action_ENABLE_RULE: