feat: Write roles fingerprints to /var/log/sysroles.jsonl [citest_skip] - #892
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 Structured fingerprint logging
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches📝 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
🧹 Nitpick comments (1)
library/sr_fingerprint.py (1)
130-139: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove standard imports before the Ansible import and run
tox -e black,flake8.This file has the Ansible import before the standard-library imports, so group the imports before changing anything else. Then run
tox -e black,flake8before committing, as the Python path requires the required checks.🤖 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 130 - 139, Reorder the imports in sr_fingerprint.py so all standard-library imports precede the AnsibleModule import, keeping the existing import set unchanged. Run tox -e black,flake8 and resolve any reported formatting or lint issues before committing.Source: Path instructions
🤖 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 283-287: Update _format_fingerprint_key_value to transform control
characters, including carriage return, newline, and NUL, into visible escaped
sequences before formatting and joining fingerprint key-value pairs. Preserve
the existing quoting and quote-doubling behavior, and add tests verifying
formatted output contains no raw CR, LF, or NUL characters.
- Around line 231-232: Update the log-writing flow around _trim_log_file to
calculate and pass only the actual overflow, cur_size + len(new_line) -
max_size, when trimming is needed. Define and enforce behavior for records
larger than max_size so appending cannot exceed the documented limit, and add
coverage for partial overflow and oversized records.
- Around line 220-237: Harden _write_jsonl_log against symlink substitution by
persisting only in a trusted, non-attacker-writable directory, opening both
log_file and lock_path with no-follow semantics, and validating that each opened
file is a regular file owned by the expected user before use. Ensure lock
acquisition is performed on the safely opened lock descriptor without trusting a
replaceable pathname, and add a regression test covering a symlinked lock
sidecar.
---
Nitpick comments:
In `@library/sr_fingerprint.py`:
- Around line 130-139: Reorder the imports in sr_fingerprint.py so all
standard-library imports precede the AnsibleModule import, keeping the existing
import set unchanged. Run tox -e black,flake8 and resolve any reported
formatting or lint issues before committing.
🪄 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: 6d5c6c97-b2c4-4c1f-a8a6-867fef2edded
📒 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") | ||
| try: | ||
| fcntl.flock(lock_fd, fcntl.LOCK_EX) | ||
| try: | ||
| cur_size = os.path.getsize(log_file) | ||
| except OSError: | ||
| cur_size = 0 | ||
| if max_size > 0 and cur_size + len(new_line) > max_size and cur_size > 0: | ||
| _trim_log_file(log_file, len(new_line)) | ||
| with open(log_file, "a") as log_fd: | ||
| log_fd.write(new_line) | ||
| finally: | ||
| fcntl.flock(lock_fd, fcntl.LOCK_UN) | ||
| lock_fd.close() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Prevent symlink attacks on the log and lock paths.
If a privileged play writes to a configured path in an attacker-writable directory, a local user can replace <log_file>.lock with a symlink. Line 224 follows that symlink and truncates its target before flock runs. Line 233 also follows a substituted log_file.
Use a trusted, non-attacker-writable directory for persistence. Open both files with no-follow semantics and verify regular-file ownership before use. A no-follow open alone is not sufficient if an attacker can replace the lock pathname between writers.
Add a regression test for a symlinked lock sidecar.
🧰 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)
[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 220 - 237, Harden _write_jsonl_log
against symlink substitution by persisting only in a trusted,
non-attacker-writable directory, opening both log_file and lock_path with
no-follow semantics, and validating that each opened file is a regular file
owned by the expected user before use. Ensure lock acquisition is performed on
the safely opened lock descriptor without trusting a replaceable pathname, and
add a regression test covering a symlinked lock sidecar.
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.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Trim only the actual overflow and handle oversized records.
_trim_log_file() receives len(new_line), but the required space is cur_size + len(new_line) - max_size. When the file is below the limit and records have different lengths, this removes more complete records than required. If new_line alone exceeds max_size, the function appends it and exceeds the documented maximum.
Calculate the overflow before trimming. Reject or explicitly define the behavior for a single record larger than max_log_size. Add tests for partial overflow and an oversized record.
🧰 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-writing
flow around _trim_log_file to calculate and pass only the actual overflow,
cur_size + len(new_line) - max_size, when trimming is needed. Define and enforce
behavior for records larger than max_size so appending cannot exceed the
documented limit, and add coverage for partial overflow and oversized records.
| 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 control characters in syslog fields.
role_name, role_path, and distribution values can contain control characters. The current formatter preserves CR, LF, and NUL because it only handles spaces, quotes, and =. A newline splits the formatted fingerprint into multiple log lines and can inject misleading syslog fields.
Escape control characters to visible sequences before joining the key-value pairs. Add tests that assert formatted output contains no raw CR, LF, or NUL.
🤖 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 transform control characters, including
carriage return, newline, and NUL, into visible escaped sequences before
formatting and joining fingerprint key-value pairs. Preserve the existing
quoting and quote-doubling behavior, and add tests verifying formatted output
contains no raw CR, LF, or NUL characters.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| os.fchmod(fd, stat.S_IMODE(orig_stat.st_mode)) | ||
| try: | ||
| os.fchown(fd, orig_stat.st_uid, orig_stat.st_gid) | ||
| except OSError: |
| except BaseException: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except OSError: |
| for path in (log_file, log_file + ".lock"): | ||
| try: | ||
| os.unlink(path) | ||
| except OSError: |
spetrosi
changed the title
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>
16186ec to
3158d71
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] |
The fingerprint commit inadvertently replaced this file with a minimal template, losing role-specific test dependencies. Restore the original. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
[citest] |
3 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