Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mirror port #219

Closed
l-rossetti opened this issue Feb 9, 2022 · 9 comments
Closed

Mirror port #219

l-rossetti opened this issue Feb 9, 2022 · 9 comments

Comments

@l-rossetti
Copy link
Contributor

Is it possibile to inject a mirror (span) port into a pod? Can't find any reference in code or documentation. Thanks

@phoracek
Copy link
Member

phoracek commented Feb 9, 2022

It is not possible with the current implementation. Having OVS' port mirroring available in the CNI would be nice, do you have an idea of how the API could look like?

@l-rossetti
Copy link
Contributor Author

To create a mirror it could be done in two places:

  • at NetworkAttachmentDefinition level, so that all the ports of a specific vlan are mirrored: mirror: "mirror0"
  • at pod level, so that the specific port of this pod is mirrored: mirror: "mirror0"

While to inject the mirror interface we may need on the destination pod an annotation like bind_to_mirror: "mirror0"

Do you think it's reasonable?

@phoracek
Copy link
Member

From the two options, I think it should be on the NetworkAttachmentDefinition level. This object is managed by an administrator while Pod belongs to untrustworthy users (sorry users). If we exposed this option on Pod, users could abuse it and force their traffic to be mirrored where it should not. Having this on NetworkAttachmentDefinition sounds good.

Could be the bind-to-mirror attribute part of the NetworkAttachmentDefinition too? For the same reasons as described in the paragraph above.


One remaining question is about scheduling - I assume that mirroring is limited to the node, right? So somebody would need to make sure that both the producer and consumer pods are on the same node. We can make this easy for ourselves by saying that this is a responsibility of the owner of the consumer pod - they have to set their podAffinity to land on the same node as the producer. Would that be sufficient?

@l-rossetti
Copy link
Contributor Author

l-rossetti commented Feb 11, 2022

From the two options, I think it should be on the NetworkAttachmentDefinition level. This object is managed by an administrator while Pod belongs to untrustworthy users (sorry users). If we exposed this option on Pod, users could abuse it and force their traffic to be mirrored where it should not. Having this on NetworkAttachmentDefinition sounds good.

I partially agree with this concern. Allowing users to mirror the pod traffic with an annotation on the pod itself can be an issue only in terms on waste of resources, but is not adding any security risk. The risk is more on allowing consumers to read mirrored traffic. Moreover having the annotation only at NetworkAttachmentDefinition means that the whole VLAN would be mirrored even tough you may want to mirror just the traffic of a subset of pods/ports.
Anyway I believe that starting with NetworkAttachmentDefinition can be already a good starting point.
Having the "configuration" at pod vs NAD impacts on the flexibility of the mirroring feature of the CNI plugin both for consumers and producers.

Could be the bind-to-mirror attribute part of the NetworkAttachmentDefinition too? For the same reasons as described in the paragraph above.

I have some doubts on how the bind-to-mirror might work if we declare it at NetworkAttachmentDefinition level. The thing is that we need to able to say "I want this pod to be able to read the mirrored traffic". How can we do that from NAD?

One remaining question is about scheduling - I assume that mirroring is limited to the node, right? So somebody would need to make sure that both the producer and consumer pods are on the same node. We can make this easy for ourselves by saying that this is a responsibility of the owner of the consumer pod - they have to set their podAffinity to land on the same node as the producer. Would that be sufficient?

I guess what you stated is correct. Not sure how this mirroring can be done in a distributed environment, probably either you can't or it is very complex and network-bandwidth consuming. Anyway I think it's up to the owner responsibility to schedule pods on the right nodes via podAffinity.

@phoracek
Copy link
Member

Thanks for the detailed response, I will answer inline.

I partially agree with this concern. Allowing users to mirror the pod traffic with an annotation on the pod itself can be an issue only in terms on waste of resources, but is not adding any security risk.

But if there is an option for a third Pod to inject its traffic into the mirror, you would no longer be able to prove the source of the sniffed traffic and it could add noise to your captured data, no? I don't have experience with use of port mirroring, so pardon my ignorance.

The risk is more on allowing consumers to read mirrored traffic. Moreover having the annotation only at NetworkAttachmentDefinition means that the whole VLAN would be mirrored even tough you may want to mirror just the traffic of a subset of pods/ports.

I see your point here. The option would be to create a duplicated NetworkAttachmentDefinition for the Pod you want to monitor, with additional mirror attribute. But that may cause problems if the IPAM you use treats each NetworkAttachmentDefinition individually - e.g. creates a subnet for each.

On the other hand, I don't know if Multus allows you to pass arbitrary arguments in the networks label. If it does not, it would be tough to find a good place for the new API.

Anyway I believe that starting with NetworkAttachmentDefinition can be already a good starting point. Having the "configuration" at pod vs NAD impacts on the flexibility of the mirroring feature of the CNI plugin both for consumers and producers.

Sounds good to me.

I have some doubts on how the bind-to-mirror might work if we declare it at NetworkAttachmentDefinition level. The thing is that we need to able to say "I want this pod to be able to read the mirrored traffic". How can we do that from NAD?

We could take an approach that is similar to the producer - you would create a duplicated NetworkAttachmentDefinition. It would be the same as the regular one, but also set the bind-to-mirror attribute.

I wonder, would it make sense to treat this as a standalone CNI (it could live under this project and use the infrastructure). I'm thinking - ovs-mirror-producer CNI would allow configuration of the producer via chaining it with the ovs CNI. And then ovs-cni-mirror-consumer can be its own standalone CNI, that when requested, would insert an interface into the consumer Pod with all the mirrored traffic... Just an idea.

