Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow pxssh to ssh twice from the same class. #472

Closed
wants to merge 4 commits into from
Closed

Allow pxssh to ssh twice from the same class. #472

wants to merge 4 commits into from

Conversation

Red-M
Copy link
Member

@Red-M Red-M commented Mar 17, 2018

Allow ssh'ing from one machine that is remote to another remote machine form the first one.

Allow forwarding of the ssh agent socket.

…ne form the first one.

Allow forwarding of the ssh agent.
pexpect/pxssh.py Outdated
@@ -232,6 +232,7 @@ def login (self, server, username, password='', terminal_type='ansi',
password_regex=r'(?i)(?:password:)|(?:passphrase for key)',
auto_prompt_reset=True, ssh_key=None, quiet=True,
sync_multiplier=1, check_local_ip=True,
spawn_local_ssh=True,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be placed at the end of the keyword args list to maintain the argument list for anyone using this function?

pexpect/pxssh.py Outdated
raise ExceptionPxssh('private ssh key does not exist')
ssh_options = ssh_options + ' -i %s' % (ssh_key)
# Allow forwarding our SSH key to the current session
if ssh_key:

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will always evaluate to true because of if ssh_key is not None: on line 289 above and a string is considered True when evaluated this way.

>>> test="my_string"
>>> if test:
...  print("Test evaluated to True")
... else:
...  print("Test evaluated to False")
...
Test evaluated to True

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this also might explain why your code coverage decreased.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was following the convention and forgot what I was testing so this is fair enough.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually just need to do this PR again really, I've actually fixed this in #473

session to do so. Setting this option to `False` and not having an active session
will trigger an error.

Set ``ssh_key`` to `True` to force passing the current SSH authentication socket to the

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic seems incorrect. To use -A, don't you want to call the first time you ssh to your server, and it will then use that ssh authentication information if you then ssh to another remote server inside that session?

In which case, you would likely need -A in addition to your ssh_key identify file string, so the if statements down below don't make sense.

The right thing to do here might be to add a keyword argument to forward_auth=False by default, and add a -A if its set to True.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You would use this on the 2nd ssh session to pass the original key across to the 3rd session that you might want to start. This option is used instead of specifying a key file because it assumes that you have keys loaded in the ssh key agent.

If you know the key you want to use on the remote system, you'd either load it into the current agent or pass the current agent further down the line.

@Red-M
Copy link
Member Author

Red-M commented Mar 24, 2018

Please see #473

@Red-M Red-M closed this Mar 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants