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

Bring the SSL support (take 2) #68

Merged
merged 6 commits into from Apr 5, 2017

Conversation

Projects
None yet
7 participants
@wkoszek
Copy link
Contributor

wkoszek commented Mar 27, 2017

The expectation is to use http:// URL for connecting. I want to use HTTPS for this. Below change adds the support for this.

==
Unfortunately my XenServer is coming with a self-signed certificate, which breaks XMLRPC::Client, so I can't really test that. With this, XMLRPC fails deep down in it's call graph. I'm testing with this line:

@factory.instance_variable_get(:@http).instance_variable_set(:@verify_mode, OpenSSL::SSL::VERIFY_NONE)

put in the initialize in lib/fog/xen_server/connection.rb, so that cert isn't checked.

If we could commit the change from below, and (maybe?) add a conditional work-around for the self-signed problem, that would be great.

wkoszek added some commits Mar 26, 2017

def initialize(host, timeout)
@factory = XMLRPC::Client.new(host, "/")
def initialize(host, port, use_ssl, timeout)
@factory = XMLRPC::Client.new3(host: host, port: port, use_ssl: use_ssl, path: "/")

This comment has been minimized.

@houndci-bot

houndci-bot Mar 27, 2017

Line is too long. [91/80]

@connection = Fog::XenServer::Connection.new(@host, @timeout)
@use_ssl = options[:xenserver_use_ssl] || false
@port = options[:xenserver_port] || true
@connection = Fog::XenServer::Connection.new(@host, @port, @use_ssl, @timeout)

This comment has been minimized.

@houndci-bot

houndci-bot Mar 27, 2017

Line is too long. [89/80]

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 27, 2017

Coverage Status

Coverage increased (+0.007%) to 94.085% when pulling 8dad45d on wkoszek:master into e1c2329 on fog:master.

@@ -10,7 +10,9 @@ def initialize(options={})
@password = options[:xenserver_password]
@defaults = options[:xenserver_defaults] || {}
@timeout = options[:xenserver_timeout] || 30
@connection = Fog::XenServer::Connection.new(@host, @timeout)
@use_ssl = options[:xenserver_use_ssl] || false
@port = options[:xenserver_port] || true

This comment has been minimized.

@plribeiro3000

plribeiro3000 Mar 27, 2017

Member

Shouldn't we have a default port value in here instead of true?

@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Mar 27, 2017

@wkoszek I'm ok merging this in if it does not break the current behaviour. In case we do reach the ssl goal, we can do a release candidate and let it sit in there for a while before release a major version including this change. =)

@mmoll

This comment has been minimized.

Copy link

mmoll commented Mar 27, 2017

@NeilHanlon maybe you want to test this? :)

@NeilHanlon

This comment has been minimized.

Copy link

NeilHanlon commented Mar 28, 2017

@mmoll yeah I more or less have the same thing monkey patched at work. I will verify this tomorrow in the office.

(Now only if github could send me reminders....)

@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Mar 28, 2017

How did you guys use/test it before? Over HTTP?

I'm going to make the fixes, and re-test. The port thing is a bug.

I think the patching is super ugly, but I don't see any other way to fix that, unless we fix 'ruby/xmlrpc' to pass the "don't validate the cert" flag.

So for now I think I'll just enable that is use_ssl is set to 1

def initialize(host, port, use_ssl, timeout)
@factory = XMLRPC::Client.new3(host: host, port: port, use_ssl: (use_ssl != false), path: "/")
if use_ssl == -1
@factory.instance_variable_get(:@http).instance_variable_set(:@verify_mode, OpenSSL::SSL::VERIFY_NONE)

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2017

Line is too long. [112/80]

def initialize(host, timeout)
@factory = XMLRPC::Client.new(host, "/")
def initialize(host, port, use_ssl, timeout)
@factory = XMLRPC::Client.new3(host: host, port: port, use_ssl: (use_ssl != false), path: "/")

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2017

Line is too long. [102/80]

def initialize(host, port, use_ssl, timeout)
@factory = XMLRPC::Client.new3(host: host, port: port, use_ssl: (use_ssl != false), path: "/")
if use_ssl == -1
@factory.instance_variable_get(:@http).instance_variable_set(:@verify_mode, OpenSSL::SSL::VERIFY_NONE)

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2017

