Role escalation prevention by DaanHoogland · Pull Request #5879 · apache/cloudstack

DaanHoogland · 2022-01-19T17:28:04Z

Description

This PR...

Fixes: #5781

Doc PR: apache/cloudstack-documentation#260

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)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Major
Minor

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

yadvr · 2022-01-20T08:19:00Z

Thanks @DaanHoogland, I'll review and get back in a few days. Meanwhile, do add/ask other devs.

sureshanaparti

code LGTM

blueorangutan · 2022-01-24T06:29:03Z

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

Pearl1594 · 2022-01-24T06:38:05Z

Most / all test failures report the following error: Execute cmd: createaccount failed, due to: errorCode: 531, errorText:user of account admin/1 (5af79319-7414-4df3-bffb-ec4ef9728066) can not create an account with access to notifyBaremetalProvisionDone\n'] and the notifyBaremetalProvisionDone is only allowed for an account having User role.

DaanHoogland · 2022-01-25T10:47:14Z

Most / all test failures report the following error: Execute cmd: createaccount failed, due to: errorCode: 531, errorText:user of account admin/1 (5af79319-7414-4df3-bffb-ec4ef9728066) can not create an account with access to notifyBaremetalProvisionDone\n'] and the notifyBaremetalProvisionDone is only allowed for an account having User role.

ok, that is not good: description = "Notify provision has been done on a host. This api is for baremetal virtual router service, not for end user", it is hard-coded to be only allowed for the role user but is not allowed for normal users. Also I tested creating accounts as domain admin. I wonder why I didn't notice this one!?

I removed the restriction to RoleTupe.User, which should do the trick. If not I'll add all role types.

blueorangutan · 2022-02-07T11:04:03Z

@sureshanaparti a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-07T11:40:15Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2495

sureshanaparti · 2022-02-07T11:43:33Z

ping @rohityadavcloud can you test this changes.

yadvr · 2022-02-08T09:16:08Z

@sureshanaparti I'll review the code changes but not able to test it. Pl ask @borisstoyanov @vladimirpetrov or one of the 4.16.1 team.

sureshanaparti · 2022-02-08T11:30:38Z

@sureshanaparti I'll review the code changes but not able to test it. Pl ask @borisstoyanov @vladimirpetrov or one of the 4.16.1 team.

sure @rohityadavcloud , please review the changes. thanks.

yadvr · 2022-02-08T17:47:46Z

@blueorangutan package

blueorangutan · 2022-02-08T17:48:03Z

@rohityadavcloud a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2022-02-08T18:25:07Z

Packaging result: ✔️ el7 ✔️ el8 ✔️ debian ✔️ suse15. SL-JID 2532

yadvr · 2022-02-08T18:45:16Z

@blueorangutan test

blueorangutan · 2022-02-08T18:46:03Z

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

blueorangutan · 2022-02-09T02:07:51Z

Trillian test result (tid-3252)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 25131 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5879-t3252-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_primary_storage.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py

borisstoyanov

LGTM, manually tested, restricted domain admin cannot create a role with higher privileges

sureshanaparti · 2022-02-09T11:50:51Z

@blueorangutan test

blueorangutan · 2022-02-09T11:52:03Z

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

DaanHoogland · 2022-02-09T16:20:40Z

Do we need this re-test

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

I think the one before showed no errors and we didn't re-build in between.

sureshanaparti · 2022-02-09T16:27:24Z

Do we need this re-test
@sureshanaparti a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests
I think the one before showed no errors and we didn't re-build in between.

couldn't see the results here, let's wait for test results.
#5879 (comment)

blueorangutan · 2022-02-09T20:41:34Z

Trillian test result (tid-3264)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 30453 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr5879-t3264-kvm-centos7.zip
Smoke tests completed. 92 look OK, 0 have errors
Only failed tests results shown below:

Test	Result	Time (s)	Test File

Pearl1594

LGTM - verified behavior.

yadvr

FWIW - LGTM, read the critical code in checkRoleEscalation, will leave some remarks

yadvr · 2022-02-10T07:26:22Z

@DaanHoogland one remark this doesn't fix/address if the issue already exists, we should update the doc (if not already done) to explain how roles work and precautions for non-admin roles that can create accounts.

DaanHoogland · 2022-02-10T07:54:00Z

@DaanHoogland one remark this doesn't fix/address if the issue already exists, we should update the doc (if not already done) to explain how roles work and precautions for non-admin roles that can create accounts.

I don't agree, there is a doc PR out. Please comment there if there is anything missing.

sureshanaparti · 2022-02-10T08:01:55Z

@DaanHoogland one remark this doesn't fix/address if the issue already exists, we should update the doc (if not already done) to explain how roles work and precautions for non-admin roles that can create accounts.
I don't agree, there is a doc PR out. Please comment there if there is anything missing.

@rohityadavcloud Doc PR here, apache/cloudstack-documentation#260, please take a look and provide your feedback. @DaanHoogland can create a separate doc PR to include if any more details has to be added to the documentation.

yadvr · 2022-02-10T08:14:15Z

Cool thanks for the link @sureshanaparti @DaanHoogland - as I said my remark was only if you've not already done the doc PR - which you've.

* prevent role access escallation * hierarchy issue fixed * create api list in account manager for checking new account access * full api list check * strange role restriction removed for BareMetal * add role check on upfdate account as well * more selective use of api checkers * error msg and var name Co-authored-by: Daan Hoogland <dahn@onecht.net>

prevent role access escallation
cca1f3a

DaanHoogland added this to the 4.16.1.0 milestone Jan 19, 2022

DaanHoogland self-assigned this Jan 19, 2022

DaanHoogland marked this pull request as draft January 19, 2022 17:30

DaanHoogland requested a review from yadvr January 19, 2022 17:32

hierarchy issue fixed
fb37d92

sureshanaparti reviewed Jan 20, 2022
View reviewed changes

Comment threadserver/src/main/java/com/cloud/user/AccountManagerImpl.java Outdated

sureshanaparti approved these changes Jan 20, 2022
View reviewed changes

sureshanaparti requested a review from Pearl1594 January 20, 2022 08:26

Pearl1594 reviewed Jan 20, 2022
View reviewed changes

Comment threadserver/src/main/java/com/cloud/user/AccountManagerImpl.java Outdated

yadvr added the Severity:Critical Critical bug label Jan 20, 2022

Daan Hoogland added 2 commits January 20, 2022 16:32

create api list in account manager for checking new account access
7cab7b8

full api list check
777d6bf

apache deleted a comment from blueorangutan Jan 21, 2022

apache deleted a comment from sureshanaparti Jan 21, 2022

apache deleted a comment from blueorangutan Jan 21, 2022