Repository: cloudstack
Updated Branches:
  refs/heads/4.9 a661a11d7 -> cc043e9f8


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/4.9
Commit: a43abbe47bb88bd6dd727e84a8aa2977fee114ee
Parents: b9801ef
Author: Murali Reddy <muralimmre...@gmail.com>
Authored: Tue Sep 20 16:56:06 2016 +0530
Committer: Murali Reddy <muralimmre...@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
         """

Reply via email to