Duplication of information inside list operand makes handwriting rules difficult

[stusmall]

Quick description of the bug The heart of the issue is that the UI duplicates data in created list rules and uses the data in two different contexts. This makes managing JSON rules outside the UI either error prone or will crash the UI rule builder.

Detailed description of the bug If a rule is disabled the operators inside of a list will only be included as a serialized JSON string inside data. When the rules is enabled the data will still be included as a serialized JSON string on data but also as a JSON list in the list property. For example see these two rules:

{
  "created": "2023-09-29T04:08:53.437867406-06:00",
  "updated": "2023-09-29T04:08:53.437875749-06:00",
  "name": "example",
  "enabled": false,
  "precedence": false,
  "action": "allow",
  "duration": "always",
  "operator": {
    "type": "list",
    "operand": "list",
    "sensitive": false,
    "data": "[{\"type\": \"simple\", \"operand\": \"process.path\", \"data\": \"something\", \"sensitive\": false}, {\"type\": \"simple\", \"operand\": \"dest.ip\", \"data\": \"1.2.3.4\", \"sensitive\": false}]",
    "list": []
  }
}

and

{
  "created": "2023-09-29T04:19:04.242958133-06:00",
  "updated": "2023-09-29T04:19:04.242983423-06:00",
  "name": "example",
  "enabled": true,
  "precedence": false,
  "action": "allow",
  "duration": "always",
  "operator": {
    "type": "list",
    "operand": "list",
    "sensitive": false,
    "data": "[{\"type\": \"simple\", \"operand\": \"process.path\", \"data\": \"something\", \"sensitive\": false}, {\"type\": \"simple\", \"operand\": \"dest.ip\", \"data\": \"1.2.3.4\", \"sensitive\": false}]",
    "list": [
      {
        "type": "simple",
        "operand": "process.path",
        "sensitive": false,
        "data": "something",
        "list": null
      },
      {
        "type": "simple",
        "operand": "dest.ip",
        "sensitive": false,
        "data": "1.2.3.4",
        "list": null
      }
    ]
  }
}

where the only difference in the UI is if enabled is selected.

From experimentation it appears that the UI is using the contents of data to populate the rule builder but the daemon uses the contents of list to evaluate the rule.

The issue for me as a user is because I use NixOS. All opensnitch rules are written in the nix language and then translated into JSON on a system rebuild. This gives me the choice of trying to replicate the UI's behavior when writing rules or to keep it DRY. When replicating the UI's behavior I found it to be error prone trying to keep the list and data fields in sync.

I've been going the route where a list operator leaves data null and only puts the nested rules under list. For example the following nix rule:

      rule-004-ntp = {
        name = "Allow NTP";
        enabled = true;
        action = "allow";
        duration = "always";
        operator = {
          type = "list";
          operand = "list";
          list = [
            {
              type ="simple";
              sensitive = false;
              operand = "process.path";
              data = "${lib.getBin pkgs.systemd}/lib/systemd/systemd-timesyncd";
            }
            {
              type = "regexp";
              operand = "dest.host";
              data = ".*\\.nixos\\.pool\\.ntp\\.org";
            }
          ];
        };
      };

produces the following JSON

{
  "action": "allow",
  "duration": "always",
  "enabled": true,
  "name": "Allow NTP",
  "operator": {
    "list": [
      {
        "data": "/nix/store/9gzw98jc64qkwd17a6qqm63w25zysi57-systemd-253.6/lib/systemd/systemd-timesyncd",
        "operand": "process.path",
        "sensitive": false,
        "type": "simple"
      },
      {
        "data": ".*\\.nixos\\.pool\\.ntp\\.org",
        "operand": "dest.host",
        "type": "regexp"
      }
    ],
    "operand": "list",
    "type": "list"
  }
}

This rule behaves exactly how I want it to except it will crash the UI if I ever try to edit the rule. The stack trace from this crash is included later. This isn't the end of the world. The program behaves mostly as it should. When managing rules with nix I usually don't want to also manage it in the UI. It's just the UI is helpful for troubleshooting or finetuning rules.

System Info

• OpenSnitch version: 1.5.2
• OS: NixOS
• Version: 23.05
• Window Manager: Gnome
• Kernel version: Linux nixos 6.5.5 #1-NixOS SMP PREEMPT_DYNAMIC Sat Sep 23 09:14:39 UTC 2023 x86_64 GNU/Linux

To Reproduce Steps to reproduce the behavior:

1. Load opensnitch UI with the above included nix generated ruled called "Allow NTP".
2. click on the rules tab
3. select the rule
4. click edit
5. It crashes

Post error logs:

Themes not available. Install qt-material if you want to change GUI's appearance: pip3 install qt-material.
Loading translations: /nix/store/jj920ac1p6ys61iybmg8sidqq6nmvi5x-opensnitch-ui-1.5.2/lib/python3.10/site-packages/opensnitch/i18n locale: en_US
Warning: Ignoring XDG_SESSION_TYPE=wayland on Gnome. Use QT_QPA_PLATFORM=wayland to run on Wayland anyway.
using IPASN DB: /nix/store/i66m2s6gwahwbwz4mc1wm60ba4w379b6-python3.10-pyasn-1.6.1/lib/python3.10/site-packages/pyasn/data/ipasn_20140513_v12.dat.gz
new node connected, listening for client responses... /local
Traceback (most recent call last):
  File "/nix/store/jj920ac1p6ys61iybmg8sidqq6nmvi5x-opensnitch-ui-1.5.2/lib/python3.10/site-packages/opensnitch/dialogs/stats.py", line 1283, in _cb_edit_rule_clicked
    self._rules_dialog.edit_rule(records, self.nodeRuleLabel.text())
  File "/nix/store/jj920ac1p6ys61iybmg8sidqq6nmvi5x-opensnitch-ui-1.5.2/lib/python3.10/site-packages/opensnitch/dialogs/ruleseditor.py", line 803, in edit_rule
    if self._load_rule(addr=_addr, rule=self.rule):
  File "/nix/store/jj920ac1p6ys61iybmg8sidqq6nmvi5x-opensnitch-ui-1.5.2/lib/python3.10/site-packages/opensnitch/dialogs/ruleseditor.py", line 350, in _load_rule
    rule_options = json.loads(self.rule.operator.data)
  File "/nix/store/bc45k1n0pkrdkr3xa6w84w1xhkl1kkyp-python3-3.10.12/lib/python3.10/json/__init__.py", line 346, in loads
    return _default_decoder.decode(s)
  File "/nix/store/bc45k1n0pkrdkr3xa6w84w1xhkl1kkyp-python3-3.10.12/lib/python3.10/json/decoder.py", line 337, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "/nix/store/bc45k1n0pkrdkr3xa6w84w1xhkl1kkyp-python3-3.10.12/lib/python3.10/json/decoder.py", line 355, in raw_decode
    raise JSONDecodeError("Expecting value", s, err.value) from None
json.decoder.JSONDecodeError: Expecting value: line 1 column 1 (char 0)
Aborted (core dumped)

Additional context I just wanted to save massive thank you for this project! It's extremely well made and useful!

[gustavo-iniguez-goya]

Hi @stusmall ,

Yes, you're correct. This has been tagged as FIXME for some years now, and since we're introducing some unstable changes maybe it's time to rewrite it.

[stusmall]

@gustavo-iniguez-goya is there any known risk in continuing to declare rules in this way? Do you know of any impact it will have besides crashing the rule builder UI?

Also is there an existing issue I can point to and close this as a dupe? Or should I leave this issue open?

[gustavo-iniguez-goya]

The daemon loads the rule from disk, and it only parses the List field, it ignores the "data" field of the operator. So it should work just fine. But take a look at the logs, if there's any problem loading or saving the rules there should be an ERR log.

Let's keep this issue open. There was a request to always expand the json encoded data in the List json field, but I haven't found the issue number.

[gustavo-iniguez-goya]

hey @stusmall , I've changed the behaviour to no longer convert operator list to a JSON string. The conversion is still needed on the GUI side, in order to save it to the DB, because the DB is not normalized.

I'd have preferred to defuse a bomb rather than changing this part of the app :)

Could you test that everything keeps working as expected?

[stusmall]

Will do! Next chance I can I'll pull down, build and test it. It'll probably be pretty late in the week until I can