Skip to content

Conversation

nikhim-um
Copy link
Contributor

@nikhim-um nikhim-um commented Mar 20, 2025

6.aa.core.log
6.core.log
The changes are related to

IcM 526315073
Work Item: 29003600

In existing behavior the service, timer files and the auto assessment script file are getting created with 775 permissions which means other than root users also can execute and modify the file. Which may lead to unauthorized modification of files. This change intends to restrict the execution permissions to root user.

Test Results:

image image image

Periodic Assessment result after 12hrs:

image image

Copy link

codecov bot commented Mar 20, 2025

Codecov Report

Attention: Patch coverage is 92.59259% with 4 lines in your changes missing coverage. Please review.

Project coverage is 92.49%. Comparing base (cb5eb95) to head (7c41e05).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/core/tests/Test_ServiceManager.py 92.00% 2 Missing ⚠️
src/core/tests/Test_TimerManager.py 92.00% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   92.43%   92.49%   +0.06%     
==========================================
  Files          97       99       +2     
  Lines       16237    16287      +50     
==========================================
+ Hits        15008    15064      +56     
+ Misses       1229     1223       -6     
Flag Coverage Δ
python27 92.49% <92.59%> (-0.03%) ⬇️
python39 92.49% <92.59%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kjohn-msft kjohn-msft added the bug Something isn't working label Mar 20, 2025
@kjohn-msft kjohn-msft changed the title Restricting execution permissions to root user/ owner Bugfix: Auto-assessment - Restricting execution permissions to root user/ owner Mar 20, 2025
Copy link
Contributor

@rane-rajasi rane-rajasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test for an auto assessment run that is auto triggered on schedule. The one added to this description is a default one that triggers during ConfigurePatching. You'll have to wait for ~12 hours for the auto assessment service to run without any external push. It will write status to the previous seqno but the activityId between PatchAssessmentResult and ConfigurePatchingResult will be different alongwith the startTime

@kjohn-msft
Copy link
Collaborator

Please provide the *.aa.log file into PR description showing an actual AA run (edit -> attach). Also include a regular log file showing the aa configuration by core. Status entries as 'Platform' also come from non-auto-assessment configurePatching calls and is not conclusive on which code path executed.

Copy link
Collaborator

@kjohn-msft kjohn-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on log notes inline.

General comment on adding comments also.

@feng-j678
Copy link
Contributor

feng-j678 commented Mar 20, 2025

lets not include any msft internal application docs/links etc here, security concern

Copy link
Contributor

@feng-j678 feng-j678 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since this pr touches 2 files with lowest code coverage, this will flag code coverage while merge current state it won't affect coverage %, but best practice is to add ut coverage for other logics in servicemanager and timermanager.py, so our overall coverage will reach target goal: 95%

@nikhim-um nikhim-um closed this Mar 21, 2025
@nikhim-um nikhim-um reopened this Mar 21, 2025
@nikhim-um
Copy link
Contributor Author

Please provide the *.aa.log file into PR description showing an actual AA run (edit -> attach). Also include a regular log file showing the aa configuration by core. Status entries as 'Platform' also come from non-auto-assessment configurePatching calls and is not conclusive on which code path executed.

attached

@nikhim-um
Copy link
Contributor Author

Test for an auto assessment run that is auto triggered on schedule. The one added to this description is a default one that triggers during ConfigurePatching. You'll have to wait for ~12 hours for the auto assessment service to run without any external push. It will write status to the previous seqno but the activityId between PatchAssessmentResult and ConfigurePatchingResult will be different alongwith the startTime

Provided screenshot to verify this

Copy link
Collaborator

@kjohn-msft kjohn-msft left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments inline -- pulling code locally right now to see tests running. Should not have any other comments, but sharing what's going on.

@nikhim-um nikhim-um enabled auto-merge (squash) March 21, 2025 18:13
@nikhim-um nikhim-um merged commit af59b83 into master Mar 21, 2025
6 checks passed
@nikhim-um nikhim-um deleted the nikhim-service-permissions branch March 21, 2025 18:15
@rane-rajasi rane-rajasi mentioned this pull request Apr 25, 2025
rane-rajasi added a commit that referenced this pull request Apr 25, 2025
This release includes:
[x] Engg. hygiene: Remove TelemetryWriter related log noise
[#312](#312)
[x] Bugfix: Mitigate external Ubuntu Pro Client issue
[#308](#308)
[x] Feature: Adding support for Azure Linux 2.0 in Tdnf Package Manager
[#311](#311)
[x] Eng. sys: Upgrade CICD pipeline from Python 3.9 to Python 3.12
[#309](#309)
[x] Coverage: Increase code coverage - TimerManager and ServiceManager
[#307](#307)
[x] Bugfix: Unit tests broken in Python 3.12
[#306](#306)
[x] Feature: Adding Azure Linux 3.0 Base Support
[#293](#293)
[x] Bugfix: Retry Handler to Prevent Unbounded Retries while trying to
Mitigate YUM Update Errors
[#303](#303)
[x] BugFix: CentOS VMs not installing patches during Auto Patching
[#298](#298)
[x] Bugfix: Auto-assessment - Restricting execution permissions to root
user/ owner
[#299](#299)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants