Fixed: swap objects of 'user' and objects of rules

and removed attribute expandedUser of type service
hknutzen · Feb 28, 2024 · 39a1666 · 39a1666
1 parent 9662966
commit 39a1666
Show file tree

Hide file tree

Showing 5 changed files with 166 additions and 22 deletions.
diff --git a/go/pkg/pass1/check-service-owner.go b/go/pkg/pass1/check-service-owner.go
@@ -167,7 +167,10 @@ func (c *spoc) checkServiceOwner(sRules *serviceRules) {
 			sameObjects bool
 			// Is set, if all rules are coupling rules.
 			isCoupling bool
-			objects    map[srvObj]bool
+			// Collect non 'user' objects.
+			objects map[srvObj]bool
+			// List of lists of users.
+			users [][]srvObj
 		}
 		service2info := make(map[*service]*svcInfo)
 
@@ -183,8 +186,6 @@ func (c *spoc) checkServiceOwner(sRules *serviceRules) {
 					}
 					service2info[svc] = info
 				}
-
-				// Collect non 'user' objects.
 				objects := info.objects
 
 				// Check, if service contains only coupling rules with only
@@ -222,10 +223,14 @@ func (c *spoc) checkServiceOwner(sRules *serviceRules) {
 						}
 					}
 				}
-				if hasUser != "src" {
+				if hasUser == "src" {
+					info.users = append(info.users, rule.src)
+				} else {
 					check(rule.src)
 				}
-				if hasUser != "dst" {
+				if hasUser == "dst" {
+					info.users = append(info.users, rule.dst)
+				} else {
 					check(rule.dst)
 				}
 				info.objects = objects
@@ -278,20 +283,22 @@ func (c *spoc) checkServiceOwner(sRules *serviceRules) {
 					// if objects of user and objects of rules are swapped.
 					var userOwner *owner
 					simpleUser := true
-					for _, user := range svc.expandedUser {
-						var o *owner
-						if obj, ok := user.(srvObj); ok {
-							o = obj.getOwner()
-						}
-						if o == nil {
-							simpleUser = false
-							break
-						}
-						if userOwner == nil {
-							userOwner = o
-						} else if userOwner != o {
-							simpleUser = false
-							break
+					for _, users := range info.users {
+						for _, user := range users {
+							var o *owner
+							if obj, ok := user.(srvObj); ok {
+								o = obj.getOwner()
+							}
+							if o == nil {
+								simpleUser = false
+								break
+							}
+							if userOwner == nil {
+								userOwner = o
+							} else if userOwner != o {
+								simpleUser = false
+								break
+							}
 						}
 					}
 					if simpleUser && userOwner != nil {

diff --git a/go/pkg/pass1/normalize-services.go b/go/pkg/pass1/normalize-services.go
@@ -203,9 +203,7 @@ func (c *spoc) normalizeSrcDstList(
 
 func (c *spoc) normalizeServiceRules(s *service, sRules *serviceRules) {
 	user := c.expandUser(s)
-	s.expandedUser = user
 	hasRules := false
-
 	for _, uRule := range s.rules {
 		deny := uRule.action == "deny"
 		var store *serviceRuleList

diff --git a/go/pkg/pass1/types.go b/go/pkg/pass1/types.go
@@ -598,7 +598,6 @@ type service struct {
 	unenforceableMap           map[objPair]bool
 	unknownOwner               bool
 	user                       []ast.Element
-	expandedUser               groupObjList
 }
 
 func (x *service) String() string { return x.name }

diff --git a/go/testdata/ipv6/owner_ipv6.t b/go/testdata/ipv6/owner_ipv6.t
@@ -835,6 +835,44 @@ Warning: service:s1 has multiple owners:
  o1, o2
 =END=
 
+############################################################
+=TITLE=Useless multi_owner when expanded user objects have single owner
+=PARAMS=--ipv6
+=INPUT=
+owner:o1 = { admins = a1@b.c; }
+owner:o2 = { admins = a2@b.c; }
+owner:o3 = { admins = a3@b.c; }
+network:n1 = {
+ ip = ::a01:100/120;
+ owner = o3;
+ host:h11 = { ip = ::a01:10a; owner = o1; }
+ host:h12 = { ip = ::a01:10b; owner = o2; }
+}
+router:asa1 = {
+ managed;
+ model = ASA;
+ interface:n1 = { ip = ::a01:101; hardware = n1; }
+ interface:n2 = { ip = ::a01:201; hardware = n2; }
+}
+network:n2 = {
+ ip = ::a01:200/120;
+ host:h21 = { ip = ::a01:20a; owner = o1; }
+ host:h22 = { ip = ::a01:20b; owner = o2; }
+}
+group:g1 = host:h11, host:h12;
+service:s1 = {
+ multi_owner;
+ user = group:g1;
+ permit src = network:[user]; dst = host:h21, host:h22; prt = tcp 80;
+ permit src = host:h21, host:h22; dst = network:[user]; prt = tcp 81;
+}
+=WARNING=
+Warning: Unnecessary 'multi_owner' at service:s1
+ All 'user' objects belong to single owner:o3.
+ Either swap objects of 'user' and objects of rules,
+ or split service into multiple parts, one for each owner.
+=END=
+
 ############################################################
 =TITLE=Useless multi_owner when user objects have single owner
 =PARAMS=--ipv6
@@ -869,6 +907,39 @@ Warning: Unnecessary 'multi_owner' at service:s1
  or split service into multiple parts, one for each owner.
 =END=
 
+############################################################
+=TITLE=Need multi_owner even though user seems to have single owner
+=PARAMS=--ipv6
+=INPUT=
+owner:o1 = { admins = a1@b.c; }
+owner:o2 = { admins = a2@b.c; }
+owner:o3 = { admins = a3@b.c; }
+network:n1 = {
+ ip = ::a01:100/120;
+ owner = o3;
+ host:h11 = { ip = ::a01:10a; owner = o1; }
+ host:h12 = { ip = ::a01:10b; owner = o2; }
+}
+router:asa1 = {
+ managed;
+ model = ASA;
+ interface:n1 = { ip = ::a01:101; hardware = n1; }
+ interface:n2 = { ip = ::a01:201; hardware = n2; }
+}
+network:n2 = {
+ ip = ::a01:200/120;
+ host:h21 = { ip = ::a01:20a; owner = o1; }
+ host:h22 = { ip = ::a01:20b; owner = o2; }
+}
+service:s1 = {
+ multi_owner;
+ user = network:n1;
+ permit src = host:[user]; dst = host:h21, host:h22; prt = tcp 80;
+ permit src = host:h21, host:h22; dst = user; prt = tcp 81;
+}
+# No Warning: Unnecessary 'multi_owner' at service:s1
+=WARNING=NONE
+
 ############################################################
 =TITLE=multi_owner ok with empty user objects
 =PARAMS=--ipv6

diff --git a/go/testdata/owner.t b/go/testdata/owner.t
@@ -799,6 +799,43 @@ Warning: service:s1 has multiple owners:
  o1, o2
 =END=
 
+############################################################
+=TITLE=Useless multi_owner when expanded user objects have single owner
+=INPUT=
+owner:o1 = { admins = a1@b.c; }
+owner:o2 = { admins = a2@b.c; }
+owner:o3 = { admins = a3@b.c; }
+network:n1 = {
+ ip = 10.1.1.0/24;
+ owner = o3;
+ host:h11 = { ip = 10.1.1.10; owner = o1; }
+ host:h12 = { ip = 10.1.1.11; owner = o2; }
+}
+router:asa1 = {
+ managed;
+ model = ASA;
+ interface:n1 = { ip = 10.1.1.1; hardware = n1; }
+ interface:n2 = { ip = 10.1.2.1; hardware = n2; }
+}
+network:n2 = {
+ ip = 10.1.2.0/24;
+ host:h21 = { ip = 10.1.2.10; owner = o1; }
+ host:h22 = { ip = 10.1.2.11; owner = o2; }
+}
+group:g1 = host:h11, host:h12;
+service:s1 = {
+ multi_owner;
+ user = group:g1;
+ permit src = network:[user]; dst = host:h21, host:h22; prt = tcp 80;
+ permit src = host:h21, host:h22; dst = network:[user]; prt = tcp 81;
+}
+=WARNING=
+Warning: Unnecessary 'multi_owner' at service:s1
+ All 'user' objects belong to single owner:o3.
+ Either swap objects of 'user' and objects of rules,
+ or split service into multiple parts, one for each owner.
+=END=
+
 ############################################################
 =TITLE=Useless multi_owner when user objects have single owner
 =INPUT=
@@ -832,6 +869,38 @@ Warning: Unnecessary 'multi_owner' at service:s1
  or split service into multiple parts, one for each owner.
 =END=
 
+############################################################
+=TITLE=Need multi_owner even though user seems to have single owner
+=INPUT=
+owner:o1 = { admins = a1@b.c; }
+owner:o2 = { admins = a2@b.c; }
+owner:o3 = { admins = a3@b.c; }
+network:n1 = {
+ ip = 10.1.1.0/24;
+ owner = o3;
+ host:h11 = { ip = 10.1.1.10; owner = o1; }
+ host:h12 = { ip = 10.1.1.11; owner = o2; }
+}
+router:asa1 = {
+ managed;
+ model = ASA;
+ interface:n1 = { ip = 10.1.1.1; hardware = n1; }
+ interface:n2 = { ip = 10.1.2.1; hardware = n2; }
+}
+network:n2 = {
+ ip = 10.1.2.0/24;
+ host:h21 = { ip = 10.1.2.10; owner = o1; }
+ host:h22 = { ip = 10.1.2.11; owner = o2; }
+}
+service:s1 = {
+ multi_owner;
+ user = network:n1;
+ permit src = host:[user]; dst = host:h21, host:h22; prt = tcp 80;
+ permit src = host:h21, host:h22; dst = user; prt = tcp 81;
+}
+# No Warning: Unnecessary 'multi_owner' at service:s1
+=WARNING=NONE
+
 ############################################################
 =TITLE=multi_owner ok with empty user objects
 =INPUT=