Fix to raise specific error when ssh user is not provided and root is used as default user. #466

Vasu1105 · 2019-06-07T06:14:45Z

Signed-off-by: Vasu1105 vasundhara.jagdale@msystechnologies.com

Description

When ssh user is not provided while doing Train connection it throws following error

Train::PlatformDetectionFailed (Sorry, we are unable to detect your platform)

This issue we come across when we trying to bootstrap the node using ssh connection in Chef as mentioned here chef/chef#8550

And while debugging realized that the fix should go in the Train

Following are the way to reproduce the issue using train.

t = Train.create('ssh', host: "host-name", port: 22, user: "root", key_files: "/home/msys/.ssh/id_rsa", verify_host_key: :accept_new, sudo: true)
conn = t.connection

Below is the trackback of the error

Traceback (most recent call last):
       12: from /opt/chef/embedded/bin/irb:23:in `<main>'
       11: from /opt/chef/embedded/bin/irb:23:in `load'
       10: from /opt/chef/embedded/lib/ruby/gems/2.6.0/gems/irb-1.0.0/exe/irb:11:in `<top (required)>'
        9: from (irb):2
        8: from /home/msys/workspace/train/lib/train/transports/ssh.rb:82:in `connection'
        7: from /home/msys/workspace/train/lib/train/transports/ssh.rb:238:in `create_new_connection'
        6: from /home/msys/workspace/train/lib/train/transports/ssh.rb:238:in `new'
        5: from /home/msys/workspace/train/lib/train/transports/ssh_connection.rb:53:in `initialize'
        4: from /home/msys/workspace/train/lib/train/extras/command_wrapper.rb:169:in `load'
        3: from /home/msys/workspace/train/lib/train/plugins/base_connection.rb:110:in `platform'
        2: from /home/msys/workspace/train/lib/train/platforms/detect.rb:9:in `scan'
        1: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:41:in `scan'
Train::PlatformDetectionFailed (Sorry, we are unable to detect your platform)

After giving the fix the output of the same on console:

Traceback (most recent call last):
        Traceback (most recent call last):
       16: from /home/msys/workspace/train/lib/train/transports/ssh.rb:238:in `create_new_connection'
       15: from /home/msys/workspace/train/lib/train/transports/ssh.rb:238:in `new'
       14: from /home/msys/workspace/train/lib/train/transports/ssh_connection.rb:53:in `initialize'
       13: from /home/msys/workspace/train/lib/train/extras/command_wrapper.rb:165:in `load'
       12: from /home/msys/workspace/train/lib/train/plugins/base_connection.rb:110:in `platform'
       11: from /home/msys/workspace/train/lib/train/platforms/detect.rb:9:in `scan'
       10: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:27:in `scan'
        9: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:27:in `each'
        8: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:33:in `block in scan'
        7: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:45:in `scan_children'
        6: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:45:in `each'
        5: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:46:in `block in scan_children'
        4: from /home/msys/workspace/train/lib/train/platforms/detect/scanner.rb:46:in `instance_eval'
        3: from /home/msys/workspace/train/lib/train/platforms/detect/specifications/os.rb:46:in `block in load'
        2: from /home/msys/workspace/train/lib/train/platforms/detect/helpers/os_common.rb:46:in `unix_uname_s'
        1: from /home/msys/workspace/train/lib/train/platforms/detect/helpers/os_common.rb:38:in `command_output'
Train::UserError (SSH failed: Please login as the user "azure" rather than the user "root".)

And this is the output of fix when using with knife bootstrap

bundle exec knife bootstrap <host-name> -N vj-test-ssh -c ~/workspace/chef-repo/.chef/knife.rb --ssh-identity-file ~/.ssh/id_rsa --sudo --yes
Connecting to 104.211.52.56
WARN: [SSH] PTY requested: stderr will be merged into stdout
WARN: [SSH] PTY requested: stderr will be merged into stdout
ERROR: Train::UserError: SSH failed: Please login as the user "azure" rather than the user "root".

Related Issue

chef/chef#8550

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.

zenspider

Thank you for this PR!

I'm not sure this is the right place to put this check (but I'm new, so...). I've also made some other recommendation to make the ruby more idiomatic. Finally, I think this needs tests to show how it works in both the positive and exceptional cases.

zenspider · 2019-06-10T22:23:00Z

lib/train/extras/command_wrapper.rb

@@ -162,7 +162,13 @@ class CommandWrapper
    include_options WindowsCommand

    def self.load(transport, options)
-      if transport.os.unix?
+      if transport.class == Train::Transports::SSH::Connection && !transport.connected?


I'm pretty new so I'm not sure my analysis of train itself is all that accurate... That said:

It looks to me like it is transport.os.unix? itself that is the method call that causes an actual connection to be established, so having this code before that call is ensuring that transport.connected? is false.

Thank you for this PR!

I'm not sure this is the right place to put this check (but I'm new, so...). I've also made some other recommendation to make the ruby more idiomatic. Finally, I think this needs tests to show how it works in both the positive and exceptional cases.

@zenspider I was also not sure whether this the right place to do this. So I was debugging into the code more and also wanted some input so that I get right direction. I have removed this now and fixed in the os_common.rb file where the run_command is executing when we call transport.os.unix? in command_wrapper.rb. So as this is the fix for ssh connection I am checking the class of the connection. Second as the command execution with root user for ssh connection results in standard output Please login as the user "azure" rather than the user "root" I am doing string match. This looks to be a right fix as it raises appropriate error to user on root level. But Ia m open for suggestions. I am trying to understand the existing test suite so that I can add code coverage to this.

zenspider · 2019-06-10T22:24:23Z

lib/train/extras/command_wrapper.rb

-      if transport.os.unix?
+      if transport.class == Train::Transports::SSH::Connection && !transport.connected?
+        # Need to call run_command here to get the message and show it to user while raising exception
+        msg = transport.run_command("whoami").stdout


Presuming that we are in an error state, the use of whoami is arbitrary (we might be trying to connect to windows or other systems). It might be clearer to use a command like "does_not_exist".

zenspider · 2019-06-10T22:25:12Z

lib/train/extras/command_wrapper.rb

+        msg = transport.run_command("whoami").stdout
+        # this will throw the error as
+        # Train::UserError: Ssh failed: Please login as the user "username" rather than the user "root".
+        raise Train::UserError.new("Ssh failed: #{msg}")


A better (more rubyish) way to say this is: raise Train::UserError, "Ssh failed: #{msg}"

zenspider · 2019-06-10T22:30:31Z

lib/train/extras/command_wrapper.rb

+        # this will throw the error as
+        # Train::UserError: Ssh failed: Please login as the user "username" rather than the user "root".
+        raise Train::UserError.new("Ssh failed: #{msg}")
+      elsif transport.os.unix?


So, my understanding of what this code is trying to do is to take the situation where the OS detection failed (because of auth/connection issues) but provide an error message that makes that more clear. I think it makes more sense from a design / logical flow perspective to push that down to a rescue on this method. IOW, react to the bad condition, rather than make it part of the normal checks:

def self.load(transport, options) if transport.os.unix? # ... rescue Train::PlatformDetectionFailed => e transport.valididate_connection # either raises or: raise e end

This would require valididate_connection to be an abstract method (stubbed out in the superclass and raises if not overridden) on Transport::BaseConnection and on all supported Transports, which might be out of scope for this PR (not my call).

zenspider · 2019-06-10T22:42:04Z

lib/train/transports/ssh_connection.rb

@@ -101,6 +101,11 @@ def login_command
      LoginCommand.new("ssh", args)
    end

+    def connected?
+      return false if session.nil?
+      session.exec!("whoami").match?(/Please login as the user/) ? false : true


Any time you have a conditional whose branches evaluate to straight booleans is an opportunity to remove the conditional entirely. For example:

# original return false if session.nil? session.exec!("whoami").match?(/Please login as the user/) ? false : true # abstract: return false if x.nil? x.y ? false : true # avoid `nil?`: return false if !x x.y ? false : true # combine the logic: !x || x.y ? false : true # flip the logic to put true first: !(!x || x.y) ? true : false # at this point: we don't need a conditional or the true/false: !(!x || x.y) # apply DeMorgan's Law to clean it up: !(a||b) -> !a&&!b x && !x.y # unabstract: session && !session.exec!("whoami").match?(/Please login as the user/)

The final version actually reads like what it is trying to do: "If there is a session and running whoami doesn't match 'blah'".

I'd take it one step further and not use match:

session && !session.exec!("whoami") =~ /Please login as the user/ # then push `!` down: session && session.exec!("whoami") !~ /Please login as the user/

(but that is personal preference (but also bears out in benchmarks))

zenspider · 2019-06-10T22:44:47Z

Also, instead of "Do not merge" there's a new PR type for drafts! It's in the pop-down in the button as you're submitting the PR.

Vasu1105 · 2019-06-11T05:40:01Z

Thank you for this PR!

I'm not sure this is the right place to put this check (but I'm new, so...). I've also made some other recommendation to make the ruby more idiomatic. Finally, I think this needs tests to show how it works in both the positive and exceptional cases.

Thank you @zenspider for your valuable review comments. I am also new to the train connection code so definitely will have a look at all your suggestions and if require will connect with you if there are any blockers.

Vasu1105 · 2019-06-11T05:40:20Z

Also, instead of "Do not merge" there's a new PR type for drafts! It's in the pop-down in the button as you're submitting the PR.

Good to know 👍

zenspider · 2019-06-11T07:09:30Z

@Vasu1105 btw... I'm in UTC-8 but I'm also a nocturne, so we can arrange to chat live if it is needed / helps.

Vasu1105 · 2019-06-11T07:20:20Z

@Vasu1105 btw... I'm in UTC-8 but I'm also a nocturne, so we can arrange to chat live if it is needed / helps.

Sure. Thank you.

… ssh connection and it don't have access previlages Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

zenspider suggested changes Jun 10, 2019

View reviewed changes

Vasu1105 changed the title ~~Fix wrong error message when root is used as default ssh user when not provided and if the root user don't have ssh permissions~~ Fix to raise specific error when ssh user is not provided and root is used as default user. Jun 13, 2019

Vasu1105 force-pushed the vasundhara/Fix_for_giving_right_login_user_error__to_user branch 2 times, most recently from 5fec477 to 6b271fc Compare June 14, 2019 06:19

MSYS-1047 Fix for raising error when root is used as default user for…

cb36939

… ssh connection and it don't have access previlages Signed-off-by: Vasu1105 <vasundhara.jagdale@msystechnologies.com>

Vasu1105 force-pushed the vasundhara/Fix_for_giving_right_login_user_error__to_user branch from 6b271fc to cb36939 Compare June 14, 2019 08:35

btm approved these changes Jun 26, 2019

View reviewed changes

tas50 merged commit 1306b0a into inspec:master Jun 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to raise specific error when ssh user is not provided and root is used as default user. #466

Fix to raise specific error when ssh user is not provided and root is used as default user. #466

Vasu1105 commented Jun 7, 2019 •

edited

zenspider left a comment

zenspider Jun 10, 2019

Vasu1105 Jun 11, 2019 •

edited

zenspider Jun 10, 2019

zenspider Jun 10, 2019

zenspider Jun 10, 2019

zenspider Jun 10, 2019

zenspider commented Jun 10, 2019

Vasu1105 commented Jun 11, 2019

Vasu1105 commented Jun 11, 2019 •

edited

zenspider commented Jun 11, 2019

Vasu1105 commented Jun 11, 2019

Fix to raise specific error when ssh user is not provided and root is used as default user. #466

Fix to raise specific error when ssh user is not provided and root is used as default user. #466

Conversation

Vasu1105 commented Jun 7, 2019 • edited

Description

Related Issue

Types of changes

Checklist:

zenspider left a comment

Choose a reason for hiding this comment

zenspider Jun 10, 2019

Choose a reason for hiding this comment

Vasu1105 Jun 11, 2019 • edited

Choose a reason for hiding this comment

zenspider Jun 10, 2019

Choose a reason for hiding this comment

zenspider Jun 10, 2019

Choose a reason for hiding this comment

zenspider Jun 10, 2019

Choose a reason for hiding this comment

zenspider Jun 10, 2019

Choose a reason for hiding this comment

zenspider commented Jun 10, 2019

Vasu1105 commented Jun 11, 2019

Vasu1105 commented Jun 11, 2019 • edited

zenspider commented Jun 11, 2019

Vasu1105 commented Jun 11, 2019

Vasu1105 commented Jun 7, 2019 •

edited

Vasu1105 Jun 11, 2019 •

edited

Vasu1105 commented Jun 11, 2019 •

edited