Line is too long. [112/80]

def initialize(host, timeout)
@factory = XMLRPC::Client.new(host, "/")
def initialize(host, port, use_ssl, timeout)
@factory = XMLRPC::Client.new3(host: host, port: port, use_ssl: (use_ssl != false), path: "/")

This comment has been minimized.

@houndci-bot

houndci-bot Mar 28, 2017

Line is too long. [102/80]

Fix a port assignment bug.
Spotted by:	Paulo Henrique Lopes Ribeiro (@plribeiro3000)
@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Mar 28, 2017

@plribeiro3000 Fixed the port assignment bug, thanks. The logic of the code is follows:

use_ssl == true: enable SSL, normal SSL certificate validation
use_ssl == false: no SSL
use_ssl == -1: enable SSL, patched workaround for ignoring exception for self-signed cert.

I've tested with this repo: https://github.com/bvox/fog-xenserver-examples with list_vm_ips.rb modified:

require 'fog'
require 'pp'

conn = Fog::Compute.new({
  :provider => 'XenServer',
  :xenserver_url => 'my.host',
  :xenserver_use_ssl => -1,
  :xenserver_port => 443,
  :xenserver_username => 'root',
  :xenserver_password => 'mypass',
  :xenserver_defaults => {
    :template => "squeeze-test"
  }
})

conn.servers.each do |vm|
  pp vm.name
  if vm.tools_installed?
    vm.guest_metrics.networks.each do |k,v|
      puts v
    end
  end
end

@NeilHanlon Any chance you could give that a shot?

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 28, 2017

Coverage Status

Coverage increased (+0.007%) to 94.085% when pulling 765ed87 on wkoszek:master into e1c2329 on fog:master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 28, 2017

Coverage Status

Coverage increased (+0.007%) to 94.085% when pulling 765ed87 on wkoszek:master into e1c2329 on fog:master.

@NeilHanlon

This comment has been minimized.

Copy link

NeilHanlon commented Mar 28, 2017

fwiw, I do the following. But I'm happy for any solution that fixes and will be happy to test

require 'xmlrpc/client'


class XMLRPC::Client
  # WEAK: Enrich the Client with a method for disabling SSL VERIFICATION
  # See /usr/lib/ruby/1.9.1/xmlrpc/client.rb:324
  # Bad hack but it works
  def disable_ssl_verification
    @http.verify_mode = OpenSSL::SSL::VERIFY_NONE
    warn "Proxyman SSL Verification disabled"
  end
end


module Fog
  module XenServer
    class Connection
      attr_reader :credentials

      def initialize(host, timeout)
        #have to pass all these to allow SSL
        @factory = XMLRPC::Client.new(host, "/", 443, nil, nil, nil, nil, true)
        @factory.set_parser(NokogiriStreamParser.new)
        @factory.disable_ssl_verification
        @factory.timeout = timeout
      end

in lib/fog/xen_server/connection.rb

@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Mar 28, 2017

@wkoszek @NeilHanlon Do we have a link of this issue in XMLRPC?

Just so we can link this to the other and then bring more people with deep knowledge about XMLRPC to this conversation?

@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Mar 28, 2017

@plribeiro3000 The ruby/xmlrpc repo https://github.com/ruby/xmlrpc doesn't have GitHub Issues enabled. I've asked guys from IRC on #ruby to see if I should report it at ruby/ruby, but nobody knew.

I've pinged an author of ruby/xmlrpc via e-mail.

@hsbt Ping :-)

@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Mar 28, 2017

@wkoszek Nice. =)

I just want to make sure this is indeed a bug and not a design decision.
Once we know this it will be easier to figure out the best course to solve this. =)

@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Mar 28, 2017

Ok. Sounds good.

@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Mar 31, 2017

@plribeiro3000 I've pushed a fix suggested by @HSTB

@coveralls

This comment has been minimized.

Copy link

coveralls commented Mar 31, 2017

Coverage Status

Coverage increased (+0.007%) to 94.085% when pulling 9cc90f3 on wkoszek:master into e1c2329 on fog:master.

@@ -5,8 +5,11 @@ module XenServer
class Connection
attr_reader :credentials

def initialize(host, timeout)
@factory = XMLRPC::Client.new(host, "/")
def initialize(host, port, use_ssl, timeout)

