More Pod attributes for PDP: SA and labels #665

zivnevo · 2024-07-04T07:40:17Z

This PR adds more client-Pod attributes for the PDP to consider. We now have:

clusterlink/metadata.clientName - the value of the Pod's label app (if it has one)
clusterlink/metadata.clientNamespace - the Pod's namespace
clusterlink/metadata.clientServiceAccount - the Pod's service account
client/metadata.<key> - the value of the Pod's label <key> (one such attribute for each label)

Open questions:

Do we still need clusterlink/metadata.clientName? Users can now simply use client/metadata.app instead
How should we handle Pods for which CL has no info (yet)? Deny their request? Provide the PDP with an empty set of attributes?
E2E testing issue: how to make sure Pod info was read by CL before the Pod can start sending requests? Currently, the solution is sleep 3. Any better ideas?

Signed-off-by: Ziv Nevo <[email protected]>

orozery · 2024-07-04T09:21:30Z

pkg/controlplane/authz/manager.go

+	podInfo := m.getPodInfoByIP(req.IP)
+	if podInfo == nil {
+		m.logger.Infof("Pod has no info: IP=%v.", req.IP)
+		return clientAttrs // better return an error here?


Returning an error will simply yield a RST to the client connection.
I think it's better to keep it the way it is (which still allows for allow-all policies to proceed).

This is a bit problematic when having policies with negative selectors (using the NotIn and DoesNotExist operators).
E.g., I have a policy to deny all connections, except from those originating from namespace foo.
Connections from IPs for which CL cannot link a Pod, will always be allowed.

apiVersion: clusterlink.net/v1alpha1 kind: AccessPolicy metadata: name: deny-except-from-foo spec: action: deny from: - workloadSelector: matchExpressions: - key: clusterlink/metadata.clientNamespace operator: NotIn value: foo to: - workloadSelector: {}

Right.
I think the right thing to do is to do a non-cached GET from the API-server to get the pod info in this case.
I did not find a way to do this via the given client. This will be the best option.

Found this:
kubernetes-sigs/controller-runtime#585 (comment)

So there's a non-cached reader, but it's not available to authz.Manager.
Need to propagate it from cl-controlplane/app/server.go.

Apparently, even this is not good enough. Pod may start running and send requests, even before its IP is updated in etcd. From what I see, some non-cached List() calls return the Pod with its Node IP, but without its Pod IP. It only gets updated later.

How long is this window of a working pod with etcd not yet updated?
If not long, maybe we can stall a bit to wait for the update.

Or deny and let the client retry?

I changed the implementation: now if the client has no attributes (no pod info) AND the PDP has at least one policy which depends on client attributes, the request will be denied, and the client will have to retry.
Note that even if the PDP has no attribute-dependent policies, attribute-less requests can still be denied, depending on the policies (e.g., DENY policies take precedence, or no ALLOW policies)
This is enforced both on egress and on ingress.

orozery · 2024-07-04T09:22:47Z

pkg/controlplane/authz/manager.go

+	clientAttrs[ClientSALabel] = podInfo.serviceAccount
+
+	if src, ok := podInfo.labels["app"]; ok {
+		clientAttrs[ServiceNameLabel] = src // deprecated


What does it take to remove the deprecated attributes?

I think they are being used currently in some experiments, so better wait a bit.
They will not appear in the documentation.

orozery · 2024-07-04T09:24:45Z

tests/e2e/k8s/services/httpecho/client.go

 		Namespace: server.Namespace,
 		Image:     "curlimages/curl",
 		Args:      []string{"curl", "-s", "-m", "1", "http://" + server.Name},
 	})
 	return strings.TrimSpace(body), err
 }
