feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #140
Conversation
|
Important Review skippedIgnore keyword(s) in the title. ⛔ Ignored keywords (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesThe Role fingerprint module
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@library/sr_fingerprint.py`:
- Around line 231-232: Update the log-appending flow around _trim_log_file to
handle records where len(new_line) exceeds max_size before trimming or writing:
consistently reject or skip the oversized row, including in check mode, so the
JSONL file never exceeds a positive limit. Add coverage asserting the file
remains within max_size after an oversized record is processed.
- Around line 220-224: Harden _write_jsonl_log and every log_file read/write
path against symlink attacks by opening files with no-follow semantics,
validating each resulting descriptor is a regular file, and avoiding any
subsequent path-based open after validation. Apply the same protections to the
<log_file>.lock sidecar, replacing the current open(lock_path, "w") flow while
preserving locking and logging behavior.
- Around line 283-287: Update _format_fingerprint_key_value to escape
backslashes, quotes, carriage returns, line feeds, and tabs in values before
emitting quoted syslog fields, while preserving existing unquoted formatting
behavior. Add a regression test covering a newline in a field value and verify
the resulting fingerprint value cannot span syslog lines.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 24c4ff37-c3fc-4eb0-8d6d-0330e08bddc7
📒 Files selected for processing (2)
library/sr_fingerprint.pytests/unit/test_sr_fingerprint.py
| def _write_jsonl_log(log_file, record, max_size=0): | ||
| _ensure_parent_dir(log_file) | ||
| new_line = _format_fingerprint_jsonl(record) + "\n" | ||
| lock_path = log_file + ".lock" | ||
| lock_fd = open(lock_path, "w") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Prevent symlink attacks on the log file and lock file.
Line 224 opens <log_file>.lock with "w". If log_file is under a locally writable directory, a local attacker can replace the sidecar with a symlink. A privileged module execution then truncates the symlink target before it acquires the lock.
Apply no-follow descriptor opens and regular-file validation to the lock file and every log_file read or write path. Do not use a path-following open after validation.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 223-223: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(lock_path, "w")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@library/sr_fingerprint.py` around lines 220 - 224, Harden _write_jsonl_log
and every log_file read/write path against symlink attacks by opening files with
no-follow semantics, validating each resulting descriptor is a regular file, and
avoiding any subsequent path-based open after validation. Apply the same
protections to the <log_file>.lock sidecar, replacing the current
open(lock_path, "w") flow while preserving locking and logging behavior.
Source: Linters/SAST tools
| if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0: | ||
| _trim_log_file(log_file, len(new_line)) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Enforce the configured JSONL size limit for oversized records.
If new_line alone exceeds max_size, Line 231 either skips trimming for an empty file or removes every old row and then appends the oversized row. The file remains larger than max_log_size.
Define an oversized-record policy before appending. Reject or intentionally skip a row that cannot fit. Apply the same policy in check mode. Add a test that asserts the file never exceeds a positive max_size.
🧰 Tools
🪛 ast-grep (0.45.0)
[warning] 232-232: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(log_file, "a")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@library/sr_fingerprint.py` around lines 231 - 232, Update the log-appending
flow around _trim_log_file to handle records where len(new_line) exceeds
max_size before trimming or writing: consistently reject or skip the oversized
row, including in check mode, so the JSONL file never exceeds a positive limit.
Add coverage asserting the file remains within max_size after an oversized
record is processed.
| def _format_fingerprint_key_value(field, value): | ||
| text = "" if value is None else str(value) | ||
| if any(char in text for char in ' "='): | ||
| return '%s="%s"' % (field, text.replace('"', '""')) | ||
| return "%s=%s" % (field, text) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Escape line-control characters in syslog values.
role_name, role_path, and distribution values can contain CR or LF. The current formatter emits them unchanged. A value with a line break can create forged or malformed syslog records.
Escape backslashes, quotes, CR, LF, and tabs before formatting a quoted value. Add a regression test with a newline in a field value.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@library/sr_fingerprint.py` around lines 283 - 287, Update
_format_fingerprint_key_value to escape backslashes, quotes, carriage returns,
line feeds, and tabs in values before emitting quoted syslog fields, while
preserving existing unquoted formatting behavior. Add a regression test covering
a newline in a field value and verify the resulting fingerprint value cannot
span syslog lines.
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users. Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl Signed-off-by: Sergei Petrosian <spetrosi@redhat.com>
1c20fee to
e7e6cb4
Compare
The sr_fingerprint module was rewritten to accept structured parameters (status, role_name, role_path, etc.) instead of a free-form sr_message. Update the role tasks and tests to match the new module interface. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[citest] |
2 participants
Feature: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip]
Reason: By default logs are printed to rsyslog. This change adds a possibility to write logs to a file on the system for the downstream users.
Result: For the upstream, this makes rsyslog log message more detailed. For the downstream - also writes logs to /var/log/sysroles.jsonl