You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ra...@apache.org on 2016/09/21 09:30:57 UTC

[1/3] git commit: updated refs/heads/master to 43f82c9

Repository: cloudstack
Updated Branches:
  refs/heads/master 3f6faeb4c -> 43f82c991


fix egress rule incorrect behavior

CLOUDSTACK-9480: Egress Firewall: Incorrect use of Allow/Deny for ICMP

     fix ensures, ICMP, TCP, UDP are handled similalry w.r.t egress rule action

CLOUDSTACK-9495: Egress rules functionalty broken when protocol=all specified

     when protocol=all specified, CIDR was ignored. Fix ensures if CIDR is specified
     its always used in configuring iptable rules

 2 new test cased to test /32 CIDR


Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/a43abbe4
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/a43abbe4
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/a43abbe4

Branch: refs/heads/master
Commit: a43abbe47bb88bd6dd727e84a8aa2977fee114ee
Parents: b9801ef
Author: Murali Reddy <mu...@gmail.com>
Authored: Tue Sep 20 16:56:06 2016 +0530
Committer: Murali Reddy <mu...@gmail.com>
Committed: Tue Sep 20 16:56:06 2016 +0530

----------------------------------------------------------------------
 .../debian/config/opt/cloud/bin/configure.py    | 69 ++++++++++----------
 .../component/test_egress_fw_rules.py           | 56 ++++++++++++++++
 2 files changed, 90 insertions(+), 35 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/cloudstack/blob/a43abbe4/systemvm/patches/debian/config/opt/cloud/bin/configure.py
----------------------------------------------------------------------
diff --git a/systemvm/patches/debian/config/opt/cloud/bin/configure.py b/systemvm/patches/debian/config/opt/cloud/bin/configure.py
index dd164a2..3c01574 100755
--- a/systemvm/patches/debian/config/opt/cloud/bin/configure.py
+++ b/systemvm/patches/debian/config/opt/cloud/bin/configure.py
@@ -46,9 +46,9 @@ from cs.CsStaticRoutes import CsStaticRoutes
 
 
 class CsPassword(CsDataBag):
-    
+
     TOKEN_FILE="/tmp/passwdsrvrtoken"
-    
+
     def process(self):
         for item in self.dbag:
             if item == "id":
@@ -99,7 +99,7 @@ class CsAcl(CsDataBag):
 
             self.rule['allowed'] = True
             self.rule['action'] = "ACCEPT"
-                
+
             if self.rule['type'] == 'all' and not obj['source_cidr_list']:
                 self.rule['cidr'] = ['0.0.0.0/0']
             else:
@@ -145,41 +145,40 @@ class CsAcl(CsDataBag):
             logging.debug("Current ACL IP direction is ==> %s", self.direction)
             if self.direction == 'egress':
                 self.fw.append(["filter", "", " -A FW_OUTBOUND -j FW_EGRESS_RULES"])
-                if rule['protocol'] == "icmp":
-                    self.fw.append(["filter", "front",
-                                    " -A FW_EGRESS_RULES" +
-                                    " -s %s " % cidr +
-                                    " -p %s " % rule['protocol'] +
-                                    " -m %s " % rule['protocol'] +
-                                    " --icmp-type %s -j %s" % (icmp_type, self.rule['action'])])
+
+                fwr = " -I FW_EGRESS_RULES"
+                # In case we have a default rule (accept all or drop all), we have to evaluate the action again.
+                if rule['type'] == 'all' and not rule['source_cidr_list']:
+                    fwr = " -A FW_EGRESS_RULES"
+                    # For default egress ALLOW or DENY, the logic is inverted.
+                    # Having default_egress_policy == True, means that the default rule should have ACCEPT,
+                    # otherwise DROP. The rule should be appended, not inserted.
+                    if self.rule['default_egress_policy']:
+                        self.rule['action'] = "ACCEPT"
+                    else:
+                        self.rule['action'] = "DROP"
                 else:
-                    fwr = " -I FW_EGRESS_RULES"
-                    #In case we have a default rule (accept all or drop all), we have to evaluate the action again.
-                    if rule['type'] == 'all' and not rule['source_cidr_list']:
-                        fwr = " -A FW_EGRESS_RULES"
-                        # For default egress ALLOW or DENY, the logic is inverted.
-                        # Having default_egress_policy == True, means that the default rule should have ACCEPT,
-                        # otherwise DROP. The rule should be appended, not inserted.
-                        if self.rule['default_egress_policy']:
-                            self.rule['action'] = "ACCEPT"
-                        else:
-                            self.rule['action'] = "DROP"
+                    # For other rules added, if default_egress_policy == True, following rules should be DROP,
+                    # otherwise ACCEPT
+                    if self.rule['default_egress_policy']:
+                        self.rule['action'] = "DROP"
                     else:
-                        # For other rules added, if default_egress_policy == True, following rules should be DROP,
-                        # otherwise ACCEPT
-                        if self.rule['default_egress_policy']:
-                            self.rule['action'] = "DROP"
-                        else:
-                            self.rule['action'] = "ACCEPT"
-
-                    if rule['protocol'] != "all":
-                        fwr += " -s %s " % cidr + \
-                               " -p %s " % rule['protocol'] + \
-                               " -m %s " % rule['protocol'] + \
-                               " --dport %s" % rnge
-
-                    self.fw.append(["filter", "", "%s -j %s" % (fwr, rule['action'])])
+                        self.rule['action'] = "ACCEPT"
 
+                if rule['protocol'] == "icmp":
+                    fwr += " -s %s " % cidr + \
+                                    " -p %s " % rule['protocol'] + \
+                                    " -m %s " % rule['protocol'] + \
+                                    " --icmp-type %s" % icmp_type
+                elif rule['protocol'] != "all":
+                    fwr += " -s %s " % cidr + \
+                           " -p %s " % rule['protocol'] + \
+                           " -m %s " % rule['protocol'] + \
+                           " --dport %s" % rnge
+                elif rule['protocol'] == "all":
+                    fwr += " -s %s " % cidr
+
+                self.fw.append(["filter", "", "%s -j %s" % (fwr, rule['action'])])
                 logging.debug("EGRESS rule configured for protocol ==> %s, action ==> %s", rule['protocol'], rule['action'])
 
     class AclDevice():

http://git-wip-us.apache.org/repos/asf/cloudstack/blob/a43abbe4/test/integration/component/test_egress_fw_rules.py
----------------------------------------------------------------------
diff --git a/test/integration/component/test_egress_fw_rules.py b/test/integration/component/test_egress_fw_rules.py
index f362421..9d13a23 100755
--- a/test/integration/component/test_egress_fw_rules.py
+++ b/test/integration/component/test_egress_fw_rules.py
@@ -348,6 +348,26 @@ class TestEgressFWRules(cloudstackTestCase):
         except Exception as e:
             self.fail("Warning! Cleanup failed: %s" % e)
 
