diff mbox series

[ovs-dev,3/4] rbac: Restrict IGMP_Group updates to relevant chassis.

Message ID 20240119213331.454896-3-mmichels@redhat.com
State Superseded
Headers show
Series [ovs-dev,1/4] rbac: MAC_Bindings can only be updated by the inserting chassis. | expand

Checks

Context Check Description
ovsrobot/apply-robot warning apply and check: warning
ovsrobot/github-robot-_Build_and_Test fail github build: failed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Mark Michelson Jan. 19, 2024, 9:33 p.m. UTC 
RBAC did not restrict which chassis could update IGMP_Groups. With this
change, we add a new "chassis_name" column to IGMP_Group.

This may seem odd since there is already a "chassis" column in
IGMP_Group. But RBAC specifically works by string matching based on the
certificate common name. Therefore, we need to have a chassis_name
string column instead of a chassis UUID column.
---
 controller/ip-mcast.c | 20 ++++++++++++++------
 controller/ip-mcast.h |  6 ++++--
 controller/pinctrl.c  | 16 +++++++++++++---
 northd/ovn-northd.c   |  2 +-
 ovn-sb.ovsschema      |  7 ++++---
 ovn-sb.xml            |  5 +++++
 6 files changed, 41 insertions(+), 15 deletions(-)

Comments

0-day Robot Jan. 19, 2024, 9:45 p.m. UTC | #1 
Bleep bloop.  Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Author Mark Michelson <mmichels@redhat.com> needs to sign off.
WARNING: Line is 88 characters long (recommended limit is 79)
#152 FILE: controller/pinctrl.c:5446:
                                                   pinctrl.igmp_group_has_chassis_name);

Lines checked: 212, Warnings: 1, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
diff mbox series

Patch

diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c
index a870fb29e..6150cece0 100644
--- a/controller/ip-mcast.c
+++ b/controller/ip-mcast.c
@@ -38,7 +38,8 @@  static struct sbrec_igmp_group *
 igmp_group_create_(struct ovsdb_idl_txn *idl_txn,
                    const char *addr_str,
                    const struct sbrec_datapath_binding *datapath,
-                   const struct sbrec_chassis *chassis);
+                   const struct sbrec_chassis *chassis,
+                   bool igmp_group_has_chassis_name);
 
 struct ovsdb_idl_index *
 igmp_group_index_create(struct ovsdb_idl *idl)
@@ -86,7 +87,8 @@  struct sbrec_igmp_group *
 igmp_group_create(struct ovsdb_idl_txn *idl_txn,
                   const struct in6_addr *address,
                   const struct sbrec_datapath_binding *datapath,
-                  const struct sbrec_chassis *chassis)
+                  const struct sbrec_chassis *chassis,
+                  bool igmp_group_has_chassis_name)
 {
     char addr_str[INET6_ADDRSTRLEN];
 
@@ -94,16 +96,18 @@  igmp_group_create(struct ovsdb_idl_txn *idl_txn,
         return NULL;
     }
 
-    return igmp_group_create_(idl_txn, addr_str, datapath, chassis);
+    return igmp_group_create_(idl_txn, addr_str, datapath, chassis,
+                              igmp_group_has_chassis_name);
 }
 
 struct sbrec_igmp_group *
 igmp_mrouter_create(struct ovsdb_idl_txn *idl_txn,
                     const struct sbrec_datapath_binding *datapath,
-                    const struct sbrec_chassis *chassis)
+                    const struct sbrec_chassis *chassis,
+                    bool igmp_group_has_chassis_name)
 {
     return igmp_group_create_(idl_txn, OVN_IGMP_GROUP_MROUTERS, datapath,
-                              chassis);
+                              chassis, igmp_group_has_chassis_name);
 }
 
 void
@@ -249,13 +253,17 @@  static struct sbrec_igmp_group *
 igmp_group_create_(struct ovsdb_idl_txn *idl_txn,
                    const char *addr_str,
                    const struct sbrec_datapath_binding *datapath,
-                   const struct sbrec_chassis *chassis)
+                   const struct sbrec_chassis *chassis,
+                   bool igmp_group_has_chassis_name)
 {
     struct sbrec_igmp_group *g = sbrec_igmp_group_insert(idl_txn);
 
     sbrec_igmp_group_set_address(g, addr_str);
     sbrec_igmp_group_set_datapath(g, datapath);
     sbrec_igmp_group_set_chassis(g, chassis);
+    if (igmp_group_has_chassis_name) {
+        sbrec_igmp_group_set_chassis_name(g, chassis->name);
+    }
 
     return g;
 }
