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

Issue #451: Check for "password:" as prompt instead of "password" #452

Merged
merged 1 commit into from Nov 9, 2017

Conversation

Projects
None yet
4 participants
@orulz

orulz commented Oct 25, 2017

pxssh checks for 'password' as the password prompt rather than 'password:'. This breaks things when the word 'password' is in the banner - which is rather more common than you might think. I think what is happening is, pxssh is sending the password, but then seeing the word 'password' in the banner and thinking that it has failed to log in.

This extremely simple fix should work, as long as:

  1. There aren't any legitimate instances where SSH will prompt for password without a colon (I have never observed this)
  2. It still doesn't cover the case where the banner actually contains the word password followed by a colon - "password:" - handling this would require a more substantial change.

I would respectfully suggest consideration of merging this pull request unless there are legitimate known situations where the password prompt comes without a colon.

Any feedback on this?

@orulz

This comment has been minimized.

orulz commented Oct 25, 2017

I believe this pull request will address Issues #353 and #451

@takluyver

This comment has been minimized.

Member

takluyver commented Oct 26, 2017

Seems fine to me. I'll give @jquast and @MountainRider a couple of days to comment in case they know of any cases where the colon isn't there. If you don't hear anything else in a few days, ping me and I'll merge it.

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Oct 26, 2017

Wouldn't a more general solution be to add a method that allows the user to change the string used to expect the password prompt? This would avoid the possibility that changing the default would break any existing code.

@jnatschev

This comment has been minimized.

jnatschev commented Oct 28, 2017

@MountainRider

This comment has been minimized.

Contributor

MountainRider commented Oct 29, 2017

Does -q suppress banners?

-q Quiet mode. Causes most warning and diagnostic messages to be suppressed.

@orulz

This comment has been minimized.

orulz commented Oct 30, 2017

@MountainRider The confusion happens when the banner is sent by the remote host AFTER a login. Login is successful but pxssh thinks that it isn't successful because the banner contains the word "password". Even with ssh -q, the remote host will still send its banner after login, so this would not solve the problem.

@takluyver

This comment has been minimized.

Member

takluyver commented Oct 30, 2017

Configurability is fine, but it makes sense to have sensible defaults as well. This is a simple change, and I'm in favour of merging it unless we know of a situation that it will break.

@jnatschev

This comment has been minimized.

jnatschev commented Oct 30, 2017

@takluyver takluyver merged commit 3de3eee into pexpect:master Nov 9, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 88.76%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment