Skip to content

Compare IP fact rules with address facts by address instead of regex prefix - #3425

Open
bacescuandrei wants to merge 1 commit into
apache:masterfrom
bacescuandrei:fix/ip-rule-exact-match
Open

bacescuandrei wants to merge 1 commit into
apache:masterfrom
bacescuandrei:fix/ip-rule-exact-match

Conversation

@bacescuandrei

Copy link
Copy Markdown

Description

The fieldmanual documents a rule's match as a regex. RuleSet also has IP-specific handling: _is_ip_rule_match compares addresses and subnets, and its docstring says only string-equivalent addresses should match. However, is_fact_allowed() still evaluates the rule as re.match(rule.match, fact.value) whenever that IP comparison does not match. An IPv4 address is a regex prefix of longer addresses, so:

  • DENY .* followed by ALLOW 10.0.0.1 also allows 10.0.0.10–10.0.0.19 and 10.0.0.100–10.0.0.199. Rules bound the fact values the planner may use, so an allow-listed host widens the operation's scope.
  • DENY 127.0.0.1 also denies 127.0.0.10.

This change makes IP-shaped rules compare as addresses when the fact value is an IPv4 address. The rule can be an address or a network. In that case the IP comparison decides alone and the regex fallback is not used.

Behaviour change: DENY 127.0.0.1 no longer denies 127.0.0.10, and ALLOW 10.0.0.1 no longer allows 10.0.0.15.

Unchanged:

  • Facts whose value is a network (CIDR) keep the existing regex fallback, so a DENY address rule still blocks a network fact that covers it. For example, DENY 10.0.0.1 still denies 10.0.0.1/32, and DENY 10.0.0.0 still denies 10.0.0.0/24.
  • Regex rules, including .*, are unchanged.
  • Subnet rules against address facts behave as before.

Users who relied on prefix matching with an IP-shaped pattern can keep that behaviour with an explicit regex, for example 10\.0\.0\.1\d*.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • This change requires a documentation update. The fieldmanual "Rules" section could note that IP-shaped matches compare as addresses; the fieldmanual is a separate submodule repository.

How Has This Been Tested?

  • Added four tests to tests/utility/test_ip_rule.py:

    • an address rule does not match a longer address;
    • ALLOW of a single address after DENY .*;
    • address rules still deny covering network facts;
    • an explicit regex rule still applies to IP facts.
  • On master, the first two fail and the other two pass; on this branch all pass.

  • I compared allow/deny decisions on master and on this branch for these cases:

    • address rules against longer addresses, the same address, /32 and /24 facts;
    • DENY 0.0.0.0/1 against 0.0.0.0/16;
    • DENY .*; ALLOW 10.0.0.0 against 10.0.0.0/24;
    • a subnet DENY against an address;
    • a subnet ALLOW after DENY .*.

    Only the two address-against-longer-address rows change.

  • Full suite on Python 3.12 (python -m pytest --asyncio-mode=auto tests): 565 passed, 1 xfailed. Master: 561 passed, 1 xfailed.

  • flake8 7.0.0 with flake8-bugbear: clean on the changed files.

  • bandit -r app -ll --skip=B303: no issues.

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have made corresponding changes to the documentation (fieldmanual is a separate repository)
  • I have added tests that prove my fix is effective or that my feature works

…refix

RuleSet.is_fact_allowed() fell back to re.match() whenever the IP
comparison did not match. An address rule is a valid regex that also
matches longer addresses, so ALLOW 10.0.0.1 after DENY .* also allowed
10.0.0.10-19 and 10.0.0.100-199, and DENY 127.0.0.1 also denied
127.0.0.10.

When the rule is an IPv4 address or network and the fact value is an
IPv4 address, decide by the IP comparison only. Facts that are networks
keep the regex fallback, so a DENY address rule still blocks a network
fact that covers it. Regex rules, including .*, are unchanged.
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.

1 participant