You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "hsato03 (via GitHub)" <gi...@apache.org> on 2023/08/10 19:42:23 UTC
[GitHub] [cloudstack] hsato03 opened a new pull request, #7853: Fix role escalation prevention
hsato03 opened a new pull request, #7853:
URL: https://github.com/apache/cloudstack/pull/7853
### Description
While checking an account access to prevent the role escalation, the `checkAccess` method is using both `DynamicRoleBasedAPIAccessChecker` and `StaticRoleBasedAPIAccessChecker` (legacy) classes and that is causing some inconsistences in the validations, consequently allowing the role escalation.
For this reason, the `StaticRoleBasedAPIAccessChecker` was disabled if `DynamicRoleBasedAPIAccessChecker` is enabled.
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] 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)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [X] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
- I have created a domain admin custom role (DA2) and allowed all API's to it using the `*` regex;
- Removed an API that it had in common with default domain admin role;
- Tried to create a default domain admin account using a DA2 account;
- Received an exception from the role eslacation prevention.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on a diff in pull request #7853: Fix role escalation prevention
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on code in PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#discussion_r1294304022
##########
plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java:
##########
@@ -107,6 +107,10 @@ public boolean checkAccess(User user, String commandName) throws PermissionDenie
@Override
public boolean checkAccess(Account account, String commandName) {
+ if (isEnabled()) {
+ return true;
+ }
Review Comment:
yes, it looks like a bug @DaanHoogland
we should change the return value of `isEnabled` in this class
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] codecov[bot] commented on pull request #7853: Fix role escalation prevention
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1676835896
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/7853?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#7853](https://app.codecov.io/gh/apache/cloudstack/pull/7853?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (9d395b0) into [4.18](https://app.codecov.io/gh/apache/cloudstack/commit/b4032d9984e5d2640a231edeb0363c491cddc2f9?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (b4032d9) will **decrease** coverage by `0.01%`.
> Report is 15 commits behind head on 4.18.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## 4.18 #7853 +/- ##
============================================
- Coverage 13.02% 13.02% -0.01%
- Complexity 9029 9032 +3
============================================
Files 2720 2720
Lines 257010 257082 +72
Branches 40083 40088 +5
============================================
+ Hits 33467 33476 +9
- Misses 219342 219402 +60
- Partials 4201 4204 +3
```
| [Files Changed](https://app.codecov.io/gh/apache/cloudstack/pull/7853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...loudstack/acl/StaticRoleBasedAPIAccessChecker.java](https://app.codecov.io/gh/apache/cloudstack/pull/7853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-cGx1Z2lucy9hY2wvc3RhdGljLXJvbGUtYmFzZWQvc3JjL21haW4vamF2YS9vcmcvYXBhY2hlL2Nsb3Vkc3RhY2svYWNsL1N0YXRpY1JvbGVCYXNlZEFQSUFjY2Vzc0NoZWNrZXIuamF2YQ==) | `0.00% <0.00%> (ø)` | |
... and [15 files with indirect coverage changes](https://app.codecov.io/gh/apache/cloudstack/pull/7853/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7853: Fix role escalation prevention
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#discussion_r1293352562
##########
plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java:
##########
@@ -107,6 +107,10 @@ public boolean checkAccess(User user, String commandName) throws PermissionDenie
@Override
public boolean checkAccess(Account account, String commandName) {
+ if (isEnabled()) {
+ return true;
+ }
Review Comment:
if this prevents the escalation talked about fine, but it seems illogical to return true to prevent access. would return false not make more sense?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1680066334
@weizhouapache a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1678883731
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6785
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681243282
<b>[SF] Trillian test result (tid-7434)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 47901 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7853-t7434-kvm-centos7.zip
Smoke tests completed. 107 look OK, 1 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
test_list_vms_metrics_admin | `Error` | 3608.79 | test_metrics_api.py
test_list_vms_metrics_history | `Error` | 5.39 | test_metrics_api.py
test_list_volumes_metrics_history | `Error` | 7.45 | test_metrics_api.py
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] hsato03 commented on pull request #7853: Fix role escalation prevention
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1675170869
Hello @rohityadavcloud. You can test it by doing the same tests I did. Before the changes, the account will be created and no exception will be thrown.
- Create a domain admin custom role (DA2) and allow all APIs to it using the `*` regex;
- Still in DA2, remove an API that it has in common with the default domain admin role (`listProjects` for an example);
- Login into a DA2 account and try to create an account that has the default domain admin role;
- The account should not be created as the caller role (DA2) does not have a permission (`listProjects`) that the role's account being created has (default domain admin).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681116420
@weizhouapache a [SF] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] kiranchavala commented on pull request #7853: Fix role escalation prevention
Posted by "kiranchavala (via GitHub)" <gi...@apache.org>.
kiranchavala commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1676678242
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #7853: Fix role escalation prevention
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681935852
@DaanHoogland @rohityadavcloud
@hsato03 has changed the return value of `isEnabled` method in the class, the code lgtm and trillian test results look good. do you have any other concerns ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1678809909
@weizhouapache a [SF] 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681179177
Packaging result [SF]: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: el9 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 6810
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #7853: Fix role escalation prevention
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1678808211
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] hsato03 commented on a diff in pull request #7853: Fix role escalation prevention
Posted by "hsato03 (via GitHub)" <gi...@apache.org>.
hsato03 commented on code in PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#discussion_r1293951567
##########
plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java:
##########
@@ -107,6 +107,10 @@ public boolean checkAccess(User user, String commandName) throws PermissionDenie
@Override
public boolean checkAccess(Account account, String commandName) {
+ if (isEnabled()) {
+ return true;
+ }
Review Comment:
@DaanHoogland I don't think so. Actually this `isEnabled` method checks if `DynamicRoleBasedAPIAccessChecker` is enabled. Returning true here is just passing the check responsibility to another class.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] rohityadavcloud commented on pull request #7853: Fix role escalation prevention
Posted by "rohityadavcloud (via GitHub)" <gi...@apache.org>.
rohityadavcloud commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1674317701
Hi @hsato03 thanks for the PR. The dynamic checker is used by default and we don't ship commands.properties with cloudstack for several years now, how can we test/verify your claims?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7853: Fix role escalation prevention
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#discussion_r1294253886
##########
plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java:
##########
@@ -107,6 +107,10 @@ public boolean checkAccess(User user, String commandName) throws PermissionDenie
@Override
public boolean checkAccess(Account account, String commandName) {
+ if (isEnabled()) {
+ return true;
+ }
Review Comment:
yes @hsato03 I think you are right. I consider it a bug that the StaticRoleBasedAPIAccessChecker.isEnabled() would return true if it is disabled (but the dynamic one is enabled). Maybe this is a discussion out of scope for your change. As said, "if this prevents the escalation talked about (it is) fine"
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #7853: Fix role escalation prevention
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681115643
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland merged pull request #7853: Fix role escalation prevention
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #7853: Fix role escalation prevention
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681206322
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] boring-cyborg[bot] commented on pull request #7853: Fix role escalation prevention
Posted by "boring-cyborg[bot] (via GitHub)" <gi...@apache.org>.
boring-cyborg[bot] commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1683490769
Awesome work, congrats on your first merged pull request!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] DaanHoogland commented on a diff in pull request #7853: Fix role escalation prevention
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#discussion_r1294309746
##########
plugins/acl/static-role-based/src/main/java/org/apache/cloudstack/acl/StaticRoleBasedAPIAccessChecker.java:
##########
@@ -107,6 +107,10 @@ public boolean checkAccess(User user, String commandName) throws PermissionDenie
@Override
public boolean checkAccess(Account account, String commandName) {
+ if (isEnabled()) {
+ return true;
+ }
Review Comment:
cc @rohityadavcloud ^
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] weizhouapache commented on pull request #7853: Fix role escalation prevention
Posted by "weizhouapache (via GitHub)" <gi...@apache.org>.
weizhouapache commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1680066014
@blueorangutan test
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681207467
@weizhouapache a [SF] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cloudstack] blueorangutan commented on pull request #7853: Fix role escalation prevention
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #7853:
URL: https://github.com/apache/cloudstack/pull/7853#issuecomment-1681779467
<b>[SF] Trillian test result (tid-7451)</b>
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 39277 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr7853-t7451-kvm-centos7.zip
Smoke tests completed. 108 look OK, 0 have errors, 0 did not run
Only failed and skipped tests results shown below:
Test | Result | Time (s) | Test File
--- | --- | --- | ---
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org