-
Notifications
You must be signed in to change notification settings - Fork 1.3k
fix that log sensitive infomation in cmd of script #12024
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix that log sensitive infomation in cmd of script #12024
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.20 #12024 +/- ##
=========================================
Coverage 16.24% 16.25%
- Complexity 13396 13401 +5
=========================================
Files 5658 5658
Lines 499273 499291 +18
Branches 60600 60598 -2
=========================================
+ Hits 81125 81143 +18
Misses 409104 409104
Partials 9044 9044
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 15701 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-14794)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses a security issue where sensitive information (such as passwords) was being logged in plain text within the Script class. The fix introduces a new addSensitive() method to mark specific command arguments as sensitive, ensuring they are masked with "******" in logs and command-line representations.
Key changes:
- Added
sensitiveArgIndicesSet to track which arguments contain sensitive data - Implemented
addSensitive()method for explicitly marking sensitive arguments - Updated all logging statements throughout the
execute()methods to conditionally log sanitized messages when sensitive arguments are present
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 13 comments.
| File | Description |
|---|---|
| Script.java | Core implementation adding sensitive argument tracking and comprehensive logging changes to mask sensitive data |
| ScriptTest.java | Test cases validating that sensitive arguments are properly masked in command-line output |
| LibvirtUpdateHostPasswordCommandWrapper.java | Updated to use addSensitive() for password arguments |
| CitrixUpdateHostPasswordCommandWrapper.java | Updated logging to mask password in debug output |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…ithub.com/YLChen-007/cloudstack into my-fix-420-mask-script-cmd-sensitive-info
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 16497 |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16501 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian Build Failed (tid-15250) |
sureshanaparti
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clgtm
|
@blueorangutan test |
|
@borisstoyanov a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
|
[SF] Trillian test result (tid-15303)
|
borisstoyanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, based on test results and code review
Description
This PR fixes that log sensitive infomation in cmd of cloud.utils.script.Script. #12005