Skip to content

Role escalation prevention#5879

Merged
sureshanaparti merged 11 commits intoapache:4.16from
shapeblue:roleEscalation
Feb 10, 2022
Merged

Role escalation prevention#5879
sureshanaparti merged 11 commits intoapache:4.16from
shapeblue:roleEscalation

Conversation

@DaanHoogland
Copy link
Copy Markdown
Contributor

@DaanHoogland DaanHoogland commented Jan 19, 2022 •
edited by sureshanaparti
Loading

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

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

@DaanHoogland DaanHoogland added this to the 4.16.1.0 milestone Jan 19, 2022
@DaanHoogland DaanHoogland self-assigned this Jan 19, 2022
@DaanHoogland DaanHoogland marked this pull request as draft January 19, 2022 17:30
@DaanHoogland DaanHoogland requested a review from yadvr January 19, 2022 17:32
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Jan 20, 2022

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

sureshanaparti
sureshanaparti reviewed Jan 20, 2022
View reviewed changes
sureshanaparti
sureshanaparti approved these changes Jan 20, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

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

Pearl1594
Pearl1594 reviewed Jan 20, 2022
View reviewed changes
@yadvr yadvr added the Severity:Critical Critical bug label Jan 20, 2022
@apache apache deleted a comment from blueorangutan Jan 21, 2022
@apache apache deleted a comment from sureshanaparti Jan 21, 2022
@apache apache deleted a comment from blueorangutan Jan 21, 2022
@sureshanaparti sureshanaparti linked an issue Jan 21, 2022 that may be closed by this pull request
Dynamic Role Model partly allows users to create users with more rights #5781
Closed
@apache apache deleted a comment from blueorangutan Jan 23, 2022
@apache apache deleted a comment from blueorangutan Jan 23, 2022
@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Jan 24, 2022

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

@Pearl1594
Copy link
Copy Markdown
Contributor

Pearl1594 commented Jan 24, 2022

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
Copy link
Copy Markdown
Contributor Author

DaanHoogland commented Jan 25, 2022

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.

@apache apache deleted a comment from blueorangutan Jan 25, 2022
@sureshanaparti
Copy link
Copy Markdown
Contributor

sureshanaparti commented Feb 7, 2022

ping @rohityadavcloud can you test this changes.

@apache apache deleted a comment from blueorangutan Feb 7, 2022
@yadvr
Copy link
Copy Markdown
Member

yadvr commented Feb 8, 2022

@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
Copy link
Copy Markdown
Contributor

sureshanaparti commented Feb 8, 2022

@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
Copy link
Copy Markdown
Member

yadvr commented Feb 8, 2022

@blueorangutan package

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Feb 8, 2022

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

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Feb 8, 2022

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

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Feb 8, 2022

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Feb 8, 2022

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

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Feb 9, 2022

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
borisstoyanov approved these changes Feb 9, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@borisstoyanov borisstoyanov left a comment

Choose a reason for hiding this comment

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

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

@borisstoyanov borisstoyanov removed their assignment Feb 9, 2022
@sureshanaparti
Copy link
Copy Markdown
Contributor

sureshanaparti commented Feb 9, 2022

@blueorangutan test

@blueorangutan
Copy link
Copy Markdown

blueorangutan commented Feb 9, 2022

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

@DaanHoogland
Copy link
Copy Markdown
Contributor Author

DaanHoogland commented Feb 9, 2022

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
Copy link
Copy Markdown
Contributor

sureshanaparti commented Feb 9, 2022

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
Copy link
Copy Markdown

blueorangutan commented Feb 9, 2022

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
Pearl1594 approved these changes Feb 10, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@Pearl1594 Pearl1594 left a comment

Choose a reason for hiding this comment

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

LGTM - verified behavior.

@sureshanaparti sureshanaparti merged commit a6d9fa6 into apache:4.16 Feb 10, 2022
@sureshanaparti sureshanaparti mentioned this pull request Feb 10, 2022
Closed
yadvr
yadvr reviewed Feb 10, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@yadvr yadvr left a comment

Choose a reason for hiding this comment

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

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

@yadvr
Copy link
Copy Markdown
Member

yadvr commented Feb 10, 2022

@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
Copy link
Copy Markdown
Contributor Author

DaanHoogland commented Feb 10, 2022

@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
Copy link
Copy Markdown
Contributor

sureshanaparti commented Feb 10, 2022

@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
Copy link
Copy Markdown
Member

yadvr commented Feb 10, 2022

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.

Pearl1594 pushed a commit to shapeblue/cloudstack that referenced this pull request Feb 14, 2022
@DaanHoogland @Pearl1594
Role escalation prevention (apache#5879)
3450d45 
* 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>
RodrigoDLopez added a commit to RodrigoDLopez/cloudstack that referenced this pull request Aug 23, 2022
@RodrigoDLopez @SadiJr
Role escalation prevention (apache#5879)
d31fc62
RodrigoDLopez pushed a commit to RodrigoDLopez/cloudstack that referenced this pull request Aug 23, 2022
@SadiJr
Merge branch 'cherry-pick-pr-5879' into '4.16.0.0-scclouds'
1e12025 
Role escalation prevention (apache#5879)

See merge request scclouds/scclouds!216
@weizhouapache weizhouapache mentioned this pull request Jul 3, 2025
Closed
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yadvr yadvr yadvr left review comments

@sureshanaparti sureshanaparti sureshanaparti approved these changes

@Pearl1594 Pearl1594 Pearl1594 approved these changes

@borisstoyanov borisstoyanov borisstoyanov approved these changes

@nvazquez nvazquez Awaiting requested review from nvazquez

Assignees

No one assigned

Labels

Severity:Critical Critical bug

Projects

No open projects
Apache CloudStack 4.16.1
Status: Done

Milestone

4.16.1.0

Development

Successfully merging this pull request may close these issues.

Dynamic Role Model partly allows users to create users with more rights

7 participants

@DaanHoogland @yadvr @blueorangutan @Pearl1594 @sureshanaparti @nvazquez @borisstoyanov