This comment has been minimized.

@plribeiro3000

plribeiro3000 Mar 31, 2017

Member

This signature is bothering me. use_ssl can be either true, false or -1.

What if use_ssl accepts only true or false and we support another parameter called verify_mode with a sane default?
The signature would look like initialize(host, port, use_ssl, verify_mode, timeout). WDYT?

This comment has been minimized.

@wkoszek

wkoszek Mar 31, 2017

Contributor

Feel free to change it to look like that.

This comment has been minimized.

@plribeiro3000

plribeiro3000 Mar 31, 2017

Member

Unfortunately i don't have the spare time to do so. =s

@wkoszek Do you think you could add this change yourself? I would be happy to bring this in if we make this change.

@use_ssl = options[:xenserver_use_ssl] || false
@port = options[:xenserver_port] || 80
@verify_mode = options[:xenserver_verify_mode] || OpenSSL::SSL::VERIFY_PEER
@connection = Fog::XenServer::Connection.new(@host, @port, @use_ssl, @verify_mode, @timeout)

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Line is too long. [103/80]

@connection = Fog::XenServer::Connection.new(@host, @timeout)
@use_ssl = options[:xenserver_use_ssl] || false
@port = options[:xenserver_port] || 80
@verify_mode = options[:xenserver_verify_mode] || OpenSSL::SSL::VERIFY_PEER

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Line is too long. [85/80]

@use_ssl = options[:xenserver_use_ssl] || false
@port = options[:xenserver_port] || 80
@verify_mode = options[:xenserver_verify_mode] || OpenSSL::SSL::VERIFY_PEER
@connection = Fog::XenServer::Connection.new(@host, @port, @use_ssl, @verify_mode, @timeout)

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Line is too long. [103/80]

@connection = Fog::XenServer::Connection.new(@host, @timeout)
@use_ssl = options[:xenserver_use_ssl] || false
@port = options[:xenserver_port] || 80
@verify_mode = options[:xenserver_verify_mode] || OpenSSL::SSL::VERIFY_PEER

This comment has been minimized.

@houndci-bot

houndci-bot Apr 4, 2017

Line is too long. [85/80]

@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Apr 4, 2017

@plribeiro3000 I've pushed the change, bundle install and rake test and it seems to work:

Finished in 1.505997 seconds.
423 tests, 423 passed, 0 failures, 0 errors, 0 skips, 428 assertions

and my stuff seems to work as well--I'm able to list VMs on my Xenserver machine.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Apr 4, 2017

Coverage Status

Coverage increased (+0.01%) to 94.089% when pulling 03cf184 on wkoszek:master into e1c2329 on fog:master.

@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Apr 5, 2017

@plribeiro3000 ping :)

@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Apr 5, 2017

Hey @wkoszek . Sorry for the delay. =)

Taking a look now!

@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Apr 5, 2017

Thats way better. Thanks for your efforts and patience on this. 👍

@plribeiro3000 plribeiro3000 merged commit 50ef013 into fog:master Apr 5, 2017

2 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
coverage/coveralls Coverage increased (+0.01%) to 94.089%
Details
hound 1 violation found.
@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Apr 5, 2017

@wkoszek Would you add an example on the README please?

@wkoszek

This comment has been minimized.

Copy link
Contributor

wkoszek commented Apr 5, 2017

@plribeiro3000 @NeilHanlon @mmoll Do you guys have any snippets that you actively use for this, where e.g.: from the command line you go from 0 to Ubuntu 14.04 or FreeBSD 10+ VM running on XenServer?

I'd love to grab those and (a) steal them from you (b) bring examples to README.md

@mmoll

This comment has been minimized.

Copy link

mmoll commented Apr 6, 2017

@wkoszek I don't use xen personally, but I hope that @NeilHanlon can use the current code in master to patch theforeman/foreman-xen and maybe that can lead to an example for the README.

@bdonnahue

This comment has been minimized.

Copy link

bdonnahue commented Nov 19, 2018

Is anyone working on this? theforeman/foreman-xen seems unusable

@plribeiro3000

This comment has been minimized.

Copy link
Member

plribeiro3000 commented Nov 19, 2018

@bdonnahue This was merged and release already.

If you feel like something is not working as expected or broken, plz open a new issue with a backtrace of your error so someone can check it out. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment