New Pod attributes to be considered by PDP + consistent attribute nam…

…ing (clusterlink-net#665) * More Pod attributes for PDP: SA and labels * Allow clients without pod-info only if policies have no attrs * removing client-name label * block attribute-less clients on ingress as well * A more consistent attribute naming Signed-off-by: Ziv Nevo <[email protected]>
zivnevo · Jul 17, 2024 · b13602e · b13602e
1 parent d2506e4
commit b13602e
Show file tree

Hide file tree

Showing 13 changed files with 262 additions and 81 deletions.
diff --git a/demos/bookinfo/manifests/clusterlink/deny-server1-policy.yaml b/demos/bookinfo/manifests/clusterlink/deny-server1-policy.yaml
@@ -12,7 +12,7 @@ spec:
         [{
             workloadSelector: {
                 matchLabels: {
-                    clusterlink/metadata.gatewayName: server1
+                    peer.clusterlink.net/name: server1
                 }
             }
         }

diff --git a/demos/bookinfo/test.py b/demos/bookinfo/test.py
@@ -132,8 +132,8 @@ def applyPolicy(cl:Cluster, type):
         cl.imports.update(reviewSvc,  namespace, srcK8sSvcPort,  ["peer2","peer3"], [reviewSvc,reviewSvc], [namespace,namespace],"static")
     elif type == "diff":
         cl.policies.delete(name="allow-all",namespace= namespace)
-        cl.policies.create(name="src1topeer2",namespace= namespace, action="allow", from_attribute=[{"workloadSelector": {"matchLabels": {"clusterlink/metadata.serviceName": srcSvc1}}}],to_attribute=[{"workloadSelector": {"matchLabels": {"clusterlink/metadata.serviceName": reviewSvc, "clusterlink/metadata.gatewayName": "peer2"}}}])
-        cl.policies.create(name="src2topeer3",namespace= namespace, action="allow", from_attribute=[{"workloadSelector": {"matchLabels": {"clusterlink/metadata.serviceName": srcSvc2}}}],to_attribute=[{"workloadSelector": {"matchLabels": {"clusterlink/metadata.serviceName": reviewSvc, "clusterlink/metadata.gatewayName": "peer3"}}}])
+        cl.policies.create(name="src1topeer2",namespace= namespace, action="allow", from_attribute=[{"workloadSelector": {"matchLabels": {"client.clusterlink.net/labels.app": srcSvc1}}}],to_attribute=[{"workloadSelector": {"matchLabels": {"export.clusterlink.net/name": reviewSvc, "peer.clusterlink.net/name": "peer2"}}}])
+        cl.policies.create(name="src2topeer3",namespace= namespace, action="allow", from_attribute=[{"workloadSelector": {"matchLabels": {"client.clusterlink.net/labels.app": srcSvc2}}}],to_attribute=[{"workloadSelector": {"matchLabels": {"export.clusterlink.net/name": reviewSvc, "peer.clusterlink.net/name": "peer3"}}}])
     elif type == "show":
         runcmd(f'kubectl get imports.clusterlink.net')
     elif type == "clean":

diff --git a/demos/speedtest/testdata/policy/denyFromGw.yaml b/demos/speedtest/testdata/policy/denyFromGw.yaml
@@ -8,7 +8,7 @@ spec:
     from: [{
             workloadSelector: {
                 matchLabels: {
-                    clusterlink/metadata.gatewayName: peer3
+                    peer.clusterlink.net/name: peer3
                 }
             }
         }

diff --git a/demos/speedtest/testdata/policy/denyToSpeedtest.yaml b/demos/speedtest/testdata/policy/denyToSpeedtest.yaml
@@ -8,15 +8,15 @@ spec:
     from: [{
             workloadSelector: {
                 matchLabels: {
-                    clusterlink/metadata.serviceName: firefox
+                    client.clusterlink.net/labels.app: firefox
                 }
             }
         }
     ]
     to: [{
         workloadSelector: {
             matchLabels: {
-                clusterlink/metadata.serviceName: openspeedtest
+                export.clusterlink.net/name: openspeedtest
             }
         }
         }]

diff --git a/examples/policies/allowToReviews.json b/examples/policies/allowToReviews.json
@@ -16,7 +16,7 @@
             {
                 "workloadSelector": {
                     "matchLabels": {
-                        "clusterlink/metadata.serviceName": "reviews"
+                        "export.clusterlink.net/name": "reviews"
                     }
                 }
             }

diff --git a/examples/policies/deny_from_gw1.json b/examples/policies/deny_from_gw1.json
@@ -11,7 +11,7 @@
             {
                 "workloadSelector": {
                     "matchLabels": {
-                        "clusterlink/metadata.gatewayName": "gw1"
+                        "peer.clusterlink.net/name": "gw1"
                     }
                 }
             }

diff --git a/pkg/controlplane/authz/connectivitypdp/connectivity_pdp.go b/pkg/controlplane/authz/connectivitypdp/connectivity_pdp.go
@@ -75,7 +75,7 @@ func NewPDP() *PDP {
 	}
 }
 
-// Returns a slice of copies of the non-privileged policies stored in the PDP.
+// GetPrivilegedPolicies returns a slice of copies of the non-privileged policies stored in the PDP.
 func (pdp *PDP) GetPrivilegedPolicies() []v1alpha1.PrivilegedAccessPolicy {
 	pols := pdp.privilegedPolicies.getPolicies()
 	res := []v1alpha1.PrivilegedAccessPolicy{}
@@ -88,7 +88,7 @@ func (pdp *PDP) GetPrivilegedPolicies() []v1alpha1.PrivilegedAccessPolicy {
 	return res
 }
 
-// Returns a slice of copies of the non-privileged policies stored in the PDP.
+// GetPolicies returns a slice of copies of the non-privileged policies stored in the PDP.
 func (pdp *PDP) GetPolicies() []v1alpha1.AccessPolicy {
 	pols := pdp.regularPolicies.getPolicies()
 	res := []v1alpha1.AccessPolicy{}
@@ -101,6 +101,13 @@ func (pdp *PDP) GetPolicies() []v1alpha1.AccessPolicy {
 	return res
 }
 
+// DependsOnClientAttrs returns whether the PDP holds a policy which depends on attributes
+// the client workload (From field) may or may not have.
+func (pdp *PDP) DependsOnClientAttrs() bool {
+	return pdp.privilegedPolicies.dependsOnClientAttrs() ||
+		pdp.regularPolicies.dependsOnClientAttrs()
+}
+
 // AddOrUpdatePolicy adds an AccessPolicy to the PDP.
 // If a policy with the same name already exists in the PDP,
 // it is updated (including updating the Action field).
@@ -172,6 +179,11 @@ func (pt *policyTier) getPolicies() connPolicyMap {
 	return res
 }
 
+// dependsOnClientAttrs returns whether any of the tier's policies has a From field which depends on specific attributes.
+func (pt *policyTier) dependsOnClientAttrs() bool {
+	return pt.denyPolicies.dependsOnClientAttrs() || pt.allowPolicies.dependsOnClientAttrs()
+}
+
 // addPolicy adds an access policy to the given tier, based on its action.
 // Note that within a tier, no two policies can have the same name, even if one is deny and the other is allow.
 func (pt *policyTier) addPolicy(policyName types.NamespacedName, policySpec *v1alpha1.AccessPolicySpec) {
@@ -259,6 +271,22 @@ func (cpm connPolicyMap) decide(src WorkloadAttrs, dest *DestinationDecision, pr
 	return false, nil
 }
 
+// dependsOnClientAttrs returns whether any of the policies has a From field which depends on specific attributes.
+func (cpm connPolicyMap) dependsOnClientAttrs() bool {
+	for _, policySpec := range cpm {
+		for i := range policySpec.From {
+			selector := policySpec.From[i].WorkloadSelector
+			if selector == nil {
+				continue
+			}
+			if len(selector.MatchExpressions) > 0 || len(selector.MatchLabels) > 0 {
+				return true
+			}
+		}
+	}
+	return false
+}
+
 // accessPolicyDecide returns a policy's decision on a given connection.
 // If the policy matches the connection, a decision based on its Action is returned.
 // Otherwise, it returns an "undecided" value.

diff --git a/pkg/controlplane/authz/connectivitypdp/connectivity_pdp_test.go b/pkg/controlplane/authz/connectivitypdp/connectivity_pdp_test.go
@@ -186,6 +186,61 @@ func getNameOfFirstPolicyInPDP(pdp *connectivitypdp.PDP, action v1alpha1.AccessP
 	return ""
 }
 
+func TestDependsOnClientAttrs(t *testing.T) {
+	emptyWorkloadSet := []v1alpha1.WorkloadSetOrSelector{{WorkloadSelector: &metav1.LabelSelector{}}}
+	nonEmptyWorkloadSet := []v1alpha1.WorkloadSetOrSelector{trivialWorkloadSet}
+	allowFromAllClientsPol := v1alpha1.AccessPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "allow-from-all-clients",
+			Namespace: defaultNS,
+		},
+		Spec: v1alpha1.AccessPolicySpec{
+			Action: v1alpha1.AccessPolicyActionAllow,
+			From:   emptyWorkloadSet,
+			To:     nonEmptyWorkloadSet,
+		},
+	}
+	allowFromSomeClientsPol := v1alpha1.AccessPolicy{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "reg",
+			Namespace: defaultNS,
+		},
+		Spec: v1alpha1.AccessPolicySpec{
+			Action: v1alpha1.AccessPolicyActionAllow,
+			From:   nonEmptyWorkloadSet,
+			To:     nonEmptyWorkloadSet,
+		},
+	}
+	allowFromSomeClientsPrivPol := v1alpha1.PrivilegedAccessPolicy{
+		ObjectMeta: metav1.ObjectMeta{Name: "priv"},
+		Spec: v1alpha1.AccessPolicySpec{
+			Action: v1alpha1.AccessPolicyActionDeny,
+			From:   nonEmptyWorkloadSet,
+			To:     nonEmptyWorkloadSet,
+		},
+	}
+
+	pdp := connectivitypdp.NewPDP()
+	// no policy -> no dependency on client attributes
+	require.False(t, pdp.DependsOnClientAttrs())
+
+	// adding a policy with allow-all From field -> still no dependency on client attributes
+	require.Nil(t, pdp.AddOrUpdatePolicy(connectivitypdp.PolicyFromCR(&allowFromAllClientsPol)))
+	require.False(t, pdp.DependsOnClientAttrs())
+
+	// adding a policy that only allows clients with specific labels -> now we have a dependency on client attributes
+	require.Nil(t, pdp.AddOrUpdatePolicy(connectivitypdp.PolicyFromCR(&allowFromSomeClientsPol)))
+	require.True(t, pdp.DependsOnClientAttrs())
+
+	// deleting this last policy -> no dependency on client attributes again
+	require.Nil(t, pdp.DeletePolicy(types.NamespacedName{Namespace: defaultNS, Name: allowFromSomeClientsPol.Name}, false))
+	require.False(t, pdp.DependsOnClientAttrs())
+
+	// adding a privileged policy that only allows clients with specific labels -> a dependency on client attributes
+	require.Nil(t, pdp.AddOrUpdatePolicy(connectivitypdp.PolicyFromPrivilegedCR(&allowFromSomeClientsPrivPol)))
+	require.True(t, pdp.DependsOnClientAttrs())
+}
+
 func TestDeleteNonexistingPolicies(t *testing.T) {
 	pdp := connectivitypdp.NewPDP()
 	err := pdp.DeletePolicy(types.NamespacedName{Name: "no-such-policy"}, true)

diff --git a/pkg/controlplane/authz/manager.go b/pkg/controlplane/authz/manager.go
@@ -40,10 +40,13 @@ const (
 	// the number of seconds a JWT access token is valid before it expires.
 	jwtExpirySeconds = 5
 
-	ServiceNameLabel      = "clusterlink/metadata.serviceName"
-	ServiceNamespaceLabel = "clusterlink/metadata.serviceNamespace"
-	ServiceLabelsPrefix   = "service/metadata."
-	GatewayNameLabel      = "clusterlink/metadata.gatewayName"
+	ClientNamespaceLabel  = "client.clusterlink.net/namespace"
+	ClientSALabel         = "client.clusterlink.net/service-account"
+	ClientLabelsPrefix    = "client.clusterlink.net/labels."
+	ServiceNameLabel      = "export.clusterlink.net/name"
+	ServiceNamespaceLabel = "export.clusterlink.net/namespace"
+	ServiceLabelsPrefix   = "export.clusterlink.net/labels."
+	PeerNameLabel         = "peer.clusterlink.net/name"
 )
 
 // egressAuthorizationRequest (from local dataplane)
@@ -84,9 +87,10 @@ type ingressAuthorizationResponse struct {
 }
 
 type podInfo struct {
-	name      string
-	namespace string
-	labels    map[string]string
+	name           string
+	namespace      string
+	serviceAccount string
+	labels         map[string]string
 }
 
 // Manager manages the authorization dataplane connections.
@@ -166,7 +170,12 @@ func (m *Manager) addPod(pod *v1.Pod) {
 	defer m.podLock.Unlock()
 
 	podID := types.NamespacedName{Name: pod.Name, Namespace: pod.Namespace}
-	m.podList[podID] = podInfo{name: pod.Name, namespace: pod.Namespace, labels: pod.Labels}
+	m.podList[podID] = podInfo{
+		name:           pod.Name,
+		namespace:      pod.Namespace,
+		labels:         pod.Labels,
+		serviceAccount: pod.Spec.ServiceAccountName,
+	}
 	for _, ip := range pod.Status.PodIPs {
 		// ignoring host-networked Pod IPs
 		if ip.IP != pod.Status.HostIP {
@@ -211,19 +220,35 @@ func (m *Manager) getPodInfoByIP(ip string) *podInfo {
 	return nil
 }
 
+func (m *Manager) getClientAttributes(req *egressAuthorizationRequest) connectivitypdp.WorkloadAttrs {
+	podInfo := m.getPodInfoByIP(req.IP)
+	if podInfo == nil {
+		m.logger.Infof("Pod has no info: IP=%v.", req.IP)
+		return nil
+	}
+
+	clientAttrs := connectivitypdp.WorkloadAttrs{
+		PeerNameLabel:        m.getPeerName(),
+		ClientNamespaceLabel: podInfo.namespace,
+		ClientSALabel:        podInfo.serviceAccount,
+	}
+
+	for k, v := range podInfo.labels {
+		clientAttrs[ClientLabelsPrefix+k] = v
+	}
+
+	m.logger.Debugf("Client attributes: %v.", clientAttrs)
+
+	return clientAttrs
+}
+
 // authorizeEgress authorizes a request for accessing an imported service.
 func (m *Manager) authorizeEgress(ctx context.Context, req *egressAuthorizationRequest) (*egressAuthorizationResponse, error) {
 	m.logger.Infof("Received egress authorization request: %v.", req)
 
-	srcAttributes := connectivitypdp.WorkloadAttrs{GatewayNameLabel: m.getPeerName()}
-	podInfo := m.getPodInfoByIP(req.IP)
-	if podInfo != nil {
-		srcAttributes[ServiceNamespaceLabel] = podInfo.namespace
-
-		if src, ok := podInfo.labels["app"]; ok { // TODO: Add support for labels other than just the "app" key.
-			m.logger.Infof("Received egress authorization srcLabels[app]: %v.", podInfo.labels["app"])
-			srcAttributes[ServiceNameLabel] = src
-		}
+	srcAttributes := m.getClientAttributes(req)
+	if len(srcAttributes) == 0 && m.connectivityPDP.DependsOnClientAttrs() {
+		return nil, fmt.Errorf("failed to extract client attributes, however, access policies depend on such attributes")
 	}
 
 	var imp v1alpha1.Import
@@ -262,7 +287,7 @@ func (m *Manager) authorizeEgress(ctx context.Context, req *egressAuthorizationR
 
 		dstAttributes[ServiceNameLabel] = importSource.ExportName
 		dstAttributes[ServiceNamespaceLabel] = importSource.ExportNamespace
-		dstAttributes[GatewayNameLabel] = importSource.Peer
+		dstAttributes[PeerNameLabel] = importSource.Peer
 
 		decision, err := m.connectivityPDP.Decide(srcAttributes, dstAttributes, req.ImportName.Namespace)
 		if err != nil {
@@ -371,12 +396,18 @@ func (m *Manager) authorizeIngress(
 	dstAttributes := connectivitypdp.WorkloadAttrs{
 		ServiceNameLabel:      export.Name,
 		ServiceNamespaceLabel: export.Namespace,
-		GatewayNameLabel:      m.getPeerName(),
+		PeerNameLabel:         m.getPeerName(),
 	}
 	for k, v := range export.Labels { // add export labels to destination attributes
 		dstAttributes[ServiceLabelsPrefix+k] = v
 	}
 
+	// do not allow requests from clients with no attributes if the PDP has attribute-dependent policies
+	if len(req.SrcAttributes) == 0 && m.connectivityPDP.DependsOnClientAttrs() {
+		resp.Allowed = false
+		return resp, nil
+	}
+
 	decision, err := m.connectivityPDP.Decide(req.SrcAttributes, dstAttributes, req.ServiceName.Namespace)
 	if err != nil {
 		return nil, fmt.Errorf("error deciding on an ingress connection: %w", err)

diff --git a/tests/e2e/k8s/services/httpecho/client.go b/tests/e2e/k8s/services/httpecho/client.go
@@ -29,6 +29,8 @@ import (
 	"github.com/clusterlink-net/clusterlink/tests/e2e/k8s/util"
 )
 
+const EchoClientPodName = "echo-client"
+
 func GetEchoValue(cluster *util.KindCluster, server *util.Service) (string, error) {
 	port, err := cluster.ExposeNodeport(server)
 	if err != nil {
@@ -76,11 +78,12 @@ func GetEchoValue(cluster *util.KindCluster, server *util.Service) (string, erro
 }
 
 func RunClientInPod(cluster *util.KindCluster, server *util.Service) (string, error) {
+	url := "http://" + server.Name
 	body, err := cluster.RunPod(&util.Pod{
-		Name:      "echo-client",
+		Name:      EchoClientPodName,
 		Namespace: server.Namespace,
 		Image:     "curlimages/curl",
-		Args:      []string{"curl", "-s", "-m", "1", "http://" + server.Name},
+		Args:      []string{"curl", "-s", "-m", "10", "--retry", "10", "--retry-delay", "1", "--retry-all-errors", url},
 	})
 	return strings.TrimSpace(body), err
 }