Skip to content

Conversation

@JSCU-CNI
Copy link
Contributor

This PR adds basic x509 certificate parsing to the webserver plugins of apache and nginx.

yield WebserverCertificateRecord(
ts=cert_path.lstat().st_mtime,
webserver="nginx",
**cert._asdict(),
Copy link
Member

Choose a reason for hiding this comment

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

@yunzheng what are your thoughts on this pattern?

Copy link
Member

Choose a reason for hiding this comment

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

Nothing wrong with this pattern. Other options could've been using a webserver metadata RecordDescriptor in combination with GroupedRecord, or using extend_record. But it seems there are already other Webserver-like RecordDescriptors defined so I think this is fine.

It will however, overwrite the _source, _generated and _version fields. But depending if that's an issue or not, can be fixed by using cert._asdict(exclude=["_source", "_generated", "_version"]).

@JSCU-CNI JSCU-CNI requested a review from Schamper December 4, 2025 12:16
@EinatFox EinatFox linked an issue Dec 4, 2025 that may be closed by this pull request
@JSCU-CNI JSCU-CNI requested a review from Schamper December 11, 2025 13:17
Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

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

Looks like the certlog plugin now has some field conflicts, can you resolve?

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 11, 2025

Merging this PR will not alter performance

✅ 9 untouched benchmarks


Comparing JSCU-CNI:webserver-certs (82f9193) with main (e0cbfed)

Open in CodSpeed

@JSCU-CNI
Copy link
Contributor Author

Looks like the certlog plugin now has some field conflicts, can you resolve?

Temporarily fixed in 1f9b66a. Created a separate issue for further normalization of CertLog certificates in #1452.

@JSCU-CNI JSCU-CNI requested a review from Schamper December 11, 2025 14:50
@codecov
Copy link

codecov bot commented Dec 11, 2025

Codecov Report

❌ Patch coverage is 80.55556% with 21 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.38%. Comparing base (e0cbfed) to head (82f9193).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
dissect/target/helpers/certificate.py 78.00% 11 Missing ⚠️
dissect/target/plugins/apps/webserver/nginx.py 80.64% 6 Missing ⚠️
dissect/target/plugins/apps/webserver/apache.py 84.00% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1415      +/-   ##
==========================================
- Coverage   80.38%   80.38%   -0.01%     
==========================================
  Files         392      393       +1     
  Lines       34446    34539      +93     
==========================================
+ Hits        27691    27764      +73     
- Misses       6755     6775      +20     
Flag Coverage Δ
unittests 80.38% <80.55%> (-0.01%) ⬇️

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.

Copy link
Member

@Schamper Schamper left a comment

Choose a reason for hiding this comment

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

@JSCU-CNI
Copy link
Contributor Author

I am unable to reproduce the issue locally unfortunately.

@Schamper
Copy link
Member

Seems pretty consistent on Windows 3.10 and 3.11. If you can't reproduce I'll take a look somewhere next week.

@Schamper
Copy link
Member

I'll take a look somewhere next week.

I'm otherwise a very honest and dependable person.

Anyway, the problem was that some fields that were typed as path would be initialized with strings. I'm not entirely sure if fox-it/flow.record#200 would've fixed this too, but at least now we properly pass in Path-like objects,

@Schamper Schamper merged commit 4b7bddc into fox-it:main Jan 12, 2026
15 of 22 checks passed
@JSCU-CNI JSCU-CNI deleted the webserver-certs branch January 12, 2026 14:59
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.

Add certificate parsing to webserver plugins

3 participants