Fix being able to expunge a VM through destroyVirtualMachine even when role rule does not allow by gp-santos · Pull Request #8689 · apache/cloudstack

gp-santos · 2024-02-21T14:08:52Z

Description

This PR adds a role access check to the expungeVirtualMachine command when calling destroyVirtualMachine with the expunge parameter.

Currently, if you are an admin (even if not Root), it bypasses the allow.user.expunge.recover.vm verification and you are always allowed to expunge when calling for destroyVirtualMachine.

The use case that called for this change was a need for a role of type domain admin to be unable to expunge VMs. It was then found that even with the DENY rule, the user could still expunge through destroyVirtualMachine (even on already destroyed VMs, with an API call) and the setting allow.user.expunge.recover.vm did nothing.

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

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

I created a role, based on the default Domain Admin, and changed the expungeVirtualMachine rule to DENY. I then created an account with said role.

I created two VMs and destroyed one of them, verifying that the expunge option did not show up on the GUI.

I then ran destroy virtualmachine on cloudmonkey with expunge = true on both VMs and both returned the error Account does not have permission for expunging. Calling the same command without the parameter destroyed the running VM successfully.

I repeated the tests with a role based on default User:

With allow.user.expunge.recover.vm = true, it behaved the same as the DomainAdmin-based one.

With allow.user.expunge.recover.vm = false, it did not allow the expunge action, no matter the role rules. Without the expunge parameter, the destroy action worked as expected.

…VirtualMachine.

codecov · 2024-02-21T14:28:56Z

Codecov Report

Attention: Patch coverage is 50.00000% with 8 lines in your changes are missing coverage. Please review.

Project coverage is 23.13%. Comparing base (592038a) to head (007f1e1).
Report is 81 commits behind head on main.

Files	Patch %	Lines
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java	38.46%	4 Missing and 4 partials ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8689      +/-   ##
============================================
- Coverage     23.14%   23.13%   -0.02%     
- Complexity    23348    23485     +137     
============================================
  Files          5219     5234      +15     
  Lines        353412   355729    +2317     
  Branches      50883    51238     +355     
============================================
+ Hits          81805    82294     +489     
- Misses       259762   261540    +1778     
- Partials      11845    11895      +50

Flag	Coverage Δ
simulator-marvin-tests	`24.80% <50.00%> (-0.02%)`	⬇️
uitests	`4.34% <ø> (-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.

DaanHoogland

clgtm

DaanHoogland · 2024-02-23T08:16:29Z

@blueorangutan package

blueorangutan · 2024-02-23T08:18:03Z

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

blueorangutan · 2024-02-23T09:20:46Z

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

blueorangutan · 2024-02-23T12:48:44Z

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

DaanHoogland · 2024-02-23T12:57:30Z

@gpordeus , this sounds like a good use case for an integration test. Will you consider that?

gp-santos · 2024-02-23T13:51:57Z

@gpordeus , this sounds like a good use case for an integration test. Will you consider that?

Sure, on it.

weizhouapache · 2024-02-23T21:48:07Z

@blueorangutan package

blueorangutan · 2024-02-23T21:50:04Z

@weizhouapache 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 · 2024-02-23T21:59:23Z

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

blueorangutan · 2024-02-26T11:13:10Z

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

blueorangutan · 2024-02-26T14:15:08Z

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

weizhouapache · 2024-03-07T10:01:01Z

@blueorangutan package

blueorangutan · 2024-03-07T10:02:03Z

@weizhouapache 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 · 2024-03-07T11:10:55Z

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

DaanHoogland · 2024-03-11T15:08:36Z

@blueorangutan test alma9 kvm-alma9

blueorangutan · 2024-03-11T15:10:03Z

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

blueorangutan · 2024-03-11T15:14:52Z

[SF] Trillian Build Failed (tid-9434)

DaanHoogland · 2024-06-07T08:45:08Z

not sure why the bot removed the conflict sticker, but you still have some @gpordeus

…on-destroyvm

gp-santos · 2024-06-07T17:21:00Z

@DaanHoogland Fixed, thanks for letting me know.

BryanMLima · 2024-06-12T19:16:06Z

@weizhouapache, are your concerns met?

@DaanHoogland, could you trigger the CI one last time?

BryanMLima · 2024-06-12T19:19:48Z

@DaanHoogland, my bad, I did not see the comment at #8878 (comment), you can ignore my other comments.

JoaoJandre · 2024-07-24T12:26:32Z

@blueorangutan package

blueorangutan · 2024-07-24T12:28:06Z

@JoaoJandre 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 · 2024-07-24T15:37:07Z

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

JoaoJandre · 2024-08-13T19:45:30Z

@blueorangutan package

blueorangutan · 2024-08-13T19:46:04Z

@JoaoJandre 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 · 2024-08-13T20:45:20Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10642

blueorangutan · 2024-08-14T14:31:59Z

Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 10661

blueorangutan · 2024-08-16T07:19:29Z

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

DaanHoogland · 2024-08-16T09:30:43Z

@blueorangutan LLtest

blueorangutan · 2024-08-16T09:32:03Z

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

blueorangutan · 2024-08-16T09:34:53Z

[LL] Trillian Build Failed (tid-6968)

DaanHoogland · 2024-08-16T09:54:40Z

@blueorangutan test

blueorangutan · 2024-08-16T09:56:03Z

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

blueorangutan · 2024-08-17T01:08:43Z

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

Test	Result	Time (s)	Test File
test_DeployVmAffinityGroup	`Error`	1.45	test_affinity_groups.py
test_01_non_strict_host_anti_affinity	`Error`	2.63	test_nonstrict_affinity_group.py
test_02_non_strict_host_affinity	`Error`	1.58	test_nonstrict_affinity_group.py

DaanHoogland · 2024-08-20T09:27:56Z

@weizhouapache are you alright with this one, now?

weizhouapache · 2024-08-20T09:44:55Z

@DaanHoogland
code lgtm

not tested

DaanHoogland · 2024-08-20T10:01:45Z

@DaanHoogland code lgtm

not tested

good, @lucas-a-martins has: #8689 (review)

…n role rule does not allow (apache#8689)

gp-santos added 5 commits February 20, 2024 09:55

Add expungeVirtualMachine restriction to expunge parameter on destroy…

f2f8ac4

…VirtualMachine.

fix tests

1b9d35d

fix checkstyle and logs

6ae0d7c

unneeded exception

6eadb19

import adjust

c53258d

boring-cyborg Bot added component:networking component:UI labels Feb 21, 2024

weizhouapache reviewed Feb 21, 2024

View reviewed changes

Comment thread server/src/main/java/com/cloud/vm/UserVmManagerImpl.java Outdated

swap entityownerid for callingaccountid

716c9f9

DaanHoogland approved these changes Feb 23, 2024

View reviewed changes

integration test

a171365

boring-cyborg Bot added the component:integration-test label Mar 12, 2024

DaanHoogland added the status:has-conflicts label Jun 7, 2024

Merge branch 'main' into main-add-api-rolecheck-to-expunge-parameter-…

4dde768

…on-destroyvm

github-actions Bot removed the status:has-conflicts label Jun 7, 2024

gp-santos closed this Jun 10, 2024

gp-santos reopened this Jun 10, 2024

oops, fix missing closing }

bfbb0a5

DaanHoogland merged commit f84e043 into apache:main Aug 20, 2024

dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Aug 22, 2024

Fix being able to expunge a VM through destroyVirtualMachine even whe…

8945867

…n role rule does not allow (apache#8689)

DaanHoogland added this to the 4.20.0.0 milestone Sep 6, 2024

Conversation

gp-santos commented Feb 21, 2024

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

Uh oh!

codecov Bot commented Feb 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Feb 23, 2024

Uh oh!

blueorangutan commented Feb 23, 2024

Uh oh!

blueorangutan commented Feb 23, 2024

Uh oh!

blueorangutan commented Feb 23, 2024

Uh oh!

DaanHoogland commented Feb 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gp-santos commented Feb 23, 2024

Uh oh!

weizhouapache commented Feb 23, 2024

Uh oh!

blueorangutan commented Feb 23, 2024

Uh oh!

blueorangutan commented Feb 23, 2024

Uh oh!

blueorangutan commented Feb 26, 2024

Uh oh!

blueorangutan commented Feb 26, 2024

Uh oh!

weizhouapache commented Mar 7, 2024

Uh oh!

blueorangutan commented Mar 7, 2024

Uh oh!

blueorangutan commented Mar 7, 2024

Uh oh!

DaanHoogland commented Mar 11, 2024

Uh oh!

blueorangutan commented Mar 11, 2024

Uh oh!

blueorangutan commented Mar 11, 2024

Uh oh!

DaanHoogland commented Jun 7, 2024

Uh oh!

gp-santos commented Jun 7, 2024

Uh oh!

BryanMLima commented Jun 12, 2024

Uh oh!

BryanMLima commented Jun 12, 2024

Uh oh!

JoaoJandre commented Jul 24, 2024

Uh oh!

blueorangutan commented Jul 24, 2024

Uh oh!

blueorangutan commented Jul 24, 2024

Uh oh!

JoaoJandre commented Aug 13, 2024

Uh oh!

blueorangutan commented Aug 13, 2024

Uh oh!

blueorangutan commented Aug 13, 2024

Uh oh!

blueorangutan commented Aug 14, 2024

Uh oh!

blueorangutan commented Aug 16, 2024

Uh oh!

DaanHoogland commented Aug 16, 2024

Uh oh!

blueorangutan commented Aug 16, 2024

Uh oh!

codecov Bot commented Feb 21, 2024 •

edited

Loading

DaanHoogland commented Feb 23, 2024 •

edited

Loading