diff --git a/controller/ip-mcast.h b/controller/ip-mcast.h
index 326f39db1..a2d531097 100644
--- a/controller/ip-mcast.h
+++ b/controller/ip-mcast.h
@@ -39,11 +39,13 @@  struct sbrec_igmp_group *igmp_group_create(
     struct ovsdb_idl_txn *idl_txn,
     const struct in6_addr *address,
     const struct sbrec_datapath_binding *datapath,
-    const struct sbrec_chassis *chassis);
+    const struct sbrec_chassis *chassis,
+    bool igmp_group_has_chassis_name);
 struct sbrec_igmp_group *igmp_mrouter_create(
     struct ovsdb_idl_txn *idl_txn,
     const struct sbrec_datapath_binding *datapath,
-    const struct sbrec_chassis *chassis);
+    const struct sbrec_chassis *chassis,
+    bool igmp_group_has_chassis_name);
 
 void igmp_group_update_ports(const struct sbrec_igmp_group *g,
                              struct ovsdb_idl_index *datapaths,
diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index a00cdceea..1e3df02af 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -181,6 +181,7 @@  struct pinctrl {
     bool fdb_can_timestamp;
     bool dns_supports_ovn_owned;
     bool mac_binding_has_chassis_name;
+    bool igmp_group_has_chassis_name;
 };
 
 static struct pinctrl pinctrl;
@@ -3600,6 +3601,13 @@  pinctrl_update(const struct ovsdb_idl *idl, const char *br_int_name)
         notify_pinctrl_handler();
     }
 
+    bool igmp_group_has_chassis_name =
+        sbrec_server_has_igmp_group_table_col_chassis_name(idl);
+    if (igmp_group_has_chassis_name != pinctrl.igmp_group_has_chassis_name) {
+        pinctrl.igmp_group_has_chassis_name = igmp_group_has_chassis_name;
+        notify_pinctrl_handler();
+    }
+
     ovs_mutex_unlock(&pinctrl_mutex);
 }
 
@@ -5417,8 +5425,9 @@  ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn,
             sbrec_igmp = igmp_group_lookup(sbrec_igmp_groups, &mc_group->addr,
                                            local_dp->datapath, chassis);
             if (!sbrec_igmp) {
-                sbrec_igmp = igmp_group_create(ovnsb_idl_txn, &mc_group->addr,
-                                               local_dp->datapath, chassis);
+                sbrec_igmp = igmp_group_create(
+                    ovnsb_idl_txn, &mc_group->addr, local_dp->datapath,
+                    chassis, pinctrl.igmp_group_has_chassis_name);
             }
 
             igmp_group_update_ports(sbrec_igmp, sbrec_datapath_binding_by_key,
@@ -5433,7 +5442,8 @@  ip_mcast_sync(struct ovsdb_idl_txn *ovnsb_idl_txn,
         if (!sbrec_ip_mrouter) {
             sbrec_ip_mrouter = igmp_mrouter_create(ovnsb_idl_txn,
                                                    local_dp->datapath,
-                                                   chassis);
+                                                   chassis,
+                                                   pinctrl.igmp_group_has_chassis_name);
         }
         igmp_mrouter_update_ports(sbrec_ip_mrouter,
                                   sbrec_datapath_binding_by_key,
diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
index ef580b561..8f70d5241 100644
--- a/northd/ovn-northd.c
+++ b/northd/ovn-northd.c
@@ -118,7 +118,7 @@  static const char *rbac_svc_monitor_auth[] =
 static const char *rbac_svc_monitor_auth_update[] =
     {"status"};
 static const char *rbac_igmp_group_auth[] =
-    {""};
+    {"chassis_name"};
 static const char *rbac_igmp_group_update[] =
     {"address", "chassis", "datapath", "ports"};
 static const char *rbac_bfd_auth[] =
diff --git a/ovn-sb.ovsschema b/ovn-sb.ovsschema
index 563d1a215..0e601f4e3 100644
--- a/ovn-sb.ovsschema
+++ b/ovn-sb.ovsschema
@@ -1,7 +1,7 @@ 
 {
     "name": "OVN_Southbound",
-    "version": "20.32.0",
-    "cksum": "482767101 31276",
+    "version": "20.33.0",
+    "cksum": "3042447672 31328",
     "tables": {
         "SB_Global": {
             "columns": {
@@ -494,7 +494,8 @@ 
                 "ports": {"type": {"key": {"type": "uuid",
                                            "refTable": "Port_Binding",
                                            "refType": "weak"},
-                                   "min": 0, "max": "unlimited"}}},
+                                   "min": 0, "max": "unlimited"}},
+                "chassis_name": {"type": "string"}},
             "indexes": [["address", "datapath", "chassis"]],
             "isRoot": true},
         "Service_Monitor": {
diff --git a/ovn-sb.xml b/ovn-sb.xml
index 046913201..833e53114 100644
--- a/ovn-sb.xml
+++ b/ovn-sb.xml
@@ -4770,6 +4770,11 @@  tcp.flags = RST;
     <column name="ports">
       The destination port bindings for this IGMP group.
     </column>
+
+    <column name="chassis_name">
+      The chassis that inserted this record. This column is used for RBAC
+      purposes only.
+    </column>
   </table>
 
   <table name="Service_Monitor">