From 0f94b038e1c98dc4c684b0fb9c76c9714ecf6428 Mon Sep 17 00:00:00 2001 From: Jared Hendrickson Date: Fri, 29 Jan 2021 14:13:17 -0700 Subject: [PATCH 1/3] Fixed bug that did not consider 'any' as a valid outbound NAT rule protocol, updated unit test to test for this bug in the future --- .../APIFirewallNATOutboundMappingCreate.inc | 13 +++++--- .../APIFirewallNATOutboundMappingUpdate.inc | 9 ++++-- ...st_api_v1_firewall_nat_outbound_mapping.py | 30 ++++++++++++++++-- .../__pycache__/__init__.cpython-38.pyc | Bin 0 -> 6923 bytes 4 files changed, 43 insertions(+), 9 deletions(-) create mode 100644 tests/unit_test_framework/__pycache__/__init__.cpython-38.pyc diff --git a/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingCreate.inc b/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingCreate.inc index fc2ffcc09..97dddbdbf 100644 --- a/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingCreate.inc +++ b/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingCreate.inc @@ -28,7 +28,7 @@ class APIFirewallNATOutboundMappingCreate extends APIModel { parent::__construct(); $this->change_note = "Added outbound NAT mapping via API"; $this->privileges = ["page-all", "page-firewall-nat-outbound-edit"]; - $this->protocols = ["tcp", "udp", "tcp/udp", "icmp", "esp", "ah", "gre", "ipv6", "igmp", "pim", "ospf"]; + $this->protocols = ["any", "tcp", "udp", "tcp/udp", "icmp", "esp", "ah", "gre", "ipv6", "igmp", "pim", "ospf"]; $this->pool_options = ["round-robin", "round-robin sticky-address", "random", "random sticky-address", "source-hash", "bitmask"]; $this->port_supported = false; $this->pool_source_hash_supported = false; @@ -65,10 +65,13 @@ class APIFirewallNATOutboundMappingCreate extends APIModel { if (isset($this->initial_data['protocol'])) { # Require protocol to be a known/supported protocol if (in_array($this->initial_data['protocol'], $this->protocols)) { - $this->validated_data["protocol"] = $this->initial_data['protocol']; - # Set our port supported toggle to true if our protocol uses ports - if (in_array($this->validated_data["protocol"], ["tcp", "udp", "tcp/udp"])) { - $this->port_supported = true; + # Only add the protocol if it is not any (XML expects no entry for any) + if ($this->initial_data["protocol"] !== "any") { + $this->validated_data["protocol"] = $this->initial_data['protocol']; + # Set our port supported toggle to true if our protocol uses ports + if (in_array($this->validated_data["protocol"], ["tcp", "udp", "tcp/udp"])) { + $this->port_supported = true; + } } } else { $this->errors[] = APIResponse\get(4089); diff --git a/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingUpdate.inc b/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingUpdate.inc index 883aabafa..5bdc940e7 100644 --- a/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingUpdate.inc +++ b/pfSense-pkg-API/files/etc/inc/api/models/APIFirewallNATOutboundMappingUpdate.inc @@ -28,7 +28,7 @@ class APIFirewallNATOutboundMappingUpdate extends APIModel { parent::__construct(); $this->change_note = "Modified outbound NAT mapping via API"; $this->privileges = ["page-all", "page-firewall-nat-outbound-edit"]; - $this->protocols = ["tcp", "udp", "tcp/udp", "icmp", "esp", "ah", "gre", "ipv6", "igmp", "pim", "ospf"]; + $this->protocols = ["any", "tcp", "udp", "tcp/udp", "icmp", "esp", "ah", "gre", "ipv6", "igmp", "pim", "ospf"]; $this->pool_options = ["", "round-robin", "round-robin sticky-address", "random", "random sticky-address", "source-hash", "bitmask"]; $this->port_supported = false; $this->pool_source_hash_supported = false; @@ -76,7 +76,12 @@ class APIFirewallNATOutboundMappingUpdate extends APIModel { if (isset($this->initial_data['protocol'])) { # Require protocol to be a known/supported protocol if (in_array($this->initial_data['protocol'], $this->protocols)) { - $this->validated_data["protocol"] = $this->initial_data['protocol']; + # Unset the protocol value if it is any (XML expects no entry for any). Otherwise update value. + if ($this->initial_data["protocol"] === "any") { + unset($this->validated_data["protocol"]); + } else { + $this->validated_data["protocol"] = $this->initial_data['protocol']; + } } else { $this->errors[] = APIResponse\get(4089); } diff --git a/tests/test_api_v1_firewall_nat_outbound_mapping.py b/tests/test_api_v1_firewall_nat_outbound_mapping.py index 6584a138d..84d625a89 100644 --- a/tests/test_api_v1_firewall_nat_outbound_mapping.py +++ b/tests/test_api_v1_firewall_nat_outbound_mapping.py @@ -31,28 +31,54 @@ class APIUnitTestFirewallNATOutboundMapping(unit_test_framework.APIUnitTest): "descr": "Unit Test", "nosync": True, "top": True + }, + { + "interface": "WAN", + "protocol": "any", + "src": "any", + "dst": "1.1.1.1", + "target": "192.168.1.123/24", + "poolopts": "round-robin", + "descr": "Unit Test 2", + "nosync": True, + "top": True } ] put_payloads = [ { "id": 0, "interface": "WAN", + "protocol": "any", + "src": "any", + "dst": "1.1.1.1", + "target": "192.168.1.123/24", + "poolopts": "round-robin", + "descr": "Updated Unit Test", + "nonat": True, + "disabled": True, + "nosync": True, + "top": True + }, + { + "id": 1, + "interface": "WAN", "protocol": "udp", "src": "any", "srcport": "433", "dst": "1.1.1.1", "dstport": "443", "target": "192.168.1.123/24", - "natstaticport": True, + "staticnatport": True, "poolopts": "round-robin", "descr": "Updated Unit Test", - "nonat": True, + "nonat": False, "disabled": True, "nosync": True, "top": True } ] delete_payloads = [ + {"id": 0}, {"id": 0} ] diff --git a/tests/unit_test_framework/__pycache__/__init__.cpython-38.pyc b/tests/unit_test_framework/__pycache__/__init__.cpython-38.pyc new file mode 100644 index 0000000000000000000000000000000000000000..6a897edeb17eaca89fb0fc70100f3e21f7623321 GIT binary patch literal 6923 zcmcIpNpl=Y6|TK{)$W$%wY~?Rf&<4nNH-k8ksE)2pFmU)TsVOL06`dn;d@!#a;q(vi5aFk@@3}B{k_b$_1$7I zr{MeBAOGsTbwpAALY4kcN98h7*aTn-Q|pSG=Blg8y5?#!*IixahHJ>&bWNFCt|fEZ zwUKM}Oe5=NRfQ|HTrFQK@GR4xtEzHcVFojwDa?GTxkaYVD`o2?)>P)oYErt<40*L9 z_#OUfhlkNUUo?Z}QdxB~RX^fOEwNgqzWv|U&i@Q7S7^7c7AUUH4Q_5&o61uaJHiZ1 zbF)afCB@An6_AQZLrB9&BS@o2V@zkpGsPWeCbN)FFq>tN?_yb&LtbKeRzN<g!(xZ4b(1yCnZUaB~Al|b*KvVrsg zQus@NNZCNDsgYLGH#JpRQ#OoEh4PwNvzR)Wmh4IJY*L9b5}$pV;&asMdIVS(wN zX}}dR&rmwg#&9E@%9M`Ih~7%{O4o-+k|H|AdHmu6;Osh3`8akg3ycr&U5EG~45 zQEP=a%exXQ43?UHJ+bC*T)#25Ac()ou;+#TBKLS$_1oO@k})76UqqfSmfF4u31^@j zVtjR(H+W(!w?HC2j6`CzTLQ(7s3)0D$VJmfd#3G&;p3KINx|<#%U;`Gt+#weooU<> z2WWSq#A3Y8BaWF?!^GfEf+)!n^r|h!U#N)@^7`TnMI6LFU)`U%i}}Nu+A?o45mZ;g zR&(YK$CN?T5&;iqt_RU_=iy9yG32=G6~7(KMBrsO(*ajKDte0o`@_ywW;`z-tT)|W zO)|a8X=3ddfTE758TGU}p=XfOFP`YT9@D)6+b)QPaW;~?AeT`9la#B%tF>WcM22vm;=zNl+FYVvpIsFvAjXRw@FFBod_%er| z!oh#A!{NvJd7VzT?ffJ^Gf48m{|N=AGVPhNu98+@c;+?8eP$q;&((Ep4SIrE%#I9} zA&vLk#N#I%wBDLZI*;_-bE}ITWn<_$&L);L(t514)iTL;qh*Tp-I9(Rx^Sy)KRbZ7 zgQWAe=7-(`4qC4d2i+H$>?Abn>wc%$TkJHeuxU1vjwKxrjRdO$!syK0w!@ROBXu~B z6rKc#(DkVbS*J!S5#p(~VvC~@=5RT@7l6D?Lh4v-{b>i$#yEI-en}UR%cpo6K8DI1}^+;Sr&9xgm z@*nvk&QvO?9Uz7XZ!AYq+cgP>@iAKJ?wy-X+T*`0*uRQvgV{lqhhml%rlsP1r6TQy z2F!pY`?_dK$|t6@XA-^kIEs(Zf*`s>y>JBr7_*?HLqof(Psh1RrDvOnaoV%#vqsR2 z_mM?G77O+4n}9ohI9)bglcH~Ln5$>qhzXj@?%Esi{@Y!v=Z)DO!!U`nl?o|bT=c~5 z7G9D%jgNegvX-{pZE)&ABZ!=!>A+@hHCafbA93*zuOhL#1veMM@Znx0I{aE(x*CLS zsB)*=AO<&5|qZGWiucy5!aCj?nPZoy}a%s=_6#0ZUfm+-mN z9z;thia?ZgX*bBb=325*3_h{~`rStge-5B%8Ou-$pIf@2YQ5h-ExWI#YOkzJuWnJF zzItY$j@Ip?zn~T$_}`DaN+P}YkyO|c@4v-MV(K~xbrSB{iY5-PX&W%uVf@2l3zT)E z`F^)mN9%{okTtku(h-1DFsTG8dhP4>ve1jcWrNj6f;Mf9k4JVbvj&T?mW^^7`8C*x zuoY?5!WtCEb8Fqar!-F)3i=gmLn}G)I~7g=+=5}uGXh1D-PE39NB5<3@UgN6CvqkG zD^>h>&8&^G+=@o!cx27o$dTR2^6M66+=m%+wXxbbX8cDwBhG8xSDG5;urTIYI)W9Z z6jqe8oW#Cp#UaT1;ph6gjk$C>pWMetH1T=_xjmcOy4@^wTSuw2w(AZ2n8c6v@HmxC zr;!vWVHdv!2IDp+$^8Ug2ktivPLsDgU2Q&fipJ-ziR<8?_z=J?UV(gon-EL!@Je+A zSv#@)KKBI|4kQv?dPS+qO#RkO_F!0uL?fgb?BobLY;Sk)6_c%Tw}cifgoatJB7bxY&h?6n#==ka2bt$sYm1|}j$~H=dJP#_b z;YVI0T=G0OpKexqb)-~i)50C5Q(l!7sk5b+fi`G0Bgk=aQ*OW8c^@heSkgzapof-Q zOjLk#?XXjY?g#~$Z;6h*yLJ$Ia@RUm9~4I92y7ySa{!8J=moW)T3SKfgEXRPDhVC~ zmVl;0^t{St#>f%WOK2e{a7@kVmPR;BJD}~>N_s}msYAMj-tq2!(bDgLEmX-)>U|ap zWd2B_@iGczD^XyCnV%W!#+oil%xY@PUN`Y_Gmss|nmPsPz_O_vL5YF~FfFME?5X@* zT-f{I{J92P#k~*CG~yy^=NsfH$tpEoXuye(wPDoGHc}&-5YP~Zu?sOx;5>nE1H|L9 z+qEk zEI@QVNku1wZ*E@sI2A?txsiLVGPzQ>$ zq`;xl4*@8C7?Lln$cLe+$JG)&4(Qo2GM4vlXUR6HQY9Cm_fdI^C+AR!hY&x5Y9r+d z^>YC9(?Gw|Dg7dyLL}6dDxhvSL<-olnP{yrF+CYEvTx0&@?#Gs6?*|-{mV#Kr{S71 z;4^rzJUaayDYg4?%cR{uaFlI9v2E9jgghnrf{7cnxfkkOy3Bqc1{XX7+`o}RNs)nA zQZIDz#cPP=t+{tvg{TUGZLPAapEp~e3!e9B$FHX?sr({=pBSA^ zz+98y1y=`5MIovGG*WkjN;3p55TKZ(I7Z+sfwu{Km%t$c6txs2y97nI1VwoSc_T6$ zC`i>v5tkImJ_)K_yMly+pa(1HJqfz)f^MUr%OZ%Gf@m#>5Pk1l%1(!HIsl2G@G?L~ zgS!KZVuzoeDQ0@#hnaVC`Akt$b9M$c#p|D(tvr-Udve%Pp Date: Fri, 29 Jan 2021 14:13:33 -0700 Subject: [PATCH 2/3] Fixed bug that did not consider 'any' as a valid outbound NAT rule protocol, updated unit test to test for this bug in the future --- .../__pycache__/__init__.cpython-38.pyc | Bin 6923 -> 0 bytes 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 tests/unit_test_framework/__pycache__/__init__.cpython-38.pyc diff --git a/tests/unit_test_framework/__pycache__/__init__.cpython-38.pyc b/tests/unit_test_framework/__pycache__/__init__.cpython-38.pyc deleted file mode 100644 index 6a897edeb17eaca89fb0fc70100f3e21f7623321..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 6923 zcmcIpNpl=Y6|TK{)$W$%wY~?Rf&<4nNH-k8ksE)2pFmU)TsVOL06`dn;d@!#a;q(vi5aFk@@3}B{k_b$_1$7I zr{MeBAOGsTbwpAALY4kcN98h7*aTn-Q|pSG=Blg8y5?#!*IixahHJ>&bWNFCt|fEZ zwUKM}Oe5=NRfQ|HTrFQK@GR4xtEzHcVFojwDa?GTxkaYVD`o2?)>P)oYErt<40*L9 z_#OUfhlkNUUo?Z}QdxB~RX^fOEwNgqzWv|U&i@Q7S7^7c7AUUH4Q_5&o61uaJHiZ1 zbF)afCB@An6_AQZLrB9&BS@o2V@zkpGsPWeCbN)FFq>tN?_yb&LtbKeRzN<g!(xZ4b(1yCnZUaB~Al|b*KvVrsg zQus@NNZCNDsgYLGH#JpRQ#OoEh4PwNvzR)Wmh4IJY*L9b5}$pV;&asMdIVS(wN zX}}dR&rmwg#&9E@%9M`Ih~7%{O4o-+k|H|AdHmu6;Osh3`8akg3ycr&U5EG~45 zQEP=a%exXQ43?UHJ+bC*T)#25Ac()ou;+#TBKLS$_1oO@k})76UqqfSmfF4u31^@j zVtjR(H+W(!w?HC2j6`CzTLQ(7s3)0D$VJmfd#3G&;p3KINx|<#%U;`Gt+#weooU<> z2WWSq#A3Y8BaWF?!^GfEf+)!n^r|h!U#N)@^7`TnMI6LFU)`U%i}}Nu+A?o45mZ;g zR&(YK$CN?T5&;iqt_RU_=iy9yG32=G6~7(KMBrsO(*ajKDte0o`@_ywW;`z-tT)|W zO)|a8X=3ddfTE758TGU}p=XfOFP`YT9@D)6+b)QPaW;~?AeT`9la#B%tF>WcM22vm;=zNl+FYVvpIsFvAjXRw@FFBod_%er| z!oh#A!{NvJd7VzT?ffJ^Gf48m{|N=AGVPhNu98+@c;+?8eP$q;&((Ep4SIrE%#I9} zA&vLk#N#I%wBDLZI*;_-bE}ITWn<_$&L);L(t514)iTL;qh*Tp-I9(Rx^Sy)KRbZ7 zgQWAe=7-(`4qC4d2i+H$>?Abn>wc%$TkJHeuxU1vjwKxrjRdO$!syK0w!@ROBXu~B z6rKc#(DkVbS*J!S5#p(~VvC~@=5RT@7l6D?Lh4v-{b>i$#yEI-en}UR%cpo6K8DI1}^+;Sr&9xgm z@*nvk&QvO?9Uz7XZ!AYq+cgP>@iAKJ?wy-X+T*`0*uRQvgV{lqhhml%rlsP1r6TQy z2F!pY`?_dK$|t6@XA-^kIEs(Zf*`s>y>JBr7_*?HLqof(Psh1RrDvOnaoV%#vqsR2 z_mM?G77O+4n}9ohI9)bglcH~Ln5$>qhzXj@?%Esi{@Y!v=Z)DO!!U`nl?o|bT=c~5 z7G9D%jgNegvX-{pZE)&ABZ!=!>A+@hHCafbA93*zuOhL#1veMM@Znx0I{aE(x*CLS zsB)*=AO<&5|qZGWiucy5!aCj?nPZoy}a%s=_6#0ZUfm+-mN z9z;thia?ZgX*bBb=325*3_h{~`rStge-5B%8Ou-$pIf@2YQ5h-ExWI#YOkzJuWnJF zzItY$j@Ip?zn~T$_}`DaN+P}YkyO|c@4v-MV(K~xbrSB{iY5-PX&W%uVf@2l3zT)E z`F^)mN9%{okTtku(h-1DFsTG8dhP4>ve1jcWrNj6f;Mf9k4JVbvj&T?mW^^7`8C*x zuoY?5!WtCEb8Fqar!-F)3i=gmLn}G)I~7g=+=5}uGXh1D-PE39NB5<3@UgN6CvqkG zD^>h>&8&^G+=@o!cx27o$dTR2^6M66+=m%+wXxbbX8cDwBhG8xSDG5;urTIYI)W9Z z6jqe8oW#Cp#UaT1;ph6gjk$C>pWMetH1T=_xjmcOy4@^wTSuw2w(AZ2n8c6v@HmxC zr;!vWVHdv!2IDp+$^8Ug2ktivPLsDgU2Q&fipJ-ziR<8?_z=J?UV(gon-EL!@Je+A zSv#@)KKBI|4kQv?dPS+qO#RkO_F!0uL?fgb?BobLY;Sk)6_c%Tw}cifgoatJB7bxY&h?6n#==ka2bt$sYm1|}j$~H=dJP#_b z;YVI0T=G0OpKexqb)-~i)50C5Q(l!7sk5b+fi`G0Bgk=aQ*OW8c^@heSkgzapof-Q zOjLk#?XXjY?g#~$Z;6h*yLJ$Ia@RUm9~4I92y7ySa{!8J=moW)T3SKfgEXRPDhVC~ zmVl;0^t{St#>f%WOK2e{a7@kVmPR;BJD}~>N_s}msYAMj-tq2!(bDgLEmX-)>U|ap zWd2B_@iGczD^XyCnV%W!#+oil%xY@PUN`Y_Gmss|nmPsPz_O_vL5YF~FfFME?5X@* zT-f{I{J92P#k~*CG~yy^=NsfH$tpEoXuye(wPDoGHc}&-5YP~Zu?sOx;5>nE1H|L9 z+qEk zEI@QVNku1wZ*E@sI2A?txsiLVGPzQ>$ zq`;xl4*@8C7?Lln$cLe+$JG)&4(Qo2GM4vlXUR6HQY9Cm_fdI^C+AR!hY&x5Y9r+d z^>YC9(?Gw|Dg7dyLL}6dDxhvSL<-olnP{yrF+CYEvTx0&@?#Gs6?*|-{mV#Kr{S71 z;4^rzJUaayDYg4?%cR{uaFlI9v2E9jgghnrf{7cnxfkkOy3Bqc1{XX7+`o}RNs)nA zQZIDz#cPP=t+{tvg{TUGZLPAapEp~e3!e9B$FHX?sr({=pBSA^ zz+98y1y=`5MIovGG*WkjN;3p55TKZ(I7Z+sfwu{Km%t$c6txs2y97nI1VwoSc_T6$ zC`i>v5tkImJ_)K_yMly+pa(1HJqfz)f^MUr%OZ%Gf@m#>5Pk1l%1(!HIsl2G@G?L~ zgS!KZVuzoeDQ0@#hnaVC`Akt$b9M$c#p|D(tvr-Udve%Pp Date: Fri, 29 Jan 2021 15:46:53 -0700 Subject: [PATCH 3/3] Fixed minor bug that prevent interfaces from being update with the same if value --- .../files/etc/inc/api/models/APIInterfaceUpdate.inc | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/pfSense-pkg-API/files/etc/inc/api/models/APIInterfaceUpdate.inc b/pfSense-pkg-API/files/etc/inc/api/models/APIInterfaceUpdate.inc index b83f173de..593b919c8 100644 --- a/pfSense-pkg-API/files/etc/inc/api/models/APIInterfaceUpdate.inc +++ b/pfSense-pkg-API/files/etc/inc/api/models/APIInterfaceUpdate.inc @@ -53,11 +53,13 @@ class APIInterfaceUpdate extends APIModel { private function __validate_if() { if (isset($this->initial_data["if"])) { - $this->validated_data["if"] = trim($this->initial_data["if"]); - // Check that our interface exists and is not in use - if (!array_key_exists($this->initial_data["if"], $this->if_list)) { + $if_info = $this->if_list[$this->initial_data["if"]]; + # Return an error if the requested physical interface does not exist + if (empty($if_info)) { $this->errors[] = APIResponse\get(3000); - } elseif (isset($this->if_list[$this->initial_data["if"]]["in_use"])) { + } + # Return an error if the physical interface is already in use by a different interface object + elseif (isset($if_info["in_use"]) and $if_info["in_use"] !== $this->id) { $this->errors[] = APIResponse\get(3001); } $this->validated_data["if"] = $this->initial_data["if"];