[CLOUDSTACK-10230] User should not be able to use removed “Guest OS type” by rafaelweingartner · Pull Request #2404 · apache/cloudstack

rafaelweingartner · 2018-01-12T16:27:59Z

Users are able to change the OS type of VMs to a “Guest OS type” that has been removed. This becomes a security issue when we try to force users to use HVM VMs (Meltdown/Spectre thing). A removed “guest os type” should not be usable by any users in the cloud.

rafaelweingartner · 2018-03-19T12:26:35Z

@blueorangutan package

blueorangutan · 2018-03-19T12:26:48Z

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

blueorangutan · 2018-03-19T13:44:37Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1791

rafaelweingartner · 2018-03-20T16:06:34Z

@blueorangutan package

blueorangutan · 2018-03-20T16:06:59Z

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

blueorangutan · 2018-03-20T17:43:42Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1798

… been removed Users are able to change the OS type of VMs to “Guest OS type” that has been removed. This becomes a security issue when we try to force users to use HVM VMs (Meltdown/Spectre thing). A removed “guest os type” should not be usable by any users in the cloud.

rafaelweingartner · 2018-03-30T15:26:09Z

@borisstoyanov can you run tests here?

DaanHoogland · 2018-03-31T16:14:05Z

yes, he can ;)
@blueorangutan test

DaanHoogland

looks good

DaanHoogland · 2018-03-31T16:15:42Z

 private static final Logger s_logger = Logger.getLogger(UserVmManagerImpl.class);

- private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 3; // 3 seconds
+ private static final int ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION = 3;


why not leave a // in seconds
usefull comment for a change ;)

Agreed. I probably removed without thinking.
I re-introduced this documentation.

DaanHoogland · 2018-03-31T16:20:03Z

-final Long diskOfferingId, final Long diskSize, final String userData, final Account caller, final Boolean isDisplayVm, final String keyboard,
-final long accountId, final long userId, final ServiceOfferingVO offering, final boolean isIso, final String sshPublicKey, final LinkedHashMap<String, NicProfile> networkNicMap,
-final long id, final String instanceName, final String uuidName, final HypervisorType hypervisorType, final Map<String, String> customParameters, final Map<String, Map<Integer, String>> extraDhcpOptionMap, final Map<Long, DiskOffering> dataDiskTemplateToDiskOfferingMap) throws InsufficientCapacityException {
+ final Long diskOfferingId, final Long diskSize, final String userData, final Account caller, final Boolean isDisplayVm, final String keyboard,


hm, i actually liked the alignment of the parameters. good old days ;)

methods with hundreds of parameters... the glory of the old days...

DaanHoogland · 2018-03-31T16:23:26Z

 return false;
 }
-}
+}


it is hard to find functional changes in this file. the new 2380-2424 are it, are they?

Yeas, it is.
That is why I am not doing like this anymore. I am always splitting up the PR into multiple commits. It was one of your suggestions some time ago, which I liked. However, I created this PR before that.
The changes are basically at lines 2380-2424.

borisstoyanov · 2018-04-02T07:31:23Z

@blueorangutan package

blueorangutan · 2018-04-02T07:32:15Z

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

blueorangutan · 2018-04-02T08:48:35Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-1858

borisstoyanov · 2018-04-02T08:52:09Z

@blueorangutan test

blueorangutan · 2018-04-02T08:52:15Z

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

rafaelweingartner · 2018-04-02T11:14:46Z

@DaanHoogland thanks for your time reviewing this PR. I re-introduced the documentation of ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERATION variable as you suggested.

…TION variable

DaanHoogland · 2018-04-02T12:24:46Z

+ /**
+ * The number of bytes in a GiB.
+ */
+ private static final long GiB_TO_BYTES = 1024 * 1024 * 1024;


this one is actually self documenting!

Sure, removed.

i enjoyed the renaming of it very much though ;)

come to think about it, and don't take this as criticism on your naming to much, I think constants like this one should be appended with "_FACTOR" to be completely self documenting.

no problem. I just renamed to GiB, because the previous GB prefix that was being used is not accurate.
Sure the wording "factor" might be more appropriate, but I think we should look for a different solution. We have these constant all over our code base. We should look for a "MetricUtils/UnitUtils" or something like that to convert between different units (bytes to KB, KiB, MB, MiB, GB, GiB, TB, TiB, hertz and Ghz and so on)

yes, my remark was about the self documenting quality of the constant name; it is not GiB_TO_BYTES, as it cannot push all information contained in a GiB into a small number of bytes as anyone as ignorant as myself might think. The contant is and can only be used as a factor to convert from GiB to a number of B so it should be called GiB_TO_B_FACTOR or some such name. having said that, another implementator would choose to have a MetricUtil.convertGiBtoB() method.
/me is not a bigot on that difference.

borisstoyanov · 2018-04-04T10:27:39Z

@blueorangutan package

blueorangutan · 2018-04-04T10:28:37Z

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

blueorangutan · 2018-04-04T11:16:48Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-1871

borisstoyanov · 2018-04-04T12:09:05Z

@blueorangutan test

blueorangutan · 2018-04-04T12:09:36Z

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

blueorangutan · 2018-04-05T21:07:19Z

Trillian test result (tid-2463)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 46315 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2404-t2463-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_public_ip_range.py
Intermitten failure detected: /marvin/tests/smoke/test_reset_vm_on_reboot.py
Intermitten failure detected: /marvin/tests/smoke/test_router_dhcphosts.py
Intermitten failure detected: /marvin/tests/smoke/test_router_dns.py
Intermitten failure detected: /marvin/tests/smoke/test_router_dnsservice.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_iptables_default_policy.py
Intermitten failure detected: /marvin/tests/smoke/test_routers_network_ops.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_secondary_storage.py
Intermitten failure detected: /marvin/tests/smoke/test_service_offerings.py
Intermitten failure detected: /marvin/tests/smoke/test_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_ssvm.py
Intermitten failure detected: /marvin/tests/smoke/test_templates.py
Intermitten failure detected: /marvin/tests/smoke/test_usage.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_life_cycle.py
Intermitten failure detected: /marvin/tests/smoke/test_vm_snapshots.py
Intermitten failure detected: /marvin/tests/smoke/test_volumes.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_router_nics.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Intermitten failure detected: /marvin/tests/smoke/test_host_maintenance.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 45 look OK, 22 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_03_vpc_privategw_restart_vpc_cleanup	`Failure`	187.11	test_privategw_acl.py
ContextSuite context=TestResetVmOnReboot>:setup	`Error`	0.00	test_reset_vm_on_reboot.py
ContextSuite context=TestRouterDHCPHosts>:setup	`Error`	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDHCPOpts>:setup	`Error`	0.00	test_router_dhcphosts.py
ContextSuite context=TestRouterDns>:setup	`Error`	0.00	test_router_dns.py
ContextSuite context=TestVmSnapshot>:setup	`Error`	0.07	test_vm_snapshots.py
ContextSuite context=TestRouterDnsService>:setup	`Error`	0.00	test_router_dnsservice.py
ContextSuite context=TestRouterIpTablesPolicies>:setup	`Error`	0.00	test_routers_iptables_default_policy.py
ContextSuite context=TestVPCIpTablesPolicies>:setup	`Error`	0.00	test_routers_iptables_default_policy.py
test_01_isolate_network_FW_PF_default_routes_egress_true	`Error`	0.18	test_routers_network_ops.py
test_02_isolate_network_FW_PF_default_routes_egress_false	`Error`	0.17	test_routers_network_ops.py
ContextSuite context=TestRedundantIsolateNetworks>:setup	`Error`	1.35	test_routers_network_ops.py
ContextSuite context=TestRouterServices>:setup	`Error`	0.00	test_routers.py
test_01_sys_vm_start	`Failure`	0.11	test_secondary_storage.py
test_02_sys_template_ready	`Failure`	0.10	test_secondary_storage.py
ContextSuite context=TestCpuCapServiceOfferings>:setup	`Error`	0.00	test_service_offerings.py
ContextSuite context=TestServiceOfferings>:setup	`Error`	0.20	test_service_offerings.py
ContextSuite context=TestSnapshotRootDisk>:setup	`Error`	0.00	test_snapshots.py
test_01_list_sec_storage_vm	`Failure`	0.04	test_ssvm.py
test_02_list_cpvm_vm	`Failure`	0.04	test_ssvm.py
test_03_ssvm_internals	`Failure`	0.04	test_ssvm.py
test_04_cpvm_internals	`Failure`	0.04	test_ssvm.py
test_05_stop_ssvm	`Failure`	0.04	test_ssvm.py
test_06_stop_cpvm	`Failure`	0.04	test_ssvm.py
test_07_reboot_ssvm	`Failure`	0.04	test_ssvm.py
test_08_reboot_cpvm	`Failure`	0.04	test_ssvm.py
test_09_destroy_ssvm	`Failure`	0.04	test_ssvm.py
test_10_destroy_cpvm	`Failure`	0.04	test_ssvm.py
test_02_create_template_with_checksum_sha1	`Error`	65.55	test_templates.py
test_03_create_template_with_checksum_sha256	`Error`	65.49	test_templates.py
test_04_create_template_with_checksum_md5	`Error`	65.51	test_templates.py
test_05_create_template_with_no_checksum	`Error`	65.49	test_templates.py
test_02_deploy_vm_from_direct_download_template	`Error`	4.42	test_templates.py
test_03_deploy_vm_wrong_checksum	`Error`	5.50	test_templates.py
ContextSuite context=TestTemplates>:setup	`Error`	19.14	test_templates.py
ContextSuite context=TestISOUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestLBRuleUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestNatRuleUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestPublicIPUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestSnapshotUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVmUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVolumeUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestVpnUsage>:setup	`Error`	0.00	test_usage.py
ContextSuite context=TestCreateVolume>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestVolumes>:setup	`Error`	0.00	test_volumes.py
ContextSuite context=TestDeployVM>:setup	`Error`	0.00	test_vm_life_cycle.py
ContextSuite context=TestVMLifeCycle>:setup	`Error`	0.00	test_vm_life_cycle.py
test_01_create_redundant_VPC_2tiers_4VMs_4IPs_4PF_ACL	`Error`	4.12	test_vpc_redundant.py
test_02_redundant_VPC_default_routes	`Error`	4.17	test_vpc_redundant.py
test_03_create_redundant_VPC_1tier_2VMs_2IPs_2PF_ACL_reboot_routers	`Error`	4.14	test_vpc_redundant.py
test_04_rvpc_network_garbage_collector_nics	`Error`	4.15	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	3.09	test_vpc_redundant.py
test_01_VPC_nics_after_destroy	`Error`	4.10	test_vpc_router_nics.py
test_02_VPC_default_routes	`Error`	4.11	test_vpc_router_nics.py
test_01_redundant_vpc_site2site_vpn	`Failure`	3.40	test_vpc_vpn.py
test_01_vpc_site2site_vpn_multiple_options	`Failure`	3.36	test_vpc_vpn.py
test_01_vpc_remote_access_vpn	`Failure`	2.19	test_vpc_vpn.py
test_01_vpc_site2site_vpn	`Failure`	2.35	test_vpc_vpn.py
test_02_cancel_host_maintenace_with_migration_jobs	`Error`	2.39	test_host_maintenance.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	3.69	test_hostha_kvm.py

rafaelweingartner · 2018-04-06T10:36:37Z

Something must have happened with the CI system. This PR's changes would not cause these failures.

borisstoyanov · 2018-04-10T07:46:27Z

@blueorangutan test

blueorangutan · 2018-04-10T07:47:36Z

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

PaulAngus · 2018-04-10T12:13:55Z

@blueorangutan test

blueorangutan · 2018-04-10T12:14:46Z

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

blueorangutan · 2018-04-11T14:14:05Z

Trillian test result (tid-2484)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 91748 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2404-t2484-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_certauthority_root.py
Intermitten failure detected: /marvin/tests/smoke/test_routers.py
Intermitten failure detected: /marvin/tests/smoke/test_hostha_kvm.py
Smoke tests completed. 65 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_04_restart_network_wo_cleanup	`Failure`	2.96	test_routers.py
test_hostha_enable_ha_when_host_in_maintenance	`Error`	4.70	test_hostha_kvm.py

borisstoyanov

LGTM

yadvr · 2018-05-04T08:14:00Z

@rafaelweingartner I scanned PRs merged on master but not 4.11 and came across this PR, since this is marked 'bug' do you think this would be useful for 4.11? If so, can you help create a backport PR for 4.11?

rafaelweingartner · 2018-05-04T09:59:12Z

This PR might be interesting. However, due to the amount of changes (I refactored the whole method), I preferred to introduced it only on master.

…ype” (apache#2404) * [CLOUDSTACK-10230] User is able to change to “Guest OS type” that has been removed Users are able to change the OS type of VMs to “Guest OS type” that has been removed. This becomes a security issue when we try to force users to use HVM VMs (Meltdown/Spectre thing). A removed “guest os type” should not be usable by any users in the cloud.

* CLOUDSTACK-10147 Disabled Xenserver Cluster can still deploy VM's. Added code to skip disabled clusters when selecting a host (#2442) (cherry picked from commit c3488a5) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> * CLOUDSTACK-10318: Bug on sorting ACL rules list in chrome (#2478) (cherry picked from commit 4412563) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> * CLOUDSTACK-10284:Creating a snapshot from VM Snapshot generates error if hypervisor is not KVM. Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> * CLOUDSTACK-10221: Allow IPv6 when creating a Basic Network (#2397) Since CloudStack 4.10 Basic Networking supports IPv6 and thus should be allowed to be specified when creating a network. Signed-off-by: Wido den Hollander <wido@widodh.nl> (cherry picked from commit 9733a10) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> * CLOUDSTACK-10214: Unable to remove local primary storage (#2390) Allow admins to remove primary storage pool. Cherry-picked from eba2e1d Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> * dateutil: constistency of tzdate input and output (#2392) Signed-off-by: Yoan Blanc <yoan.blanc@exoscale.ch> Signed-off-by: Daan Hoogland <daan.hoogland@shapeblue.com> (cherry picked from commit 2ad5202) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> * CLOUDSTACK-10054:Volume download times out in 3600 seconds (#2244) (cherry picked from commit bb607d0) Signed-off-by: Rohit Yadav <rohit.yadav@shapeblue.com> * When creating a new account (via domain admin) it is possible to select “root admin” as the role for the new user (#2606) * create account with domain admin showing 'root admin' role Domain admins should not be able to assign the role of root admin to new users. Therefore, the role ‘root admin’ (or any other of the same type) should not be visible to domain admins. * License and formatting * Break long sentence into multiple lines * Fix wording of method 'getCurrentAccount' * fix typo in variable name * [CLOUDSTACK-10259] Missing float part of secondary storage data in listAccounts * [CLOUDSTACK-9338] ACS not accounting resources of VMs with custom service offering ACS is accounting the resources properly when deploying VMs with custom service offerings. However, there are other methods (such as updateResourceCount) that do not execute the resource accounting properly, and these methods update the resource count for an account in the database. Therefore, if a user deploys VMs with custom service offerings, and later this user calls the “updateResourceCount” method, it (the method) will only account for VMs with normal service offerings, and update this as the number of resources used by the account. This will result in a smaller number of resources to be accounted for the given account than the real used value. The problem becomes worse because if the user starts to delete these VMs, it is possible to reach negative values of resources allocated (breaking all of the resource limiting for accounts). This is a very serious attack vector for public cloud providers! * [CLOUDSTACK-10230] User should not be able to use removed “Guest OS type” (#2404) * [CLOUDSTACK-10230] User is able to change to “Guest OS type” that has been removed Users are able to change the OS type of VMs to “Guest OS type” that has been removed. This becomes a security issue when we try to force users to use HVM VMs (Meltdown/Spectre thing). A removed “guest os type” should not be usable by any users in the cloud. * Remove trailing lines that are breaking build due to checkstyle compliance * Remove unused imports * fix classes that were in the wrong folder structure * Updates to capacity management

rafaelweingartner changed the title ~~[CLOUDSTACK-10230] User is able to useto a removed “Guest OS type”~~[CLOUDSTACK-10230] User should not be able to use removed “Guest OS type”Jan 12, 2018

rafaelweingartner force-pushed the CLOUDSTACK-10230 branch 2 times, most recently from db548d0 to 9a413caCompare January 15, 2018 16:53

rafaelweingartner force-pushed the CLOUDSTACK-10230 branch 3 times, most recently from 909cb1e to 6f53255Compare January 26, 2018 10:09

rafaelweingartner self-assigned this Jan 29, 2018

rafaelweingartner requested review from DaanHoogland, GabrielBrascher and nvazquez January 30, 2018 00:09

rafaelweingartner added type:bug component:management-server type:security labels Jan 30, 2018

rafaelweingartner added this to the 4.12 milestone Jan 30, 2018

rafaelweingartner force-pushed the CLOUDSTACK-10230 branch 2 times, most recently from 9164341 to 1edf496Compare March 22, 2018 15:27

rafaelweingartner force-pushed the CLOUDSTACK-10230 branch from 1edf496 to 90b80a5Compare March 30, 2018 13:15

DaanHoogland approved these changes Mar 31, 2018
View reviewed changes

re-introduce documentation of ACQUIRE_GLOBAL_LOCK_TIMEOUT_FOR_COOPERA…
7f4b908
…TION variable

DaanHoogland reviewed Apr 2, 2018
View reviewed changes

remove unecesssary documentation
259c7df

borisstoyanov approved these changes Apr 12, 2018
View reviewed changes

DaanHoogland merged commit 91d9821 into apache:masterApr 12, 2018

yadvr mentioned this pull request May 3, 2018
Backports for 4.11 branch #2621
Merged
12 tasks

Uh oh!

Conversation

rafaelweingartner commented Jan 12, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelweingartner commented Mar 19, 2018

Uh oh!

blueorangutan commented Mar 19, 2018

Uh oh!

blueorangutan commented Mar 19, 2018

Uh oh!

rafaelweingartner commented Mar 20, 2018

Uh oh!

blueorangutan commented Mar 20, 2018

Uh oh!

blueorangutan commented Mar 20, 2018

Uh oh!

rafaelweingartner commented Mar 30, 2018

Uh oh!

DaanHoogland commented Mar 31, 2018

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borisstoyanov commented Apr 2, 2018

Uh oh!

blueorangutan commented Apr 2, 2018

Uh oh!

blueorangutan commented Apr 2, 2018

Uh oh!

borisstoyanov commented Apr 2, 2018

Uh oh!

blueorangutan commented Apr 2, 2018

Uh oh!

rafaelweingartner commented Apr 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

borisstoyanov commented Apr 4, 2018

Uh oh!

blueorangutan commented Apr 4, 2018

Uh oh!

blueorangutan commented Apr 4, 2018

Uh oh!

borisstoyanov commented Apr 4, 2018

Uh oh!

blueorangutan commented Apr 4, 2018

Uh oh!

blueorangutan commented Apr 5, 2018

Uh oh!

rafaelweingartner commented Apr 6, 2018

Uh oh!

borisstoyanov commented Apr 10, 2018

Uh oh!

blueorangutan commented Apr 10, 2018

Uh oh!

PaulAngus commented Apr 10, 2018

Uh oh!

rafaelweingartner commented Jan 12, 2018•
edited
Loading