Skip to content
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

Remove sensitive params (VmPassword, etc) from VMWork log #8553

Merged
merged 1 commit into from
Feb 5, 2024

Conversation

sureshanaparti
Copy link
Contributor

@sureshanaparti sureshanaparti commented Jan 23, 2024

Description

The VM's password & other details sent via VM Details Map during VmWorkStart are logged as base64 encoded strings.

2024-01-22 11:22:53,879 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-4:ctx-8c25a1bb job-4690/job-4691 ctx-18a45cc5) (logid:76180ad6) Execute VM work job: com.cloud.vm.VmWorkStart{"dcId":1,"podId":1,"clusterId":1,"hostId":2,"rawParams":{"ConsiderLastHost":"rO0ABXQABHRydWU","VmPassword":"rO0ABXQADnNhdmVkX3Bhc3N3b3Jk"},"userId":2,"accountId":2,"vmId":415,"handlerName":"VirtualMachineManagerImpl"}
...
2024-01-22 11:22:59,456 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-4:ctx-8c25a1bb job-4690/job-4691 ctx-18a45cc5) (logid:76180ad6) Done executing VM work job: com.cloud.vm.VmWorkStart{"dcId":1,"podId":1,"clusterId":1,"hostId":2,"rawParams":{"ConsiderLastHost":"rO0ABXQABHRydWU","VmPassword":"rO0ABXQADnNhdmVkX3Bhc3N3b3Jk"},"userId":2,"accountId":2,"vmId":415,"handlerName":"VirtualMachineManagerImpl"}

This PR improves VMWork log to not include any sensitive params (VmPassword, etc).

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • build/CI

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Tested VM start and reboot operations, confirmed 'VmPassword' not shown in the log.

Log before changes =>

2024-01-22 11:22:53,879 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-4:ctx-8c25a1bb job-4690/job-4691 ctx-18a45cc5) (logid:76180ad6) Execute VM work job: com.cloud.vm.VmWorkStart{"dcId":1,"podId":1,"clusterId":1,"hostId":2,"rawParams":{"ConsiderLastHost":"rO0ABXQABHRydWU","VmPassword":"rO0ABXQADnNhdmVkX3Bhc3N3b3Jk"},"userId":2,"accountId":2,"vmId":415,"handlerName":"VirtualMachineManagerImpl"}


2024-01-22 11:22:59,456 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-4:ctx-8c25a1bb job-4690/job-4691 ctx-18a45cc5) (logid:76180ad6) Done executing VM work job: com.cloud.vm.VmWorkStart{"dcId":1,"podId":1,"clusterId":1,"hostId":2,"rawParams":{"ConsiderLastHost":"rO0ABXQABHRydWU","VmPassword":"rO0ABXQADnNhdmVkX3Bhc3N3b3Jk"},"userId":2,"accountId":2,"vmId":415,"handlerName":"VirtualMachineManagerImpl"}

Log after changes =>

2024-01-22 17:59:32,005 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-1:ctx-e027727c job-4730/job-4731 ctx-6c00584c) (logid:24c6cb7e) Execute VM work job: com.cloud.vm.VmWorkStart{"accountId":2,"dcId":1,"vmId":415,"hostId":2,"handlerName":"VirtualMachineManagerImpl","clusterId":1,"userId":2,"podId":1,"rawParams":{"ConsiderLastHost":"rO0ABXQABHRydWU"}}


2024-01-22 17:59:36,550 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-1:ctx-e027727c job-4730/job-4731 ctx-6c00584c) (logid:24c6cb7e) Done executing VM work job: com.cloud.vm.VmWorkStart{"accountId":2,"dcId":1,"vmId":415,"hostId":2,"handlerName":"VirtualMachineManagerImpl","clusterId":1,"userId":2,"podId":1,"rawParams":{"ConsiderLastHost":"rO0ABXQABHRydWU"}}

How did you try to break this feature and the system with this change?

Copy link

codecov bot commented Jan 23, 2024

Codecov Report

Attention: 4 lines in your changes are missing coverage. Please review.

Comparison is base (6d916ca) 30.85% compared to head (15d9705) 30.78%.
Report is 37 commits behind head on main.

Files Patch % Lines
...ponents-api/src/main/java/com/cloud/vm/VmWork.java 73.33% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##               main    #8553      +/-   ##
============================================
- Coverage     30.85%   30.78%   -0.07%     
+ Complexity    34048    33982      -66     
============================================
  Files          5341     5341              
  Lines        374861   375048     +187     
  Branches      54518    54557      +39     
============================================
- Hits         115659   115457     -202     
- Misses       243973   244332     +359     
- Partials      15229    15259      +30     
Flag Coverage Δ
simulator-marvin-tests 24.63% <82.60%> (-0.13%) ⬇️
uitests 4.39% <ø> (-0.01%) ⬇️
unit-tests 16.51% <60.86%> (+0.04%) ⬆️

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.

@sureshanaparti
Copy link
Contributor Author

@blueorangutan package

@blueorangutan
Copy link

@sureshanaparti 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.

@blueorangutan
Copy link

Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8418

@sureshanaparti
Copy link
Contributor Author

@blueorangutan test

@blueorangutan
Copy link

@sureshanaparti a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

@blueorangutan
Copy link

[SF] Trillian test result (tid-8922)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 49893 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8553-t8922-kvm-centos7.zip
Smoke tests completed. 121 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test Result Time (s) Test File

Copy link
Contributor

@harikrishna-patnala harikrishna-patnala left a comment

Choose a reason for hiding this comment

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

code LGTM

Copy link
Contributor

@kiranchavala kiranchavala left a comment

Choose a reason for hiding this comment

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

LGTM , tested manually by deploying a vm with a password enabled template

Before fix

2024-01-24 10:47:34,021 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-13:ctx-2cfca443 job-66/job-67 ctx-a58c4ec9) (logid:ee57b953) Execute VM work job: com.cloud.vm.VmWorkStart{"dcId":1,"podId":1,"clusterId":1,"hostId":1,"rawParams":{"VmPassword":"rO0ABXQABmg3VDlYSw"},"userId":2,"accountId":2,"vmId":8,"handlerName":"VirtualMachineManagerImpl"}

2024-01-24 10:48:25,567 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-13:ctx-2cfca443 job-66/job-67 ctx-a58c4ec9) (logid:ee57b953) Done executing VM work job: com.cloud.vm.VmWorkStart{"dcId":1,"podId":1,"clusterId":1,"hostId":1,"rawParams":{"VmPassword":"rO0ABXQABmg3VDlYSw"},"userId":2,"accountId":2,"vmId":8,"handlerName":"VirtualMachineManagerImpl"}

After fix

2024-01-24 11:49:32,315 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-3:ctx-7971f576 job-33/job-34 ctx-ec622985) (logid:0a621004) Execute VM work job: com.cloud.vm.VmWorkStart{"accountId":2,"dcId":1,"vmId":3,"hostId":1,"handlerName":"VirtualMachineManagerImpl","clusterId":1,"userId":2,"podId":1,"rawParams":{}}

2024-01-24 11:50:23,512 DEBUG [c.c.v.VmWorkJobHandlerProxy] (Work-Job-Executor-3:ctx-7971f576 job-33/job-34 ctx-ec622985) (logid:0a621004) Done executing VM work job: com.cloud.vm.VmWorkStart{"accountId":2,"dcId":1,"vmId":3,"hostId":1,"handlerName":"VirtualMachineManagerImpl","clusterId":1,"userId":2,"podId":1,"rawParams":{}}

@rohityadavcloud rohityadavcloud added this to the 4.19.1.0 milestone Jan 24, 2024
@rohityadavcloud
Copy link
Member

LGTM - let's merge for 4.19.1.0, after the freeze/4.19.0.0 GA.

Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

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

CLGTM

@rohityadavcloud rohityadavcloud changed the base branch from main to 4.19 February 5, 2024 07:56
@rohityadavcloud rohityadavcloud merged commit f702f7f into apache:4.19 Feb 5, 2024
24 of 25 checks passed
@rohityadavcloud rohityadavcloud deleted the vm-params-log-update branch February 5, 2024 07:56
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Feb 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

None yet

6 participants