Duplication of information inside list operand makes handwriting rules difficult #1047

stusmall · 2023-09-29T10:46:02Z

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:

Load opensnitch UI with the above included nix generated ruled called "Allow NTP".
click on the rules tab
select the rule
click edit
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!

The text was updated successfully, but these errors were encountered:

gustavo-iniguez-goya · 2023-10-04T13:18:03Z

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 · 2023-10-04T13:32:40Z

@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 · 2023-10-04T21:41:56Z

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 · 2023-10-09T15:34:53Z

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 · 2023-10-09T15:38:09Z

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

Operator list was not converted to JSON string when saving the rule to the DB. Related: #1047

Previously when creating a new rule we followed these steps: - Create a new protobuf Rule object from the ruleseditor or the pop-ups. - If the rule contained more than one operator, we converted the list of operators to a JSON string. - This JSON string was sent back to the daemon, and saved to the DB. - The list of operators were never expanded on the GUI, i.e., they were not saved as a list of protobuf Operator objects. - Once received in the daemon, the JSON string was parsed and converted to a protobuf Operator list of objects. Both, the JSON string and the list of protobuf Operator objects were saved to disk, but the JSON string was ignored when loading the rules. Saving the list of operators as a JSON string was a problem if you wanted to create or modify rules without the GUI. Now when creating or modifying rules from the GUI, the list of operators is no longer converted to JSON string. Instead the list is sent to the daemon as a list of protobuf Operators, and saved as JSON objects. Notes: - The JSON string is no longer saved to disk as part of the rules. - The list of operators is still saved as JSON string to the DB. - About not enabled rules: Previously, not enabled rules only had the list of operators as JSON string, with the field list:[] empty. Now the list of operators is saved as JSON objects, but if the rule is not enabled, it won't be parsed/loaded. Closes #1047 (cherry picked from commit b930510)

Operator list was not converted to JSON string when saving the rule to the DB. Related: #1047 (cherry picked from commit 6714926)

gustavo-iniguez-goya closed this as completed in b930510 Oct 9, 2023

gustavo-iniguez-goya added a commit that referenced this issue Oct 10, 2023

ui,ruleseditor: add rules correctly to the db

6714926

Operator list was not converted to JSON string when saving the rule to the DB. Related: #1047

gustavo-iniguez-goya added a commit that referenced this issue Jun 27, 2024

ui,ruleseditor: add rules correctly to the db

a6fe895

Operator list was not converted to JSON string when saving the rule to the DB. Related: #1047 (cherry picked from commit 6714926)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Duplication of information inside list operand makes handwriting rules difficult #1047

Duplication of information inside list operand makes handwriting rules difficult #1047

stusmall commented Sep 29, 2023 •

edited

Loading

gustavo-iniguez-goya commented Oct 4, 2023

stusmall commented Oct 4, 2023 •

edited

Loading

gustavo-iniguez-goya commented Oct 4, 2023

gustavo-iniguez-goya commented Oct 9, 2023

stusmall commented Oct 9, 2023

Duplication of information inside list operand makes handwriting rules difficult #1047

Duplication of information inside list operand makes handwriting rules difficult #1047

Comments

stusmall commented Sep 29, 2023 • edited Loading

gustavo-iniguez-goya commented Oct 4, 2023

stusmall commented Oct 4, 2023 • edited Loading

gustavo-iniguez-goya commented Oct 4, 2023

gustavo-iniguez-goya commented Oct 9, 2023

stusmall commented Oct 9, 2023

stusmall commented Sep 29, 2023 •

edited

Loading

stusmall commented Oct 4, 2023 •

edited

Loading