| Message ID | 20241204215246.642636-1-mmichels@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev,1/3] northd: Add "allow-established" ACL action. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Wed, Dec 4, 2024 at 10:53 PM Mark Michelson <mmichels@redhat.com> wrote: > "allow-established" ACLs are identical to allow-related ACLs, with one > exception. On allow-related ACLs, if the ACL changes and no longer > matches an established session, then traffic will no longer > automatically be allowed. Instead, traffic on the established session > will be subject to ACL matching, and therefore the traffic may be > dropped. > > For allow-established ACLs, this behavior is altered. We set a specific > bit in the ct_mark to indicate this is an allow-established ACL. If this > bit is set, then established session traffic will always be allowed, > even if the ACL is altered to no longer match the traffic. > > Upcoming commits will put in place methods so that deleting the ACL or > changing the action type on the ACL will remove the conntrack entry that > allows the established traffic. > > Signed-off-by: Mark Michelson <mmichels@redhat.com> > --- > Hi Mark, thank you for the patch, I have a few minor comments down below. > include/ovn/logical-fields.h | 1 + > lib/logical-fields.c | 5 ++ > northd/en-ls-stateful.c | 3 +- > northd/northd.c | 36 +++++++- > ovn-nb.ovsschema | 7 +- > ovn-nb.xml | 11 +++ > tests/automake.mk | 4 +- > tests/client.py | 38 ++++++++ > tests/ovn-northd.at | 134 +++++++++++++++++++++++++++++ > tests/server.py | 30 +++++++ > tests/system-ovn.at | 162 +++++++++++++++++++++++++++++++++++ > 11 files changed, 424 insertions(+), 7 deletions(-) > create mode 100755 tests/client.py > create mode 100755 tests/server.py > > diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h > index d563e044c..c7e4baa51 100644 > --- a/include/ovn/logical-fields.h > +++ b/include/ovn/logical-fields.h > @@ -203,6 +203,7 @@ const struct ovn_field *ovn_field_from_name(const char > *name); > #define OVN_CT_LB_FORCE_SNAT_BIT 3 > #define OVN_CT_OBS_STAGE_1ST_BIT 4 > #define OVN_CT_OBS_STAGE_END_BIT 5 > +#define OVN_CT_ALLOW_ESTABLISHED_BIT 6 > > #define OVN_CT_BLOCKED 1 > #define OVN_CT_NATTED 2 > diff --git a/lib/logical-fields.c b/lib/logical-fields.c > index 5a8b53f2b..5b578b5c2 100644 > --- a/lib/logical-fields.c > +++ b/lib/logical-fields.c > @@ -171,6 +171,11 @@ ovn_init_symtab(struct shash *symtab) > OVN_CT_STR(OVN_CT_OBS_STAGE_END_BIT) > "]", > WR_CT_COMMIT); > + expr_symtab_add_subfield_scoped(symtab, "ct_mark.allow_established", > NULL, > + "ct_mark[" > + > OVN_CT_STR(OVN_CT_ALLOW_ESTABLISHED_BIT) > + "]", > + WR_CT_COMMIT); > expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_collector_id", > NULL, > "ct_mark[16..23]", WR_CT_COMMIT); > > diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c > index 44f74ea08..1dc8ce01f 100644 > --- a/northd/en-ls-stateful.c > +++ b/northd/en-ls-stateful.c > @@ -412,7 +412,8 @@ ls_stateful_record_set_acl_flags_(struct > ls_stateful_record *ls_stateful_rec, > ls_stateful_rec->max_acl_tier = acl->tier; > } > if (!ls_stateful_rec->has_stateful_acl > - && !strcmp(acl->action, "allow-related")) { > + && (!strcmp(acl->action, "allow-related") > + || !strcmp(acl->action, "allow-established"))) { > nit: Formatting ls_stateful_rec->has_stateful_acl = true; > } > if (ls_stateful_rec->has_stateful_acl && > diff --git a/northd/northd.c b/northd/northd.c > index 3a488ff3d..0aae627a2 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -124,6 +124,7 @@ static bool vxlan_mode; > #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" > #define REGBIT_FROM_ROUTER_PORT "reg0[18]" > #define REGBIT_IP_FRAG "reg0[19]" > +#define REGBIT_ACL_PERSIST_ID "reg0[20]" > > #define REG_ORIG_DIP_IPV4 "reg1" > #define REG_ORIG_DIP_IPV6 "xxreg1" > @@ -7121,7 +7122,8 @@ consider_acl(struct lflow_table *lflows, const > struct ovn_datapath *od, > } > > if (!strcmp(acl->action, "allow") > - || !strcmp(acl->action, "allow-related")) { > + || !strcmp(acl->action, "allow-related") > + || !strcmp(acl->action, "allow-established")) { > /* If there are any stateful flows, we must even commit "allow" > * actions. This is because, while the initiater's > * direction may not have any stateful rules, the server's > @@ -7147,6 +7149,11 @@ consider_acl(struct lflow_table *lflows, const > struct ovn_datapath *od, > ds_truncate(actions, log_verdict_len); > ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); > > + if (!strcmp(acl->action, "allow-established")) { > + ds_put_format(actions, > + REGBIT_ACL_PERSIST_ID " = 1; "); > + } > + > /* For stateful ACLs sample "new" and "established" packets. */ > build_acl_sample_label_action(actions, acl, acl->sample_new, > acl->sample_est, obs_stage); > @@ -7626,6 +7633,26 @@ build_acls(const struct ls_stateful_record > *ls_stateful_rec, > REGBIT_ACL_HINT_ALLOW_REL" == 1", > REGBIT_ACL_VERDICT_ALLOW " = 1; next;", > lflow_ref); > + > + /* Ingress and egress ACL Table (Priority 65535). > + * > + * Allow traffic that is established if the ACL has a persistent > + * conntrack ID configured. > + */ > + ds_clear(&match); > + ds_put_format(&match, "ct.est && ct_mark.allow_established == 1"); > + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_EVAL, UINT16_MAX, > + ds_cstr(&match), > + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", > + lflow_ref); > + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB_EVAL, > UINT16_MAX, > + ds_cstr(&match), > + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", > + lflow_ref); > + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL_EVAL, UINT16_MAX, > + ds_cstr(&match), > + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", > + lflow_ref); > } > > /* Ingress and Egress ACL Table (Priority 65532). > @@ -8362,6 +8389,7 @@ build_stateful(struct ovn_datapath *od, struct > lflow_table *lflows, > ds_put_cstr(&actions, > "ct_commit { " > "ct_mark.blocked = 0; " > + "ct_mark.allow_established = " REGBIT_ACL_PERSIST_ID > "; " > > nit: Extra space > "ct_mark.obs_stage = " REGBIT_ACL_OBS_STAGE "; " > "ct_mark.obs_collector_id = " > REG_OBS_COLLECTOR_ID_EST "; " > "ct_label.obs_point_id = " REG_OBS_POINT_ID_EST "; " > @@ -8382,7 +8410,11 @@ build_stateful(struct ovn_datapath *od, struct > lflow_table *lflows, > * any packet that makes it this far is part of a connection we > * want to allow to continue. */ > ds_clear(&actions); > - ds_put_cstr(&actions, "ct_commit { ct_mark.blocked = 0; }; next;"); > + ds_put_cstr(&actions, > + "ct_commit { " > + "ct_mark.blocked = 0; " > + "ct_mark.allow_established = " REGBIT_ACL_PERSIST_ID > "; " > nit: Extra space + "}; next;"); > ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, > REGBIT_CONNTRACK_COMMIT" == 1 && " > REGBIT_ACL_LABEL" == 0", > diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema > index c4a48183d..a02ea67b0 100644 > --- a/ovn-nb.ovsschema > +++ b/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "7.7.0", > - "cksum": "116357561 38626", > + "version": "7.8.0", > + "cksum": "640528173 38695", > "tables": { > "NB_Global": { > "columns": { > @@ -295,7 +295,8 @@ > "enum": ["set", > ["allow", "allow-related", > "allow-stateless", "drop", > - "reject", "pass"]]}}}, > + "reject", "pass", > + "allow-established"]]}}}, > "log": {"type": "boolean"}, > "severity": {"type": {"key": {"type": "string", > "enum": ["set", > diff --git a/ovn-nb.xml b/ovn-nb.xml > index 5114bbc2e..8f1ca302d 100644 > --- a/ovn-nb.xml > +++ b/ovn-nb.xml > @@ -2489,6 +2489,17 @@ or > (e.g. inbound replies to an outbound connection). > </li> > > + <li> > + <code>allow-established</code>: This is similar to > + <code>allow-related</code>. The difference is that if the ACL > + <code>match</code> is changed, then established traffic can > + still pass, even if the traffic does not match the ACL any > longer. > + When the ACL is deleted, or if the action changes to something > + other than <code>allow-established</code>, then the > established > + traffic is no longer automatically allowed. At that point, the > + traffic is subject to other configured ACLs. > + </li> > + > <li> > <code>drop</code>: Silently drop the packet. > </li> > diff --git a/tests/automake.mk b/tests/automake.mk > index 3899c9e80..940f5b923 100644 > --- a/tests/automake.mk > +++ b/tests/automake.mk > @@ -313,7 +313,9 @@ CHECK_PYFILES = \ > tests/uuidfilt.py \ > tests/test-tcp-rst.py \ > tests/check_acl_log.py \ > - tests/scapy-server.py > + tests/scapy-server.py \ > + tests/client.py \ > + tests/server.py > There are some flake8 errors in the new python files, please check it out: tests/client.py:4:1: F401 'os' imported but unused tests/client.py:24:80: E501 line too long (95 > 79 characters) tests/server.py:15:80: E501 line too long (80 > 79 characters) > > EXTRA_DIST += $(CHECK_PYFILES) > PYCOV_CLEAN_FILES += $(CHECK_PYFILES:.py=.py,cover) .coverage > diff --git a/tests/client.py b/tests/client.py > new file mode 100755 > index 000000000..7662347ff > --- /dev/null > +++ b/tests/client.py > @@ -0,0 +1,38 @@ > +#!/usr/bin/env python3 > + > +import socket > +import os > +import time > +import argparse > + > + > +def send_data_from_fifo_to_server( > + fifo_path='/tmp/myfifo', host='127.0.0.1', port=10000 > +): > + # Open the FIFO for reading (blocking mode) > + with open(fifo_path, 'r') as fifo_file: > + with socket.socket( > + socket.AF_INET, socket.SOCK_STREAM > + ) as client_socket: > + client_socket.connect((host, port)) > + # Continuously read from the FIFO and send to the server > + while True: > + data = fifo_file.readline().strip() > + if data: > + client_socket.sendall(data.encode()) > + else: > + # If no data is read (blocking mode), add a small > delay or handle as needed > + time.sleep(0.1) > + > + > +if __name__ == "__main__": > + parser = argparse.ArgumentParser() > + group = parser.add_argument_group() > + group.add_argument("-f", "--fifo_path") > + group.add_argument("-i", "--server-host") > + group.add_argument("-p", "--server-port", type=int) > + args = parser.parse_args() > + > + send_data_from_fifo_to_server( > + args.fifo_path, args.server_host, args.server_port > + ) > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 4eae1c67c..1d90622e1 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -14253,3 +14253,137 @@ AT_CHECK([grep "lr_in_dnat" lr1flows | > ovn_strip_lflows | grep "30.0.0.1"], [0], > > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([ACL persistent ID - Logical Flows]) > +ovn_start > + > +dnl For this test, we want to ensure that the logical flows for ACLs are > +dnl what we expect. > +dnl > +dnl First, we'll ensure that an ACL that is not "allow-established" sets > the > +dnl relevant CT values to 0. > +dnl > +dnl Then we'll change the ACL to be "allow-established" and ensure the > logical > +dnl flows do set the appropriate values. > +dnl > +dnl Then finally, we'll set the ACL back away from "allow-established" and > +dnl ensure that the logical flows set the relevant CT values to 0. > + > +check ovn-nbctl ls-add sw > + > +check ovn-nbctl acl-add sw from-lport 1001 "tcp" allow-related > +check ovn-nbctl acl-add sw to-lport 1002 "ip" allow-related > +check ovn-nbctl --apply-after-lb acl-add sw from-lport 1003 "udp" > allow-related > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep > priority=2001 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && > (tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && > (tcp)), action=(reg8[[16]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep > priority=2003 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == > 1 && (udp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == > 1 && (udp)), action=(reg8[[16]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep > priority=2002 | ovn_strip_lflows], [0], [dnl > + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && > (ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) > + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && > (ip)), action=(reg8[[16]] = 1; next;) > +]) > + > +ingress_uuid=$(fetch_column nb:ACL _uuid priority=1001) > +egress_uuid=$(fetch_column nb:ACL _uuid priority=1002) > +after_lb_uuid=$(fetch_column nb:ACL _uuid priority=1003) > + > +check ovn-nbctl set acl $ingress_uuid action=allow-established > +check ovn-nbctl set acl $egress_uuid action=allow-established > +check ovn-nbctl set acl $after_lb_uuid action=allow-established > + > +dnl Now we should see the registers being set to the appropriate values. > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep > priority=2001 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && > (tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) > + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && > (tcp)), action=(reg8[[16]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep > priority=2003 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == > 1 && (udp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == > 1 && (udp)), action=(reg8[[16]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep > priority=2002 | ovn_strip_lflows], [0], [dnl > + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && > (ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) > + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && > (ip)), action=(reg8[[16]] = 1; next;) > +]) > + > +dnl Now try the other ACL verdicts and ensure that they do not > +dnl try to set the values. > +for verdict in allow allow-stateless > +do > + echo "verdict is $verdict" > + check ovn-nbctl set acl $ingress_uuid action=$verdict > + check ovn-nbctl set acl $egress_uuid action=$verdict > + check ovn-nbctl set acl $after_lb_uuid action=$verdict > + > + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep > priority=2001 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), > action=(reg8[[16]] = 1; next;) > +]) > + > + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | > grep priority=2003 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), > action=(reg8[[16]] = 1; next;) > +]) > + > + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep > priority=2002 | ovn_strip_lflows], [0], [dnl > + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), > action=(reg8[[16]] = 1; next;) > +]) > +done > + > +check ovn-nbctl set acl $ingress_uuid action=drop > +check ovn-nbctl set acl $egress_uuid action=drop > +check ovn-nbctl set acl $after_lb_uuid action=drop > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep > priority=2001 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), > action=(reg8[[17]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep > priority=2003 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), > action=(reg8[[17]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep > priority=2002 | ovn_strip_lflows], [0], [dnl > + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), > action=(reg8[[17]] = 1; next;) > +]) > + > +check ovn-nbctl set acl $ingress_uuid action=reject > +check ovn-nbctl set acl $egress_uuid action=reject > +check ovn-nbctl set acl $after_lb_uuid action=reject > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep > priority=2001 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), > action=(reg8[[18]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep > priority=2003 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), > action=(reg8[[18]] = 1; next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep > priority=2002 | ovn_strip_lflows], [0], [dnl > + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), > action=(reg8[[18]] = 1; next;) > +]) > + > +check ovn-nbctl set acl $ingress_uuid action=pass > +check ovn-nbctl set acl $egress_uuid action=pass > +check ovn-nbctl set acl $after_lb_uuid action=pass > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep > priority=2001 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), > action=(next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep > priority=2003 | ovn_strip_lflows], [0], [dnl > + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), > action=(next;) > +]) > + > +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep > priority=2002 | ovn_strip_lflows], [0], [dnl > + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), > action=(next;) > +]) > + > +AT_CLEANUP > +]) > diff --git a/tests/server.py b/tests/server.py > new file mode 100755 > index 000000000..1eb378fd2 > --- /dev/null > +++ b/tests/server.py > @@ -0,0 +1,30 @@ > +#!/usr/bin/env python3 > + > +import socket > +import argparse > + > + > +def start_server(host='127.0.0.1', port=10000): > + # Create a TCP/IP socket > + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as > server_socket: > + server_socket.bind((host, port)) > + server_socket.listen() > + while True: > + client_socket, client_address = server_socket.accept() > + with client_socket: > + # Receive the data from the client in chunks and write to > a file > + data = client_socket.recv(1024) > + while data: > + with open("output.txt", "a") as f: > + f.write(data.decode() + "\n") > + data = client_socket.recv(1024) > + > + > +if __name__ == "__main__": > + parser = argparse.ArgumentParser() > + group = parser.add_argument_group() > + group.add_argument("-i", "--bind-host") > + group.add_argument("-p", "--bind-port", type=int) > + args = parser.parse_args() > + > + start_server(args.bind_host, args.bind_port) > diff --git a/tests/system-ovn.at b/tests/system-ovn.at > index 4452d5676..a6ddda0f7 100644 > --- a/tests/system-ovn.at > +++ b/tests/system-ovn.at > @@ -14117,5 +14117,167 @@ as > OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d > /failed to query port patch-.*/d > /.*terminating with signal 15.*/d"]) > + > +AT_CLEANUP > +]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([ACLs - persistent sessions]) > + > +CHECK_CONNTRACK() > +CHECK_CONNTRACK_NAT() > +ovn_start > +OVS_TRAFFIC_VSWITCHD_START() > +ADD_BR([br-int]) > + > +ovs-vsctl set-fail-mode br-ext standalone > +# Set external-ids in br-int needed for ovn-controller > +ovs-vsctl \ > + -- set Open_vSwitch . external-ids:system-id=hv1 \ > + -- set Open_vSwitch . > external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ > + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ > + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ > + -- set bridge br-int fail-mode=secure > other-config:disable-in-band=true > + > +start_daemon ovn-controller > + > +# For this test, we want to ensure that established traffic > +# is allowed on ACLs with allow-established. > +# > +# To start, we will set up allow-related ACLs. > +# We will send traffic and ensure it is allowed. Then we will adjust > +# the ACL so it no longer matches, and we will ensure that the traffic > +# is no longer allowed. > +# > +# Next, we will reset the ACL to its initial state, but we will also > +# change the ACL to be allow-established. We will flush conntrack, > +# and rerun the test exactly as before. The difference this time is > +# that after we adjust the ACL so it no longer matches, the traffic > +# should still be allowed. > + > +check ovn-nbctl ls-add sw > +check ovn-nbctl lsp-add sw swp1 -- lsp-set-addresses swp1 > "00:00:00:00:00:01 192.168.1.1" > +check ovn-nbctl lsp-add sw swp2 -- lsp-set-addresses swp2 > "00:00:00:00:00:02 192.168.1.2" > + > +ADD_NAMESPACES(swp1) > +ADD_VETH(swp1, swp1, br-int, "192.168.1.1/24", "00:00:00:00:00:01") > + > +ADD_NAMESPACES(swp2) > +ADD_VETH(swp2, swp2, br-int, "192.168.1.2/24", "00:00:00:00:00:02") > + > +# Start a TCP server on swp2. > +NETNS_DAEMONIZE(swp2, [server.py -i 192.168.1.2 -p 10000], [server.pid]) > + > +# Make a FIFO and send its output to a client > +# from swp1 > +mkfifo /tmp/myfifo > +on_exit 'rm -rf /tmp/myfifo' > + > +NETNS_DAEMONIZE(swp1, [client.py -f "/tmp/myfifo" -i 192.168.1.2 -p > 10000], [client.pid]) > + > +# First, ensure that we have basic connectivity before we even start > setting > +# up ACLs. > +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl > +]) > + > +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl > +test > +]) > + > +: > output.txt > + > +check ovn-nbctl acl-add sw from-lport 1000 'ip4.dst == 192.168.1.2' > allow-related > +check ovn-nbctl acl-add sw from-lport 0 '1' drop > + > +# Do another basic connectivity check to ensure the ACL is allowing > traffic as expected. > +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl > +]) > + > +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl > +test > +]) > + > +: > output.txt > + > +# At this point, I need to adjust the ACL so it no longer matches. We > then need > +# to ensure that the traffic does not pass. How we test this > is...interesting. I'm > +# not sure how to test for a negative condition accurately. > + > +acl_uuid=$(fetch_column nb:ACL _uuid priority=1000) > + > +# Update the ACL so that it no longer matches our client-server traffic > +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.3\"" > + > +# Send another packet from the client to the server. > +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl > +]) > + > +# The traffic should be blocked. We'll check the "drop" ACL to see if it > has > +# been hit. We can't predict the number of packets that will be seen, but > we know > +# it will be non-zero. > +oftable=$(ovn-debug lflow-stage-to-oftable ls_in_acl_eval) > +OVS_WAIT_FOR_OUTPUT([ovs-ofctl dump-flows br-int | grep table="$oftable" > | grep "priority=1000" | grep -v commit | grep -c "n_packets=[[1-9]]"], > [0], [dnl > +1 > +]) > + > +# And just to be safe, let's make sure the output file is still empty > +AT_CHECK([cat output.txt], [0], [dnl > +]) > + > +# Reset the client and server processes so that we create a new connection > +client_pid=$(cat client.pid) > +server_pid=$(cat server.pid) > +kill $server_pid > +kill $client_pid > +OVS_WAIT_WHILE([kill -0 $server_pid 2>/dev/null]) > +OVS_WAIT_WHILE([kill -0 $client_pid 2>/dev/null]) > + > +NETNS_DAEMONIZE(swp2, [server.py -i 192.168.1.2 -p 20000], [server.pid]) > +NETNS_DAEMONIZE(swp1, [client.py -f "/tmp/myfifo" -i 192.168.1.2 -p > 20000], [client.pid]) > + > +# Now we'll re-set the ACL to allow the traffic. > +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.2\"" > + > +# We'll also change to allow-established > +check ovn-nbctl set ACL $acl_uuid action=allow-established > + > +# Make sure traffic passes > +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl > +]) > + > +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl > +test > +]) > + > +: > output.txt > + > +# Adjust the ACL so that it no longer matches > +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.3\"" > + > +# Traffic should still pass > +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl > +]) > + > +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl > +test > +]) > + > +: > output.txt > + > +OVS_APP_EXIT_AND_WAIT([ovn-controller]) > + > +as ovn-sb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as ovn-nb > +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) > + > +as northd > +OVS_APP_EXIT_AND_WAIT([ovn-northd]) > + > +as > +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d > +/connection dropped.*/d"]) > + > AT_CLEANUP > ]) > -- > 2.45.2 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > With those addressed: Reviewed-by: Ales Musil <amusil@redhat.com> Thanks, Ales
diff --git a/include/ovn/logical-fields.h b/include/ovn/logical-fields.h index d563e044c..c7e4baa51 100644 --- a/include/ovn/logical-fields.h +++ b/include/ovn/logical-fields.h @@ -203,6 +203,7 @@ const struct ovn_field *ovn_field_from_name(const char *name); #define OVN_CT_LB_FORCE_SNAT_BIT 3 #define OVN_CT_OBS_STAGE_1ST_BIT 4 #define OVN_CT_OBS_STAGE_END_BIT 5 +#define OVN_CT_ALLOW_ESTABLISHED_BIT 6 #define OVN_CT_BLOCKED 1 #define OVN_CT_NATTED 2 diff --git a/lib/logical-fields.c b/lib/logical-fields.c index 5a8b53f2b..5b578b5c2 100644 --- a/lib/logical-fields.c +++ b/lib/logical-fields.c @@ -171,6 +171,11 @@ ovn_init_symtab(struct shash *symtab) OVN_CT_STR(OVN_CT_OBS_STAGE_END_BIT) "]", WR_CT_COMMIT); + expr_symtab_add_subfield_scoped(symtab, "ct_mark.allow_established", NULL, + "ct_mark[" + OVN_CT_STR(OVN_CT_ALLOW_ESTABLISHED_BIT) + "]", + WR_CT_COMMIT); expr_symtab_add_subfield_scoped(symtab, "ct_mark.obs_collector_id", NULL, "ct_mark[16..23]", WR_CT_COMMIT); diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c index 44f74ea08..1dc8ce01f 100644 --- a/northd/en-ls-stateful.c +++ b/northd/en-ls-stateful.c @@ -412,7 +412,8 @@ ls_stateful_record_set_acl_flags_(struct ls_stateful_record *ls_stateful_rec, ls_stateful_rec->max_acl_tier = acl->tier; } if (!ls_stateful_rec->has_stateful_acl - && !strcmp(acl->action, "allow-related")) { + && (!strcmp(acl->action, "allow-related") + || !strcmp(acl->action, "allow-established"))) { ls_stateful_rec->has_stateful_acl = true; } if (ls_stateful_rec->has_stateful_acl && diff --git a/northd/northd.c b/northd/northd.c index 3a488ff3d..0aae627a2 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -124,6 +124,7 @@ static bool vxlan_mode; #define REGBIT_ACL_HINT_ALLOW_REL "reg0[17]" #define REGBIT_FROM_ROUTER_PORT "reg0[18]" #define REGBIT_IP_FRAG "reg0[19]" +#define REGBIT_ACL_PERSIST_ID "reg0[20]" #define REG_ORIG_DIP_IPV4 "reg1" #define REG_ORIG_DIP_IPV6 "xxreg1" @@ -7121,7 +7122,8 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, } if (!strcmp(acl->action, "allow") - || !strcmp(acl->action, "allow-related")) { + || !strcmp(acl->action, "allow-related") + || !strcmp(acl->action, "allow-established")) { /* If there are any stateful flows, we must even commit "allow" * actions. This is because, while the initiater's * direction may not have any stateful rules, the server's @@ -7147,6 +7149,11 @@ consider_acl(struct lflow_table *lflows, const struct ovn_datapath *od, ds_truncate(actions, log_verdict_len); ds_put_cstr(actions, REGBIT_CONNTRACK_COMMIT" = 1; "); + if (!strcmp(acl->action, "allow-established")) { + ds_put_format(actions, + REGBIT_ACL_PERSIST_ID " = 1; "); + } + /* For stateful ACLs sample "new" and "established" packets. */ build_acl_sample_label_action(actions, acl, acl->sample_new, acl->sample_est, obs_stage); @@ -7626,6 +7633,26 @@ build_acls(const struct ls_stateful_record *ls_stateful_rec, REGBIT_ACL_HINT_ALLOW_REL" == 1", REGBIT_ACL_VERDICT_ALLOW " = 1; next;", lflow_ref); + + /* Ingress and egress ACL Table (Priority 65535). + * + * Allow traffic that is established if the ACL has a persistent + * conntrack ID configured. + */ + ds_clear(&match); + ds_put_format(&match, "ct.est && ct_mark.allow_established == 1"); + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_EVAL, UINT16_MAX, + ds_cstr(&match), + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", + lflow_ref); + ovn_lflow_add(lflows, od, S_SWITCH_IN_ACL_AFTER_LB_EVAL, UINT16_MAX, + ds_cstr(&match), + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", + lflow_ref); + ovn_lflow_add(lflows, od, S_SWITCH_OUT_ACL_EVAL, UINT16_MAX, + ds_cstr(&match), + REGBIT_ACL_VERDICT_ALLOW " = 1; next;", + lflow_ref); } /* Ingress and Egress ACL Table (Priority 65532). @@ -8362,6 +8389,7 @@ build_stateful(struct ovn_datapath *od, struct lflow_table *lflows, ds_put_cstr(&actions, "ct_commit { " "ct_mark.blocked = 0; " + "ct_mark.allow_established = " REGBIT_ACL_PERSIST_ID "; " "ct_mark.obs_stage = " REGBIT_ACL_OBS_STAGE "; " "ct_mark.obs_collector_id = " REG_OBS_COLLECTOR_ID_EST "; " "ct_label.obs_point_id = " REG_OBS_POINT_ID_EST "; " @@ -8382,7 +8410,11 @@ build_stateful(struct ovn_datapath *od, struct lflow_table *lflows, * any packet that makes it this far is part of a connection we * want to allow to continue. */ ds_clear(&actions); - ds_put_cstr(&actions, "ct_commit { ct_mark.blocked = 0; }; next;"); + ds_put_cstr(&actions, + "ct_commit { " + "ct_mark.blocked = 0; " + "ct_mark.allow_established = " REGBIT_ACL_PERSIST_ID "; " + "}; next;"); ovn_lflow_add(lflows, od, S_SWITCH_IN_STATEFUL, 100, REGBIT_CONNTRACK_COMMIT" == 1 && " REGBIT_ACL_LABEL" == 0", diff --git a/ovn-nb.ovsschema b/ovn-nb.ovsschema index c4a48183d..a02ea67b0 100644 --- a/ovn-nb.ovsschema +++ b/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", - "version": "7.7.0", - "cksum": "116357561 38626", + "version": "7.8.0", + "cksum": "640528173 38695", "tables": { "NB_Global": { "columns": { @@ -295,7 +295,8 @@ "enum": ["set", ["allow", "allow-related", "allow-stateless", "drop", - "reject", "pass"]]}}}, + "reject", "pass", + "allow-established"]]}}}, "log": {"type": "boolean"}, "severity": {"type": {"key": {"type": "string", "enum": ["set", diff --git a/ovn-nb.xml b/ovn-nb.xml index 5114bbc2e..8f1ca302d 100644 --- a/ovn-nb.xml +++ b/ovn-nb.xml @@ -2489,6 +2489,17 @@ or (e.g. inbound replies to an outbound connection). </li> + <li> + <code>allow-established</code>: This is similar to + <code>allow-related</code>. The difference is that if the ACL + <code>match</code> is changed, then established traffic can + still pass, even if the traffic does not match the ACL any longer. + When the ACL is deleted, or if the action changes to something + other than <code>allow-established</code>, then the established + traffic is no longer automatically allowed. At that point, the + traffic is subject to other configured ACLs. + </li> + <li> <code>drop</code>: Silently drop the packet. </li> diff --git a/tests/automake.mk b/tests/automake.mk index 3899c9e80..940f5b923 100644 --- a/tests/automake.mk +++ b/tests/automake.mk @@ -313,7 +313,9 @@ CHECK_PYFILES = \ tests/uuidfilt.py \ tests/test-tcp-rst.py \ tests/check_acl_log.py \ - tests/scapy-server.py + tests/scapy-server.py \ + tests/client.py \ + tests/server.py EXTRA_DIST += $(CHECK_PYFILES) PYCOV_CLEAN_FILES += $(CHECK_PYFILES:.py=.py,cover) .coverage diff --git a/tests/client.py b/tests/client.py new file mode 100755 index 000000000..7662347ff --- /dev/null +++ b/tests/client.py @@ -0,0 +1,38 @@ +#!/usr/bin/env python3 + +import socket +import os +import time +import argparse + + +def send_data_from_fifo_to_server( + fifo_path='/tmp/myfifo', host='127.0.0.1', port=10000 +): + # Open the FIFO for reading (blocking mode) + with open(fifo_path, 'r') as fifo_file: + with socket.socket( + socket.AF_INET, socket.SOCK_STREAM + ) as client_socket: + client_socket.connect((host, port)) + # Continuously read from the FIFO and send to the server + while True: + data = fifo_file.readline().strip() + if data: + client_socket.sendall(data.encode()) + else: + # If no data is read (blocking mode), add a small delay or handle as needed + time.sleep(0.1) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser() + group = parser.add_argument_group() + group.add_argument("-f", "--fifo_path") + group.add_argument("-i", "--server-host") + group.add_argument("-p", "--server-port", type=int) + args = parser.parse_args() + + send_data_from_fifo_to_server( + args.fifo_path, args.server_host, args.server_port + ) diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 4eae1c67c..1d90622e1 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -14253,3 +14253,137 @@ AT_CHECK([grep "lr_in_dnat" lr1flows | ovn_strip_lflows | grep "30.0.0.1"], [0], AT_CLEANUP ]) + +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([ACL persistent ID - Logical Flows]) +ovn_start + +dnl For this test, we want to ensure that the logical flows for ACLs are +dnl what we expect. +dnl +dnl First, we'll ensure that an ACL that is not "allow-established" sets the +dnl relevant CT values to 0. +dnl +dnl Then we'll change the ACL to be "allow-established" and ensure the logical +dnl flows do set the appropriate values. +dnl +dnl Then finally, we'll set the ACL back away from "allow-established" and +dnl ensure that the logical flows set the relevant CT values to 0. + +check ovn-nbctl ls-add sw + +check ovn-nbctl acl-add sw from-lport 1001 "tcp" allow-related +check ovn-nbctl acl-add sw to-lport 1002 "ip" allow-related +check ovn-nbctl --apply-after-lb acl-add sw from-lport 1003 "udp" allow-related + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && (tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && (tcp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == 1 && (udp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == 1 && (udp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && (ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; next;) + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && (ip)), action=(reg8[[16]] = 1; next;) +]) + +ingress_uuid=$(fetch_column nb:ACL _uuid priority=1001) +egress_uuid=$(fetch_column nb:ACL _uuid priority=1002) +after_lb_uuid=$(fetch_column nb:ACL _uuid priority=1003) + +check ovn-nbctl set acl $ingress_uuid action=allow-established +check ovn-nbctl set acl $egress_uuid action=allow-established +check ovn-nbctl set acl $after_lb_uuid action=allow-established + +dnl Now we should see the registers being set to the appropriate values. +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[7]] == 1 && (tcp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) + table=??(ls_in_acl_eval ), priority=2001 , match=(reg0[[8]] == 1 && (tcp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[7]] == 1 && (udp)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=(reg0[[8]] == 1 && (udp)), action=(reg8[[16]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[7]] == 1 && (ip)), action=(reg8[[16]] = 1; reg0[[1]] = 1; reg0[[20]] = 1; next;) + table=??(ls_out_acl_eval ), priority=2002 , match=(reg0[[8]] == 1 && (ip)), action=(reg8[[16]] = 1; next;) +]) + +dnl Now try the other ACL verdicts and ensure that they do not +dnl try to set the values. +for verdict in allow allow-stateless +do + echo "verdict is $verdict" + check ovn-nbctl set acl $ingress_uuid action=$verdict + check ovn-nbctl set acl $egress_uuid action=$verdict + check ovn-nbctl set acl $after_lb_uuid action=$verdict + + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(reg8[[16]] = 1; next;) +]) + + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(reg8[[16]] = 1; next;) +]) + + AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(reg8[[16]] = 1; next;) +]) +done + +check ovn-nbctl set acl $ingress_uuid action=drop +check ovn-nbctl set acl $egress_uuid action=drop +check ovn-nbctl set acl $after_lb_uuid action=drop + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(reg8[[17]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(reg8[[17]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(reg8[[17]] = 1; next;) +]) + +check ovn-nbctl set acl $ingress_uuid action=reject +check ovn-nbctl set acl $egress_uuid action=reject +check ovn-nbctl set acl $after_lb_uuid action=reject + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(reg8[[18]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(reg8[[18]] = 1; next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(reg8[[18]] = 1; next;) +]) + +check ovn-nbctl set acl $ingress_uuid action=pass +check ovn-nbctl set acl $egress_uuid action=pass +check ovn-nbctl set acl $after_lb_uuid action=pass + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_eval | grep priority=2001 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_eval ), priority=2001 , match=((tcp)), action=(next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_in_acl_after_lb_eval | grep priority=2003 | ovn_strip_lflows], [0], [dnl + table=??(ls_in_acl_after_lb_eval), priority=2003 , match=((udp)), action=(next;) +]) + +AT_CHECK([ovn-sbctl lflow-list sw | grep ls_out_acl_eval | grep priority=2002 | ovn_strip_lflows], [0], [dnl + table=??(ls_out_acl_eval ), priority=2002 , match=((ip)), action=(next;) +]) + +AT_CLEANUP +]) diff --git a/tests/server.py b/tests/server.py new file mode 100755 index 000000000..1eb378fd2 --- /dev/null +++ b/tests/server.py @@ -0,0 +1,30 @@ +#!/usr/bin/env python3 + +import socket +import argparse + + +def start_server(host='127.0.0.1', port=10000): + # Create a TCP/IP socket + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as server_socket: + server_socket.bind((host, port)) + server_socket.listen() + while True: + client_socket, client_address = server_socket.accept() + with client_socket: + # Receive the data from the client in chunks and write to a file + data = client_socket.recv(1024) + while data: + with open("output.txt", "a") as f: + f.write(data.decode() + "\n") + data = client_socket.recv(1024) + + +if __name__ == "__main__": + parser = argparse.ArgumentParser() + group = parser.add_argument_group() + group.add_argument("-i", "--bind-host") + group.add_argument("-p", "--bind-port", type=int) + args = parser.parse_args() + + start_server(args.bind_host, args.bind_port) diff --git a/tests/system-ovn.at b/tests/system-ovn.at index 4452d5676..a6ddda0f7 100644 --- a/tests/system-ovn.at +++ b/tests/system-ovn.at @@ -14117,5 +14117,167 @@ as OVS_TRAFFIC_VSWITCHD_STOP(["/.*error receiving.*/d /failed to query port patch-.*/d /.*terminating with signal 15.*/d"]) + +AT_CLEANUP +]) + +OVN_FOR_EACH_NORTHD([ +AT_SETUP([ACLs - persistent sessions]) + +CHECK_CONNTRACK() +CHECK_CONNTRACK_NAT() +ovn_start +OVS_TRAFFIC_VSWITCHD_START() +ADD_BR([br-int]) + +ovs-vsctl set-fail-mode br-ext standalone +# Set external-ids in br-int needed for ovn-controller +ovs-vsctl \ + -- set Open_vSwitch . external-ids:system-id=hv1 \ + -- set Open_vSwitch . external-ids:ovn-remote=unix:$ovs_base/ovn-sb/ovn-sb.sock \ + -- set Open_vSwitch . external-ids:ovn-encap-type=geneve \ + -- set Open_vSwitch . external-ids:ovn-encap-ip=169.0.0.1 \ + -- set bridge br-int fail-mode=secure other-config:disable-in-band=true + +start_daemon ovn-controller + +# For this test, we want to ensure that established traffic +# is allowed on ACLs with allow-established. +# +# To start, we will set up allow-related ACLs. +# We will send traffic and ensure it is allowed. Then we will adjust +# the ACL so it no longer matches, and we will ensure that the traffic +# is no longer allowed. +# +# Next, we will reset the ACL to its initial state, but we will also +# change the ACL to be allow-established. We will flush conntrack, +# and rerun the test exactly as before. The difference this time is +# that after we adjust the ACL so it no longer matches, the traffic +# should still be allowed. + +check ovn-nbctl ls-add sw +check ovn-nbctl lsp-add sw swp1 -- lsp-set-addresses swp1 "00:00:00:00:00:01 192.168.1.1" +check ovn-nbctl lsp-add sw swp2 -- lsp-set-addresses swp2 "00:00:00:00:00:02 192.168.1.2" + +ADD_NAMESPACES(swp1) +ADD_VETH(swp1, swp1, br-int, "192.168.1.1/24", "00:00:00:00:00:01") + +ADD_NAMESPACES(swp2) +ADD_VETH(swp2, swp2, br-int, "192.168.1.2/24", "00:00:00:00:00:02") + +# Start a TCP server on swp2. +NETNS_DAEMONIZE(swp2, [server.py -i 192.168.1.2 -p 10000], [server.pid]) + +# Make a FIFO and send its output to a client +# from swp1 +mkfifo /tmp/myfifo +on_exit 'rm -rf /tmp/myfifo' + +NETNS_DAEMONIZE(swp1, [client.py -f "/tmp/myfifo" -i 192.168.1.2 -p 10000], [client.pid]) + +# First, ensure that we have basic connectivity before we even start setting +# up ACLs. +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +check ovn-nbctl acl-add sw from-lport 1000 'ip4.dst == 192.168.1.2' allow-related +check ovn-nbctl acl-add sw from-lport 0 '1' drop + +# Do another basic connectivity check to ensure the ACL is allowing traffic as expected. +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +# At this point, I need to adjust the ACL so it no longer matches. We then need +# to ensure that the traffic does not pass. How we test this is...interesting. I'm +# not sure how to test for a negative condition accurately. + +acl_uuid=$(fetch_column nb:ACL _uuid priority=1000) + +# Update the ACL so that it no longer matches our client-server traffic +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.3\"" + +# Send another packet from the client to the server. +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +# The traffic should be blocked. We'll check the "drop" ACL to see if it has +# been hit. We can't predict the number of packets that will be seen, but we know +# it will be non-zero. +oftable=$(ovn-debug lflow-stage-to-oftable ls_in_acl_eval) +OVS_WAIT_FOR_OUTPUT([ovs-ofctl dump-flows br-int | grep table="$oftable" | grep "priority=1000" | grep -v commit | grep -c "n_packets=[[1-9]]"], [0], [dnl +1 +]) + +# And just to be safe, let's make sure the output file is still empty +AT_CHECK([cat output.txt], [0], [dnl +]) + +# Reset the client and server processes so that we create a new connection +client_pid=$(cat client.pid) +server_pid=$(cat server.pid) +kill $server_pid +kill $client_pid +OVS_WAIT_WHILE([kill -0 $server_pid 2>/dev/null]) +OVS_WAIT_WHILE([kill -0 $client_pid 2>/dev/null]) + +NETNS_DAEMONIZE(swp2, [server.py -i 192.168.1.2 -p 20000], [server.pid]) +NETNS_DAEMONIZE(swp1, [client.py -f "/tmp/myfifo" -i 192.168.1.2 -p 20000], [client.pid]) + +# Now we'll re-set the ACL to allow the traffic. +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.2\"" + +# We'll also change to allow-established +check ovn-nbctl set ACL $acl_uuid action=allow-established + +# Make sure traffic passes +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +# Adjust the ACL so that it no longer matches +check ovn-nbctl set ACL $acl_uuid match="\"ip4.dst == 192.168.1.3\"" + +# Traffic should still pass +AT_CHECK([printf "test\n" > /tmp/myfifo], [0], [dnl +]) + +OVS_WAIT_FOR_OUTPUT([cat output.txt], [0], [dnl +test +]) + +: > output.txt + +OVS_APP_EXIT_AND_WAIT([ovn-controller]) + +as ovn-sb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as ovn-nb +OVS_APP_EXIT_AND_WAIT([ovsdb-server]) + +as northd +OVS_APP_EXIT_AND_WAIT([ovn-northd]) + +as +OVS_TRAFFIC_VSWITCHD_STOP(["/failed to query port patch-.*/d +/connection dropped.*/d"]) + AT_CLEANUP ])
"allow-established" ACLs are identical to allow-related ACLs, with one exception. On allow-related ACLs, if the ACL changes and no longer matches an established session, then traffic will no longer automatically be allowed. Instead, traffic on the established session will be subject to ACL matching, and therefore the traffic may be dropped. For allow-established ACLs, this behavior is altered. We set a specific bit in the ct_mark to indicate this is an allow-established ACL. If this bit is set, then established session traffic will always be allowed, even if the ACL is altered to no longer match the traffic. Upcoming commits will put in place methods so that deleting the ACL or changing the action type on the ACL will remove the conntrack entry that allows the established traffic. Signed-off-by: Mark Michelson <mmichels@redhat.com> --- include/ovn/logical-fields.h | 1 + lib/logical-fields.c | 5 ++ northd/en-ls-stateful.c | 3 +- northd/northd.c | 36 +++++++- ovn-nb.ovsschema | 7 +- ovn-nb.xml | 11 +++ tests/automake.mk | 4 +- tests/client.py | 38 ++++++++ tests/ovn-northd.at | 134 +++++++++++++++++++++++++++++ tests/server.py | 30 +++++++ tests/system-ovn.at | 162 +++++++++++++++++++++++++++++++++++ 11 files changed, 424 insertions(+), 7 deletions(-) create mode 100755 tests/client.py create mode 100755 tests/server.py