Collect AIACAs with empty CACert BED-7513#274
Merged
Conversation
WalkthroughAdds a byte-validation guard for CACertificate data and applies it in Root, AIA, and Enterprise CA readers to skip certificate parsing when bytes are null, empty, or a single 0x00. Expands unit tests to cover valid and empty CACertificate scenarios across CA readers. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/unit/LdapPropertyTests.cs (2)
721-721: Parameter naming: Consider using camelCase.The parameter
CACertBytesuses PascalCase. C# conventions typically use camelCase for method parameters (e.g.,caCertBytes). This applies to all threeNoCACertificatetest methods.♻️ Suggested fix
-public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CACertBytes) { +public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] caCertBytes) {Apply similarly to
LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificateandLDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/LdapPropertyTests.cs` at line 721, The parameter name CACertBytes in the test method LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate (and the analogous parameters in LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate and LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate) should be renamed to camelCase (caCertBytes); update the method signatures and every usage inside each method to the new name so the tests compile and follow C# parameter naming conventions.
893-899: Multi-byte zero arrays would bypass theHasBytesguard and reach X.509 parsing.The
HasBytesimplementation (lines 941-944 ofLdapPropertyProcessor.cs) only rejects empty arrays and single-byte zero values. A multi-byte all-zero array like{ 0x00, 0x00 }would pass this guard and be passed to theParsedCertificateconstructor, which could trigger certificate parsing failures if such data occurs in AD.The current test data appropriately covers the specific
{ 0x00 }edge case mentioned in the bug report. Consider adding a test case for multi-byte zero arrays to document whether this scenario is expected or should be guarded against.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/LdapPropertyTests.cs` around lines 893 - 899, Add a test case for multi-byte all-zero certificate bytes to LdapPropertyTests.EmptyCertBytes to cover the scenario where HasBytes in LdapPropertyProcessor allows arrays longer than 1 that contain only 0x00; update the test data to include e.g. new byte[] { 0x00, 0x00 } so that behavior when passing such arrays into ParsedCertificate (constructor used by LdapPropertyProcessor) is exercised and documents whether additional guarding is needed in HasBytes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/unit/LdapPropertyTests.cs`:
- Line 781: Remove the stray double semicolon at the end of the assertion in the
test: locate the Assert.Contains("basicconstraintpathlength", keys);; statement
in LdapPropertyTests (the failing test uses Assert.Contains) and delete the
extra semicolon so the line ends with a single semicolon.
---
Nitpick comments:
In `@test/unit/LdapPropertyTests.cs`:
- Line 721: The parameter name CACertBytes in the test method
LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate (and the analogous
parameters in LDAPPropertyProcessor_ReadAIACAProperties_NoCACertificate and
LDAPPropertyProcessor_ReadEnterpriseCAProperties_NoCACertificate) should be
renamed to camelCase (caCertBytes); update the method signatures and every usage
inside each method to the new name so the tests compile and follow C# parameter
naming conventions.
- Around line 893-899: Add a test case for multi-byte all-zero certificate bytes
to LdapPropertyTests.EmptyCertBytes to cover the scenario where HasBytes in
LdapPropertyProcessor allows arrays longer than 1 that contain only 0x00; update
the test data to include e.g. new byte[] { 0x00, 0x00 } so that behavior when
passing such arrays into ParsedCertificate (constructor used by
LdapPropertyProcessor) is exercised and documents whether additional guarding is
needed in HasBytes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d28c6894-a0ee-47b7-be8d-aa46c1090479
📒 Files selected for processing (1)
test/unit/LdapPropertyTests.cs
mykeelium
approved these changes
Mar 5, 2026
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/LdapPropertyTests.cs (1)
719-745: Consider using camelCase for the test parameter.The test logic is correct and covers all edge cases for empty certificates. Minor style note: C# conventions typically use camelCase for method parameters (
caCertBytesinstead ofCACertBytes).🔧 Suggested fix
[Theory] [MemberData(nameof(EmptyCertBytes))] -public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] CACertBytes) { +public void LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate(byte[] caCertBytes) { var mock = new MockDirectoryObject( "CN\u003dDUMPSTER-DC01-CA,CN\u003dAIA,CN\u003dPUBLIC KEY SERVICES,CN\u003dSERVICES,CN\u003dCONFIGURATION,DC\u003dDUMPSTER,DC\u003dFIRE", new Dictionary<string, object> { {"description", null}, {"domain", "DUMPSTER.FIRE"}, {"name", "DUMPSTER-DC01-CA@DUMPSTER.FIRE"}, {"domainsid", "S-1-5-21-2697957641-2271029196-387917394"}, {"whencreated", 1683986131}, - {LDAPProperties.CACertificate, CACertBytes} + {LDAPProperties.CACertificate, caCertBytes} }, "","2F9F3630-F46A-49BF-B186-6629994EBCF9");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unit/LdapPropertyTests.cs` around lines 719 - 745, The test method LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate uses a PascalCase parameter name CACertBytes which violates C# parameter naming conventions; rename the parameter to camelCase (e.g., caCertBytes) in the method signature and update all usages inside the test (the MockDirectoryObject initialization and the MemberData reference stays the same) so the variable name matches; ensure the MemberData attribute name (EmptyCertBytes) remains unchanged and run tests to confirm no references are broken.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unit/LdapPropertyTests.cs`:
- Around line 719-745: The test method
LDAPPropertyProcessor_ReadRootCAProperties_NoCACertificate uses a PascalCase
parameter name CACertBytes which violates C# parameter naming conventions;
rename the parameter to camelCase (e.g., caCertBytes) in the method signature
and update all usages inside the test (the MockDirectoryObject initialization
and the MemberData reference stays the same) so the variable name matches;
ensure the MemberData attribute name (EmptyCertBytes) remains unchanged and run
tests to confirm no references are broken.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: de76bbe9-9c59-44fc-8a33-6e4dd7a7758e
📒 Files selected for processing (1)
test/unit/LdapPropertyTests.cs
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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
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
Documentation