Make VPN ACL filter more precise (#50)

* Switch inverse rule to `and_ids` * Anchor `contains` matcher for reversed VPN header
stackitcloud · May 6, 2024 · 0b5ed40 · 0b5ed40
1 parent b6d8590
commit 0b5ed40
Show file tree

Hide file tree

Showing 3 changed files with 23 additions and 13 deletions.
diff --git a/docs/adr/02_envoyfilter_patching.md b/docs/adr/02_envoyfilter_patching.md
@@ -130,18 +130,18 @@ configPatches:
                   permissions:
                   - any: true
                   principals:
-                    - or_ids:
+                    - and_ids:
                         ids:
                           - not_id:
                               header:
                                 name: reversed-vpn
                                 string_match:
-                                  contains: shoot--projectname--shootname1
+                                  contains: .shoot--projectname--shootname1.
                           - not_id:
                               header:
                                 name: reversed-vpn
                                 string_match:
-                                  contains: shoot--projectname--shootname2
+                                  contains: .shoot--projectname--shootname2.
               # this is the shoot-specific policy, matching on both the header
               # AND the provided list of allowed IP ranges
               shoot--projectname--shootname1:
@@ -164,7 +164,7 @@ configPatches:
                     - header:
                         name: reversed-vpn
                         string_match:
-                          contains: shoot--projectname--shootname
+                          contains: .shoot--projectname--shootname.
               # one more policy for each shoot that has the extension enabled
               shoot-projectname-shootname2:
                 ...

diff --git a/pkg/envoyfilters/envoyfilters.go b/pkg/envoyfilters/envoyfilters.go
@@ -201,7 +201,7 @@ func CreateVPNConfigPatchFromRule(
 
 	policies := map[string]interface{}{}
 
-	policies[rbacName+"-inverse"] = createInversedVPNPolicy(mappings)
+	policies[rbacName+"-inverse"] = createInverseVPNPolicy(mappings)
 
 	for i := range mappings {
 		mapping := &mappings[i]
@@ -361,7 +361,12 @@ func createVPNPolicyForShoot(rule *ACLRule, alwaysAllowedCIDRs []string, technic
 							"header": map[string]interface{}{
 								"name": "reversed-vpn",
 								"string_match": map[string]interface{}{
-									"contains": technicalShootID,
+									// The actual header value will look something like
+									// `outbound|1194||vpn-seed-server.<technical-ID>.svc.cluster.local`.
+									// Include dots in the contains matcher as anchors, to always match the entire technical shoot ID.
+									// Otherwise, if there was one cluster named `foo` and one named `foo-bar` (in the same project),
+									// `foo` would effectively inherit the ACL of `foo-bar`.
+									"contains": "." + technicalShootID + ".",
 								},
 							},
 						},
@@ -372,7 +377,7 @@ func createVPNPolicyForShoot(rule *ACLRule, alwaysAllowedCIDRs []string, technic
 	}
 }
 
-func createInversedVPNPolicy(mappings []ACLMapping) map[string]interface{} {
+func createInverseVPNPolicy(mappings []ACLMapping) map[string]interface{} {
 	notHeaderPrincipals := []map[string]interface{}{}
 
 	for i := range mappings {
@@ -381,7 +386,12 @@ func createInversedVPNPolicy(mappings []ACLMapping) map[string]interface{} {
 				"header": map[string]interface{}{
 					"name": "reversed-vpn",
 					"string_match": map[string]interface{}{
-						"contains": mappings[i].ShootName,
+						// The actual header value will look something like
+						// `outbound|1194||vpn-seed-server.<technical-ID>.svc.cluster.local`.
+						// Include dots in the contains matcher as anchors, to always match the entire technical shoot ID.
+						// Otherwise, if there was one cluster named `foo` and one named `foo-bar` (in the same project),
+						// `foo` would effectively inherit the ACL of `foo-bar`.
+						"contains": "." + mappings[i].ShootName + ".",
 					},
 				},
 			},
@@ -394,7 +404,7 @@ func createInversedVPNPolicy(mappings []ACLMapping) map[string]interface{} {
 		},
 		"principals": []map[string]interface{}{
 			{
-				"or_ids": map[string]interface{}{
+				"and_ids": map[string]interface{}{
 					"ids": notHeaderPrincipals,
 				},
 			},

diff --git a/pkg/envoyfilters/testdata/vpnEnvoyFilterSpecWithOneAllowRule.yaml b/pkg/envoyfilters/testdata/vpnEnvoyFilterSpecWithOneAllowRule.yaml
@@ -17,13 +17,13 @@ configPatches:
                   permissions:
                   - any: true
                   principals:
-                    - or_ids:
+                    - and_ids:
                         ids:
                           - not_id:
                               header:
                                 name: reversed-vpn
                                 string_match:
-                                  contains: shoot--projectname--shootname
+                                  contains: .shoot--projectname--shootname.
               shoot--projectname--shootname:
                 permissions:
                 - any: true
@@ -44,9 +44,9 @@ configPatches:
                     - header:
                         name: reversed-vpn
                         string_match:
-                          contains: shoot--projectname--shootname
+                          contains: .shoot--projectname--shootname.
           stat_prefix: envoyrbac
 workloadSelector:
   labels:
     app: istio-ingressgateway
-    istio: ingressgateway
+    istio: ingressgateway