oracle_session and mssql_session improvement #1857

chris-rock · 2017-05-29T20:48:44Z

fixes Inspec oracledb_session resource not connecting to the database. #1821
fix: SID does not work with sqlplus, we need to use service names
use password instead of pass as argument
parse result table and make it accessible
support oracle sqlci tool
tested against oracle 12c and mysql server 2016

TODO:

update docs
unit tests

aaronlippold · 2017-05-30T15:14:44Z

no love for postgres_session? :)

aaronlippold · 2017-05-30T15:28:12Z

@chris-rock did you cover the case where the SID and SERVICE_NAME change? With one you need the port/host with the other you don't.

I don't see a service_name param in the current push.

    def db_conn
      if @sid
        "#{@user}/#{@password}@#{@host}:#{@port}/#{@sid}"
      else # using a 'service_name'
        "#{@user}/#{@password}@/#{@service_name}"
      end
    end

With a call like:

 escaped_query = Shellwords.escape(@query)
      cmd = inspec.command("echo -ne \"set heading off\nset echo off\n#{escaped_query}\" | #{@sqlplus_bin} -s #{db_conn_string}")

code:

require 'shellwords'

module Inspec::Resources
  class OracledbSession < Inspec.resource(1)
    name 'oracledb_session'
    desc 'Use the oracledb_session InSpec resource to test commands against an Oracle database'
    example "
      describe oracledb_session(user: 'my_user',
                                password: 'password',
                                host: '192.168.0.10',
                                service_name: 'TESTDB',
                                query: 'SELECT NAME FROM v$database;') do
        it { should be_successful }
        its('output') { should_not match /test/ }
      end

      Required values:
       - user
       - pass
       - sid or service_name
       - query

      Optional values:
       - host (default: localhost)
       - port (default: 1526)
    "

    attr_reader :user, :password, :host, :sid, :sqlplus_bin, :service_name, :port

    def initialize(opts = {})
      @user = opts[:user]
      @host = opts[:host] || 'localhost'
      @port = opts[:port] || '1526'
      @sid = opts[:sid]
      @query = opts[:query]
      @service_name = opts[:service_name]
      @sqlplus_bin = opts[:sqlplus_bin] || 'sqlplus'

      # legacy option was "pass" but we prefer "password" for clarity
      if opts[:pass]
        warn '[DEPRECATED] use `password` option to supply password instead of `pass`'
        @password = opts[:pass]
      else
        @password = opts[:password]
      end

      return skip_resource 'The `oracledb_session` resource does not support Windows yet' if inspec.os.windows?
      return skip_resource 'Username and password are required' if @user.nil? || @password.nil?
      return skip_resource 'You must provide at least an SID and/or a Service Name for the session' if @sid.nil? && @service_name.nil?
    end

    def output
      run_query
      @output
    end

    def errors
      run_query
      @errors
    end

    def successful?
      run_query
      @errors.nil? || @errors.empty?
    end

    def query(q)
      warn '[DEPRECATED] pass query in as an option when creating oracledb_session resource'
      @query = q
      run_query
      self
    end

    def run_query
      return if @query_already_run

      db_conn_string = db_conn
      # escaped_query = @query.gsub(/\\/, '\\\\').gsub(/"/, '\\"')
      escaped_query = Shellwords.escape(@query)
      cmd = inspec.command("echo -ne \"set heading off\nset echo off\n#{escaped_query}\" | #{@sqlplus_bin} -s #{db_conn_string}")
      @output = cmd.stdout
      @errors = cmd.stderr
      @query_already_run = true
    end

    def stdout
      warn '[DEPRECATED] use the `output` method to get the output of the oracle query'
      @output
    end

    def stderr
      warn '[DEPRECATED] use the `errors` method to get the errors from the oracle query'
      @errors
    end

    def db_conn
      if @sid
        "#{@user}/#{@password}@#{@host}:#{@port}/#{@sid}"
      else # using a 'service_name'
        "#{@user}/#{@password}@/#{@service_name}"
      end
    end

    def to_s
      'Oracle Session'
    end
  end
end

chris-rock · 2017-05-30T17:38:36Z

@aaronlippold postgres is another iteration

chris-rock · 2017-05-30T17:39:07Z

@aaronlippold we always use a service name, no sid

aaronlippold · 2017-05-30T18:09:52Z

Hi In the case of Oracle shouldn't we support both side service name? On May 30, 2017 13:39, "Christoph Hartmann" <notifications@github.com> wrote: @aaronlippold <https://github.com/aaronlippold> we always use a service name, no sid — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1857 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABauaPuJopmLMeRNpk7ed4taPIu09_5Qks5r_FQ-gaJpZM4Npn-4> .

adamleff

@chris-rock I know this is a WIP, but I have some feedback I'm hoping you'll take into consideration during your next iteration.

adamleff · 2017-05-31T19:00:52Z

Gemfile

@@ -8,6 +8,7 @@ if Gem::Version.new(RUBY_VERSION) < Gem::Version.new('2.2.2')
 end

 gem 'ffi', '>= 1.9.14'
+gem 'htmlentities'


This should go in the gemspec since the InSpec core (and therefore users who install the gem) is relying on it.

My bad. Thank you for the reminder!

adamleff · 2017-05-31T19:02:33Z

lib/resources/mssql_session.rb

      end
-      cmd
+      SQLQueryResult.new(cmd, parse_result(cmd))


parse_result doesn't exist... did you mean parse_csv_result which is below in line 68?

adamleff · 2017-05-31T19:03:13Z

lib/resources/oracledb_session.rb

      @sqlplus_bin = opts[:sqlplus_bin] || 'sqlplus'
-      return skip_resource("Can't run Oracle checks without authentication") if @user.nil? or @pass.nil?
+
+      return skip_resource "Can't run Oracle checks without authentication" if @user.nil? or @password.nil?


Use || instead of or

adamleff · 2017-05-31T19:04:14Z

