-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fixed User type accounts being able to change resource limits of their own domain and account #12046
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
base: main
Are you sure you want to change the base?
Fixed User type accounts being able to change resource limits of their own domain and account #12046
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #12046 +/- ##
============================================
+ Coverage 17.55% 17.76% +0.21%
- Complexity 15538 15859 +321
============================================
Files 5910 5923 +13
Lines 529334 530541 +1207
Branches 64654 64827 +173
============================================
+ Hits 92905 94248 +1343
+ Misses 425973 425749 -224
- Partials 10456 10544 +88
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:
|
cedc91c to
123a0a8
Compare
lucas-a-martins
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.
Maybe I'm missing something, but it seems that your test description explains the problem itself rather than how you tested the solution. Other than that, CLGTM.
server/src/main/java/com/cloud/resourcelimit/ResourceLimitManagerImpl.java
Outdated
Show resolved
Hide resolved
|
@blueorangutan package |
|
@bernardodemarco a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
bernardodemarco
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, I manually tested it in a local environment.
- Created an
Useraccount with a custom role with access to theupdateResourceLimitAPI - Tried to execute the API with the
Useraccount and verified that an error message was returned:
(u1) 🐱 > update resourcelimit resourcetype=1 max=10
🙈 Error: (HTTP 531, error code 4365) Your account does not have the right access level to update resource limits.
(u1) 🐱 > update resourcelimit resourcetype=1 max=-1
🙈 Error: (HTTP 531, error code 4365) Your account does not have the right access level to update resource limits.Co-authored-by: Lucas Martins <56271185+lucas-a-martins@users.noreply.github.com>
|
Hello, @bernardodemarco and @lucas-a-martins Thanks for the reviews, and @lucas-a-martins, thanks for the suggestion. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 16304 |
|
@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-15150)
|
Description
The
updateResourceLimitAPI, by default, is allowed only to the Root Admin, Domain Admin, and Resource Admin role types. These role types cannot change their account and domain limits using the API. However, if an account is created with a User type role, and the permission to this API is granted, they will be capable of changing their own account and domain resource limits and bypassing what has been allocated to them. In this scenario, the user could also cause a DoS by allocating all the resources (for instance, all the IP addresses).This PR fixes this by implementing an account type validation that throws an exception if the caller's account is of type
User.Types of changes
Feature/Enhancement Scale or Bug Severity
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
In my local environment, a
Modified Userrole was created, based on the defaultUserrole. Then, the permission to call theupdateResourceLimitAPI was granted to it. A new account, nameduser, was created using theModified Userrole. By default, this account had a limit of20Public IP to allocate.I authenticated using the
useraccount credentials in CMK, and executed this command, to unlimit the number of Public IP the user could allocate:With that, the limitation was removed, and the
useraccount can now exhaust the available public IPs in the environment. This also applies for the other limit types.Besides changing the account limit, I could also change its domain limits by running the following command:
The user with these permission cannot change the limits of other accounts within its domain or other domains' limits.
After testing that, I applied the updated packages to my local environment and executed the same commands, which resulted in errors as expected.