fix: block wireserver port 80 traffic in multitenancy #2395

QxBytes · 2023-11-16T21:41:09Z

Reason for Change:

Pods should not be able to communicate with the wireserver on port 80.

Issue Fixed:

See above.

Requirements:

uses conventional commit messages
includes documentation
adds unit tests
relevant PR labels added

Notes:

wanted to create a unit test for the iptables command failing but there is no mock interface or way of injecting a mock "iptables" dependency.
considered placing the code for the vm iptable rule in transparent vlan endpoint client, but this iptable rule only needs to be run once.
wanted to match the capitalization convention of enabling ipv4 forwarding, but the linter (only now) is giving me warnings about capitalization (not sure why it didn't warn me about the capitalization before).

tamilmani1989 · 2023-11-17T18:07:15Z

network/networkutils/networkutils_linux.go


+func BlockWireserverTraffic() error {
+	// iptables -t filter -I FORWARD -j DROP -d <wireserver ip>/32 -p tcp -m tcp --dport 80
+	dropWireserver := fmt.Sprintf("-d %s/32 -p tcp -m tcp --dport 80", AzureDNS)


should drop for udp as well

Replicating capz rule to block for tcp protocol only as we discussed.

tamilmani1989 · 2023-11-17T18:10:03Z

network/networkutils/networkutils_linux.go

 	return nil
 }

+func BlockWireserverTraffic() error {


can it be BlockEgressTrafficFromContainer(ipAddress, port)?

vipul-21 · 2023-11-20T23:50:27Z

network/network_linux.go

 		}
 		logger.Info("Ipv4 forwarding enabled")
+		if err := networkutils.BlockEgressTrafficFromContainer(networkutils.AzureDNS, iptables.HTTPPort); err != nil {
+			return nil, fmt.Errorf("unable to insert vm iptables rule drop all wireserver port 80 packets: %w", err)


Port 80 in the error message is from iptables.HTTPPort ? if so can we use the variable instead of hard coding.

nit: Should we be using errors.wrapf (not sure but want to keep the cni files consistent going forward)?
@tamilmani1989

Port 80 in the error message is from iptables.HTTPPort ? if so can we use the variable instead of hard coding.

The insert ip table function calls the ExecuteCommand function (which logs the command itself) so I'll probably remove the details (like port or ip) from the error message altogether.

yes we should errors.Wrapf .. @QxBytes can you make that change?

vipul-21 · 2023-11-20T23:57:36Z

network/networkutils/networkutils_linux.go

+func BlockEgressTrafficFromContainer(ipAddress string, port int) error {
+	// iptables -t filter -I FORWARD -j DROP -d <ip>/32 -p tcp -m tcp --dport <port>
+	dropTraffic := fmt.Sprintf("-d %s/32 -p tcp -m tcp --dport %d", ipAddress, port)
+	return errors.Wrap(iptables.InsertIptableRule(iptables.V4, iptables.Filter, iptables.Forward, dropTraffic, iptables.Drop), "iptables block traffic failed")


We are just blocking ipv4 ? Should we make the function intake the type of address as well ? (ipv4 or ipv6)

there is no ipv6 wireserver endpoint. but i asked @QxBytes to create separate PR for blocking all ipv6 addrs from pod.

tamilmani1989 · 2023-11-20T19:10:47Z

network/networkutils/networkutils_linux.go

 	return nil
 }

+func BlockEgressTrafficFromContainer(ipAddress string, port int) error {


should we take protocol also as part of arg?

tamilmani1989 · 2023-11-21T19:27:52Z

network/networkutils/networkutils_linux.go

+func BlockEgressTrafficFromContainer(ipAddress string, port int) error {
+	// iptables -t filter -I FORWARD -j DROP -d <ip>/32 -p tcp -m tcp --dport <port>
+	dropTraffic := fmt.Sprintf("-d %s/32 -p tcp -m tcp --dport %d", ipAddress, port)
+	return errors.Wrap(iptables.InsertIptableRule(iptables.V4, iptables.Filter, iptables.Forward, dropTraffic, iptables.Drop), "iptables block traffic failed")


there is no ipv6 wireserver endpoint. but i asked @QxBytes to create separate PR for blocking all ipv6 addrs from pod.

tamilmani1989 · 2023-11-21T19:28:39Z

network/network_linux.go

 		}
 		logger.Info("Ipv4 forwarding enabled")
+		if err := networkutils.BlockEgressTrafficFromContainer(networkutils.AzureDNS, iptables.HTTPPort); err != nil {
+			return nil, fmt.Errorf("unable to insert vm iptables rule drop all wireserver port 80 packets: %w", err)


yes we should errors.Wrapf .. @QxBytes can you make that change?

tamilmani1989 · 2023-11-21T19:29:03Z

network/network_linux.go

 		}
 		logger.Info("Ipv4 forwarding enabled")
+		if err := networkutils.BlockEgressTrafficFromContainer(iptables.V4, networkutils.AzureDNS, iptables.HTTPPort); err != nil {
+			return nil, fmt.Errorf("unable to insert vm iptables rule drop wireserver packets: %w", err)


should use errors.wrapf

tamilmani1989 · 2023-11-27T18:25:38Z

network/transparent_vlan_endpointclient_linux.go

 	if err := iptables.InsertIptableRule(iptables.V4, "mangle", "PREROUTING", match, "ACCEPT"); err != nil {
 		return errors.Wrap(err, "unable to insert iptables rule accept all incoming from vlan interface")
 	}
+


can you add a comment this is for blocking wireserver traffic from customer vnet nic

tamilmani1989 · 2023-11-27T18:26:11Z

network/network_linux.go

+			return nil, errors.Wrap(err, "ipv4 forwarding failed")
 		}
 		logger.Info("Ipv4 forwarding enabled")
+		if err := networkutils.BlockEgressTrafficFromContainer(iptables.V4, networkutils.AzureDNS, iptables.TCP, iptables.HTTPPort); err != nil {


add a comment this is for blocking wireserver traffic from apipa nic

* Add vm and vnet ns block wireserver port 80 rule * Use existing variable for known ip * Move code to networkutils * Address feedback * Address iptables version feedback * Address protocol and format feedback * Add comments * Remove cidr in case ipv6 is used

QxBytes added cni Related to CNI. fix Fixes something. multitenancy labels Nov 16, 2023

QxBytes force-pushed the alew/block-wireserver branch from 2a4d40d to c6acc01 Compare November 16, 2023 22:13

QxBytes marked this pull request as ready for review November 16, 2023 22:34

QxBytes requested a review from a team as a code owner November 16, 2023 22:34

QxBytes requested a review from aranta-rokade November 16, 2023 22:34

QxBytes force-pushed the alew/block-wireserver branch from 82e82a1 to 658d3c9 Compare November 16, 2023 23:02

tamilmani1989 requested changes Nov 17, 2023

View reviewed changes

QxBytes force-pushed the alew/block-wireserver branch from 987e54b to 744d816 Compare November 18, 2023 00:16

vipul-21 reviewed Nov 20, 2023

View reviewed changes

QxBytes force-pushed the alew/block-wireserver branch 2 times, most recently from aa25ae8 to 87aee51 Compare November 21, 2023 18:00

tamilmani1989 requested changes Nov 21, 2023

View reviewed changes

vipul-21 previously approved these changes Nov 21, 2023

View reviewed changes

QxBytes force-pushed the alew/block-wireserver branch from 2d7b3b2 to 90e6e83 Compare November 22, 2023 00:04

tamilmani1989 reviewed Nov 27, 2023

View reviewed changes

QxBytes dismissed vipul-21’s stale review via 6a87b0e November 27, 2023 18:45

tamilmani1989 previously approved these changes Nov 28, 2023

View reviewed changes

QxBytes dismissed tamilmani1989’s stale review via 252da62 November 28, 2023 18:40

QxBytes force-pushed the alew/block-wireserver branch from 252da62 to 317948f Compare November 28, 2023 19:21

vipul-21 approved these changes Nov 28, 2023

View reviewed changes

tamilmani1989 approved these changes Nov 29, 2023

View reviewed changes

QxBytes added 7 commits November 29, 2023 10:08

Add vm and vnet ns block wireserver port 80 rule

485f115

Use existing variable for known ip

19a258e

Move code to networkutils

9cc28e0

Address feedback

9f959ef

Address iptables version feedback

9cfde29

Address protocol and format feedback

169841d

Add comments

b6a97e4

Remove cidr in case ipv6 is used

79a2698

QxBytes force-pushed the alew/block-wireserver branch from 317948f to 79a2698 Compare November 29, 2023 18:08

QxBytes merged commit 2382637 into master Nov 29, 2023

QxBytes deleted the alew/block-wireserver branch November 29, 2023 21:37

QxBytes mentioned this pull request Feb 6, 2024

fix: block pod to wireserver port 80 traffic on windows multitenancy #2515

Merged

4 tasks

fix: block wireserver port 80 traffic in multitenancy #2395

fix: block wireserver port 80 traffic in multitenancy #2395

Uh oh!

Conversation

QxBytes commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

QxBytes commented Nov 16, 2023 •

edited

Loading