Fixes #21731 - sudo can be used with passwords now #342

dmitri-d · 2018-04-04T22:26:57Z

No description provided.

houndci-bot · 2018-04-04T22:27:06Z

test/unit/remote_execution_provider_test.rb

+    describe 'sudo password' do
+      it 'uses the remote_execution_sudo_password on the host param' do
+        host.params['remote_execution_sudo_password'] = 'mypassword'
+        host.host_parameters << FactoryBot.create(:host_parameter, :host => host, :name => 'remote_execution_sudo_password', :value => 'mypassword')


Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
Metrics/LineLength: Line is too long. [148/80]

houndci-bot · 2018-04-04T22:27:06Z

lib/foreman_remote_execution_core/script_runner.rb


      channel = session.open_channel do |ch|
-        ch.on_data { |_, data| stdout.concat(data) }
+        ch.on_data do |ch, data|


Lint/ShadowingOuterLocalVariable: Shadowing outer local variable - ch.

houndci-bot · 2018-04-04T22:27:06Z

lib/foreman_remote_execution/engine.rb

-      require_dependency File.expand_path('../../../app/models/setting/remote_execution.rb', __FILE__) if (Setting.table_exists? rescue(false))
+      if (Setting.table_exists? rescue(false))
+        ::Setting::BLANK_ATTRS << 'remote_execution_sudo_password'
+        require_dependency File.expand_path('../../../app/models/setting/remote_execution.rb', __FILE__)


Style/ExpandPathArguments: Use expand_path('../../app/models/setting/remote_execution.rb', dir) instead of expand_path('../../../app/models/setting/remote_execution.rb', FILE).
Metrics/LineLength: Line is too long. [104/80]

houndci-bot · 2018-04-04T22:27:07Z

lib/foreman_remote_execution/engine.rb

@@ -10,7 +10,10 @@ class Engine < ::Rails::Engine


    initializer 'foreman_remote_execution.load_default_settings', :before => :load_config_initializers do
-      require_dependency File.expand_path('../../../app/models/setting/remote_execution.rb', __FILE__) if (Setting.table_exists? rescue(false))
+      if (Setting.table_exists? rescue(false))


Style/RescueModifier: Avoid using rescue in its modifier form.

houndci-bot · 2018-04-04T22:27:07Z

app/models/ssh_execution_provider.rb

@@ -4,6 +4,7 @@ def proxy_command_options(template_invocation, host)
      super.merge(:ssh_user => ssh_user(host),
                  :effective_user => effective_user(template_invocation),
                  :effective_user_method => effective_user_method(host),
+                  :sudo_password => sudo_password(host),


Style/HashSyntax: Use the new Ruby 1.9 hash syntax.

houndci-bot · 2018-04-04T22:27:07Z

app/models/setting/remote_execution.rb

@@ -31,6 +31,7 @@ def self.load_defaults
                 'remote_execution_effective_user_method',
                 nil,
                 { :collection => proc { Hash[SSHExecutionProvider::EFFECTIVE_USER_METHODS.map { |method| [method, method] }] } }),
+        self.set('remote_execution_sudo_password', N_("Sudo password"), '', N_("Sudo password"), nil, {:encrypted => true}),


Style/RedundantSelf: Redundant self detected.
Style/StringLiterals: Prefer single-quoted strings when you don't need string interpolation or special symbols.
Metrics/LineLength: Line is too long. [124/80]
Layout/SpaceInsideHashLiteralBraces: Space inside { missing.
Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.
Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
Layout/SpaceInsideHashLiteralBraces: Space inside } missing.

houndci-bot · 2018-04-05T18:16:35Z

app/models/setting/remote_execution.rb

@@ -1,5 +1,6 @@
 class Setting::RemoteExecution < Setting

+  # rubocop:disable Metrics/AbcSize


Lint/MissingCopEnableDirective: Re-enable Metrics/AbcSize cop with # rubocop:enable after disabling it.

dmitri-d · 2018-04-05T18:17:24Z

disabled Rubocop on Setting.load_defaults method

dmitri-d · 2018-04-05T20:13:21Z

rebased

iNecas · 2018-04-06T19:25:18Z

lib/foreman_remote_execution_core/script_runner.rb

@@ -213,7 +237,7 @@ def run_sync(command, stdin = nil)
          started = true
        end
      end
-      session.process(0) until started
+      session.loop(0.1) { !started || (use_sudo_password? && !password_sent) }


When testing, I got the script stuck here, because some commands were actually not using sudo (the run_sync is in rex used mostly for preparing the execution environment, while the sudo is being used just to run the actual user script.

What was the reason to actually change the original line here (as well as in run_async method?). It seems that when I reverted this particular changes, the things started working.

I wasn't sure when run_sync is used and mirrored the changes in run_async. I suppose calling session.process in a loop is sufficient, but I'm curious now why session.loop gets stuck, as it is almost the same.

I think it's not about the loop, but about the actual condition (use_sudo_password? && !password_sent)as whensudo` is not used in the command, it would never be true.

OT: why do you prefer loop(0.1) over process(0)? Performance?

I think it's not about the loop, but about the actual condition(use_sudo_password? && !password_sent)as whensudo` is not used in the command, it would never be true.

Hmm. The whole expression !started || (use_sudo_password? && !password_sent) will evaluate to false (which is when the loop is terminated) once the started flag switched to true though?

OT: why do you prefer loop(0.1) over process(0)? Performance?

It looks to me that loop makes process(0) call one more time after the loop terminates. I'm not sure about exact reasoning for this, but my guess is that this eliminates/lowers the chance of losing incoming/outgoing data (which is only received from/sent onto the wire when process is called).

adamruzicka · 2018-04-09T10:12:29Z

Would it make sense to have this also exposed in the job invocation form so the user can set the sudo password on a per-job basis the same way as we do for ssh password and key passphrase?

dmitri-d · 2018-04-09T17:14:05Z

Would it make sense to have this also exposed in the job invocation form so the user can set the sudo password on a per-job basis the same way as we do for ssh password and key passphrase?

I think it would be consistent to do so. I am curious however, why ssh password/key passphrase are exposed -- they probably don't change much (if at all) and are tied to the host?

iNecas · 2018-04-11T16:06:29Z

Although I agree that this is more of an corner case, still I think it would make sense to provide this option for some cases, where one would for some reason needed to have non-unified infrastructure.

houndci-bot · 2018-04-12T00:21:35Z

lib/foreman_remote_execution_core/polling_script_runner.rb

@@ -3,8 +3,8 @@ class PollingScriptRunner < ScriptRunner

    DEFAULT_REFRESH_INTERVAL = 60

-    def initialize(options = {})
-      super(options)
+    def initialize(options = {}, user_method)


Style/OptionalArguments: Optional arguments should appear at the end of the argument list.

houndci-bot · 2018-04-12T00:21:35Z

db/migrate/20180411160809_add_sudo_password_to_job_invocation.rb

@@ -0,0 +1,5 @@
+class AddSudoPasswordToJobInvocation < ActiveRecord::Migration[4.2]


Style/FrozenStringLiteralComment: Missing magic comment # frozen_string_literal: true.

dmitri-d · 2018-04-12T00:23:45Z

extracted sudo and su logic into dedicated classes (in preparation for http://projects.theforeman.org/issues/19130)
sudo password can be provided via job invocation form now

Have yet to update tests for the these changes.

dmitri-d · 2018-04-12T18:06:55Z

updated tests

iNecas · 2018-04-23T12:14:16Z

Rubocop is not happy

dmitri-d · 2018-04-23T18:44:22Z

rebased
fixed rubocop warning

adamruzicka · 2018-04-24T12:57:03Z

lib/foreman_remote_execution_core/script_runner.rb

+                                         options.fetch(:secrets, {}).fetch(:sudo_password, nil))
+                    elsif effective_user_method == 'su'
+                      SuUserMethod.new(options.fetch(:effective_user, nil),
+                                       options.fetch(:ssh_user, 'root'))


Why not reuse the effective_user and ssh_user extracted on lines 102 and 103?

No reason. Fixed.

adamruzicka · 2018-04-24T13:25:26Z

app/views/job_invocations/_form.html.erb

@@ -93,6 +93,7 @@
    <div class="advanced hidden">
      <%= password_f f, :password, :placeholder => '*****', :label => _('Password'), :label_help => N_('Password is stored encrypted in DB until the job finishes. For future or recurring executions, it is removed after the last execution.') %>
      <%= password_f f, :key_passphrase, :placeholder => '*****', :label => _('Private key passphrase'), :label_help => N_('Key passhprase is only applicable for SSH provider. Other providers ignore this field. <br> Passphrase is stored encrypted in DB until the job finishes. For future or recurring executions, it is removed after the last execution.') %>
+      <%= password_f f, :sudo_password, :placeholder => '*****', :label => _('Sudo password'), :label_help => N_('Key passhprase is only applicable for SSH provider. Other providers ignore this field. <br> Passphrase is stored encrypted in DB until the job finishes. For future or recurring executions, it is removed after the last execution.') %>


The label help should be different from the one for private key passphrase

Updated the label.

adamruzicka · 2018-04-24T13:37:01Z

lib/foreman_remote_execution_core/script_runner.rb

+                      SuUserMethod.new(options.fetch(:effective_user, nil),
+                                       options.fetch(:ssh_user, 'root'))
+                    else
+                      raise "effective_user_method ''#{@effective_user_method}'' not supported"


Double single quotes?

adamruzicka · 2018-04-24T13:58:26Z

lib/foreman_remote_execution_core/script_runner.rb

-        channel.on_data { |ch, data| publish_data(data, 'stdout') }
+        channel.on_data do |ch, data|
+          publish_data(data, 'stdout')
+          @user_method.on_data(data, ch)


With this change the password prompt shows up in the jobs output. Maybe it would be nice if this method consumed the output if it matches?

Added filtering out of passwords in job output

adamruzicka

The happy path works for me, left some comments inline. Once I somehow got into situation where no password was provided but it still prompted for it and the whole process hang.

dmitri-d · 2018-04-24T21:03:39Z

Once I somehow got into situation where no password was provided but it still prompted for it and the whole process hang.

It now sends the password through even if it's empty; in your case instead of hanging the script should error out almost immediately.

adamruzicka · 2018-04-30T10:48:06Z

Needs a rebase

dmitri-d · 2018-04-30T20:34:06Z

*rebased

iNecas · 2018-05-03T08:43:02Z

@adamruzicka any other comments?

adamruzicka

LGTM

adamruzicka · 2018-05-03T08:47:34Z

Needs another rebase

dmitri-d · 2018-05-03T18:42:06Z

*rebased

iNecas · 2018-05-04T07:21:22Z

Test failures unrealated, Re-tested and works well. Thanks @witlessbird

houndci-bot reviewed Apr 4, 2018

View reviewed changes

theforeman-bot added Needs testing Not yet reviewed labels Apr 4, 2018

houndci-bot reviewed Apr 5, 2018

View reviewed changes

iNecas reviewed Apr 6, 2018

View reviewed changes

houndci-bot reviewed Apr 12, 2018

View reviewed changes

dmitri-d mentioned this pull request Apr 12, 2018

Fixes #19130 - dzdo support #345

Merged

adamruzicka reviewed Apr 24, 2018

View reviewed changes

adamruzicka requested changes Apr 24, 2018

View reviewed changes

theforeman-bot added Waiting on contributor Needs re-review Needs testing and removed Not yet reviewed Waiting on contributor labels Apr 24, 2018

adamruzicka approved these changes May 3, 2018

View reviewed changes

theforeman-bot removed the Needs re-review label May 3, 2018

Fixes #21731 - sudo can be used with passwords now

554a880

iNecas merged commit e65c15a into theforeman:master May 4, 2018

		@@ -1,5 +1,6 @@
		class Setting::RemoteExecution < Setting

		# rubocop:disable Metrics/AbcSize

		@@ -0,0 +1,5 @@
		class AddSudoPasswordToJobInvocation < ActiveRecord::Migration[4.2]

Fixes #21731 - sudo can be used with passwords now #342

Fixes #21731 - sudo can be used with passwords now #342

Conversation

dmitri-d commented Apr 4, 2018

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

dmitri-d commented Apr 5, 2018

dmitri-d commented Apr 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d Apr 6, 2018 • edited

Choose a reason for hiding this comment

adamruzicka commented Apr 9, 2018

dmitri-d commented Apr 9, 2018

iNecas commented Apr 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmitri-d commented Apr 12, 2018

dmitri-d commented Apr 12, 2018

iNecas commented Apr 23, 2018

dmitri-d commented Apr 23, 2018

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

adamruzicka left a comment

Choose a reason for hiding this comment

dmitri-d commented Apr 24, 2018

adamruzicka commented Apr 30, 2018

dmitri-d commented Apr 30, 2018

iNecas commented May 3, 2018

adamruzicka left a comment

Choose a reason for hiding this comment

adamruzicka commented May 3, 2018

dmitri-d commented May 3, 2018

iNecas commented May 4, 2018

dmitri-d Apr 6, 2018 •

edited