su - USER command execution support & change transport_options to attr_accessor #636

kvivek1115 · 2020-09-10T11:08:45Z

Description

Modify transport options whenever required.
su - USER command execution using -c require a password ie. su - USER -c 'ls;date;'
Append the su_password option if command execution required a password.

Related Issue

Fixes: #633

Types of changes

Bug fix (non-breaking change which fixes an issue)
New content (non-breaking change)
Breaking change (a content change which would break existing functionality or processes)

Checklist:

I have read the CONTRIBUTING document.

Signed-off-by: Vivek Singh vivek.singh@msystechnologies.com

- Set transport options whenever required. - su - USER command execution using -c require password - Append su_password option if command execution required password. Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

chef-expeditor · 2020-09-10T11:09:06Z

Hello vsingh-msys! Thanks for the pull request!

Here is what will happen next:

Your PR will be reviewed by the maintainers.
Possible Outcomes
a. If everything looks good, one of them will approve it, and your PR will be merged.
b. The maintainer may request follow-on work (e.g. code fix, linting, etc). We would encourage you to address this work in 2-3 business days to keep the conversation going and to get your contribution in sooner.
c. Cases exist where a PR is neither aligned to Chef InSpec's product roadmap, or something the team can own or maintain long-term. In these cases, the maintainer will provide justification and close out the PR.

Thank you for contributing!

lamont-granquist · 2020-09-14T16:01:48Z

lib/train/transports/ssh_connection.rb

-            yield(data) if block_given?
-            stdout += data
+            if data == "Password: "
+              channel.send_data "#{@transport_options[:su_password]}\n"


This should use the accessor. Also it looks like any time the data contains "Password: " the su_password will be output, even if it is not set? It also isn't clear to me why this isn't just done
in the caller by passing a block that looks for "Password: " and outputs the su_password, and why this needs to go into train.

Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

kvivek1115 · 2020-09-18T09:19:12Z

lib/train/transports/ssh_connection.rb

@@ -316,12 +316,12 @@ def execute_on_channel(cmd)
        channel.exec(cmd) do |_, success|
          abort "Couldn't execute command on SSH." unless success
          channel.on_data do |_, data|
-            yield(data) if block_given?
+            yield(data, channel) if block_given?


Now we can consume this on the train side with password the data block

require 'train' train = Train.create('ssh', host: 'node1.test', port: 22, user: 'vagrant', key_files: './.vagrant/machines/default/virtualbox/private_key', pty: true) conn = train.connection password = "PASSWORD" res = conn.run_command("su - root -c 'date;'") do |data, ch| ch.send_data("#{password}\n") if data.match?("Password:") end puts "STDOUT: #{res.stdout}" puts "STDERR: #{res.stderr}"

I've tested using the given code and it does work. I also verified that the new block parameter is optional, so any existing code that calls run_command without the channel param will not break.

This does put all of the responsibility for handling the password on the calling code.

kvivek1115 · 2020-09-18T09:19:56Z

lib/train/transports/ssh_connection.rb

            stdout += data
          end

          channel.on_extended_data do |_, _type, data|
-            yield(data) if block_given?
+            yield(data, channel) if block_given?


passed channel to block yield to maintain the parity with on_data block.

Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

kvivek1115 · 2020-09-18T11:55:09Z

lib/train/transports/ssh_connection.rb

+      if output && output.match?("su: Authentication failure")
+        raise Train::UserError.new(output, :bad_su_user_password)
+      end


Wrap the su: Authentication failure run_command error message and raise custom Train::UserError with reason bad_su_user_password

Any suggestions for class and reason naming I would be happy to discuss?

kvivek1115 · 2020-10-05T01:30:04Z

@clintoncwolfe could you please have a look? Thanks!

tas50 · 2020-10-26T17:32:22Z

@clintoncwolfe Any chance you can chime in on this change?

clintoncwolfe · 2020-10-26T19:33:38Z

lib/train/transports/ssh_connection.rb

-    attr_reader :hostname
-    attr_reader :transport_options
+    attr_reader   :hostname
+    attr_accessor :transport_options


