Skip to content

Commit

Permalink
northd: Add feature to log reply and related ACL traffic.
Browse files Browse the repository at this point in the history
It can be desirable for replies to stateful ACLs to be logged. And in
some cases, it can actually be a bit confusing why they aren't logged.
Consider a situation where a port group called "port_group" exists and
logical switch ports swp1 and swp2 belong to it. We create the following
ACL, where logging is enabled:

from-lport 100 'inport == @port_group' allow-stateless

swp1 sends traffic to swp2 and swp2 responds within the same connection.
In this case, the logs will show both the packets from swp1 to swp2, as
well as the response packets from swp2 to swp1. This is because the
traffic matches the ACL in both directions.

Now change the ACL:

from-lport 100 'inport == @port_group' allow-related

Now with the same traffic pattern, the packets from swp1 to swp2 are
logged, but the reply packets from swp2 to swp1 are not. Why is that?

The reason is that as a shortcut, when stateful ACLs are used, reply
traffic bypasses ACL evaluation. Therefore, even though it may appear
that the reply should match the ACL, the ACL is never even evaluated,
and therefore is not logged.

With this change, reply traffic still bypasses ACL evaluation, but the
reply can still be logged.

Since logging reply traffic requires adding more flows, it is not
enabled by default. In order to have reply traffic logged, the ACL
must have logging enabled, be stateful, have a label, and have the new
log-related option set to true.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2031150

Signed-off-by: Mark Michelson <mmichels@redhat.com>
Reviewed-by: Frode Nordahl <frode.nordahl@canonical.com>
Acked-by: Numan Siddique <numans@ovn.org>
Acked-by: Han Zhou <hzhou@ovn.org>
  • Loading branch information
putnopvut committed Feb 25, 2022
1 parent c6d5623 commit b988d5f
Show file tree
Hide file tree
Showing 9 changed files with 803 additions and 4 deletions.
4 changes: 4 additions & 0 deletions NEWS
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,10 @@ Post v21.12.0
pipeline) too.
- Introduce exclude-lb-vips-from-garp in logical_switch_port column in order
to not advertise lbs VIPs in GARPs sent by the logical router.
- ACLs now have an "options" column for configuration of extra options.
- A new ACL option, "log-related" has been added that allows for reply and
related traffic to be logged for an ACL in addition to the traffic that
directly matches the ACL.

OVN v21.12.0 - 22 Dec 2021
--------------------------
Expand Down
43 changes: 43 additions & 0 deletions northd/northd.c
Original file line number Diff line number Diff line change
Expand Up @@ -6265,6 +6265,49 @@ consider_acl(struct hmap *lflows, struct ovn_datapath *od,
acl->priority + OVN_ACL_PRI_OFFSET,
ds_cstr(match), ds_cstr(actions),
&acl->header_);