lib/resources/oracledb_session.rb

    end

    def query(q)
      escaped_query = q.gsub(/\\/, '\\\\').gsub(/"/, '\\"')
-      cmd = inspec.command("echo \"#{escaped_query}\" | #{@sqlplus_bin} -s #{@user}/#{@pass}@#{@host}/#{@sid}")
+      # escape tables with $
+      escaped_query = escaped_query.gsub('$', "\\$")


Can we use Shellwords.shellescape instead of these gsubs?

adamleff · 2017-05-31T19:05:10Z

lib/resources/oracledb_session.rb

+      if inspec.command(@sqlcl_bin).exist?
+        bin = @sqlcl_bin
+        opts = "set sqlformat csv\nSET FEEDBACK OFF"
+        p = 'parse_csv_result'


Let's use a better variable name here, such as parse_method, and since we're already going to convert it to a symbols, let's set it as a symbol here and avoid the indirection.

parse_method = :parse_csv_result

Yes, I like that

adamleff · 2017-05-31T19:05:21Z

lib/resources/oracledb_session.rb

+      elsif inspec.command(@sqlplus_bin).exist?
+        bin = @sqlplus_bin
+        opts = "SET MARKUP HTML ON\nSET FEEDBACK OFF"
+        p = 'parse_html_result'


Same comment here as on line 50.

adamleff · 2017-05-31T19:07:21Z

lib/resources/oracledb_session.rb

+          entries = row.elements.to_a('/td')
+
+          headers.each_with_index { |header, index|
+            # TODO: once nokogiri is back, we can remove html entities


Are there plans to bring nokogiri back in? It has only caused us pain on multiple projects, especially since we support Windows. Can we just remove this TODO and continue to use htmlentities if we need it?

I don't suppose there's a non-HTML markup we can use with Oracle to avoid this?

I still try to find a way to circumvent htmlentities

adamleff · 2017-05-31T19:08:04Z

lib/resources/oracledb_session.rb

+
+          headers.each_with_index { |header, index|
+            # TODO: once nokogiri is back, we can remove html entities
+            require 'htmlentities'


Can we move this to the top of the file and treat it as first class? I'd rather throw a compile-time error on this when the resource is loaded/eval'd rather than at runtime.

adamleff · 2017-05-31T19:09:08Z

lib/utils/database_helpers.rb

+      @results.empty?
+    end
+
+    def stdout


I'm nitpicking, but this and stderr really bug me because it feels like we're exposing implementation-level details to the user. They shouldn't care that we're making command calls underneath to get the data.

Can we call these output and errors? That way if we ever change the implementation, the method names will still be truthful.

Good point. I agree on that. I am going to remove that since there is no need to expose this right now

YarNhoj · 2017-06-08T16:13:20Z

Since all we are doing is shelling out to sqlplus does this resource require that the stand alone sqlplus client be installed, or are we assuming that the user running the scans has all of the environment variables set up that sqlplus requires to run correctly?

As a non Oracle DB user I would have to set LD_LIBRARY_PATH, ORACLE_HOME, and a couple of other env variables before the sqlplus client would function correctly.

Here's a link to the sqlplus documentation from Oracle http://docs.oracle.com/database/121/SQPUG/ch_two.htm#SQPUG012

In my current use case I have an audit user that checks that seed data was populated correctly after install. This user isn't a DB user and won't have all of the env variables set.

chris-rock · 2017-06-26T04:52:53Z

@YarNhoj Right now, this expects that the sqlplus client is installed and configured. Do you see any way around this?

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

chris-rock · 2017-06-28T13:03:36Z

As discussed with @adamleff in #1857 (comment) we are going to remove stdout. Therefore this is a breaking change now.

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

adamleff · 2017-06-28T13:26:22Z

It doesn't have to be... why don't we deprecate stdout with a warning now, and we can add another item to our 2.0 depredations issue to remove it then. What do you think?

chris-rock · 2017-06-28T16:12:42Z

@adamleff great idea

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

adamleff

Just a few last cleanup things that I'm happy to do myself.

adamleff · 2017-06-29T14:51:18Z

lib/resources/mssql_session.rb

 module Inspec::Resources
+  # STABILITY: Experimental
+  # This resouce needs further testing and refinement


s/resouce/resource

adamleff · 2017-06-29T14:53:05Z

lib/resources/mssql_session.rb

+      if cmd.exit_status != 0 || out =~ /Sqlcmd: Error/
+        # TODO: we need to throw an exception here
+        # change once https://github.com/chef/inspec/issues/1205 is in
+        # require "pry"; binding.pry


Should remove this line.

adamleff · 2017-06-29T14:53:42Z

lib/resources/oracledb_session.rb


 module Inspec::Resources
+  # STABILITY: Experimental
+  # This resouce needs further testing and refinement


s/resouce/resource

adamleff · 2017-06-29T14:54:25Z

lib/resources/oracledb_session.rb

-      return skip_resource("Can't run Oracle checks without authentication") if @user.nil? or @pass.nil?
+
+      return skip_resource "Can't run Oracle checks without authentication" if @user.nil? || @password.nil?
+      return skip_resource 'You must provide at least an SID and/or a Service Name for the session' if @service.nil?


We should eliminate the reference to "SID" from this skip message since we only support service name.

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

adamleff

Sweet update, @chris-rock!

chris-rock added the in progress label May 29, 2017

chris-rock mentioned this pull request May 29, 2017

added service_name + port, fixed sqlplus connection, required either … #1827

Closed

chris-rock force-pushed the chris-rock/database branch from 6428698 to ef55e69 Compare May 31, 2017 05:21

adamleff reviewed May 31, 2017

View reviewed changes

adamleff mentioned this pull request Jun 2, 2017

RFE: postgres_session/mysql_session/mssql_session resource to handle parsing of csv returns of result sets #1672

Closed

chris-rock force-pushed the chris-rock/database branch 2 times, most recently from 76da1b5 to 8e5de65 Compare June 26, 2017 05:09

chris-rock added 6 commits June 28, 2017 13:28

improve database parsing

e95d5e2

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

support sqlcli

3032720

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

ensure headers are downcast

64e94ff

externalize database helper

2715fe9

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

use password as argument

10cdfe0

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

feedback from @adamleff

39881f0

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

chris-rock force-pushed the chris-rock/database branch from 8e5de65 to 39881f0 Compare June 28, 2017 12:51

chris-rock added the breaking-change label Jun 28, 2017

inline docs update + linting

70b9e4e

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

stay backwards compatible

d3489d8

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

chris-rock removed the breaking-change label Jun 29, 2017

chris-rock force-pushed the chris-rock/database branch 2 times, most recently from 91dff67 to 4f92019 Compare June 29, 2017 09:52

chris-rock changed the title ~~WIP: oracle_session and mssql_session improvement~~ oracle_session and mssql_session improvement Jun 29, 2017

adamleff suggested changes Jun 29, 2017

View reviewed changes

adamleff added the Type: Enhancement Improves an existing feature label Jun 29, 2017

implement tests

be42822

Signed-off-by: Christoph Hartmann <chris@lollyrock.com>

chris-rock force-pushed the chris-rock/database branch from 4f92019 to be42822 Compare June 29, 2017 15:00

adamleff approved these changes Jun 29, 2017

View reviewed changes

adamleff merged commit 0839be5 into master Jun 29, 2017

adamleff deleted the chris-rock/database branch June 29, 2017 15:01

adamleff removed the in progress label Jun 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

oracle_session and mssql_session improvement #1857

oracle_session and mssql_session improvement #1857

chris-rock commented May 29, 2017 •

edited

Loading

aaronlippold commented May 30, 2017

aaronlippold commented May 30, 2017

chris-rock commented May 30, 2017

chris-rock commented May 30, 2017

aaronlippold commented May 30, 2017 via email

adamleff left a comment

adamleff May 31, 2017

chris-rock Jun 26, 2017

adamleff May 31, 2017

adamleff May 31, 2017

chris-rock Jun 26, 2017

adamleff May 31, 2017

adamleff May 31, 2017

chris-rock Jun 28, 2017

adamleff May 31, 2017

adamleff May 31, 2017

chris-rock Jun 26, 2017

adamleff May 31, 2017

chris-rock Jun 26, 2017

adamleff May 31, 2017

chris-rock Jun 26, 2017

YarNhoj commented Jun 8, 2017

chris-rock commented Jun 26, 2017

chris-rock commented Jun 28, 2017

adamleff commented Jun 28, 2017

chris-rock commented Jun 28, 2017

adamleff left a comment

adamleff Jun 29, 2017

adamleff Jun 29, 2017

adamleff Jun 29, 2017

adamleff Jun 29, 2017

adamleff left a comment

oracle_session and mssql_session improvement #1857

oracle_session and mssql_session improvement #1857

Conversation

chris-rock commented May 29, 2017 • edited Loading

aaronlippold commented May 30, 2017

aaronlippold commented May 30, 2017

chris-rock commented May 30, 2017

chris-rock commented May 30, 2017

aaronlippold commented May 30, 2017 via email

adamleff left a 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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YarNhoj commented Jun 8, 2017

chris-rock commented Jun 26, 2017

chris-rock commented Jun 28, 2017

adamleff commented Jun 28, 2017

chris-rock commented Jun 28, 2017

adamleff left a 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

Choose a reason for hiding this comment

adamleff left a comment

Choose a reason for hiding this comment

chris-rock commented May 29, 2017 •

edited

Loading