From 70a8145b3521724028de20faccb3fab473c4baaa Mon Sep 17 00:00:00 2001 From: Vladimir Lavor Date: Wed, 18 Sep 2019 07:45:43 +0200 Subject: [PATCH] Fix ACL ICMP rules (#1471) * Fix ACL ICMP rules Signed-off-by: Vladimir Lavor * Added integration test case Signed-off-by: Vladimir Lavor --- .../vppcalls/vpp1904/acl_vppcalls.go | 2 +- .../vppcalls/vpp1904/dump_vppcalls.go | 12 ++- .../vppcalls/vpp1908/acl_vppcalls.go | 2 +- .../vppcalls/vpp1908/dump_vppcalls.go | 10 ++- .../vppcalls/vpp2001/acl_vppcalls.go | 2 +- .../vppcalls/vpp2001/acl_vppcalls_test.go | 16 ++-- .../vppcalls/vpp2001/dump_vppcalls.go | 10 ++- tests/integration/vpp/040_acl_test.go | 76 ++++++++++++++----- 8 files changed, 92 insertions(+), 38 deletions(-) diff --git a/plugins/vpp/aclplugin/vppcalls/vpp1904/acl_vppcalls.go b/plugins/vpp/aclplugin/vppcalls/vpp1904/acl_vppcalls.go index e1b0272a13..40aade1f3d 100644 --- a/plugins/vpp/aclplugin/vppcalls/vpp1904/acl_vppcalls.go +++ b/plugins/vpp/aclplugin/vppcalls/vpp1904/acl_vppcalls.go @@ -312,7 +312,7 @@ func icmpACL(icmpRule *acl.ACL_Rule_IpRule_Icmp, aclRule *aclapi.ACLRule) *aclap aclRule.SrcportOrIcmptypeLast = uint16(icmpRule.IcmpTypeRange.Last) // ICMPv6 code range aclRule.DstportOrIcmpcodeFirst = uint16(icmpRule.IcmpCodeRange.First) - aclRule.DstportOrIcmpcodeLast = uint16(icmpRule.IcmpCodeRange.First) + aclRule.DstportOrIcmpcodeLast = uint16(icmpRule.IcmpCodeRange.Last) } else { aclRule.Proto = vppcalls.ICMPv4Proto // IANA ICMPv4 aclRule.IsIPv6 = 0 diff --git a/plugins/vpp/aclplugin/vppcalls/vpp1904/dump_vppcalls.go b/plugins/vpp/aclplugin/vppcalls/vpp1904/dump_vppcalls.go index d03fb0d69d..9e25e120ae 100644 --- a/plugins/vpp/aclplugin/vppcalls/vpp1904/dump_vppcalls.go +++ b/plugins/vpp/aclplugin/vppcalls/vpp1904/dump_vppcalls.go @@ -628,9 +628,15 @@ func (h *ACLVppHandler) getUDPMatchRule(r acl_api.ACLRule) *acl.ACL_Rule_IpRule_ // format into the ACL Plugin's NB format func (h *ACLVppHandler) getIcmpMatchRule(r acl_api.ACLRule) *acl.ACL_Rule_IpRule_Icmp { icmp := &acl.ACL_Rule_IpRule_Icmp{ - Icmpv6: r.IsIPv6 > 0, - IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{}, - IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{}, + Icmpv6: r.IsIPv6 > 0, + IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ + First: uint32(r.DstportOrIcmpcodeFirst), + Last: uint32(r.DstportOrIcmpcodeLast), + }, + IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ + First: uint32(r.SrcportOrIcmptypeFirst), + Last: uint32(r.SrcportOrIcmptypeLast), + }, } return icmp } diff --git a/plugins/vpp/aclplugin/vppcalls/vpp1908/acl_vppcalls.go b/plugins/vpp/aclplugin/vppcalls/vpp1908/acl_vppcalls.go index 66e534116e..45976ddf6a 100644 --- a/plugins/vpp/aclplugin/vppcalls/vpp1908/acl_vppcalls.go +++ b/plugins/vpp/aclplugin/vppcalls/vpp1908/acl_vppcalls.go @@ -312,7 +312,7 @@ func icmpACL(icmpRule *acl.ACL_Rule_IpRule_Icmp, aclRule *aclapi.ACLRule) *aclap aclRule.SrcportOrIcmptypeLast = uint16(icmpRule.IcmpTypeRange.Last) // ICMPv6 code range aclRule.DstportOrIcmpcodeFirst = uint16(icmpRule.IcmpCodeRange.First) - aclRule.DstportOrIcmpcodeLast = uint16(icmpRule.IcmpCodeRange.First) + aclRule.DstportOrIcmpcodeLast = uint16(icmpRule.IcmpCodeRange.Last) } else { aclRule.Proto = vppcalls.ICMPv4Proto // IANA ICMPv4 aclRule.IsIPv6 = 0 diff --git a/plugins/vpp/aclplugin/vppcalls/vpp1908/dump_vppcalls.go b/plugins/vpp/aclplugin/vppcalls/vpp1908/dump_vppcalls.go index ee24b1a66a..6ef65df10f 100644 --- a/plugins/vpp/aclplugin/vppcalls/vpp1908/dump_vppcalls.go +++ b/plugins/vpp/aclplugin/vppcalls/vpp1908/dump_vppcalls.go @@ -629,8 +629,14 @@ func (h *ACLVppHandler) getUDPMatchRule(r acl_api.ACLRule) *acl.ACL_Rule_IpRule_ func (h *ACLVppHandler) getIcmpMatchRule(r acl_api.ACLRule) *acl.ACL_Rule_IpRule_Icmp { icmp := &acl.ACL_Rule_IpRule_Icmp{ Icmpv6: r.IsIPv6 > 0, - IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{}, - IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{}, + IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ + First: uint32(r.DstportOrIcmpcodeFirst), + Last: uint32(r.DstportOrIcmpcodeLast), + }, + IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ + First: uint32(r.SrcportOrIcmptypeFirst), + Last: uint32(r.SrcportOrIcmptypeLast), + }, } return icmp } diff --git a/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls.go b/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls.go index 0724f08880..2bae0adfc2 100644 --- a/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls.go +++ b/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls.go @@ -312,7 +312,7 @@ func icmpACL(icmpRule *acl.ACL_Rule_IpRule_Icmp, aclRule *vpp_acl.ACLRule) *vpp_ aclRule.SrcportOrIcmptypeLast = uint16(icmpRule.IcmpTypeRange.Last) // ICMPv6 code range aclRule.DstportOrIcmpcodeFirst = uint16(icmpRule.IcmpCodeRange.First) - aclRule.DstportOrIcmpcodeLast = uint16(icmpRule.IcmpCodeRange.First) + aclRule.DstportOrIcmpcodeLast = uint16(icmpRule.IcmpCodeRange.Last) } else { aclRule.Proto = vppcalls.ICMPv4Proto // IANA ICMPv4 aclRule.IsIPv6 = 0 diff --git a/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls_test.go b/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls_test.go index d5e1decc3c..c99da26ec8 100644 --- a/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls_test.go +++ b/plugins/vpp/aclplugin/vppcalls/vpp2001/acl_vppcalls_test.go @@ -139,12 +139,12 @@ var aclIPrules = []*acl.ACL_Rule{ Icmp: &acl.ACL_Rule_IpRule_Icmp{ Icmpv6: false, IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ - First: 150, - Last: 250, + First: 1, + Last: 2, }, IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ - First: 1150, - Last: 1250, + First: 3, + Last: 4, }, }, }, @@ -156,12 +156,12 @@ var aclIPrules = []*acl.ACL_Rule{ Icmp: &acl.ACL_Rule_IpRule_Icmp{ Icmpv6: true, IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ - First: 150, - Last: 250, + First: 10, + Last: 20, }, IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ - First: 1150, - Last: 1250, + First: 30, + Last: 40, }, }, }, diff --git a/plugins/vpp/aclplugin/vppcalls/vpp2001/dump_vppcalls.go b/plugins/vpp/aclplugin/vppcalls/vpp2001/dump_vppcalls.go index eeca8f8413..70d83ee1da 100644 --- a/plugins/vpp/aclplugin/vppcalls/vpp2001/dump_vppcalls.go +++ b/plugins/vpp/aclplugin/vppcalls/vpp2001/dump_vppcalls.go @@ -629,8 +629,14 @@ func (h *ACLVppHandler) getUDPMatchRule(r vpp_acl.ACLRule) *acl.ACL_Rule_IpRule_ func (h *ACLVppHandler) getIcmpMatchRule(r vpp_acl.ACLRule) *acl.ACL_Rule_IpRule_Icmp { icmp := &acl.ACL_Rule_IpRule_Icmp{ Icmpv6: r.IsIPv6 > 0, - IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{}, - IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{}, + IcmpCodeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ + First: uint32(r.DstportOrIcmpcodeFirst), + Last: uint32(r.DstportOrIcmpcodeLast), + }, + IcmpTypeRange: &acl.ACL_Rule_IpRule_Icmp_Range{ + First: uint32(r.SrcportOrIcmptypeFirst), + Last: uint32(r.SrcportOrIcmptypeLast), + }, } return icmp } diff --git a/tests/integration/vpp/040_acl_test.go b/tests/integration/vpp/040_acl_test.go index c43c9fb4f6..89691504c9 100644 --- a/tests/integration/vpp/040_acl_test.go +++ b/tests/integration/vpp/040_acl_test.go @@ -167,9 +167,9 @@ func TestCRUDIPAcl(t *testing.T) { //RuleName: "permitIPv6", newACLIPRule(true, "dead::1/64", "dead::2/64"), //RuleName: "denyICMP", - newACLIPRuleIcmp(false, false, 150, 250, 1150, 1250), + newACLIPRuleIcmp(false, false, 10, 20, 30, 40), //RuleName: "denyICMPv6", - newACLIPRuleIcmp(false, true, 150, 250, 1150, 1250), + newACLIPRuleIcmp(false, true, 10, 20, 30, 40), //RuleName: "permitTCP", newACLIPRuleTCP(true, 10, 20, 150, 250, 1150, 1250), //RuleName: "denyUDP", @@ -189,21 +189,30 @@ func TestCRUDIPAcl(t *testing.T) { t.Log("amount of acls dumped: 1") var rules []*acl.ACL_Rule - var isPresent bool - var isForInterface bool + var isIPRulePresent, isICMPRulePresent, isICMP6RulePresent, isForInterface bool for _, item := range acls { rules = item.ACL.Rules if (item.Meta.Index == aclIdx) && (aclname == item.Meta.Tag) { t.Logf("found ACL \"%v\"", item.Meta.Tag) for _, rule := range rules { - //t.Logf("%+v", rule) + t.Logf("%+v", rule) //here maybe all rules should be checked if (rule.IpRule.GetIp().SourceNetwork == "192.168.1.1/32") && (rule.IpRule.GetIp().DestinationNetwork == "10.20.0.0/24") { - isPresent = true - break + isIPRulePresent = true + } + if (rule.IpRule.GetIcmp().GetIcmpCodeRange().GetFirst() == 10) && + (rule.IpRule.GetIcmp().GetIcmpCodeRange().GetLast() == 20) && + (rule.IpRule.GetIcmp().GetIcmpTypeRange().GetFirst() == 30) && + (rule.IpRule.GetIcmp().GetIcmpTypeRange().GetLast() == 40) { + if rule.IpRule.GetIcmp().GetIcmpv6() { + isICMP6RulePresent = true + } else { + isICMPRulePresent = true + } } } + // check assignation to interface for _, intf := range item.ACL.Interfaces.Ingress { if intf == ifName { @@ -213,7 +222,9 @@ func TestCRUDIPAcl(t *testing.T) { } } } - Expect(isPresent).To(BeTrue(), "Configured IP should be present") + Expect(isIPRulePresent).To(BeTrue(), "Configured IP rule should be present") + Expect(isICMPRulePresent).To(BeTrue(), "Configured ICMP rule should be present") + Expect(isICMP6RulePresent).To(BeTrue(), "Configured ICMPv6 rule should be present") Expect(isForInterface).To(BeTrue(), "acl should be assigned to interface") indexes := []uint32{ifIdx, ifIdx2} @@ -258,9 +269,9 @@ func TestCRUDIPAcl(t *testing.T) { //RuleName: "permitIPv6", newACLIPRule(true, "dead::1/64", "dead::2/64"), //RuleName: "denyICMP", - newACLIPRuleIcmp(false, false, 150, 250, 1150, 1250), + newACLIPRuleIcmp(false, false, 11, 21, 31, 41), //RuleName: "denyICMPv6", - newACLIPRuleIcmp(false, true, 150, 250, 1150, 1250), + newACLIPRuleIcmp(false, true, 11, 21, 31, 41), //RuleName: "permitTCP", newACLIPRuleTCP(true, 10, 20, 150, 250, 1150, 1250), //RuleName: "denyUDP", @@ -280,7 +291,9 @@ func TestCRUDIPAcl(t *testing.T) { Expect(acls).Should(HaveLen(2)) t.Log("amount of acls dumped: 2") - isPresent = false + isIPRulePresent = false + isICMPRulePresent = false + isICMP6RulePresent = false isForInterface = false for _, item := range acls { rules = item.ACL.Rules @@ -290,8 +303,17 @@ func TestCRUDIPAcl(t *testing.T) { //t.Logf("%+v", rule) if (rule.IpRule.GetIp().SourceNetwork == "192.168.1.1/32") && (rule.IpRule.GetIp().DestinationNetwork == "10.20.0.0/24") { - isPresent = true - break + isIPRulePresent = true + } + if (rule.IpRule.GetIcmp().GetIcmpCodeRange().GetFirst() == 11) && + (rule.IpRule.GetIcmp().GetIcmpCodeRange().GetLast() == 21) && + (rule.IpRule.GetIcmp().GetIcmpTypeRange().GetFirst() == 31) && + (rule.IpRule.GetIcmp().GetIcmpTypeRange().GetLast() == 41) { + if rule.IpRule.GetIcmp().GetIcmpv6() { + isICMP6RulePresent = true + } else { + isICMPRulePresent = true + } } } // check assignation to interface @@ -303,7 +325,9 @@ func TestCRUDIPAcl(t *testing.T) { } } } - Expect(isPresent).To(BeTrue(), "Configured IP should be present") + Expect(isIPRulePresent).To(BeTrue(), "Configured IP should be present") + Expect(isICMPRulePresent).To(BeTrue(), "Configured ICMP rule should be present") + Expect(isICMP6RulePresent).To(BeTrue(), "Configured ICMPv6 rule should be present") Expect(isForInterface).To(BeTrue(), "acl should be assigned to interface") //negative tests @@ -347,6 +371,7 @@ func TestCRUDIPAcl(t *testing.T) { rule2modify := []*acl.ACL_Rule{ newACLIPRule(true, "10.20.30.1/32", "10.20.0.0/24"), newACLIPRule(true, "dead:dead::3/64", "dead:dead::4/64"), + newACLIPRuleIcmp(true, false, 15, 25, 35, 45), } const aclname4 = "test_modify0" @@ -359,7 +384,8 @@ func TestCRUDIPAcl(t *testing.T) { Expect(acls).Should(HaveLen(1)) t.Log("amount of acls dumped: 1") - isPresent = false + isIPRulePresent = false + isICMPRulePresent = false isForInterface = false var modifiedacl aclplugin_vppcalls.ACLDetails for _, item := range acls { @@ -376,8 +402,17 @@ func TestCRUDIPAcl(t *testing.T) { //here maybe should be checked all rules if (rule.IpRule.GetIp().SourceNetwork == "10.20.30.1/32") && (rule.IpRule.GetIp().DestinationNetwork == "10.20.0.0/24") { - isPresent = true - break + isIPRulePresent = true + } + if (rule.IpRule.GetIcmp().GetIcmpCodeRange().GetFirst() == 15) && + (rule.IpRule.GetIcmp().GetIcmpCodeRange().GetLast() == 25) && + (rule.IpRule.GetIcmp().GetIcmpTypeRange().GetFirst() == 35) && + (rule.IpRule.GetIcmp().GetIcmpTypeRange().GetLast() == 45) { + if rule.IpRule.GetIcmp().GetIcmpv6() { + isICMP6RulePresent = true + } else { + isICMPRulePresent = true + } } } // check assignation to interface @@ -389,7 +424,8 @@ func TestCRUDIPAcl(t *testing.T) { } } } - Expect(isPresent).To(BeTrue(), "Configured IP should be present") + Expect(isIPRulePresent).To(BeTrue(), "Configured IP should be present") + Expect(isICMPRulePresent).To(BeTrue(), "Configured ICMP rule should be present") Expect(isForInterface).To(BeTrue(), "acl should be assigned to interface") // negative test @@ -407,7 +443,7 @@ func TestCRUDIPAcl(t *testing.T) { Expect(acls).Should(HaveLen(1)) t.Log("amount of acls dumped: 1") - isPresent = false + isIPRulePresent = false isForInterface = false for _, item := range acls { if item.Meta.Index == aclIdx && (aclname4 == item.Meta.Tag) { @@ -428,7 +464,7 @@ func TestCRUDIPAcl(t *testing.T) { Expect(acls).Should(HaveLen(1)) t.Log("amount of acls dumped: 1") - isPresent = false + isIPRulePresent = false isForInterface = false for _, item := range acls { if item.Meta.Index == aclIdx { //&& (aclname2 == item.Meta.Tag) {