+
+// Sleep allows more time for CL to be notified of the new Pod, so CL can retrieve the Pod's info.
+func RunClientInPodWithSleep(cluster *util.KindCluster, server *util.Service) (string, error) {


I don't think you need this function. If the non-sleep function fails, you will be covered by the allowRetry=true for the AccessService function.

How does the retry work when we run the client in a Pod?
Aren't we just creating Pods again and again?

Right, my bad.
So maybe add a flag that removes the RestartPolicy: v1.RestartPolicyNever.

Right, my bad. So maybe add a flag that removes the RestartPolicy: v1.RestartPolicyNever.

This won't work.

How about using
--retry-delay 1 --retry-all-errors
in the curl pod command line?

yes, this works.

Signed-off-by: Ziv Nevo <[email protected]>

elevran · 2024-07-04T16:36:31Z

This PR adds more client-Pod attributes for the PDP to consider. We now have:

clusterlink/metadata.clientName - the value of the Pod's label app (if it has one)

clusterlink/metadata.clientNamespace - the Pod's namespace

clusterlink/metadata.clientServiceAccount - the Pod's service account

client/metadata.<key> - the value of the Pod's label <key> (one such attribute for each label)

Is there a need to designate client vs service in attribute names?
clusterlink/metadata.clientNamespace can just be clusterlink/metadata.namespace...

Open questions:

Do we still need clusterlink/metadata.clientName? Users can now simply use client/metadata.app instead

IMO, we do not. I think it is up to the user to set the labels they care about and we should not treat app any different than other labels.

How should we handle Pods for which CL has no info (yet)? Deny their request? Provide the PDP with an empty set of attributes?

I would propose we deny on lack of sufficient context, to comply with principles of default deny, and let the client retry (e.g., we could consider reset or timeout as the response).
They could have properties that would deny the request so proceeding with insufficient knowledge seems to me as the wrong thing security wise.

E2E testing issue: how to make sure Pod info was read by CL before the Pod can start sending requests? Currently, the solution is sleep 3. Any better ideas?

Tests can explicitly confirm the Pod has IP then wait 1s or so?

pkg/controlplane/authz/manager.go

elevran · 2024-07-04T16:38:58Z

pkg/controlplane/authz/manager.go

+	podInfo := m.getPodInfoByIP(req.IP)
+	if podInfo == nil {
+		m.logger.Infof("Pod has no info: IP=%v.", req.IP)
+		return clientAttrs // better return an error here?


Or deny and let the client retry?

pkg/controlplane/authz/manager.go

Signed-off-by: Ziv Nevo <[email protected]>

zivnevo · 2024-07-15T12:08:29Z

After some rethinking, I now believe it's better for client and service to have distinct attribute prefixes. The main reason is that they don't have the exact same set of attributes (e.g., only clients have a service account, only services have a name). Having distinct prefixes will make it easier to ensure users are not using unavailable attributes in their policies. In addition, distinct prefixes make all attributes share the same pattern - a more consistent scheme.

My current proposal for attribute names (inspired by K8s well-known labels):

Client (Pod):

client.clusterlink.net/namespace - pod namespace
client.clusterlink.net/service-account - pod service account
client.clusterlink.net/labels.<label-key> - pod labels

Service (export):

export.clusterlink.net/name - Export name
export.clusterlink.net/service-name - the name of the K8s Service behind the Export (not currently implemented)
export.clusterlink.net/namespace - Export namespace
export.clusterlink.net/labels.<label-key> - Export labels
export.clusterlink.net/service-labels.<label-key> - the labels of the k8s Service behind the Export (not currently implemented)

Peer:

peer.clusterlink.net/name - Peer name
peer.clusterlink.net/labels.<label-key> - Peer labels (not currently implemented, to be set when calling clusterlink deploy peer)

@elevran , @orozery , @kfirtoledo - your thoughts?

Signed-off-by: Ziv Nevo <[email protected]>

More Pod attributes for PDP: SA and labels

ff70c1c

Signed-off-by: Ziv Nevo <[email protected]>

zivnevo requested a review from elevran as a code owner July 4, 2024 07:40

pull-request-size bot added the size/L label Jul 4, 2024

zivnevo requested review from orozery and kfirtoledo July 4, 2024 07:56

zivnevo linked an issue Jul 4, 2024 that may be closed by this pull request

Policy engine to consider more workload attributes when deciding on a connection #17

Closed

orozery reviewed Jul 4, 2024

View reviewed changes

Setting one more attribute

a7904e2

Signed-off-by: Ziv Nevo <[email protected]>

elevran reviewed Jul 4, 2024

View reviewed changes

elevran reviewed Jul 7, 2024

View reviewed changes

pkg/controlplane/authz/manager.go Outdated Show resolved Hide resolved

zivnevo and others added 4 commits July 10, 2024 16:49

Allow clients without pod-info only if policies have no attrs

aac20f0

Signed-off-by: Ziv Nevo <[email protected]>

removing client-name label

39d63ea

Signed-off-by: Ziv Nevo <[email protected]>

Merge branch 'main' into client-pod-attributes

26fdcad

block attribute-less clients on ingress as well

d252906

Signed-off-by: Ziv Nevo <[email protected]>

zivnevo requested review from orozery and elevran July 11, 2024 07:21

orozery approved these changes Jul 11, 2024

View reviewed changes

zivnevo and others added 2 commits July 11, 2024 10:26

revert unnecessary change

ab349f5

Signed-off-by: Ziv Nevo <[email protected]>

Merge branch 'main' into client-pod-attributes

7f0e384

elevran approved these changes Jul 15, 2024

View reviewed changes

zivnevo and others added 2 commits July 16, 2024 12:39

Merge branch 'main' into client-pod-attributes

ef96048

A more consistent attribute naming

3be000b

Signed-off-by: Ziv Nevo <[email protected]>

zivnevo requested a review from michalmalka as a code owner July 17, 2024 09:58

zivnevo merged commit b13602e into clusterlink-net:main Jul 17, 2024
10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More Pod attributes for PDP: SA and labels #665

More Pod attributes for PDP: SA and labels #665

zivnevo commented Jul 4, 2024 •

edited

Loading

orozery Jul 4, 2024

zivnevo Jul 4, 2024

orozery Jul 4, 2024

orozery Jul 4, 2024

zivnevo Jul 4, 2024

orozery Jul 4, 2024

elevran Jul 4, 2024

zivnevo Jul 11, 2024

orozery Jul 4, 2024

zivnevo Jul 4, 2024

orozery Jul 4, 2024

zivnevo Jul 4, 2024

orozery Jul 4, 2024

orozery Jul 8, 2024

zivnevo Jul 10, 2024

elevran commented Jul 4, 2024

elevran Jul 4, 2024

zivnevo commented Jul 15, 2024 •

edited

Loading

More Pod attributes for PDP: SA and labels #665

More Pod attributes for PDP: SA and labels #665

Conversation

zivnevo commented Jul 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elevran commented Jul 4, 2024

Choose a reason for hiding this comment

zivnevo commented Jul 15, 2024 • edited Loading

Client (Pod):

Service (export):

Peer:

zivnevo commented Jul 4, 2024 •

edited

Loading

zivnevo commented Jul 15, 2024 •

edited

Loading