Open
Conversation
WalkthroughAdds a byte-validation guard for CACertificate data and applies it across Root, AIA, and Enterprise CA readers to skip certificate parsing when bytes are null, empty, or a single 0x00. Expands unit tests to cover these edge cases and valid-certificate scenarios. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/CommonLib/Processors/LdapPropertyProcessor.cs (1)
513-516: Consider applying the same guard toReadRootCAPropertiesandReadEnterpriseCAProperties.The
HasByteshelper addresses an edge case for AIACA, butReadRootCAProperties(lines 475-482) andReadEnterpriseCAProperties(lines 526-533) have identical certificate parsing patterns without this guard:// ReadRootCAProperties (line 475) if (entry.TryGetByteProperty(LDAPProperties.CACertificate, out var rawCertificate)) { var cert = new ParsedCertificate(rawCertificate); ... } // ReadEnterpriseCAProperties (line 526) if (entry.TryGetByteProperty(LDAPProperties.CACertificate, out var rawCertificate)) { var cert = new ParsedCertificate(rawCertificate); ... }If a RootCA or EnterpriseCA could have empty/zero-byte certificate data, these would also fail. Consider applying the
HasBytesguard consistently to all certificate parsing locations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/LdapPropertyProcessor.cs` around lines 513 - 516, ReadRootCAProperties and ReadEnterpriseCAProperties both construct a ParsedCertificate from rawCertificate without the HasBytes guard; update each method so they call HasBytes(rawCertificate) (the existing helper) before instantiating ParsedCertificate and only parse/assign when it returns true, otherwise skip parsing or handle as null/absent to avoid throwing on empty/zero-byte data.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/CommonLib/Processors/LdapPropertyProcessor.cs`:
- Around line 513-516: ReadRootCAProperties and ReadEnterpriseCAProperties both
construct a ParsedCertificate from rawCertificate without the HasBytes guard;
update each method so they call HasBytes(rawCertificate) (the existing helper)
before instantiating ParsedCertificate and only parse/assign when it returns
true, otherwise skip parsing or handle as null/absent to avoid throwing on
empty/zero-byte data.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unit/LdapPropertyTests.cs (1)
672-720: Consider extracting shared CA test setup into helpers.There’s repeated cert creation and
MockDirectoryObjectscaffolding in the new CA tests; a small helper would reduce maintenance churn.Also applies to: 736-754, 807-826
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/LdapPropertyTests.cs` around lines 672 - 720, Extract the repeated CA test setup (creating the self-signed cert bytes and building the MockDirectoryObject) into a small helper method (e.g., CreateMockRootCA or BuildMockRootCAMock) and call it from LDAPPropertyProcessor_ReadRootCAProperties, LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate, and the other affected tests (lines noted around 736-754 and 807-826); the helper should return the MockDirectoryObject (or the cert bytes + dictionary) so tests can assert on LdapPropertyProcessor.ReadRootCAProperties consistently and avoid duplicating certificate creation and dictionary scaffolding (refer to ECDsa.Create, CertificateRequest.CreateSelfSigned, cert.Export, MockDirectoryObject, and LDAPProperties.CACertificate to locate code to move).src/CommonLib/Processors/LdapPropertyProcessor.cs (1)
941-944: MakeHasBytesnull-safe for safer reuse.At Line 941,
HasBytesdereferencesdatadirectly. Current call sites are short-circuited, but a null-safe helper is more robust if reused later.♻️ Proposed defensive tweak
private static bool HasBytes(byte[] data) { - return data.Length > 0 - && !(data.Length == 1 && data[0] == 0x00); + return data is { Length: > 0 } + && !(data.Length == 1 && data[0] == 0x00); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CommonLib/Processors/LdapPropertyProcessor.cs` around lines 941 - 944, The HasBytes helper dereferences the input array without null-checking; update the HasBytes(byte[] data) method to first handle null (return false for null or empty) and then keep the existing zero-length/sole-null-byte logic so callers can safely pass null. Modify the implementation inside HasBytes to check data == null (or use data?.Length) and return false early, preserving the existing special-case (data.Length == 1 && data[0] == 0x00) to detect the solitary null byte.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/CommonLib/Processors/LdapPropertyProcessor.cs`:
- Around line 941-944: The HasBytes helper dereferences the input array without
null-checking; update the HasBytes(byte[] data) method to first handle null
(return false for null or empty) and then keep the existing
zero-length/sole-null-byte logic so callers can safely pass null. Modify the
implementation inside HasBytes to check data == null (or use data?.Length) and
return false early, preserving the existing special-case (data.Length == 1 &&
data[0] == 0x00) to detect the solitary null byte.
In `@test/unit/LdapPropertyTests.cs`:
- Around line 672-720: Extract the repeated CA test setup (creating the
self-signed cert bytes and building the MockDirectoryObject) into a small helper
method (e.g., CreateMockRootCA or BuildMockRootCAMock) and call it from
LDAPPropertyProcessor_ReadRootCAProperties,
LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate, and the other
affected tests (lines noted around 736-754 and 807-826); the helper should
return the MockDirectoryObject (or the cert bytes + dictionary) so tests can
assert on LdapPropertyProcessor.ReadRootCAProperties consistently and avoid
duplicating certificate creation and dictionary scaffolding (refer to
ECDsa.Create, CertificateRequest.CreateSelfSigned, cert.Export,
MockDirectoryObject, and LDAPProperties.CACertificate to locate code to move).
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
1 participant
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
There was an edge case where an AD AIACA with a
{0}value for CACertificate attribute would cause cert parsing to fail and AIACA wasn't collected. To fix we check if byte array is not empty before attempting to parse.Motivation and Context
BED-7513
How Has This Been Tested?
As a domain admin, make a new CA with empty cACertificate
Confirm that new CA object with cACertificate of {0} exists in AIA
Run new collection and confirm that AIACA node was created
Types of changes
Checklist:
Summary by CodeRabbit
Bug Fixes
Tests