Integrate some of my original code with the newly integrated NSX-T generator #342

jcatrouillet · 2023-04-18T06:30:53Z

Fix the wrong use of notes instead of description, notes are supposed to be used for rule lock/unlock
Add comment keyword instead of notes (comments are translated now to description in NSX-T API)
Check description length against its maximum
Add check for maximum number of rules
Add support for term expiration
Add a more granular management of ip_protocol and not blindly set it to IPV4_IPV6
Fix bug convert 0.0.0.0/0 to ANY
Add support for source_excluded
Add support for verbose keyword
Add check for mixed rule (v4 and v6) with v4 only source and no non-ANY v4 destination and v6 only source and no non-ANY v6 destination
Add check for maximum number of source and destination IPs
Fix issue when str of Term return nothing
Add support for numerical protocol
Fix bug when there's a single port, don't convert it into a range

# Fix the wrong use of notes instead of description, notes are supposed to be used for rule lock/unlock # Add comment keyword instead of notes (comments are translated now to description in NSX-T API) # Check description length against its maximum # Add check for maximum number of rules # Add support for term expiration # Add a more granular management of ip_protocol and not blindly set it to IPV4_IPV6 # Fix bug convert 0.0.0.0/0 to ANY # Add support for source_excluded # Add support for verbose keyword # Add check for mixed rule (v4 and v6) with v4 only source and no non-ANY v4 destination and v6 only source and no non-ANY v6 destination # Add check for maximum number of source and destination IPs # Fix issue when str of Term return nothing # Add support for numerical protocol # Fix bug when there's a single port, don't convert it into a range

ivucica · 2023-06-07T18:23:52Z

Small heads up: I had to do an assortment of other fixes, so I did internal code changes way before seeing your pull request. Once it is submitted, I’ll try to do a rebase myself that you can squash into your pull request, since this is the second time the contribution is being preempted.

Apologies for this!

jcatrouillet · 2023-06-07T18:37:14Z

Ack, please let me know when you're done.

ivucica

I've added some notes for whoever does the followup here (whether myself or @jcatrouillet). These are not so much comment and feedback, of the type I would usually do in code review -- more notes on what was changed here, because so much of it was already done in previous commits.

The main thing to add would be the max length checks. Adding support for verbose requires some more consideration not to break existing terms/policies (including our existing internal integration).

Thank you for your contribution.

ivucica · 2024-01-12T09:54:21Z

capirca/lib/nsxt.py

    if (len(self.nsxt_policies) > 1):
-      raise NsxtUnsupportedManyPoliciesError('Only one policy can be rendered')
+      raise NsxtUnsupportedManyPolicies('Only one policy can be rendered')


The exception is named NsxtUnsupportedManyPoliciesError

ivucica · 2024-01-12T09:55:57Z

capirca/lib/nsxt.py

+          #Catch the case if term is not valid
+          try:
+            rules = rules + [json.loads(str(term))]
+          except:


It's not a great practice to catch exceptions overly widely, especially without even logging the issue. I'd say that failing loudly is better to allow for a bad term to be caught rather than to be skipped (given this is intended for securing an environment).

ivucica · 2024-01-12T09:58:24Z

capirca/lib/nsxt.py

        if term.name in term_names:
          raise NsxtDuplicateTermError('There are multiple terms named: %s' %
                                       term.name)
        term_names.add(term.name)

        term.name = self.FixTermLength(term.name)

-        new_terms.append(Term(term, filter_type, applied_to, 4))
+        # Handle term expiration


Support for this was added in 6e7f105.

ivucica · 2024-01-12T09:59:01Z

capirca/lib/nsxt.py

+
+        cnt_terms += 1
+
+        if cnt_terms > self._RULECOUNT_WARN_THRESHOLD:


This is a useful change.

ivucica · 2024-01-12T10:01:11Z

capirca/lib/nsxt.py

+          "destination_address", 4
+        ) + self.term.GetAddressOfVersion("destination_address", 6)
+        if daddrs[0].with_prefixlen in ("0.0.0.0/0", "::/0"):
+          daddrs = "ANY"


Some handling of netblocks with 0.0.0.0/32 in them (including 0.0.0.0/0) was added in 9e9812e, I believe. Combined with ANY in 6e7f105 we should be good.

Similarly, mixed + v6 policies improvements landed in 6e7f105.

ivucica · 2024-01-12T10:42:41Z

capirca/lib/nsxt.py

@@ -131,38 +157,66 @@ def get(self):
        new_service = service.copy()
        new_service['icmp_type'] = icmp_type
        services.append(new_service)
+        #Is the return supposed to be in the for loop? Potential bug to be tested with multiple ICMP types


Correct, this was a bug. Indentation fixed in 6e7f105

ivucica · 2024-01-12T10:43:07Z

capirca/lib/nsxt.py

@@ -13,13 +13,31 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
+#


Changelog shouldn't be in the code itself.

ivucica · 2024-01-12T10:57:04Z

capirca/lib/nsxt.py

+      # 'profiles': ['ANY'],
+      # 'scope': ['ANY'],
+      'logged': bool(self.term.logging),
+      'description': description,


Per read security policy API doc which says the API returns a SecurityPolicy containing the Rule array, there's both notes and description.

Maybe the same¹ string belongs in both? Not sure where these appear in the NSX-T UI.

The comments added on top of the file suggest we should indeed switch to description.

Footnotes

Not quite the same because max length of description is 1024 and max length of notes is 2048. ↩

ivucica · 2024-01-12T10:58:28Z

capirca/lib/nsxt.py

 class Term(aclgenerator.Term):
  """Creates a single ACL Term for NSX-T."""

-  def __init__(self, term, filter_type, applied_to=None, af=4):
+  _MAX_TERM_NAME_LENGTH = 255
+  _MAX_TERM_COMMENT_LENGTH = 1024


If we keep using the notes field, then this is 2048¹.

Footnotes

Source for the claim ↩

ivucica · 2024-01-12T11:02:34Z

capirca/lib/nsxt.py

@@ -13,13 +13,31 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
+#
+# VERSION 1.01
+# Fix the wrong use of notes instead of description, notes are supposed to be used for rule lock/unlock


This is an important explanation, this would be a clear reason to put the notes into description (possibly in addition to notes).

ivucica · 2024-01-17T16:31:47Z

When approaching this, I might create a completely new commit/PR and appropriately use the Co-authored-by: tag in the commit description.

jcatrouillet added 2 commits April 17, 2023 23:27

Update nsxt.md

d06084d

jcatrouillet changed the title ~~Integrate my original code with the newly integrated NSX-T generator~~ Integrate some of my original code with the newly integrated NSX-T generator Apr 18, 2023

ivucica reviewed Jan 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate some of my original code with the newly integrated NSX-T generator #342

Integrate some of my original code with the newly integrated NSX-T generator #342

jcatrouillet commented Apr 18, 2023 •

edited

Loading

ivucica commented Jun 7, 2023

jcatrouillet commented Jun 7, 2023

ivucica left a comment

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica Jan 12, 2024

ivucica commented Jan 17, 2024


		cnt_terms += 1

		if cnt_terms > self._RULECOUNT_WARN_THRESHOLD:

Integrate some of my original code with the newly integrated NSX-T generator #342

Are you sure you want to change the base?

Integrate some of my original code with the newly integrated NSX-T generator #342

Conversation

jcatrouillet commented Apr 18, 2023 • edited Loading

ivucica commented Jun 7, 2023

jcatrouillet commented Jun 7, 2023

ivucica left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

ivucica commented Jan 17, 2024

jcatrouillet commented Apr 18, 2023 •

edited

Loading