You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by rodrigo93 <gi...@git.apache.org> on 2016/05/21 23:43:47 UTC
[GitHub] cloudstack pull request: Removed unused methods from XenServerConn...
GitHub user rodrigo93 opened a pull request:
https://github.com/apache/cloudstack/pull/1557
Removed unused methods from XenServerConnectionPool
Removed the following methods that are unused from the class
_com.cloud.hypervisor.xenserver.resource.XenServerConnectionPool_:
- static void **forceSleep**(long sec)
- protected Session **slaveLocalLoginWithPassword**(Connection conn, String
username, Queue<String> password)
- static public Pool.Record **getPoolRecord**(Connection conn) throws
XmlRpcException, XenAPIException
From XenServerConnectionPool.XenServerConnection:
- public String getUsername()
- public Queue<String> getPassword()
- protected Map dispatch(String methodcall, Object[] methodparams)
I would like to make a note about the "TrustAllManager" static class
inside the XenServerConnectionPool class. I noticed methods from TrustAllManager class returns true or "nothing". This can be a security problem, since it returns true without any
verification.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rodrigo93/cloudstack lrg-cs-hackday-027
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/cloudstack/pull/1557.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #1557
----
commit dd5c9d03b3b14de13711fda6c5c4eb5ee841ef42
Author: Rodrigo Marques <ro...@gmail.com>
Date: 2016-05-22T02:34:14Z
Removed unused methods from XenServerConnectionPool
Removed unused methods from the class
com.cloud.hypervisor.xenserver.resource.XenServerConnectionPool:
static void forceSleep(long sec)
protected Session slaveLocalLoginWithPassword(Connection conn, String
username, Queue<String> password)
static public Pool.Record getPoolRecord(Connection conn) throws
XmlRpcException, XenAPIException
Inside XenServerConnectionPool.XenServerConnection
public String getUsername()
public Queue<String> getPassword()
protected Map dispatch(String methodcall, Object[] methodparams)
I would like to make a note about the "TrustAllManager" static class
inside the XenServerConnectionPool class.
I noticed the methods from TrustAllManager returns true or nothing. This
can be a security problem, since it returns true without any
verification.
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1557: Removed unused methods from XenServerConnectionPool
Posted by rhtyd <gi...@git.apache.org>.
Github user rhtyd commented on the issue:
https://github.com/apache/cloudstack/pull/1557
@blueorangutan package
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1557: Removed unused methods from XenServerConnectionPool
Posted by bvbharatk <gi...@git.apache.org>.
Github user bvbharatk commented on the issue:
https://github.com/apache/cloudstack/pull/1557
### ACS CI BVT Run
**Sumarry:**
Build Number 188
Hypervisor xenserver
NetworkType Advanced
Passed=73
Failed=0
Skipped=3
_Link to logs Folder (search by build_no):_ https://www.dropbox.com/sh/yj3wnzbceo9uef2/AAB6u-Iap-xztdm6jHX9SjPja?dl=0
**Failed tests:**
**Skipped tests:**
test_vm_nic_adapter_vmxnet3
test_static_role_account_acls
test_deploy_vgpu_enabled_vm
**Passed test suits:**
test_deploy_vm_with_userdata.py
test_affinity_groups_projects.py
test_portable_publicip.py
test_vpc_vpn.py
test_over_provisioning.py
test_global_settings.py
test_scale_vm.py
test_service_offerings.py
test_routers_iptables_default_policy.py
test_routers.py
test_reset_vm_on_reboot.py
test_snapshots.py
test_deploy_vms_with_varied_deploymentplanners.py
test_login.py
test_list_ids_parameter.py
test_public_ip_range.py
test_multipleips_per_nic.py
test_regions.py
test_affinity_groups.py
test_network_acl.py
test_pvlan.py
test_volumes.py
test_nic.py
test_deploy_vm_root_resize.py
test_resource_detail.py
test_secondary_storage.py
test_vm_life_cycle.py
test_disk_offerings.py
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unused methods from XenServerConnectionPool
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1557
@rodrigo93 @rafaelweingartner Do you have a scenario that proves these methods to be a problem? returning true or null (or void) does not imply a problem unless the result can be abused.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unused methods from XenServerConnectionPool
Posted by DaanHoogland <gi...@git.apache.org>.
Github user DaanHoogland commented on the pull request:
https://github.com/apache/cloudstack/pull/1557
No I am talking about the problems with TrustAllManager. I am not convinced there is a problem by the not by @rodrigo93 .
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1557: Removed unused methods from XenServerConnectionPool
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1557
Packaging result: \u2714centos6 \u2714centos7 \u2714debian. JID-221
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unused methods from XenServerConnectionPool
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1557
@DaanHoogland are you asking about the code at lines 53-70?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unused methods from XenServerConn...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1557#issuecomment-221028275
@rodrigo93,
I was reviewing your PR, nice work; but, I think you can remove some other lines too.
I checked and the variable \u201cs_sleepOnError \u201d is never used. I mean, it is used at lines 68 and 70; but, that is all (just logging). Also, that property \u201csleep.interval.on.error\u201d from which a new value is assigned to \u201cs_sleepOnError \u201d is never referenced anywhere else. Therefore, the \u201cs_sleepOnError \u201d variable can be removed, and the code from lines 60 \u2013 76 can also be removed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unused methods from XenServerConn...
Posted by rodrigo93 <gi...@git.apache.org>.
Github user rodrigo93 commented on the pull request:
https://github.com/apache/cloudstack/pull/1557#issuecomment-221426317
Hi @rafaelweingartner ,
Thanks for the advice. I will take a look on that and do the proper changes as soon as I can.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unused methods from XenServerConn...
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the pull request:
https://github.com/apache/cloudstack/pull/1557#issuecomment-222135268
@rodrigo93 thanks.
@DaanHoogland, @swill, @rhtyd can someone help us here with the note @rodrigo93 has highlighted?
I have sent an email to the security@cloudstack.apache.org that presents our perception of that, but we did not receive any feedback.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1557: Removed unused methods from XenServerConnectionPool
Posted by blueorangutan <gi...@git.apache.org>.
Github user blueorangutan commented on the issue:
https://github.com/apache/cloudstack/pull/1557
@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack pull request: Removed unused methods from XenServerConn...
Posted by rodrigo93 <gi...@git.apache.org>.
Github user rodrigo93 commented on the pull request:
https://github.com/apache/cloudstack/pull/1557#issuecomment-222007302
Hello again @rafaelweingartner,
I did as you suggested. I also checked that the variable **file** was not used anywhere else besides lines 60 - 76, so I also removed it.
Thanks for the suggestion.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] cloudstack issue #1557: Removed unused methods from XenServerConnectionPool
Posted by rafaelweingartner <gi...@git.apache.org>.
Github user rafaelweingartner commented on the issue:
https://github.com/apache/cloudstack/pull/1557
Hi @Daan,
We are also not sure, that is why we have sent an email to the security maligning list to start a discussion about that. The question regarding the \u201ccom.cloud.hypervisor.xenserver.resource.XenServerConnectionPool. "TrustAllManager" class was the following.
> The code at line 77-96 is setting the default SSL socket factory for HTTPS connection to an implementation (TrustAllManager class created at lines 483- 506) that always considers the certificate of the server valid (without any checks). Also, at line 90, it was overridden the host name verifier (the check that matches hostname and certificates CN) to always returns true.
We also mentioned that:
> We might be misinterpreting the java doc of methods \u201cjavax.net.ssl.HttpsURLConnection.setDefaultSSLSocketFactory(SSLSocketFactory)\u201d and \u201cjavax.net.ssl.HttpsURLConnection.setDefaultHostnameVerifier(HostnameVerifier)\u201d, but those sets seem to affect every single HTTPS request that is performed by ACS (JVM runtime in which those methods were executed). We do not know how many HTTPS requests, and to what extent they are essential to ACS workings, but that code seems to make ACS vulnerable to man in the middle attacks (if it uses HTTPS requests to execute some of its tasks).
Speculating a bit, we had an idea why that code was added:
> We believe that was done in order to allow the use of the XAPI via rest API through HTTPS, probably because the host is using self-signed certificates. If that is the case, I think we have better options to work around that, such as just disabling the check for those specific requests and or using a keystore with the hosts\u2019 public keys that could be downloaded at the host addition process.
In conclusion, if ACS uses an HTTPS connection to either download a file (template or something else) or to send a command to a remote region over the internet that connection/request may be vulnerable to MID attacks. Even though we will have an SSL connection, we will not know with whom, because the certificate checks were all disabled.
Again I would like to say that we do not know how many (if) ACS uses much HTTPS request to execute some of its tasks; that is why we raised the question.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---