fix(agentlog): use proper file permissions when appending logs
Description
Bug report
When we run “Reboot reason“ command from Edge View, the command fails due to permission error:
reboot-reason.log open /persist/log/reboot-reason.log: permission deniedWhen i check the file on eve I see that the file does not have any permission s set :
b8519a4e-1ba1-48a2-ae74-0d7668fb4457:~# ls -l /persist/log/reboot-reason.log ---------- 1 root root 621 Dec 12 04:59 /persist/log/reboot-reason.log b8519a4e-1ba1-48a2-ae74-0d7668fb4457:~#
Fix
RebootReason function in EdgeView creates /persist/log/reboot-reason.log by calling printToFile, printToFile creates the file if it doesn't exist, incorrectly passing os.ModeAppend as permission. This will lead to unix perm being 000, hence no permission.
This PR replace os.ModeAppend with 0644 in OpenFile to correctly set Unix rw-r--r-- permissions when creating or appending to log files.
PR dependencies
None
How to test and validate this PR
Apply the fixes, or use an image with the fix in place and run "Reboot reason“ command from Edge View, it should succeed.
Changelog notes
Fix “Reboot reason” command in EdgeView failing due to log file permission errors
PR Backports
To be determined.
Checklist
- [x] I've provided a proper description
- [x] I've added the proper documentation
- [ ] I've tested my PR on amd64 device
- [ ] I've tested my PR on arm64 device
- [x] I've written the test verification instructions
- [x] I've set the proper labels to this PR
For backport PRs (remove it if it's not a backport):
- [ ] I've added a reference link to the original PR
- [ ] PR's title follows the template
And the last but not least:
- [ ] I've checked the boxes above, or I've provided a good reason why I didn't check them.
Please, check the boxes above after submitting the PR in interactive mode.
You need to fix the file in pkg/pillar/agentlog/agentlog.c (as one commit), and then run make bump-eve-pillar which will update edgeview etc vendoring of pillar and commit those. [I think that will make the go.mod/go.sum shas be correct without using two separate PRs.]
Is there other code which has the same bug in the use of os.OpenFile?
as far as I know there is no way to do it in one PR: the go package needs to be published first for us to know which hash it's going to have. make bump-eve-pillar is getting that hash from the go pkg database online, not locally.
Codecov Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 28.08%. Comparing base (2281599) to head (02eaf7c).
:warning: Report is 159 commits behind head on master.
Additional details and impacted files
@@ Coverage Diff @@
## master #5487 +/- ##
==========================================
+ Coverage 19.52% 28.08% +8.55%
==========================================
Files 19 19
Lines 3021 2314 -707
==========================================
+ Hits 590 650 +60
+ Misses 2310 1520 -790
- Partials 121 144 +23
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
:rocket: New features to boost your workflow:
- :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
You need to fix the file in pkg/pillar/agentlog/agentlog.c
I can't find this file :/
You need to fix the file in pkg/pillar/agentlog/agentlog.c (as one commit), and then run make bump-eve-pillar which will update edgeview etc vendoring of pillar and commit those. [I think that will make the go.mod/go.sum shas be correct without using two separate PRs.]
Is there other code which has the same bug in the use of os.OpenFile?
Oh shoot, I see now! I fixed in the vendor directory! sorry I just search for the function and picked the wrong file!
Is there other code which has the same bug in the use of os.OpenFile?
at least Semgrep can't find anything obvious. It is not possible to do bitwise check on flag value in Semgrep so this rule is limited to string matching.
shah in 🌐 shah-MS-7E12 in eve on fix.printToFile [$?] took 5s
❯ semgrep --config=tests/semgrep-rules/os-openfile-non-perm-mode.yaml .
┌──── ○○○ ────┐
│ Semgrep CLI │
└─────────────┘
/home/shah/.local/share/pipx/venvs/semgrep/lib/python3.12/site-packages/opentelemetry/instrumentation/dependencies.py:4: UserWarning: pkg_resources is deprecated as an API. See https://setuptools.pypa.io/en/latest/pkg_resources.html. The pkg_resources package is slated for removal as early as 2025-11-30. Refrain from using this package or pin to Setuptools<81.
from pkg_resources import (
Scanning 1525 files (only git-tracked) with 1 Code rule:
CODE RULES
Scanning 567 files.
SUPPLY CHAIN RULES
No rules to run.
PROGRESS
━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 100% 0:00:00
┌──────────────┐
│ Scan Summary │
└──────────────┘
✅ Scan completed successfully.
• Findings: 0 (0 blocking)
• Rules run: 1
• Targets scanned: 567
• Parsed lines: ~100.0%
• Scan skipped:
◦ Files larger than files 1.0 MB: 7
◦ Files matching .semgrepignore patterns: 34102
• Scan was limited to files tracked by git
• For a detailed list of skipped files and lines, run semgrep with the --verbose flag
Ran 1 rule on 567 files: 0 findings.
⏫ A new version of Semgrep is available. See https://semgrep.dev/docs/upgrading
✨ If Semgrep missed a finding, please send us feedback to let us know!
See https://semgrep.dev/docs/reporting-false-negatives/
shah in 🌐 shah-MS-7E12 in eve on fix.printToFile [$+?] took 5s
❯
You need to fix the file in pkg/pillar/agentlog/agentlog.c
I can't find this file :/
Wrong programming language on my part ;-)
@shjala tests/semgrep-rules/os-openfile-non-perm-mode.yaml is missing Copyright note.
Please, once we merge it, don't forget to open the PR updating vendor files.... in all dependencies (pkg/edgeview, etc).
@shjala tests/semgrep-rules/os-openfile-non-perm-mode.yaml is missing Copyright note.
@rene added the Copyright note.
Please, once we merge it, don't forget to open the PR updating vendor files.... in all dependencies (pkg/edgeview, etc).
thanks for the reminder, do we have a script for this?
thanks for the reminder, do we have a script for this?
make bump-eve-pilllar (or something like that)