Skip to content

SPNEGO: add new from_cli_arguments function, improvements #4805

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 31, 2025

Conversation

gpotter2
Copy link
Member

@gpotter2 gpotter2 commented Jul 29, 2025

  • move code into a utility to be reusable
  • have smbclient use it
  • export rfc3961 elements by default in the cli
  • add target_name parameter in Gss_Init_sec_Context, that is populated by the various clients. SPN still overwrites it.
  • additional fixes to AutoArgparse to support byets

Copy link

codecov bot commented Jul 29, 2025

Codecov Report

❌ Patch coverage is 24.71910% with 67 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.98%. Comparing base (58526ec) to head (599b9ba).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
scapy/layers/spnego.py 13.95% 37 Missing ⚠️
scapy/utils.py 0.00% 20 Missing ⚠️
scapy/layers/kerberos.py 33.33% 4 Missing ⚠️
scapy/config.py 25.00% 3 Missing ⚠️
scapy/layers/smbclient.py 75.00% 2 Missing ⚠️
scapy/layers/ldap.py 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4805      +/-   ##
==========================================
+ Coverage   80.73%   80.98%   +0.25%     
==========================================
  Files         365      365              
  Lines       89075    89112      +37     
==========================================
+ Hits        71913    72168     +255     
+ Misses      17162    16944     -218     
Files with missing lines Coverage Δ
scapy/layers/gssapi.py 84.58% <ø> (ø)
scapy/layers/http.py 83.65% <ø> (ø)
scapy/layers/msrpce/msnrpc.py 68.43% <ø> (ø)
scapy/layers/msrpce/rpcclient.py 52.70% <100.00%> (+0.42%) ⬆️
scapy/layers/ntlm.py 81.43% <ø> (ø)
scapy/layers/smb2.py 88.49% <ø> (ø)
scapy/libs/rfc3961.py 85.84% <ø> (ø)
scapy/layers/ldap.py 66.99% <75.00%> (-0.05%) ⬇️
scapy/layers/smbclient.py 73.14% <75.00%> (+2.27%) ⬆️
scapy/config.py 79.66% <25.00%> (-0.34%) ⬇️
... and 3 more

... and 14 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copilot

This comment was marked as outdated.

@gpotter2 gpotter2 marked this pull request as draft July 30, 2025 02:20
@gpotter2 gpotter2 marked this pull request as ready for review July 30, 2025 19:35
@gpotter2 gpotter2 requested a review from Copilot July 31, 2025 01:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR enhances SPNEGO functionality by adding a new from_cli_arguments factory method to simplify authentication setup, improving code reusability across different clients. It also adds support for target_name parameter in GSS authentication flows and includes various improvements to AutoArgparse for better CLI experience.

  • Added SPNEGOSSP.from_cli_arguments() utility method for simplified authentication setup
  • Added target_name parameter to GSS_Init_sec_context across multiple layers
  • Enhanced AutoArgparse to support bytes parameters with hex input validation

Reviewed Changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.

Show a summary per file
File Description
scapy/utils.py Enhanced AutoArgparse with bytes parameter support and improved error handling
scapy/libs/rfc3961.py Reorganized exports to include KRB_FX_CF2 and SP800108_KDFCTR by default
scapy/layers/spnego.py Added from_cli_arguments factory method and target_name parameter support
scapy/layers/smbclient.py Refactored to use new SPNEGOSSP.from_cli_arguments method
scapy/layers/smb2.py Fixed trailing comma syntax
scapy/layers/ntlm.py Added target_name parameter to GSS_Init_sec_context
scapy/layers/msrpce/rpcclient.py Added target_name parameter and hostname tracking
scapy/layers/msrpce/msnrpc.py Added target_name and chan_bindings parameters
scapy/layers/ldap.py Added target_name parameter and hostname tracking
scapy/layers/kerberos.py Enhanced to use target_name when SPN not provided
scapy/layers/http.py Added target_name parameter for HTTP authentication
scapy/layers/gssapi.py Added target_name parameter to base GSS interface
scapy/config.py Added duplicate extension loading prevention
doc/scapy/layers/smb.rst Updated documentation to reflect SPN parameter changes
Comments suppressed due to low confidence (16)

scapy/utils.py:3943

  • [nitpick] The variable name 'hexarguments' should follow Python naming conventions and be 'hex_arguments' for better readability.
    hexarguments = []

scapy/utils.py:3960

  • [nitpick] Consistent with the previous comment, this should be 'hex_arguments.append(parname)' to maintain naming consistency.
            hexarguments.append(parname)

scapy/utils.py:4013

  • [nitpick] This should be 'for p in hex_arguments:' to maintain naming consistency throughout the function.
    for p in hexarguments:

scapy/layers/spnego.py:635

  • Parameter names should follow Python naming conventions. 'UPN' should be 'upn' to be consistent with snake_case naming.
        UPN: str,

scapy/layers/spnego.py:638

  • Parameter names should follow Python naming conventions. 'HashNt' should be 'hash_nt' to be consistent with snake_case naming.
        HashNt: bytes = None,

scapy/layers/spnego.py:639

  • Parameter names should follow Python naming conventions. 'HashAes256Sha96' should be 'hash_aes256_sha96' to be consistent with snake_case naming.
        HashAes256Sha96: bytes = None,

scapy/layers/spnego.py:640

  • Parameter names should follow Python naming conventions. 'HashAes128Sha96' should be 'hash_aes128_sha96' to be consistent with snake_case naming.
        HashAes128Sha96: bytes = None,

scapy/layers/spnego.py:642

  • Parameter names should follow Python naming conventions. 'ST' should be 'st' to be consistent with snake_case naming.
        ST=None,

scapy/layers/spnego.py:643

  • Parameter names should follow Python naming conventions. 'KEY' should be 'key' to be consistent with snake_case naming.
        KEY=None,

scapy/layers/spnego.py:674

  • The variable should use snake_case naming. This should be '_parse_upn(upn)' if the parameter name is changed as suggested.
            _, realm = _parse_upn(UPN)

scapy/layers/spnego.py:682

  • These variable references should use snake_case naming to match the corrected parameter names: 'hash_nt', 'st'.
        if HashNt is None and password is None and ST is None:

scapy/layers/smbclient.py:1122

  • Parameter names should follow Python naming conventions. 'HashNt' should be 'hash_nt' to be consistent with snake_case naming.
        HashNt: bytes = None,

scapy/layers/smbclient.py:1123

  • Parameter names should follow Python naming conventions. 'HashAes256Sha96' should be 'hash_aes256_sha96' to be consistent with snake_case naming.
        HashAes256Sha96: bytes = None,

scapy/layers/smbclient.py:1124

  • Parameter names should follow Python naming conventions. 'HashAes128Sha96' should be 'hash_aes128_sha96' to be consistent with snake_case naming.
        HashAes128Sha96: bytes = None,

scapy/layers/smbclient.py:1129

  • Parameter names should follow Python naming conventions. 'ST' should be 'st' to be consistent with snake_case naming.
        ST=None,

scapy/layers/smbclient.py:1130

  • Parameter names should follow Python naming conventions. 'KEY' should be 'key' to be consistent with snake_case naming.
        KEY=None,

@gpotter2 gpotter2 merged commit cc8e091 into secdev:master Jul 31, 2025
24 checks passed
@gpotter2 gpotter2 deleted the krb-im branch July 31, 2025 01:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant