allow user to supply a cerificate file for trusted SSL #254

jmartin-tech · 2016-11-03T16:13:12Z

Allow validation of a known certificate when establishing a connection to the nexpose instance.

Description

Expand on the Connection object to support a known trusted certificate.
Current implementation allows for supply of certificate data in a trust_cert parameter on connection instantiation to be used for validation when supplied. Currently maintain backwards compatibility by using VERIFY_NONE when no specific certificate data to trust is supplied.

Validation requirements do require the certificate supplied to conform for hostname verification in addition to the specific certificate to trust the connection in the default VERIFY_PEER validation mode.

Motivation and Context

Allows module users to prevent MITM SSL attacks on the nexpose connection.
#246

How Has This Been Tested?

Connected this branch to another project currently consuming the nexpose gem 5.1.0 and obtained the certificate from a test nexpose instance with a self signed web certificate generated with a matching CN hostname value.

Tests:
Tested with PEM format self signed certificate with valid CN for host

Established connection (Login)
Listed sites
Listed Assets in a site
Teardown of connection (Logout)

Types of changes

New feature (non-breaking change which adds functionality)
Currently backward compatible (happy to consider version bump and making this a breaking change)

Checklist:

I have updated the documentation accordingly (if changes are required).
I have added tests to cover my changes.
All new and existing tests passed.

houndci-bot · 2016-11-03T16:13:21Z

test/test_site.rb

@@ -66,7 +66,7 @@ def session_id

  # Monkey patch API behavior to give static responses.
  class Nexpose::APIRequest
-    def self.execute(url, req, api_version='1.1')
+    def self.execute(url, trust_store, req, api_version='1.1')


Unused method argument - trust_store. If it's necessary, use _ or _trust_store as an argument name to indicate that it won't be used.
Unused method argument - req. If it's necessary, use _ or _req as an argument name to indicate that it won't be used.
Unused method argument - api_version. If it's necessary, use _ or _api_version as an argument name to indicate that it won't be used.
Surrounding space missing in default value assignment.

houndci-bot · 2016-11-03T16:13:21Z

lib/nexpose/api_request.rb

-    def self.execute(url, req, api_version='1.1', options = {})
-      obj = self.new(req.to_s, url, api_version)
+    def self.execute(url, req, trust_store, api_version='1.1', options = {})
+      obj = self.new(req.to_s, url, trust_store, api_version)


Redundant self detected.

houndci-bot · 2016-11-03T16:13:22Z

lib/nexpose/api_request.rb

@@ -133,8 +140,8 @@ def attributes(*args)
      @res.root.attributes(*args)
    end

-    def self.execute(url, req, api_version='1.1', options = {})
-      obj = self.new(req.to_s, url, api_version)
+    def self.execute(url, req, trust_store, api_version='1.1', options = {})


Surrounding space missing in default value assignment.

gschneider-r7 · 2016-11-03T21:02:10Z

lib/nexpose/connection.rb

@@ -44,20 +44,29 @@ class Connection
    # The last XML response received by this object, useful for debugging.
    attr_reader :response_xml

+    # Enforcement of certificate validation to a CA
+    attr_reader :allow_untrusted


Is this used anywhere?

Not yet, I would like to extend this to be usable but it starts to make this more of a breaking change, can remove or expand depending on how discussion goes.

We may be doing a 6.0 release soon anyway (other breaking changes) so it's not a big deal to me either way. You could even do separate branches for non-breaking and breaking changes if you are inclined to do so.

jmartin-tech · 2016-11-03T21:50:36Z

Example usage, normally would obtain and store then load cert for connection.

require 'socket'
require 'openssl'
require 'nexpose'

host = 'r7dev'
port = 3780
user = 'nxadmin'
pass = 'nxadmin'

# get the cert from the host and hold as string
# certificate could also be loaded from file on disk with trust_store = File.load('/path/to/file.pem')
socket = TCPSocket.open(host,port)
ssl_context = OpenSSL::SSL::SSLContext.new()
ssl_context.ssl_version = :TLSv1
ssl_socket = OpenSSL::SSL::SSLSocket.new(socket, ssl_context)
ssl_socket.sync_close = true
ssl_socket.connect

cert = OpenSSL::X509::Certificate.new(ssl_socket.peer_cert)
ssl_socket.close
trust_cert = cert.to_pem

# use the loaded cert string to create a "trusted" connection
begin
  puts("Connecting to Nexpose instance at #{host}:#{port} with username #{user}...")
  nsc = Nexpose::Connection.new(host, user, pass, port, nil, nil, trust_cert)
  nsc.login
  # do some work
  nsc.logout
rescue Nexpose::APIError => e
  print_error("Connection failed: #{e.reason}")
  return
end

sgreen-r7 · 2017-02-09T19:31:11Z

@jmartin-r7 @gschneider-r7
Think I'm going to have to push back on this, in it's current format.

My suggestion would be we leave the signature in the same order, then add trust_store=nil as the final argument. Which is what we did here for Connection#initialize

Agree/Disagree?

We need to not alter the method arguments in such a way, that could cause a user to unknowingly invoke an unintended initialization. In this case, we have altered APIRequest#initialize and moved api_version to position 4, and entered our new trust_store param to position 3. So all code that has already been written that is specifying an api_version, would have to be re-written to insert nil for the 3rd argument, then move whatever they had for api_version over to the 4th argument.

Also, any code that is specifying an api_version and the customer upgraded the gem (and obviously didn't read the release notes), would start having weird SSL and keystore errors -- and that is something we should also try to avoid.

jmartin-tech · 2017-02-09T19:47:57Z

@sgreen-r7, I am aware of the alteration to APIRequest constructor, I had some discussion with @gschneider-r7 on this as to if this should be considered a breaking change, my issue here is that trust_store is not an optional parameter, to place it as the last param will mean that all previous usages for it will break anyways as usage would require 4 params vs original 2 and optional third. Note that APIRequest is not expected to be used by any class but connection as APIRequest.execute which I also need to change in the same manner and is constructing the new APIRequests. This is called on a connection object as Connection.execute(xml, version = '1.1', options = {}).

Looking at the patterns and usage here APIRequest is not expected to be an "public" class for consumption of the nexpose-client gem and so I decided the prioritization in order should be for required params to be forward in in the method signature.

jmartin-tech · 2017-02-14T16:56:28Z

@sgreen-r7, revised per your request, can we land this and release a new gem version?

gschneider-r7 · 2017-02-14T17:08:34Z

lib/nexpose/connection.rb

    # A constructor to load a Connection object from a URI
    def self.from_uri(uri, user, pass, silo_id = nil, token = nil)
      uri = URI.parse(uri)
-      new(uri.host, user, pass, uri.port, silo_id, token)
+      new(uri.host, user, pass, uri.port, silo_id, token, nil)


Not allowing trust cert to be specified with this method?

gschneider-r7 · 2017-02-14T20:59:00Z

test/test_site.rb

@@ -66,7 +66,7 @@ def session_id

  # Monkey patch API behavior to give static responses.
  class Nexpose::APIRequest
-    def self.execute(url, req, api_version='1.1')
+    def self.execute(url, _trust_store, _req, _api_version = '1.1')


Should probably put this in the correct order for consistency, even though it's just test code.

gschneider-r7 · 2017-02-14T21:11:10Z

This suffers from #163 where SSL failures are not made clear to the user. I don't think that's a show-stopper for getting an initially usable version of this out.

Also add parameter docs for the constructor.

houndci-bot · 2017-02-14T21:35:15Z

lib/nexpose/api_request.rb

-    def self.execute(url, req, api_version='1.1', options = {})
-      obj = self.new(req.to_s, url, api_version)
+    def self.execute(url, req, api_version = '1.1', options = {}, trust_store = nil)
+      obj = self.new(req.to_s, url, api_version, trust_store)


Redundant self detected.

houndci-bot · 2017-02-14T21:35:15Z

lib/nexpose/api_request.rb

@@ -93,7 +100,7 @@ def execute(options = {})
        # drops our HTTP connection before processing. We try 5 times to establish a
        # connection in these situations. The actual exception occurs in the Ruby
        # http library, which is why we use such generic error classes.
-      rescue OpenSSL::SSL::SSLError
+      rescue OpenSSL::SSL::SSLError => e


Useless assignment to variable - e.

jmartin-tech added Feature Need Feeback labels Nov 3, 2016

houndci-bot reviewed Nov 3, 2016

View reviewed changes

allow user to supply an ASCII PEM file for trusted SSL

ea36ddd

jmartin-tech force-pushed the feature/allow-certificate-validation branch from 1dd08d3 to ea36ddd Compare November 3, 2016 19:27

gschneider-r7 reviewed Nov 3, 2016

View reviewed changes

gschneider-r7 requested a review from sgreen-r7 December 8, 2016 08:03

jmartin-tech added 2 commits February 7, 2017 15:12

remove not yet used param

0b6fde4

clean up some houndci-bot complaints

e82ce42

improve api compatibility

516c4ce

jmartin-tech removed the Need Feeback label Feb 14, 2017

gschneider-r7 reviewed Feb 14, 2017

View reviewed changes

allow trust_cert supplied on from_uri

3162e1b

gschneider-r7 reviewed Feb 14, 2017

View reviewed changes

Update docs for Nexpose::Connection with more examples

bcf8510

Also add parameter docs for the constructor.

houndci-bot reviewed Feb 14, 2017

View reviewed changes

gschneider-r7 approved these changes Feb 14, 2017

View reviewed changes

address test comment

fdaa245

jmartin-tech merged commit fdaa245 into master Feb 14, 2017

jmartin-tech added a commit that referenced this pull request Feb 14, 2017

Land #254, feature allow certificate validation

959eefa

jmartin-tech deleted the feature/allow-certificate-validation branch February 14, 2017 21:50

gschneider-r7 mentioned this pull request Feb 14, 2017

Allow a user to enable certificate pinning when verifying a TLS connection to a nexpose console #246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

allow user to supply a cerificate file for trusted SSL #254

allow user to supply a cerificate file for trusted SSL #254

jmartin-tech commented Nov 3, 2016 •

edited by gschneider-r7

Loading

houndci-bot Nov 3, 2016

houndci-bot Nov 3, 2016

houndci-bot Nov 3, 2016

gschneider-r7 Nov 3, 2016

jmartin-tech Nov 3, 2016

gschneider-r7 Nov 3, 2016 •

edited

Loading

jmartin-tech commented Nov 3, 2016 •

edited by gschneider-r7

Loading

sgreen-r7 commented Feb 9, 2017

jmartin-tech commented Feb 9, 2017

jmartin-tech commented Feb 14, 2017

gschneider-r7 Feb 14, 2017

gschneider-r7 Feb 14, 2017

gschneider-r7 commented Feb 14, 2017

houndci-bot Feb 14, 2017

houndci-bot Feb 14, 2017

allow user to supply a cerificate file for trusted SSL #254

allow user to supply a cerificate file for trusted SSL #254

Conversation

jmartin-tech commented Nov 3, 2016 • edited by gschneider-r7 Loading

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschneider-r7 Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

jmartin-tech commented Nov 3, 2016 • edited by gschneider-r7 Loading

sgreen-r7 commented Feb 9, 2017

jmartin-tech commented Feb 9, 2017

jmartin-tech commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gschneider-r7 commented Feb 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartin-tech commented Nov 3, 2016 •

edited by gschneider-r7

Loading

gschneider-r7 Nov 3, 2016 •

edited

Loading

jmartin-tech commented Nov 3, 2016 •

edited by gschneider-r7

Loading