+    def create_another_vm(self):
+        self.debug("Deploying instance in the account: %s and network: %s" % (self.account.name, self.network.id))
+
+        project = None
+        self.virtual_machine1 = VirtualMachine.create(self.apiclient,
+                                                         self.services["virtual_machine"],
+                                                         accountid=self.account.name,
+                                                         domainid=self.domain.id,
+                                                         serviceofferingid=self.service_offering.id,
+                                                         mode=self.zone.networktype,
+                                                         networkids=[str(self.network.id)],
+                                                         projectid=project.id if project else None)
+        self.debug("Deployed instance %s in account: %s" % (self.virtual_machine.id,self.account.name))
+
+        # Checking if VM is running or not, in case it is deployed in error state, test case fails
+        self.vm_list = list_virtual_machines(self.apiclient, id=self.virtual_machine.id)
+
+        self.assertEqual(validateList(self.vm_list)[0], PASS, "vm list validation failed, vm list is %s" % self.vm_list)
+        self.assertEqual(str(self.vm_list[0].state).lower(),'running',"VM state should be running, it is %s" % self.vm_list[0].state)
+
     @attr(tags=["advanced"], required_hardware="true")
     def test_01_egress_fr1(self):
         """Test By-default the communication from guest n/w to public n/w is allowed.
@@ -385,6 +405,25 @@ class TestEgressFWRules(cloudstackTestCase):
                                     "['100']",
                                     negative_test=False)
 
+    @attr(tags=["advanced"], required_hardware="true")
+    def test_01_2_egress_fr1(self):
+        """Test egress rule with /32 CIDR of a VM, and check other VM in the
+           network does not have public access
+        """
+        # Validate the following:
+        # 1. deploy VM using network offering with egress policy false.
+        # 2. deploy another VM into the network created in step #1
+        # 3. create egress rule with /32 CIDR of the second VM
+        # 4. login to first VM.
+        # 5. ping public network.
+        # 6. public network should not be reachable from the first VM.
+        self.create_vm(egress_policy=False)
+        self.create_another_vm()
+        self.createEgressRule(protocol='all', cidr=self.virtual_machine1.ipaddress+"/32")
+        self.exec_script_on_user_vm('ping -c 1 www.google.com',
+                                    "| grep -oP \'\d+(?=% packet loss)\'",
+                                    "['100']",
+                                    negative_test=False)
 
     @attr(tags=["advanced"], required_hardware="true")
     def test_02_egress_fr2(self):
@@ -421,6 +460,23 @@ class TestEgressFWRules(cloudstackTestCase):
                                     negative_test=False)
 
     @attr(tags=["advanced"], required_hardware="true")
+    def test_02_2_egress_fr2(self):
+        """Test Allow Communication using Egress rule with /32 CIDR + Port Range + Protocol.
+        """
+        # Validate the following:
+        # 1. deploy VM using network offering with egress policy false.
+        # 3. create egress rule with specific /32 CIDR + port range.
+        # 4. login to VM.
+        # 5. ping public network.
+        # 6. public network should be reachable from the VM.
+        self.create_vm(egress_policy=False)
+        self.createEgressRule(cidr=self.virtual_machine.ipaddress+"/32")
+        self.exec_script_on_user_vm('ping -c 1 www.google.com',
+                                    "| grep -oP \'\d+(?=% packet loss)\'",
+                                    "['0']",
+                                    negative_test=False)
+
+    @attr(tags=["advanced"], required_hardware="true")
     def test_03_egress_fr3(self):
         """Test Communication blocked with network that is other than specified
         """


[2/3] git commit: updated refs/heads/master to 43f82c9

Posted by ra...@apache.org.
Merge pull request #1666 from murali-reddy/egress_rules

CLOUDSTACK-9480,  CLOUDSTACK-9495 fix egress rule incorrect behaviorWhen 'default egress policy' is set to 'allow' in the network offering, any egress rule that is added will 'deny' the traffic overriding the default behaviour.

Conversely, when 'default egress policy' is set to 'deny' in the network offering, any egress rule that is added will 'allow' the traffic overriding the default behaviour.

While this works for 'tcp', 'udp' as expected, for 'icmp' protocol its always set to ALLOW. This patch keeps all protocols behaviour consistent.

Results of running test/integration/component/test_egress_fw_rules.py.  With out the patch test_02_egress_fr2 test was failing. This patch fixes the test_02_egress_fr2  scenario.
-----------------------------------------------------------------------------------------------------
Test By-default the communication from guest n/w to public n/w is NOT allowed. ... === TestName: test_01_1_egress_fr1 | Status : SUCCESS ===
ok
Test By-default the communication from guest n/w to public n/w is allowed. ... === TestName: test_01_egress_fr1 | Status : SUCCESS ===
ok
Test Allow Communication using Egress rule with CIDR + Port Range + Protocol. ... === TestName: test_02_1_egress_fr2 | Status : SUCCESS ===
ok
Test Allow Communication using Egress rule with CIDR + Port Range + Protocol. ... === TestName: test_02_egress_fr2 | Status : SUCCESS ===
ok
Test Communication blocked with network that is other than specified ... === TestName: test_03_1_egress_fr3 | Status : SUCCESS ===
ok
Test Communication blocked with network that is other than specified ... === TestName: test_03_egress_fr3 | Status : SUCCESS ===
ok
Test Create Egress rule and check the Firewall_Rules DB table ... === TestName: test_04_1_egress_fr4 | Status : SUCCESS ===
ok
Test Create Egress rule and check the Firewall_Rules DB table ... === TestName: test_04_egress_fr4 | Status : SUCCESS ===
ok
Test Create Egress rule and check the IP tables ... SKIP: Skip
Test Create Egress rule and check the IP tables ... SKIP: Skip
Test Create Egress rule without CIDR ... === TestName: test_06_1_egress_fr6 | Status : SUCCESS ===
ok
Test Create Egress rule without CIDR ... === TestName: test_06_egress_fr6 | Status : SUCCESS ===
ok
Test Create Egress rule without End Port ... === TestName: test_07_1_egress_fr7 | Status : EXCEPTION ===
ERROR
Test Create Egress rule without End Port ... === TestName: test_07_egress_fr7 | Status : SUCCESS ===
ok
Test Port Forwarding and Egress Conflict ... SKIP: Skip
Test Port Forwarding and Egress Conflict ... SKIP: Skip
Test Delete Egress rule ... === TestName: test_09_1_egress_fr9 | Status : SUCCESS ===
ok
Test Delete Egress rule ... === TestName: test_09_egress_fr9 | Status : SUCCESS ===
ok
Test Invalid CIDR and Invalid Port ranges ... === TestName: test_10_1_egress_fr10 | Status : SUCCESS ===
ok
Test Invalid CIDR and Invalid Port ranges ... === TestName: test_10_egress_fr10 | Status : SUCCESS ===
ok
Test Regression on Firewall + PF + LB + SNAT ... === TestName: test_11_1_egress_fr11 | Status : SUCCESS ===
ok
Test Regression on Firewall + PF + LB + SNAT ... === TestName: test_11_egress_fr11 | Status : SUCCESS ===
ok
Test Reboot Router ... === TestName: test_12_1_egress_fr12 | Status : SUCCESS ===
ok
Test Reboot Router ... === TestName: test_12_egress_fr12 | Status : EXCEPTION ===
ERROR
Test Redundant Router : Master failover ... === TestName: test_13_1_egress_fr13 | Status : SUCCESS ===
ok
Test Redundant Router : Master failover ... === TestName: test_13_egress_fr13 | Status : SUCCESS ===
ok
-----------------------------------------------------------------------------------------------------

* pr/1666:
  fix egress rule incorrect behavior

Signed-off-by: Rajani Karuturi <ra...@accelerite.com>


Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/cc043e9f
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/cc043e9f
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/cc043e9f

Branch: refs/heads/master
Commit: cc043e9f8f834c4bc7dcf7a3a04fce63f2a8480d
Parents: a661a11 a43abbe
Author: Rajani Karuturi <ra...@accelerite.com>
Authored: Wed Sep 21 14:54:31 2016 +0530
Committer: Rajani Karuturi <ra...@accelerite.com>
Committed: Wed Sep 21 14:54:31 2016 +0530

----------------------------------------------------------------------
 .../debian/config/opt/cloud/bin/configure.py    | 69 ++++++++++----------
 .../component/test_egress_fw_rules.py           | 56 ++++++++++++++++
 2 files changed, 90 insertions(+), 35 deletions(-)
----------------------------------------------------------------------



[3/3] git commit: updated refs/heads/master to 43f82c9

Posted by ra...@apache.org.
Merge release branch 4.9 to master

* 4.9:
  fix egress rule incorrect behavior


Project: http://git-wip-us.apache.org/repos/asf/cloudstack/repo
Commit: http://git-wip-us.apache.org/repos/asf/cloudstack/commit/43f82c99
Tree: http://git-wip-us.apache.org/repos/asf/cloudstack/tree/43f82c99
Diff: http://git-wip-us.apache.org/repos/asf/cloudstack/diff/43f82c99

Branch: refs/heads/master
Commit: 43f82c991cd5c9e217489eaeec4bda2df93a549c
Parents: 3f6faeb cc043e9
Author: Rajani Karuturi <ra...@accelerite.com>
Authored: Wed Sep 21 15:00:30 2016 +0530
Committer: Rajani Karuturi <ra...@accelerite.com>
Committed: Wed Sep 21 15:00:30 2016 +0530

----------------------------------------------------------------------
 .../debian/config/opt/cloud/bin/configure.py    | 69 ++++++++++----------
 .../component/test_egress_fw_rules.py           | 56 ++++++++++++++++
 2 files changed, 90 insertions(+), 35 deletions(-)
----------------------------------------------------------------------