5
\$\begingroup\$

Here's a very small and basic script that sends commands over SSH to another computer on my network:

#!/bin/python
import sys, os
commands = ""
for i in sys.argv[1:]:
        commands += " " + i;

if len(sys.argv) <= 1:
        os.system("sshpass -p VerySecrectPassword ssh [email protected]")
else:
        os.system("sshpass -p VerySecrectPassword ssh [email protected]" + commands)

Am I being very "Pythonic"? Is it better to use a for loop to join all the commands or the join function?

commands = " ".join(sys.argv[1:])

Personally, I think the for loop reads better, but that's just my opinion and I'm a newbie. Which is considered the better way to do things?

Furthermore:

  • Are there any other areas in this script that could be improved?
  • Is it a bad idea to store the password in the script (located at /usr/bin)?

Note: This script is stored on a computer where I am the only user (except root).

\$\endgroup\$
2

1 Answer 1

10
\$\begingroup\$

To answer your questions: Yes.

Your code could be boiled down to:

#!/bin/python
import sys
import os

os.system("sshpass -p VerySecrectPassword ssh [email protected]" + " " .join(sys.argv[1:]))

Note that I also put the imports on two lines, as recommended by PEP8, Python's official style-guide.

But I would actually re-expand it to:

#!/bin/python
import sys
import os

data = {"user": "pi",
        "host": "172.16.0.141",
        "password": "VerySecret",
        "commands": " " .join(sys.argv[1:])}

command = "sshpass -p {password} ssh {user}@{host} {commands}"
os.system(command.format(**data))

This gives you some easier ways to modify the configuration. It would also be easier to implement reading this configuration from a file now.

Note that bash (or whatever shell you use) will not care about the trailing whitespace in case commands is the empty string.

The next step would be using a ssh library, like @Simon recommended in his comment.

One way to do it would be first setting up password-less login and then using this answer and a comment within it:

#!/bin/python
import sys
import paramiko

rpi = {"username": "pi",
       "hostname": "172.16.0.141"}
command = " " .join(sys.argv[1:])}

ssh = paramiko.SSHClient()
ssh.load_system_host_keys()
ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy())
ssh.connect(**rpi)
ssh_stdin, ssh_stdout, ssh_stderr = ssh.exec_command(command)

Here the ssh.set_missing_host_key_policy(paramiko.AutoAddPolicy()) is needed because paramiko does not read the hosts file correctly.

\$\endgroup\$
3
  • \$\begingroup\$ you could also take the password from an environment variable, which is probably better than having it in the source code \$\endgroup\$
    – DanJ
    Oct 8, 2016 at 17:31
  • \$\begingroup\$ Does that work out of the box or do you have to set up the config file? \$\endgroup\$
    – Simon
    Oct 9, 2016 at 14:37
  • \$\begingroup\$ Nice. Thought it just loaded the optional config file att first i.e. .ssh/config \$\endgroup\$
    – Simon
    Oct 9, 2016 at 16:51

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge that you have read and understand our privacy policy and code of conduct.

Not the answer you're looking for? Browse other questions tagged or ask your own question.