Repository navigation
Introduce login.defs parser and update the procedure to leverage it - #170
Draft
Robert Wojciechowski (robertwoj-microsoft) wants to merge 5 commits into
Draft
Robert Wojciechowski (robertwoj-microsoft) wants to merge 5 commits into
Robert Wojciechowski (robertwoj-microsoft) wants to merge 5 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Robert Wojciechowski (robertwoj-microsoft)
requested a balanced review from Copilot
October 13, 2026 13:52
Copilot started reviewing on behalf of
Robert Wojciechowski (robertwoj-microsoft)
October 13, 2026 13:53
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Production file reads truncate embedded NULs, and the new fuzz target and boundary seeds are not exercised as intended.
5 open findings
What changed in this PR
Introduces a reusable login.defs parser and migrates related compliance procedures to it.
Changes:
- Adds bounded parsing, diagnostics, unit tests, and fuzzing support.
- Updates UID and login-option procedures to consume parsed records.
- Expands malformed-input and boundary coverage.
| File | Description |
|---|---|
src/tests/fuzzer/parsers/LoginDefsTarget.cpp |
Adds parser fuzz harness. |
src/tests/fuzzer/parsers/CMakeLists.txt |
Builds the fuzz target. |
src/tests/fuzzer/CMakeLists.txt |
Includes parser fuzzers. |
src/tests/fuzzer/parsers/corpus/login-defs/01-minimal |
Minimal seed. |
.../02-typical |
Typical configuration seed. |
.../03-comment-only |
Comment-only seed. |
.../04-inline-hash |
Inline-hash seed. |
.../05-attached-hash |
Attached-hash seed. |
.../06-quotes |
Quoted-value seed. |
.../07-duplicates |
Duplicate-key seed. |
.../08-unknown |
Unknown-key seed. |
.../09-missing-extra |
Malformed-value seed. |
.../10-whitespace |
Whitespace seed. |
.../11-escapes |
Escape seed. |
.../12-empty |
Empty seed. |
.../13-crlf |
Intended CRLF seed. |
.../15-no-final-newline |
Intended unterminated-line seed. |
.../16-line-limit |
Line-size boundary seed. |
src/modules/complianceengine/tests/UsersIteratorTest.cpp |
Expands UID parsing tests. |
.../tests/procedures/SystemAccountShellTest.cpp |
Tests parser integration and limits. |
.../tests/procedures/LoginDefsOptionTest.cpp |
Tests duplicate and malformed options. |
.../tests/procedures/AuditdRulesTest.cpp |
Tests UID error handling. |
.../tests/parsers/LoginDefsTest.cpp |
Adds parser unit coverage. |
.../tests/CMakeLists.txt |
Registers parser tests. |
.../src/lib/Users.h |
Adds preloaded-content overload. |
.../src/lib/Users.cpp |
Uses the shared parser for UID_MIN. |
.../src/lib/procedures/SystemAccountShell.cpp |
Migrates UID parsing. |
.../src/lib/procedures/LoginDefsOption.cpp |
Migrates option parsing. |
.../src/lib/procedures/AuditdRules.cpp |
Refines UID read-error handling. |
.../src/lib/parsers/LoginDefs.h |
Defines parser API and limits. |
.../src/lib/parsers/LoginDefs.cpp |
Implements parsing and diagnostics. |
.../src/lib/CMakeLists.txt |
Builds the parser implementation. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| std::stringstream ss(loginDefsResult.Value()); | ||
| std::string line; | ||
| while (std::getline(ss, line)) | ||
| const auto parsed = LoginDefs::Parse(loginDefsResult.Value(), "/etc/login.defs"); |
| } | ||
|
|
||
| auto foundValue = FindLoginDefsValue(fileContents.Value(), params.option, context.GetLogHandle()); | ||
| const auto parsed = LoginDefs::Parse(fileContents.Value(), filePath); |
| # Copyright (c) Microsoft Corporation. | ||
| # Licensed under the MIT License. | ||
|
|
||
| add_executable(login-defs-parser-fuzzer LoginDefsTarget.cpp) |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Robert Wojciechowski (robertwoj-microsoft)
requested a balanced review from Copilot
October 14, 2026 13:18
Copilot started reviewing on behalf of
Robert Wojciechowski (robertwoj-microsoft)
October 14, 2026 13:19
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Duplicate handling and option restrictions introduce compatibility and correctness regressions.
6 open findings
2 resolved since last review
🧠 Review effort: Balanced
| ++keyEnd; | ||
| } | ||
| const auto key = input.substr(keyStart, keyEnd - keyStart); | ||
| const bool firstOccurrence = seen.insert(key).second; |
Comment on lines
+143
to
+151
| const auto value = TrimWhiteSpaces(input.substr(valueStart, end - valueStart)); | ||
| const auto parsed = TryStringToUint(value); | ||
| if (!parsed.HasValue()) | ||
| { | ||
| return ValueError(document, lineNumber, valueStart - start + 1, key, parsed.Error()); | ||
| } | ||
| if (parsed.Value() > std::numeric_limits<uid_t>::max()) | ||
| { | ||
| return ValueError(document, lineNumber, valueStart - start + 1, key, Error("Integer value out of uid_t range: " + value, ERANGE)); |
Comment on lines
+80
to
+83
| if (!IsNumericLoginDefsOption(params.option) && params.option != "ENCRYPT_METHOD") | ||
| { | ||
| OsConfigLogError(context.GetLogHandle(), "LoginDefsOption: unsupported option '%s'", params.option.c_str()); | ||
| return Error("Unsupported login.defs option: " + params.option, EINVAL); |
Robert Wojciechowski (robertwoj-microsoft)
requested a balanced review from Copilot
October 14, 2026 15:02
Copilot started reviewing on behalf of
Robert Wojciechowski (robertwoj-microsoft)
October 14, 2026 15:15
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Production file loading truncates embedded NULs before validation, allowing corrupted documents to produce successful audit results.
7 open findings
🧠 Review effort: Balanced
|
|
||
| auto foundValue = FindLoginDefsValue(fileContents.Value(), params.option, context.GetLogHandle()); | ||
| if (!foundValue.HasValue()) | ||
| const auto parsed = LoginDefs::Parse(std::move(fileContents.Value()), filePath, context.GetLogHandle()); |
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
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
Introduce a generic parser concept
Separate parsing logic from evalutaion logic so that parsing the same file format is shared across rules. This has few benefits:
Minor drawback:
Sometime parsing may store unnecessary data in memory, in-place parsers can short-cirquit and use lower footprint. I think this is acceptable.
Checklist
mainbranch prior to this PR submission.mainbranch.