Do not add L34 flows if remotesub missing

mchalla · tomflynn · commit 0e19e137deb7 · 2020-05-02T07:06:18.000Z
This bug was reported by Cisco IT where a secgrp with
no remote subnets basically allows all ingress traffic
due to how we program the rules. Since this is a day 0
issue we decided to fix the behavior while at the same
time providing an option for someone to revert to the
old behavior.

- If remote subnet is missing do not add L3 and L4 flows
- A new function add_l2classifier_entries takes care of
  adding l2 only flows
- Add :
    "behavior": {
        "l34flows-without-subnet" : true
    }
 to opflex config to override this behavior with old, default is false.
- verified make check passes with / without this option since I added
  default 0.0.0.0/0 ::/0 subnet to relevant secgroups.

Signed-off-by: Madhu Challa &lt;challa@gmail.com&gt;
Change-Id: Iffa03301c24bd576034f5f5858ebb8c75fa72f5e
diff --git a/agent-ovs/lib/Agent.cpp b/agent-ovs/lib/Agent.cpp
@@ -71,6 +71,7 @@ Agent::Agent(OFFramework& framework_, const LogParams& _logParams)
       prometheusEnabled(true),
       prometheusExposeLocalHostOnly(false),
       prometheusExposeEpSvcNan(false),
+      behaviorL34FlowsWithoutSubnet(false),
       logParams(_logParams) {
 #else
 Agent::Agent(OFFramework& framework_, const LogParams& _logParams)
@@ -84,6 +85,7 @@ Agent::Agent(OFFramework& framework_, const LogParams& _logParams)
       contractInterval(0), securityGroupInterval(0), interfaceInterval(0),
       spanManager(framework, agent_io),
       netflowManager(framework,agent_io),
+      behaviorL34FlowsWithoutSubnet(false),
       logParams(_logParams) {
 #endif
     std::random_device rng;
@@ -194,6 +196,7 @@ void Agent::setProperties(const boost::property_tree::ptree& properties) {
     static const std::string OPFLEX_PRR_INTERVAL("opflex.timers.prr");
     static const std::string OPFLEX_HANDSHAKE("opflex.timers.handshake-timeout");
     static const std::string DISABLED_FEATURES("feature.disabled");
+    static const std::string BEHAVIOR_L34FLOWS_WITHOUT_SUBNET("behavior.l34flows-without-subnet");
 
     // set feature flags to true
     clearFeatureFlags();
@@ -254,6 +257,12 @@ void Agent::setProperties(const boost::property_tree::ptree& properties) {
             disabledFeaturesSet.insert(v.second.data());
     }
 
+    optional<bool> behaviorAddL34FlowsWithoutSubnet =
+        properties.get_optional<bool>(BEHAVIOR_L34FLOWS_WITHOUT_SUBNET);
+    if (behaviorAddL34FlowsWithoutSubnet) {
+        behaviorL34FlowsWithoutSubnet = behaviorAddL34FlowsWithoutSubnet.get();
+    }
+
 #ifdef HAVE_PROMETHEUS_SUPPORT
     boost::optional<bool> prometheusIsEnabled =
                 properties.get_optional<bool>(PROMETHEUS_ENABLED);
diff --git a/agent-ovs/lib/include/opflexagent/Agent.h b/agent-ovs/lib/include/opflexagent/Agent.h
@@ -289,6 +289,21 @@ typedef opflex::ofcore::OFConstants::OpflexElementMode opflex_elem_t;
      */
     bool isFeatureEnabled(FeatureList feature) { return featureFlag[feature];}
 
+    /**
+     * get behavior for adding l34flows without subnet
+     * @return true if l34flows should be added without subnet,
+     * false otherwise, defaults to false
+     */
+    bool addL34FlowsWithoutSubnet() { return behaviorL34FlowsWithoutSubnet; }
+
+    /**
+     * set behavior for adding l34flows without subnet
+     * @param value the new value for behaviorL34FlowsWithoutSubnet
+     */
+    void setAddL34FlowsWithoutSubnet(bool value) {
+        behaviorL34FlowsWithoutSubnet = value;
+    }
+
     /**
      * clear feature flags. set them to true.
      */
@@ -411,6 +426,7 @@ typedef opflex::ofcore::OFConstants::OpflexElementMode opflex_elem_t;
     bool prometheusExposeEpSvcNan;
     std::unordered_set<std::string> prometheusEpAttributes;
 #endif
+    bool behaviorL34FlowsWithoutSubnet;
     LogParams logParams;
 };
 
diff --git a/agent-ovs/ovs/AccessFlowManager.cpp b/agent-ovs/ovs/AccessFlowManager.cpp
@@ -591,13 +591,18 @@ void AccessFlowManager::handleSecGrpSetUpdate(const uri_set_t& secGrps,
 
         for (shared_ptr<PolicyRule>& pc : rules) {
             uint8_t dir = pc->getDirection();
+            bool skipL34 = false;
             const shared_ptr<L24Classifier>& cls = pc->getL24Classifier();
             const URI& ruleURI = cls.get()->getURI();
             uint64_t secGrpCookie =
                 idGen.getId("l24classifierRule", ruleURI.toString());
             boost::optional<const network::subnets_t&> remoteSubs;
             if (!pc->getRemoteSubnets().empty())
                 remoteSubs = pc->getRemoteSubnets();
+            else
+                skipL34 = !agent.addL34FlowsWithoutSubnet();
+
+            LOG(DEBUG) << "skipL34 flows: " << skipL34;
 
             flowutils::ClassAction act = flowutils::CA_DENY;
             if (pc->getAllow()) {
@@ -609,6 +614,35 @@ void AccessFlowManager::handleSecGrpSetUpdate(const uri_set_t& secGrps,
                 }
             }
 
+            /*
+             * Do not program higher level protocols
+             * when remote subnet is missing
+             * except when agent.addL34FlowsWithoutSubnet() == true
+             */
+            if (skipL34) {
+                if (dir == DirectionEnumT::CONST_BIDIRECTIONAL ||
+                    dir == DirectionEnumT::CONST_IN) {
+                    flowutils::add_l2classifier_entries(*cls, act,
+                                                        OUT_TABLE_ID,
+                                                        pc->getPriority(),
+                                                        OFPUTIL_FF_SEND_FLOW_REM,
+                                                        secGrpCookie,
+                                                        secGrpSetId, 0,
+                                                        secGrpIn);
+                }
+                if (dir == DirectionEnumT::CONST_BIDIRECTIONAL ||
+                    dir == DirectionEnumT::CONST_OUT) {
+                    flowutils::add_l2classifier_entries(*cls, act,
+                                                        OUT_TABLE_ID,
+                                                        pc->getPriority(),
+                                                        OFPUTIL_FF_SEND_FLOW_REM,
+                                                        secGrpCookie,
+                                                        secGrpSetId, 0,
+                                                        secGrpOut);
+                }
+                continue;
+            }
+
             if (dir == DirectionEnumT::CONST_BIDIRECTIONAL ||
                 dir == DirectionEnumT::CONST_IN) {
                 flowutils::add_classifier_entries(*cls, act,
diff --git a/agent-ovs/ovs/FlowUtils.cpp b/agent-ovs/ovs/FlowUtils.cpp
@@ -123,6 +123,27 @@ static flow_func make_flow_functor(const network::subnet_t& ss,
     return std::bind(applyRemoteSub, _1, func, addr, ss.second, _2);
 }
 
+void add_l2classifier_entries(L24Classifier& clsfr, ClassAction act,
+                              uint8_t nextTable, uint16_t priority,
+                              uint32_t flags, uint64_t cookie,
+                              uint32_t svnid, uint32_t dvnid,
+                              /* out */ FlowEntryList& entries) {
+    if (clsfr.isProtSet())
+        return;
+
+    ovs_be64 ckbe = ovs_htonll(cookie);
+    FlowBuilder f;
+
+    f.cookie(ckbe)
+     .flags(flags);
+    flowutils::match_group(f, priority, svnid, dvnid);
+    match_protocol(f, clsfr);
+    if (act != flowutils::CA_DENY)
+        f.action().go(nextTable);
+
+    entries.push_back(f.build());
+}
+
 void add_classifier_entries(L24Classifier& clsfr, ClassAction act,
                             boost::optional<const network::subnets_t&> sourceSub,
                             boost::optional<const network::subnets_t&> destSub,
diff --git a/agent-ovs/ovs/include/FlowUtils.h b/agent-ovs/ovs/include/FlowUtils.h
@@ -118,6 +118,26 @@ void add_classifier_entries(modelgbp::gbpe::L24Classifier& clsfr,
                             uint32_t svnid, uint32_t dvnid,
                             /* out */ FlowEntryList& entries);
 
+/**
+ * Create L2 flow entries for the classifier specified and append them
+ * to the provided list.
+ *
+ * @param classifier Classifier object to get matching rules from
+ * @param act an action to take for the flows
+ * @param nextTable the table to send to if the traffic is allowed
+ * @param priority Priority of the entry created
+ * @param flags the flow flags to use
+ * @param cookie Cookie of the entry created
+ * @param svnid VNID of the source endpoint group for the entry
+ * @param dvnid VNID of the destination endpoint group for the entry
+ * @param entries List to append entry to
+ */
+void add_l2classifier_entries(modelgbp::gbpe::L24Classifier& clsfr,
+                              ClassAction act,
+                              uint8_t nextTable, uint16_t priority,
+                              uint32_t flags, uint64_t cookie,
+                              uint32_t svnid, uint32_t dvnid,
+                              /* out */ FlowEntryList& entries);
 /**
  * Add a match entry for the DHCP v4 and v6 request
  *
diff --git a/agent-ovs/ovs/test/AccessFlowManager_test.cpp b/agent-ovs/ovs/test/AccessFlowManager_test.cpp
@@ -187,29 +187,49 @@ BOOST_FIXTURE_TEST_CASE(learningBridge, AccessFlowManagerFixture) {
 BOOST_FIXTURE_TEST_CASE(secGrp, AccessFlowManagerFixture) {
     createObjects();
     createPolicyObjects();
+    shared_ptr<modelgbp::gbp::Subnets> rs;
     {
         Mutator mutator(framework, "policyreg");
+        rs = space->addGbpSubnets("subnets_rule0");
+
+        rs->addGbpSubnet("subnets_rule0_1")
+            ->setAddress("0.0.0.0")
+            .setPrefixLen(0);
+        rs->addGbpSubnet("subnets_rule0_2")
+            ->setAddress("0::")
+            .setPrefixLen(0);
+
+        shared_ptr<modelgbp::gbp::SecGroupRule> r1, r2, r3, r4, r5;
         secGrp1 = space->addGbpSecGroup("secgrp1");
-        secGrp1->addGbpSecGroupSubject("1_subject1")
-            ->addGbpSecGroupRule("1_1_rule1")
-            ->setDirection(DirectionEnumT::CONST_IN).setOrder(100)
+
+        r1 = secGrp1->addGbpSecGroupSubject("1_subject1")
+                ->addGbpSecGroupRule("1_1_rule1");
+        r1->setDirection(DirectionEnumT::CONST_IN).setOrder(100)
             .addGbpRuleToClassifierRSrc(classifier1->getURI().toString());
-        secGrp1->addGbpSecGroupSubject("1_subject1")
-            ->addGbpSecGroupRule("1_1_rule2")
-            ->setDirection(DirectionEnumT::CONST_IN).setOrder(150)
+        r1->addGbpSecGroupRuleToRemoteAddressRSrc(rs->getURI().toString());
+
+        r2 = secGrp1->addGbpSecGroupSubject("1_subject1")
+                ->addGbpSecGroupRule("1_1_rule2");
+        r2->setDirection(DirectionEnumT::CONST_IN).setOrder(150)
             .addGbpRuleToClassifierRSrc(classifier8->getURI().toString());
-        secGrp1->addGbpSecGroupSubject("1_subject1")
-            ->addGbpSecGroupRule("1_1_rule3")
-            ->setDirection(DirectionEnumT::CONST_OUT).setOrder(200)
+        r2->addGbpSecGroupRuleToRemoteAddressRSrc(rs->getURI().toString());
+
+        r3 = secGrp1->addGbpSecGroupSubject("1_subject1")
+            ->addGbpSecGroupRule("1_1_rule3");
+        r3->setDirection(DirectionEnumT::CONST_OUT).setOrder(200)
             .addGbpRuleToClassifierRSrc(classifier2->getURI().toString());
-        secGrp1->addGbpSecGroupSubject("1_subject1")
-            ->addGbpSecGroupRule("1_1_rule4")
-            ->setDirection(DirectionEnumT::CONST_IN).setOrder(300)
+
+        r4 = secGrp1->addGbpSecGroupSubject("1_subject1")
+                ->addGbpSecGroupRule("1_1_rule4");
+        r4->setDirection(DirectionEnumT::CONST_IN).setOrder(300)
             .addGbpRuleToClassifierRSrc(classifier6->getURI().toString());
-        secGrp1->addGbpSecGroupSubject("1_subject1")
-            ->addGbpSecGroupRule("1_1_rule5")
-            ->setDirection(DirectionEnumT::CONST_IN).setOrder(400)
+        r4->addGbpSecGroupRuleToRemoteAddressRSrc(rs->getURI().toString());
+
+        r5 = secGrp1->addGbpSecGroupSubject("1_subject1")
+                 ->addGbpSecGroupRule("1_1_rule5");
+        r5->setDirection(DirectionEnumT::CONST_IN).setOrder(400)
             .addGbpRuleToClassifierRSrc(classifier7->getURI().toString());
+        r5->addGbpSecGroupRuleToRemoteAddressRSrc(rs->getURI().toString());
         mutator.commit();
     }
 
@@ -238,19 +258,25 @@ BOOST_FIXTURE_TEST_CASE(secGrp, AccessFlowManagerFixture) {
     WAIT_FOR_TABLES("two-secgrp-nocon", 500);
 
     {
+        shared_ptr<modelgbp::gbp::SecGroupRule> r1, r2, r3;
+
         Mutator mutator(framework, "policyreg");
         secGrp2 = space->addGbpSecGroup("secgrp2");
-        secGrp2->addGbpSecGroupSubject("2_subject1")
-            ->addGbpSecGroupRule("2_1_rule1")
-            ->addGbpRuleToClassifierRSrc(classifier0->getURI().toString());
-        secGrp2->addGbpSecGroupSubject("2_subject1")
-            ->addGbpSecGroupRule("2_1_rule2")
-            ->setDirection(DirectionEnumT::CONST_BIDIRECTIONAL).setOrder(20)
+        r1 = secGrp2->addGbpSecGroupSubject("2_subject1")
+                ->addGbpSecGroupRule("2_1_rule1");
+        r1->addGbpRuleToClassifierRSrc(classifier0->getURI().toString());
+        r1->addGbpSecGroupRuleToRemoteAddressRSrc(rs->getURI().toString());
+
+        r2 = secGrp2->addGbpSecGroupSubject("2_subject1")
+                ->addGbpSecGroupRule("2_1_rule2");
+        r2->setDirection(DirectionEnumT::CONST_BIDIRECTIONAL).setOrder(20)
             .addGbpRuleToClassifierRSrc(classifier5->getURI().toString());
-        secGrp2->addGbpSecGroupSubject("2_subject1")
-            ->addGbpSecGroupRule("2_1_rule3")
-            ->setDirection(DirectionEnumT::CONST_OUT).setOrder(30)
+
+        r3 = secGrp2->addGbpSecGroupSubject("2_subject1")
+            ->addGbpSecGroupRule("2_1_rule3");
+        r3->setDirection(DirectionEnumT::CONST_OUT).setOrder(30)
             .addGbpRuleToClassifierRSrc(classifier9->getURI().toString());
+        r3->addGbpSecGroupRuleToRemoteAddressRSrc(rs->getURI().toString());
         mutator.commit();
     }
 
@@ -259,7 +285,6 @@ BOOST_FIXTURE_TEST_CASE(secGrp, AccessFlowManagerFixture) {
     initExpSecGrpSet12(true);
     WAIT_FOR_TABLES("two-secgrp", 500);
 
-    shared_ptr<modelgbp::gbp::Subnets> rs;
     {
         Mutator mutator(framework, "policyreg");
         rs = space->addGbpSubnets("subnets_rule1");
@@ -470,10 +495,9 @@ uint16_t AccessFlowManagerFixture::initExpSecGrp1(uint32_t setId,
             ADDF(Bldr(SEND_FLOW_REM).table(IN_POL).priority(prio).cookie(ruleId)
                  .tcp().reg(SEPG, setId).isIpSrc("10.0.0.0/8").isTpDst(80)
                  .actions().go(OUT).done());
-    } else {
-        ADDF(Bldr(SEND_FLOW_REM).table(IN_POL).priority(prio).cookie(ruleId)
-             .tcp().reg(SEPG, setId).isTpDst(80).actions().go(OUT).done());
     }
+    ADDF(Bldr(SEND_FLOW_REM).table(IN_POL).priority(prio).cookie(ruleId)
+         .tcp().reg(SEPG, setId).isTpDst(80).actions().go(OUT).done());
     /* classifer 8  */
     ruleId = idGen.getId("l24classifierRule",
                          classifier8->getURI().toString());
@@ -486,10 +510,9 @@ uint16_t AccessFlowManagerFixture::initExpSecGrp1(uint32_t setId,
                  .tcp6().reg(SEPG, setId)
                  .isIpv6Src("fd34:9c39:1374:358c::/64")
                  .isTpDst(80).actions().go(OUT).done());
-    } else {
-        ADDF(Bldr(SEND_FLOW_REM).table(IN_POL).priority(prio-128).cookie(ruleId)
-             .tcp6().reg(SEPG, setId).isTpDst(80).actions().go(OUT).done());
     }
+    ADDF(Bldr(SEND_FLOW_REM).table(IN_POL).priority(prio-128).cookie(ruleId)
+        .tcp6().reg(SEPG, setId).isTpDst(80).actions().go(OUT).done());
     /* classifier 2  */
     ruleId = idGen.getId("l24classifierRule",
                          classifier2->getURI().toString());
