Skip to content
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

Fix cloudstack-ui package: bad directory permissions and missing WEB-INF #8568

Merged
merged 5 commits into from
Feb 8, 2024

Conversation

kohrar
Copy link
Contributor

@kohrar kohrar commented Jan 26, 2024

Description

The cloudstack-ui package contains a copy of the webapp. However, the package (for both CentOS 7 and CentOS 8) have two issues:

  1. Bad permissions on directories. The 'cloud' user cannot read and serve assets from subdirectories
  2. WEB-INF/web.xml is missing. This breaks the API as the web routes are not defined.

Fixes #8558

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

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

Rebuilt the RPM packages under Rocky Linux 8 / CentOS 8 using Docker.

New cloudstack-ui packages now have the proper permissions:

# rpm2cpio  ../cloudstack-ui-4.18.2.0-SNAPSHOT.x86_64.rpm | cpio -idm                           
54793 blocks         
                                                                                       
# cd usr/share/cloudstack-ui/                                                                   

# ls -al
total 524                                                                                                   
drwxr-xr-x 7 root root    181 Jan 25 18:00 .                                                                
drwxr-xr-x 4 root root     38 Jan 25 18:00 ..                                                               
drwxr-xr-x 2 root root    175 Jan 25 18:00 assets                                                           
-rw-r--r-- 1 root root  14318 Jan 25 17:57 cloud.ico                                                        
-rw-r--r-- 1 root root 456667 Jan 25 17:57 color.less                                                       
lrwxrwxrwx 1 root root     30 Jan 25 17:57 config.json -> /etc/cloudstack/ui/config.json                    
drwxr-xr-x 2 root root   8192 Jan 25 18:00 css                                                              
-rw-r--r-- 1 root root   1228 Jan 25 17:57 error.html                                                       
-rw-r--r-- 1 root root   1220 Jan 25 17:57 example.html                                                     
-rw-r--r-- 1 root root  18123 Jan 25 17:57 index.html                                                       
drwxr-xr-x 2 root root  16384 Jan 25 18:00 js                                                               
drwxr-xr-x 2 root root    309 Jan 25 18:00 locales                                                          
drwxr-xr-x 2 root root     21 Jan 25 18:00 WEB-INF                                                          

How did you try to break this feature and the system with this change?

Affects only rpmbuild for the cloudstack-ui package. Extracted RPM package to verify change.

Updated the spec file such that directories are chmod 0755 rather than 0644 which would prevent the cloud user from reading their contents.
The cloudstack-ui package should have the same files as the bundled webapp in the management package.
Add the missing WEB-INF directory and do not set directories to 0755.
@kohrar kohrar marked this pull request as ready for review January 26, 2024 01:03
Copy link

codecov bot commented Jan 26, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (33bb92a) 13.16% compared to head (9d07d3a) 13.16%.

Additional details and impacted files
@@            Coverage Diff            @@
##               4.18    #8568   +/-   ##
=========================================
  Coverage     13.16%   13.16%           
- Complexity     9201     9203    +2     
=========================================
  Files          2724     2724           
  Lines        258077   258077           
  Branches      40224    40224           
=========================================
+ Hits          33981    33986    +5     
+ Misses       219790   219784    -6     
- Partials       4306     4307    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@DaanHoogland DaanHoogland left a comment

Choose a reason for hiding this comment

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

clgtm, it seems that the ui can now be deployed as any user (something outside the diff may force user 'cloud', not sure). is that right @kohrar ?

@DaanHoogland DaanHoogland linked an issue Jan 26, 2024 that may be closed by this pull request
@kohrar
Copy link
Contributor Author

kohrar commented Jan 26, 2024

clgtm, it seems that the ui can now be deployed as any user (something outside the diff may force user 'cloud', not sure). is that right @kohrar ?

Out of the box, CloudStack serves the UI (via the cloudstack-management service) as the 'cloud' user, as defined by the cloudstack-management service file. An operator could in theory change the daemon user to something other than cloud. I don't see anything in the CloudStack documentation about changing this.

Regardless of what user cloudstack-management runs as (cloud or otherwise), this PR will fix the case where the UI and API breaks if a CloudStack operator tries to install the cloudstack-ui package and configures the CloudStack management service to use that webapp directory with webapp.dir=/usr/share/cloudstack-ui (in /etc/cloudstack/management/server.properties).

@rohityadavcloud
Copy link
Member

@kohrar the cloudstack-ui package is for advanced user wherein we assume it's not being served from the cloudstack management server host. The cloudstack-management already bundles the UI.
cloudstack-ui can be used for adv customisation/multi-mgmt server serving (http://docs.cloudstack.apache.org/en/4.18.1.0/adminguide/ui.html#advanced-ui-customisation) or even nginx-reverse proxy use-cases https://github.com/apache/cloudstack/tree/main/ui#production

Copy link
Member

@rohityadavcloud rohityadavcloud left a comment

Choose a reason for hiding this comment

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

LGTM - I've added my comments, however, we don't assume for using cloudstack-ui pkg that the cloud user must exists or have access/ACLs according to it.

@DaanHoogland
Copy link
Contributor

@blueorangutan ui

@blueorangutan
Copy link

@DaanHoogland a Jenkins job has been kicked to build UI QA env. I'll keep you posted as I make progress.

@blueorangutan
Copy link

UI build: ✔️
Live QA URL: https://qa.cloudstack.cloud/simulator/pr/8568 (QA-JID-271)

@DaanHoogland
Copy link
Contributor

it seems to have installed correctly in qa, so I think this is ready for merge. Not sure if I miss some corner case there that should be covered as well.

@rohityadavcloud rohityadavcloud added this to the 4.18.2.0 milestone Feb 5, 2024
@rohityadavcloud
Copy link
Member

@blueorangutan package

@blueorangutan
Copy link

@rohityadavcloud 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
Copy link

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

@rohityadavcloud rohityadavcloud merged commit 3fa052c into apache:4.18 Feb 8, 2024
25 of 26 checks passed
rohityadavcloud pushed a commit that referenced this pull request Feb 8, 2024
…INF (#8568)

* Fix bad perms on sub directories for webapp asset files

Updated the spec file such that directories are chmod 0755 rather than 0644 which would prevent the cloud user from reading their contents.

* Fix bad permissions for centos8 UI files, missing WEB-INF

The cloudstack-ui package should have the same files as the bundled webapp in the management package.

* Fix bad perms and missing WEB-INF for centos7 ui rpm

Add the missing WEB-INF directory and do not set directories to 0755.

* Fix missing WEB-INF on CentOS 8 cloudstack-ui rpm

* Fix missing WEB-INF on CentOS 7 cloudstack-ui rpm
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Feb 20, 2024
…INF (apache#8568)

* Fix bad perms on sub directories for webapp asset files

Updated the spec file such that directories are chmod 0755 rather than 0644 which would prevent the cloud user from reading their contents.

* Fix bad permissions for centos8 UI files, missing WEB-INF

The cloudstack-ui package should have the same files as the bundled webapp in the management package.

* Fix bad perms and missing WEB-INF for centos7 ui rpm

Add the missing WEB-INF directory and do not set directories to 0755.

* Fix missing WEB-INF on CentOS 8 cloudstack-ui rpm

* Fix missing WEB-INF on CentOS 7 cloudstack-ui rpm
dhslove pushed a commit to ablecloud-team/ablestack-cloud that referenced this pull request Feb 20, 2024
…INF (apache#8568)

* Fix bad perms on sub directories for webapp asset files

Updated the spec file such that directories are chmod 0755 rather than 0644 which would prevent the cloud user from reading their contents.

* Fix bad permissions for centos8 UI files, missing WEB-INF

The cloudstack-ui package should have the same files as the bundled webapp in the management package.

* Fix bad perms and missing WEB-INF for centos7 ui rpm

Add the missing WEB-INF directory and do not set directories to 0755.

* Fix missing WEB-INF on CentOS 8 cloudstack-ui rpm

* Fix missing WEB-INF on CentOS 7 cloudstack-ui rpm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cloudstack-ui RPM package has bad permissions
4 participants