/* Related and reply traffic are universally allowed by priority
* 65532 flows created in build_acls(). If logging is enabled on
* the ACL, then we need to ensure that the related and reply
* traffic is logged, so we install a slightly higher-priority
* flow that matches the ACL, allows the traffic, and logs it.
*/
bool log_related = smap_get_bool(&acl->options, "log-related",
false);
if (acl->log && acl->label && log_related) {
/* Related/reply flows need to be set on the opposite pipeline
* from where the ACL itself is set.
*/
enum ovn_stage log_related_stage = ingress ?
S_SWITCH_OUT_ACL :
S_SWITCH_IN_ACL;
ds_clear(match);
ds_clear(actions);

ds_put_format(match, "ct.est && !ct.rel && !ct.new%s && "
"ct.rpl && ct_label.blocked == 0 && "
"ct_label.label == %" PRId64,
use_ct_inv_match ? " && !ct.inv" : "",
acl->label);
build_acl_log(actions, acl, meter_groups);
ds_put_cstr(actions, "next;");
ovn_lflow_add_with_hint(lflows, od, log_related_stage,
UINT16_MAX - 2,
ds_cstr(match), ds_cstr(actions),
&acl->header_);

ds_clear(match);
ds_put_format(match, "!ct.est && ct.rel && !ct.new%s && "
"ct_label.blocked == 0 && "
"ct_label.label == %" PRId64,
use_ct_inv_match ? " && !ct.inv" : "",
acl->label);
ovn_lflow_add_with_hint(lflows, od, log_related_stage,
UINT16_MAX - 2,
ds_cstr(match), ds_cstr(actions),
&acl->header_);
}

}
} else if (!strcmp(acl->action, "drop")
|| !strcmp(acl->action, "reject")) {
Expand Down
8 changes: 7 additions & 1 deletion northd/ovn-northd.8.xml
Original file line number Diff line number Diff line change
Expand Up @@ -795,14 +795,20 @@
go through the flows that implement the currently defined
policy based on ACLs. If a connection is no longer allowed by
policy, <code>ct_label.blocked</code> will get set and packets in the
reply direction will no longer be allowed, either.
reply direction will no longer be allowed, either. If ACL logging
and logging of related packets is enabled, then a companion priority-
65533 flow will be installed that accomplishes the same thing but
also logs the traffic.
</li>

<li>
A priority-65532 flow that allows any traffic that is considered
related to a committed flow in the connection tracker (e.g., an
ICMP Port Unreachable from a non-listening UDP port), as long
as the committed flow does not have <code>ct_label.blocked</code> set.
If ACL logging and logging of related packets is enabled, then a
companion priority-65533 flow will be installed that accomplishes the
same thing but also logs the traffic.
</li>

<li>
Expand Down
9 changes: 7 additions & 2 deletions ovn-nb.ovsschema
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
{
"name": "OVN_Northbound",
"version": "6.0.0",
"cksum": "1994796624 31020",
"version": "6.1.0",
"cksum": "4010776751 31237",
"tables": {
"NB_Global": {
"columns": {
Expand Down Expand Up @@ -267,6 +267,11 @@
"label": {"type": {"key": {"type": "integer",
"minInteger": 0,
"maxInteger": 4294967295}}},
"options": {
"type": {"key": "string",
"value": "string",
"min": 0,
"max": "unlimited"}},
"external_ids": {
"type": {"key": "string", "value": "string",
"min": 0, "max": "unlimited"}}},
Expand Down
23 changes: 23 additions & 0 deletions ovn-nb.xml
Original file line number Diff line number Diff line change
Expand Up @@ -2079,6 +2079,29 @@
</group>

<group title="Common Columns">
<column name="options">
This column provides general key/value settings. The supported
options are described individually below.
</column>

<group title="ACL configuration options">
<column name="options" key="log-related">
If set to <code>true</code>, then log when reply or related
traffic is admitted from a stateful ACL. In order for this
option to function, the <ref column="log"/> option must be
set to <code>true</code> and a <ref column="label"/> must
be set, and it must be unique to the ACL. The label is necessary
as it is the only means to associate the reply traffic with the
ACL to which it belongs. It must be unique, because otherwise it
is ambiguous which ACL will be matched.

Note: If this option is enabled, an extra flow is installed in
order to log the related traffic. Therefore, if this is enabled
on all ACLs, then the total number of flows necessary to log the
ACL traffic is doubled, compared to if this option is not enabled.
</column>
</group>

<column name="external_ids">
See <em>External IDs</em> at the beginning of this document.
</column>
Expand Down
3 changes: 2 additions & 1 deletion tests/automake.mk
Original file line number Diff line number Diff line change
Expand Up @@ -270,7 +270,8 @@ tests_ovstest_LDADD = $(OVS_LIBDIR)/daemon.lo \
CHECK_PYFILES = \
tests/test-l7.py \
tests/uuidfilt.py \
tests/test-tcp-rst.py
tests/test-tcp-rst.py \
tests/check_acl_log.py

EXTRA_DIST += $(CHECK_PYFILES)
PYCOV_CLEAN_FILES += $(CHECK_PYFILES:.py=.py,cover) .coverage
Expand Down
107 changes: 107 additions & 0 deletions tests/check_acl_log.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
#!/usr/bin/env python3
import argparse
import string


def strip(val):
"""Strip whitespace and quotation marks from val"""
return val.strip(f"{string.whitespace}\"'")


def parse_acl_log(line):
"""Convert an ACL log string into a dict"""
# First cut off the logging preamble.
# We're assuming the default log format.
acl_log = {}
_, _, details = line.rpartition("|")

# acl_details are things like the acl name, direction,
# verdict, and severity. packet_details are things like
# the protocol, addresses, and ports of the packet being
# logged.
acl_details, _, packet_details = details.partition(":")
for datum in acl_details.split(","):
name, _, value = datum.rpartition("=")
acl_log[strip(name)] = strip(value)

for datum in packet_details.split(","):
name, _, value = datum.rpartition("=")
if not name:
# The protocol is not preceded by "protocol="
# so we need to add it manually.
name = "protocol"
acl_log[strip(name)] = strip(value)

return acl_log


def get_acl_log(entry_num=1):
with open("ovn-controller.log", "r") as controller_log:
acl_logs = [line for line in controller_log if "acl_log" in line]
try:
return acl_logs[entry_num - 1]
except IndexError:
print(
f"There were not {entry_num} acl_log entries, \
only {len(acl_logs)}"
)
exit(1)


def add_parser_args(parser):
parser.add_argument("--entry-num", type=int, default=1)

# There are other possible things that can be in an ACL log,
# and if we need those in the future, we can add them later.
parser.add_argument("--name")
parser.add_argument("--verdict")
parser.add_argument("--severity")
parser.add_argument("--protocol")
parser.add_argument("--vlan_tci")
parser.add_argument("--dl_src")
parser.add_argument("--dl_dst")
parser.add_argument("--nw_src")
parser.add_argument("--nw_dst")
parser.add_argument("--nw_tos")
parser.add_argument("--nw_ecn")
parser.add_argument("--nw_ttl")
parser.add_argument("--icmp_type")
parser.add_argument("--icmp_code")
parser.add_argument("--tp_src")
parser.add_argument("--tp_dst")
parser.add_argument("--tcp_flags")
parser.add_argument("--ipv6_src")
parser.add_argument("--ipv6_dst")


def main():
parser = argparse.ArgumentParser()
add_parser_args(parser)
args = parser.parse_args()

acl_log = get_acl_log(args.entry_num)
parsed_log = parse_acl_log(acl_log)

# Express command line arguments as a dict, omitting any arguments that
# were not provided by the user.
expected = {k: v for k, v in vars(args).items() if v is not None}
del expected["entry_num"]

for key, val in expected.items():
try:
if parsed_log[key] != val:
print(
f"Expected log {key}={val} but got {key}={parsed_log[key]} \
in:\n\t'{acl_log}"
)
exit(1)
except KeyError:
print(
f"Expected log {key}={val} but {key} does not exist \
in:\n\t'{acl_log}'"
)
exit(1)


if __name__ == "__main__":
main()

0 comments on commit b988d5f

Please sign in to comment.