Could you help me understand why this change is needed? I don't see anywhere in the PR what the transport_option ivar is directly changed, and if you need to change one of its elements you can do so without making it read-write.

Here's the context where the goal is to change the pty option to true later in the process:

https://github.com/chef/chef/pull/10410/files#r492238172

I had the same question, but didn't know enough about train to suggest anything better.

Does this work OK with the ssh code here that looks at the options to determine if the connection can be re-used or needs restarted?

@james-stocks As we memoinzed the connection object at chef core side https://github.com/chef/chef/blob/0a29892953b07ba1fded119f5aef364e14c7dd07/lib/chef/knife/bootstrap/train_connector.rb#L51-L59, won't be a problem here, but not sure about other impacts.

Change transport_options to attr_accessor

6dfef4a

- Set transport options whenever required. - su - USER command execution using -c require password - Append su_password option if command execution required password. Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

kvivek1115 requested a review from a team as a code owner September 10, 2020 11:08

kvivek1115 requested review from stevendanna and Schwad September 10, 2020 11:08

kvivek1115 mentioned this pull request Sep 10, 2020

handles su - USER session to perform bootstrap chef/chef#10410

Merged

10 tasks

lamont-granquist reviewed Sep 14, 2020

View reviewed changes

Vivek Singh added 3 commits September 17, 2020 10:09

on_data block added channel

a0fae09

Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

revert unused

40365c5

Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

Add block param channel to on_extended_data

3a2e01c

Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

kvivek1115 commented Sep 18, 2020

View reviewed changes

kvivek1115 changed the title ~~su - USER command execution support & add su_password to transport_options~~ su - USER command execution support & change transport_options to attr_accessor Sep 18, 2020

Wrap the su authentication failure error

e86d829

Signed-off-by: Vivek Singh <vivek.singh@msystechnologies.com>

kvivek1115 commented Sep 18, 2020

View reviewed changes

clintoncwolfe reviewed Oct 26, 2020

View reviewed changes

clintoncwolfe merged commit 40ae8f0 into inspec:master Jan 26, 2021

tas50 mentioned this pull request Feb 2, 2021

handles su - USER session to perform bootstrap chef/chef#10971

Merged

10 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

su - USER command execution support & change transport_options to attr_accessor #636

su - USER command execution support & change transport_options to attr_accessor #636

kvivek1115 commented Sep 10, 2020

chef-expeditor bot commented Sep 10, 2020

lamont-granquist Sep 14, 2020

kvivek1115 Sep 18, 2020

clintoncwolfe Oct 26, 2020

kvivek1115 Sep 18, 2020 •

edited

Loading

kvivek1115 Sep 18, 2020 •

edited

Loading

kvivek1115 commented Oct 5, 2020

tas50 commented Oct 26, 2020

clintoncwolfe Oct 26, 2020

lamont-granquist Oct 26, 2020

james-stocks Oct 27, 2020

kvivek1115 Dec 10, 2020

su - USER command execution support & change transport_options to attr_accessor #636

su - USER command execution support & change transport_options to attr_accessor #636

Conversation

kvivek1115 commented Sep 10, 2020

Description

Related Issue

Types of changes

Checklist:

chef-expeditor bot commented Sep 10, 2020

lamont-granquist Sep 14, 2020

Choose a reason for hiding this comment

kvivek1115 Sep 18, 2020

Choose a reason for hiding this comment

clintoncwolfe Oct 26, 2020

Choose a reason for hiding this comment

kvivek1115 Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

kvivek1115 Sep 18, 2020 • edited Loading

Choose a reason for hiding this comment

kvivek1115 commented Oct 5, 2020

tas50 commented Oct 26, 2020

clintoncwolfe Oct 26, 2020

Choose a reason for hiding this comment

lamont-granquist Oct 26, 2020

Choose a reason for hiding this comment

james-stocks Oct 27, 2020

Choose a reason for hiding this comment

kvivek1115 Dec 10, 2020

Choose a reason for hiding this comment

kvivek1115 Sep 18, 2020 •

edited

Loading

kvivek1115 Sep 18, 2020 •

edited

Loading