Address review: drop staticmethod, use isSigningRequired() below SMB 3.0

Per review feedback on #1299:
- Make _is_signing_required a regular method instead of a staticmethod.
- Collapse the SMBv1 and sub-3.0 branches into a single isSigningRequired()
  call; impacket only force-sets RequireSigning at 3.1.1, so that flag is
  accurate below SMB 3.0. Only the 3.0+ path needs the explicit
  ServerSecurityMode check.

Co-Authored-By: Claude Opus 4.8 <[email protected]>

nxc/protocols/smb.py

@@ -592,27 +592,24 @@ class smb(connection):
             self.logger.fail("Broken Pipe Error while attempting to login")
             return False
 
-    @staticmethod
-    def _is_signing_required(conn, smbv1):
+    def _is_signing_required(self, conn, smbv1):
         """Determine whether the remote server REQUIRES SMB signing.
 
-        For SMB2/3 we read the real negotiated ``ServerSecurityMode`` rather than
-        impacket's ``RequireSigning`` flag. impacket force-sets ``RequireSigning``
-        to True for any SMB 3.1.1 negotiation regardless of the server's actual
-        policy (the 3.1.1 session setup is always signed). Relying on that flag
-        produced false negatives for relay-target discovery: 3.1.1 hosts that
-        merely *enable* (but do not *require*) signing were reported as
-        ``signing:True`` and silently dropped from ``--gen-relay-list`` output.
+        For SMB 3.0+ we read the real negotiated ``ServerSecurityMode`` rather
+        than impacket's ``RequireSigning`` flag. impacket force-sets
+        ``RequireSigning`` to True for any SMB 3.1.1 negotiation regardless of
+        the server's actual policy (the 3.1.1 session setup is always signed).
+        Relying on that flag produced false negatives for relay-target
+        discovery: 3.1.1 hosts that merely *enable* (but do not *require*)
+        signing were reported as ``signing:True`` and silently dropped from
+        ``--gen-relay-list`` output.
 
-        ``ServerSecurityMode`` is only populated by impacket for SMB 3.0+; for
-        SMB 2.0.2/2.1 we fall back to ``RequireSigning`` (accurate below 3.1.1).
+        For SMBv1 and SMB 2.0.2/2.1, ``isSigningRequired()`` is accurate: it
+        reads ``RequireSigning``, which impacket only force-sets at 3.1.1.
         """
-        if smbv1:
-            return conn.isSigningRequired()
-        connection = conn._SMBConnection._Connection
-        if connection["Dialect"] >= SMB2_DIALECT_30:
-            return bool(connection["ServerSecurityMode"] & SMB2_NEGOTIATE_SIGNING_REQUIRED)
-        return connection["RequireSigning"]
+        if not smbv1 and conn._SMBConnection._Connection["Dialect"] >= SMB2_DIALECT_30:
+            return bool(conn._SMBConnection._Connection["ServerSecurityMode"] & SMB2_NEGOTIATE_SIGNING_REQUIRED)
+        return conn.isSigningRequired()
 
     def create_smbv1_conn(self, check=False):
         self.logger.info(f"Creating SMBv1 connection to {self.host}")

tests/test_smb_signing.py

@@ -33,7 +33,11 @@ def smb_module():
 
 
 def make_smbv2_conn(dialect, server_security_mode, require_signing):
-    """Mimic the relevant slice of an impacket SMBConnection for SMB2/3."""
+    """Mimic the relevant slice of an impacket SMBConnection for SMB2/3.
+
+    ``isSigningRequired()`` returns ``RequireSigning``, matching impacket, for
+    the dialects below SMB 3.0 that fall back to it.
+    """
     inner = SimpleNamespace(
         _Connection={
             "Dialect": dialect,
@@ -41,7 +45,7 @@ def make_smbv2_conn(dialect, server_security_mode, require_signing):
             "RequireSigning": require_signing,
         }
     )
-    return SimpleNamespace(_SMBConnection=inner)
+    return SimpleNamespace(_SMBConnection=inner, isSigningRequired=lambda: require_signing)
 
 
 def make_smbv1_conn(signing_required):
@@ -56,7 +60,7 @@ def test_smb311_signing_enabled_not_required_reports_false(smb_module):
         server_security_mode=SMB2_NEGOTIATE_SIGNING_ENABLED,
         require_signing=True,
     )
-    assert smb_module.smb._is_signing_required(conn, smbv1=False) is False
+    assert smb_module.smb._is_signing_required(None, conn, smbv1=False) is False
 
 
 def test_smb311_signing_required_reports_true(smb_module):
@@ -65,7 +69,7 @@ def test_smb311_signing_required_reports_true(smb_module):
         server_security_mode=SMB2_NEGOTIATE_SIGNING_ENABLED | SMB2_NEGOTIATE_SIGNING_REQUIRED,
         require_signing=True,
     )
-    assert smb_module.smb._is_signing_required(conn, smbv1=False) is True
+    assert smb_module.smb._is_signing_required(None, conn, smbv1=False) is True
 
 
 def test_smb30_signing_not_required_reports_false(smb_module):
@@ -74,7 +78,7 @@ def test_smb30_signing_not_required_reports_false(smb_module):
         server_security_mode=SMB2_NEGOTIATE_SIGNING_ENABLED,
         require_signing=False,
     )
-    assert smb_module.smb._is_signing_required(conn, smbv1=False) is False
+    assert smb_module.smb._is_signing_required(None, conn, smbv1=False) is False
 
 
 def test_smb21_falls_back_to_require_signing(smb_module):
@@ -85,9 +89,9 @@ def test_smb21_falls_back_to_require_signing(smb_module):
         server_security_mode=0,
         require_signing=True,
     )
-    assert smb_module.smb._is_signing_required(conn, smbv1=False) is True
+    assert smb_module.smb._is_signing_required(None, conn, smbv1=False) is True
 
 
 def test_smbv1_uses_session_signing_flag(smb_module):
-    assert smb_module.smb._is_signing_required(make_smbv1_conn(True), smbv1=True) is True
-    assert smb_module.smb._is_signing_required(make_smbv1_conn(False), smbv1=True) is False
+    assert smb_module.smb._is_signing_required(None, make_smbv1_conn(True), smbv1=True) is True
+    assert smb_module.smb._is_signing_required(None, make_smbv1_conn(False), smbv1=True) is False