Before we get to implementation, it would be important to make sure the design is sound. If you decide to start working on this, would you please first post a design document under docs/ directory. It should propose the API and how will the CNI behave. Reviewing that first by the community would make sure we don't run into any unpleasant surprises later on during the implementation.

@l-rossetti
Copy link
Contributor Author

Sorry for the delay of my response, at the moment we are compiling ovs-cni plugin by adding some logs in order to get a better understanding of the overall functioning.

But if there is an option for a third Pod to inject its traffic into the mirror, you would no longer be able to prove the source of the sniffed traffic and it could add noise to your captured data, no? I don't have experience with use of port mirroring, so pardon my ignorance.

Normally you want to capture the mirrored traffic and pass it through an IDS/IPS system or a net-probe software in order to catch any unusual behaviour that could harm the network. Adding some noise doesn't cause an issue but I can think of at least two security threats:

  • from a producer point of view, we can fall into an IP spoofing attack: an attacker pod requesting to mirror its traffic might send network packets by replacing the source IP with the victim's IP, basically stealing the victim pod's identity. In this case the mirrored traffic wouldn't be 100% reliable and the attack might be hard to detect.
  • on the consumer side, if any pod is able to get an interface on the port of the mirrored traffic (e.g. via the bind-to-mirror keyword), this pod would be able to see everything on the network.

So I agree with you, we need to think really carefully on how avoid these kind of threats.

I see your point here. The option would be to create a duplicated NetworkAttachmentDefinition for the Pod you want to monitor, with additional mirror attribute. But that may cause problems if the IPAM you use treats each NetworkAttachmentDefinition individually - e.g. creates a subnet for each.

That's a good note. For our use case we are not using any IPAM cause we need a real DHCP server pod to emulate a realistic network behaviour. So this not a blocker to us but for sure this is something to take into consideration to make the plugin fully compatible with other use cases.

On the other hand, I don't know if Multus allows you to pass arbitrary arguments in the networks label. If it does not, it would be tough to find a good place for the new API

I don't know either. I've tried adding an additional param into the networks label and then trying to log all the arguments arriving to the plugin but my parameter is never shown. Anyway it sounds strange to me that Multus doesn't allow the integration of additional parameters (but I might be very wrong). What I know is that other cni plugins support additional annotations that are defined outside the networks one. Maybe we could do something similar but honestly I didn't understand how these additional annotations are interpreted and who does it.

We could take an approach that is similar to the producer - you would create a duplicated NetworkAttachmentDefinition. It would be the same as the regular one, but also set the bind-to-mirror attribute.

It makes sense. Basically we'll end up with three "types" of NADs. The first is the regular one with no mirroring, the other two allow users to either produce to the mirror or consume from the mirror.

I wonder, would it make sense to treat this as a standalone CNI (it could live under this project and use the infrastructure). I'm thinking - ovs-mirror-producer CNI would allow configuration of the producer via chaining it with the ovs CNI. And then ovs-cni-mirror-consumer can be its own standalone CNI, that when requested, would insert an interface into the consumer Pod with all the mirrored traffic... Just an idea.

From an architectural point of view it does make a lot of sense. Seems a very clean approach.

Before we get to implementation, it would be important to make sure the design is sound. If you decide to start working on this, would you please first post a design document under docs/ directory. It should propose the API and how will the CNI behave. Reviewing that first by the community would make sure we don't run into any unpleasant surprises later on during the implementation.

Sure, we can collaborate on that. Thank you for being interested in our use case! I guess we'll need support from the community cause this is the first time we look so in detail into the CNI.

@phoracek
Copy link
Member

phoracek commented Mar 2, 2022

Anyway it sounds strange to me that Multus doesn't allow the integration of additional parameters (but I might be very wrong).

I think you are right. I looked into Multus' API definitions and there is a special field in the networks annotation, cni-args which is pretty much free form: https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/pkg/types/types.go#L119

Let me know how this goes, it sounds interesting for sure o/

@l-rossetti
Copy link
Contributor Author

l-rossetti commented Mar 2, 2022

I think you are right. I looked into Multus' API definitions and there is a special field in the networks annotation, cni-args which is pretty much free form: https://github.com/k8snetworkplumbingwg/multus-cni/blob/master/pkg/types/types.go#L119

I was just now updating the Issue to give you an update on this same topic, when I've seen your message being popped up! That's crazy :) haha

Anyway... I've run a simple test to verify that, by using the following annotation:

k8s.v1.cni.cncf.io/networks: | 
          [
            {
              "name":"ovs-vlan-9",
              "interface":"eth1",
              "namespace":"emu-cni",
              "cni-args": {
                  "args1": "val1"
              }
            }
          ]

By printing the content of args.StdinData from here I'm able to see { "args1": "val1" } from the plugin (so basically NAD config is merged with cni-args):
{"level":"info","ts":1646238388.4014704,"caller":"plugin/plugin.go:363","msg":"the config data: {\"args\":{\"cni\":{\"args1\":\"val1\"}},\"bridge\":\"ovs-bridge\",\"cniVersion\":\"0.4.0\",\"name\":\"ovs-vlan-9\",\"type\":\"ovs\",\"vlan\":9}\n"}

I guess we only miss it in the NetConf struct to correctly parse cni-args, but that's good to know there is also this possibility in case. Thank you for sharing.

@l-rossetti
Copy link
Contributor Author

We opened a PR containing the proposal #227

@phoracek phoracek closed this as completed Jan 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants