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

[Ignasi Barrera]

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)

[Daniel Estévez]

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)

[Daniel Estévez]

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)

[Ignasi Barrera]

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)

[Andrew Phillips]

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)

[Andrew Phillips]

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)

[Ignasi Barrera]

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)

[Ignasi Barrera]

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)

[Daniel Estévez]

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)

[Andrea Turli]

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

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

[Daniel Estévez]

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