You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@jclouds.apache.org by Daniel Estévez <no...@github.com> on 2018/05/11 16:56:19 UTC

[jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Passwords can contain special characters as parentheses, password parameter should be wrapped in '' to avoid this syntax errors in SSH command
You can view, comment on, or merge this pull request online at:

  https://github.com/jclouds/jclouds/pull/1205

-- Commit Summary --

  * Fixes Run SSH script for passwords with special characters (as parentheses)

-- File Changes --

    M compute/src/main/java/org/jclouds/compute/callables/RunScriptOnNodeUsingSsh.java (3)
    M compute/src/test/java/org/jclouds/compute/callables/RunScriptOnNodeUsingSshTest.java (2)

-- Patch Links --

https://github.com/jclouds/jclouds/pull/1205.patch
https://github.com/jclouds/jclouds/pull/1205.diff

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.

Curious whether we need to extend the test coverage a bit here?

> @@ -103,7 +103,7 @@ public void simpleRootTestWithSudoPassword() {
       expect(sshClient.getUsername()).andReturn("tester");
       expect(sshClient.getHostAddress()).andReturn("somewhere.example.com");
       expect(
-            sshClient.exec("sudo -S sh <<'RUN_SCRIPT_AS_ROOT_SSH'\n" + "testpassword!\n" + "echo $USER\n"
+            sshClient.exec("sudo -S sh <<'RUN_SCRIPT_AS_ROOT_SSH'\n" + "'testpassword!'\n" + "echo $USER\n"

Would it make sense to modify this test so that it actually reflects the problem we're trying to fix (e.g. space in the password)? Also, would it be worth adding a test for the additional code path (see comment above)?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#pullrequestreview-119628362

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.



> @@ -103,7 +103,7 @@ public void simpleRootTestWithSudoPassword() {
       expect(sshClient.getUsername()).andReturn("tester");
       expect(sshClient.getHostAddress()).andReturn("somewhere.example.com");
       expect(
-            sshClient.exec("sudo -S sh <<'RUN_SCRIPT_AS_ROOT_SSH'\n" + "testpassword!\n" + "echo $USER\n"
+            sshClient.exec("sudo -S sh <<'RUN_SCRIPT_AS_ROOT_SSH'\n" + "'testpassword!'\n" + "echo $USER\n"

@demobox 
That line of code https://github.com/jclouds/jclouds/pull/1205#discussion-diff-187774732R118 is already covered in simpleRootTest and testUserAddAsRoot tests

This PR adds a conflictive password for all tests https://github.com/jclouds/jclouds/pull/1207


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#discussion_r188094382

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Ignasi Barrera <no...@github.com>.
nacx commented on this pull request.



> @@ -103,7 +103,7 @@ public void simpleRootTestWithSudoPassword() {
       expect(sshClient.getUsername()).andReturn("tester");
       expect(sshClient.getHostAddress()).andReturn("somewhere.example.com");
       expect(
-            sshClient.exec("sudo -S sh <<'RUN_SCRIPT_AS_ROOT_SSH'\n" + "testpassword!\n" + "echo $USER\n"
+            sshClient.exec("sudo -S sh <<'RUN_SCRIPT_AS_ROOT_SSH'\n" + "'testpassword!'\n" + "echo $USER\n"

+1. Let's change the default value used in tests to a conflicting one. @danielestevez can you have a look at these comments and submit a PR?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#discussion_r187871056

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Daniel Estévez <no...@github.com>.
It's just a sometimes since these kind of characters can be included in password random generator

https://github.com/danielestevez/jclouds/blob/9fef6ed06b027c1525579c88b58aa9f5833687fd/core/src/main/java/org/jclouds/util/PasswordGenerator.java#L65



-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#issuecomment-388467721

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Daniel Estévez <no...@github.com>.
danielestevez commented on this pull request.



> @@ -112,7 +112,8 @@ protected ExecResponse runCommand(String command) {
    public String execAsRoot(String command) {
       if (node.getCredentials().identity.equals("root")) {
       } else if (node.getCredentials().shouldAuthenticateSudo()) {
-         command = String.format("sudo -S sh <<'%s'\n%s\n%s%s\n", MARKER, node.getCredentials().getOptionalPassword().get(), command, MARKER);
+         command = String.format("sudo -S sh <<'%s'\n'%s'\n%s%s\n", MARKER, node.getCredentials().getOptionalPassword
+               ().get(), command, MARKER);
       } else {
          command = String.format("sudo sh <<'%s'\n%s%s\n", MARKER, command, MARKER);

It shouldn't need one since the problem only happens when we submit a conflictive password

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#discussion_r188088462

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Ignasi Barrera <no...@github.com>.
Closed #1205.

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#event-1622778189

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Andrew Phillips <no...@github.com>.
demobox commented on this pull request.



> @@ -112,7 +112,8 @@ protected ExecResponse runCommand(String command) {
    public String execAsRoot(String command) {
       if (node.getCredentials().identity.equals("root")) {
       } else if (node.getCredentials().shouldAuthenticateSudo()) {
-         command = String.format("sudo -S sh <<'%s'\n%s\n%s%s\n", MARKER, node.getCredentials().getOptionalPassword().get(), command, MARKER);
+         command = String.format("sudo -S sh <<'%s'\n'%s'\n%s%s\n", MARKER, node.getCredentials().getOptionalPassword
+               ().get(), command, MARKER);
       } else {
          command = String.format("sudo sh <<'%s'\n%s%s\n", MARKER, command, MARKER);

Does this line need a similar fix?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#pullrequestreview-119628326

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Ignasi Barrera <no...@github.com>.
Cherry-picked to [2.1.x](http://git-wip-us.apache.org/repos/asf/jclouds/commit/889a7f1d).

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#issuecomment-389768687

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Ignasi Barrera <no...@github.com>.
Pushed to [master](http://git-wip-us.apache.org/repos/asf/jclouds/commit/82289948). @andreaturli I think the change is safe enough to cherry-pick it to 2.1.x, WDYT?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#issuecomment-388483896

Re: [jclouds/jclouds] Fixes Run SSH script for passwords with special characters (#1205)

Posted by Andrea Turli <no...@github.com>.
Haven't tested myself but lgtm, thanks @danielestevez

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jclouds/jclouds/pull/1205#issuecomment-388466244