Skip to content

Parse Ntds DnsNode dnsRecord entries#20

Merged
Schamper merged 33 commits into
fox-it:mainfrom
william-billaud:dns_node
May 8, 2026
Merged

Parse Ntds DnsNode dnsRecord entries#20
Schamper merged 33 commits into
fox-it:mainfrom
william-billaud:dns_node

Conversation

@william-billaud
Copy link
Copy Markdown
Contributor

@william-billaud william-billaud commented Jan 28, 2026

Add function associate with the parsing of DnsNode NTDS entries. Goal is to later include this in a dissect.target plugins.

These entries allow to quickly retrieves DNS record from a Domain. E.g

<DnsNode dns_name=sevenkingdoms.local, records=|type='A' ttl_seconds=600 timestamp=2025-12-19 18:00:00+00:00 data=DnsARecord(ipv4_address='192.168.56.10')|type='NS' ttl_seconds=3600 timestamp=None data=NodeNameRecord(name_node='kingslanding.sevenkingdoms.local')|type='SOA' ttl_seconds=3600 timestamp=None data=SOARecord(name_primary_server='kingslanding.sevenkingdoms.local', refresh=900, retry=600, minimum_ttl=3600, zone_administrator_email='hostmaster.sevenkingdoms.local')|>

In terms of review difficulty I would say 3/5 : This PR does not modify existing code, and this feature mainly rely on documented structure unpacking, without difficult concepts to understand and with a good test coverage.

Some note:

  • Using dissect.cstruct, is it possible to specify that a structure member is in little endian, and other in bug endian (for the same struct, see the swap_endianess function) ?

  • I have issue with serial number of SOA records, which are not the same as observed in Lab, I can't figure out how MS handle it, I have chosen to not display this value as value is known as being wrong + this is, in my opinion not the most import information.

  • Not all DNS Record type are parsed. There is a lot of possible structure (27), some of them are nearly never found production env/obsolete or with low interest. But if someone want to, it should be easy to add a missing type.

  • closes Parse Ntds DnsNode dnsRecord entries #19

@william-billaud william-billaud marked this pull request as draft January 28, 2026 12:33
@william-billaud william-billaud marked this pull request as ready for review April 1, 2026 09:41
@william-billaud william-billaud changed the title [WIP]Parse Ntds DnsNode dnsRecord entries Parse Ntds DnsNode dnsRecord entries Apr 1, 2026
@william-billaud
Copy link
Copy Markdown
Contributor Author

@Schamper just a ping as this PR was previously in draft and I do not now what kind of notification you have, but no emergency on this topic

Comment thread dissect/database/ese/ntds/objects/c_dnsnode.py
Comment thread dissect/database/ese/ntds/objects/c_dns_record.py Outdated
Comment thread dissect/database/ese/ntds/objects/c_dns_record.py Outdated
Comment thread dissect/database/ese/ntds/objects/c_dns_record.py Outdated
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
Comment thread dissect/database/ese/ntds/ntds.py Outdated
Comment thread tests/ese/ntds/objects/test_dnsnode.py
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
Comment thread dissect/database/ese/ntds/objects/dnsnode.py
@william-billaud
Copy link
Copy Markdown
Contributor Author

Done, I just have a weird issue (or maybe intended) with cstruct : #20 (comment)

I also added some tests for errored entries.

@william-billaud william-billaud requested a review from Schamper May 6, 2026 14:02
Comment thread dissect/database/ese/ntds/objects/c_dnsnode.py
Comment thread dissect/database/ese/ntds/objects/c_dns_record.py Outdated
Comment thread dissect/database/ese/ntds/objects/c_dnsnode.py
Comment thread dissect/database/ese/ntds/objects/c_dnsnode.py
Comment thread dissect/database/ese/ntds/objects/c_dnsnode.py
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
Comment thread tests/ese/ntds/objects/test_dnsnode.py
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2026

Codecov Report

❌ Patch coverage is 0% with 179 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (b66c61d) to head (18494ac).

Files with missing lines Patch % Lines
dissect/database/ese/ntds/objects/dnsnode.py 0.00% 170 Missing ⚠️
dissect/database/ese/ntds/objects/c_dnsnode.py 0.00% 5 Missing ⚠️
dissect/database/ese/ntds/ntds.py 0.00% 2 Missing ⚠️
dissect/database/ese/ntds/util.py 0.00% 2 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff           @@
##            main     #20    +/-   ##
======================================
  Coverage   0.00%   0.00%            
======================================
  Files        151     152     +1     
  Lines       4537    4716   +179     
======================================
- Misses      4537    4716   +179     
Flag Coverage Δ
unittests 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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

william-billaud and others added 2 commits May 7, 2026 15:06
Co-authored-by: Erik Schamper <1254028+Schamper@users.noreply.github.com>
@william-billaud
Copy link
Copy Markdown
Contributor Author

I changed some docs strings with list as it was rendering like this;
screen_2026_05_07_17_20_21

After change

screen_2026_05_07_17_32_51

@william-billaud william-billaud requested a review from Schamper May 7, 2026 15:38
Comment thread dissect/database/ese/ntds/objects/dnsnode.py Outdated
def as_dict(self) -> dict[str, Any]:
result = super().as_dict()
result["distinguished_name_as_dns_name"] = self.distinguished_name_as_dns_name
result["dns_record"] = [r.as_dict() for r in self.dns_record]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As an addition to the above comment, you can still keep this line in here so that you change the DnsRecord object into a dictionary too.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done, but I'm not sure I've fully understood (my understanding is that I should not create a new dict key, but just replace the DnsRecord key in the generated dict)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, because otherwise the value "dns_record" in the dictionary will be the DnsRecord objects themselves. With this line you replace those to be dictionary's variants of those objects too, which I think is desirable behavior.

@william-billaud william-billaud requested a review from Schamper May 8, 2026 09:02
@Schamper
Copy link
Copy Markdown
Member

Schamper commented May 8, 2026

I made some small changes, let me know if you agree and then this LGTM!

@william-billaud
Copy link
Copy Markdown
Contributor Author

LGTM, thanks for the review.

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented May 8, 2026

Merging this PR will not alter performance

✅ 6 untouched benchmarks


Comparing william-billaud:dns_node (18494ac) with main (b66c61d)

Open in CodSpeed

@Schamper Schamper merged commit 62f6301 into fox-it:main May 8, 2026
24 checks passed
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.

Parse Ntds DnsNode dnsRecord entries

2 participants