Fix: persist credentials after Kerberos login (smb/mssql/ldap) - #1248
Open
Goultarde wants to merge 3 commits into
Open
Fix: persist credentials after Kerberos login (smb/mssql/ldap)#1248Goultarde wants to merge 3 commits into
Goultarde wants to merge 3 commits into
Conversation
The kerberos_login() path in smb and mssql never called db.add_credential / db.add_loggedin_relation / db.add_admin_user, so a successful auth via -k left no trace in the NXC database. ldap stored the credential but did not gate on useCache, so a ccache login with no secret in hand could still trigger the storage path. This patch mirrors the persistence logic of plaintext_login / hash_login for each affected protocol, picking the credtype from whichever input was provided: - password -> "plaintext" - ntlm_hash -> "hash" - aesKey -> intentionally skipped (no matching credtype in DB) - useCache -> intentionally skipped (no usable secret in hand) - --delegate -> skipped on smb (S4U impersonation, not our secret) Storage is gated on (self.username and a usable cred_value), so guest or null sessions stay out of the database. Per-protocol scope: - smb / mssql: full path (add_credential + add_loggedin_relation + add_admin_user when self.admin_privs). - ldap: only add_credential, since the LDAP database backend exposes neither add_loggedin_relation nor add_admin_user. Not touched: - wmi: its database backend has no add_credential (none of the login paths persist creds today). - rdp: same situation as wmi.
Goultarde
requested review from
Marshall-Hallenbeck,
NeffIsBack,
mpgn and
zblurx
as code owners
May 25, 2026 23:50
Member
|
Hi and thanks for the PR! So far credentials weren't stored when Kerberos is used because of all the false positives that can occur as you mentioned (delegation, ticket auth etc). If we decide to store creds with Kerberos now we have to be careful that we don't miss to exclude any auth that doesn't originate from the supplied hash/password. |
Contributor
Author
|
Hi ! (Sorry, I thought I had already answered) I'll take some time to think about how best to handle them and make sure only the supplied password/hash gets stored. |
Member
|
No worries, take your time :) Sounds good! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
After a successful Kerberos authentication, credentials were not being saved to the NXC database for SMB and MSSQL. LDAP had the opposite issue: it was storing credentials even when authenticating from a ccache (
--use-kcache), risking empty or incorrect entries.loggedinrelation, and admin tracking after a successful Kerberos loginuseCache=Trueto avoid storing stale/empty credentials when using--use-kcache--delegate(SMB)No additional dependencies required.
This PR was created with the assistance of AI (Claude code, used for code
structure).
Type of change
Setup guide for the review
Bug reproduction:
Authenticate against a target using Kerberos with a password or NT hash:
Before this fix,
nxc smb show credswould not show the credential after a Kerberos login. After the fix it does.For the LDAP regression, authenticate from an existing ccache and verify no empty credential is stored:
Tested on: Python 3.14, Arch Linux, against a Windows Server 2022 GOAD lab.
Screenshots (if appropriate):
Before
SMB
MSSQL
LDAP
After
SMB
MSSQL
LDAP
Checklist:
poetry run ruff check ., use--fixto automatically fix what it can)