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

Fix partial success not passing when using agent authentication #1215

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lucasvbeek
Copy link

Currently the partialSuccess variable is ignored when using agent authentication, and the client skips to the next agent key. This causes connections to fail when using an agent key + a second authentication method (like 2FA using keyboard-interactive).

With this PR, the client skips to the next authentication method instead of the next agent key when receiving a partial success.

@mscdex
Copy link
Owner

mscdex commented Sep 13, 2022

I think for behavior like this, you're better off using custom authentication handling since "partial success" could mean different things to different servers.

@lucasvbeek
Copy link
Author

Partial success is defined pretty clearly in RFC4252, and I believe this is how most clients handle it. Implementing a custom authHandler also won't help, because it's directly calling the tryNextAgentKey function

@jeanp413
Copy link

@mscdex I'm trying to replicate the same behavior as openssh client in my vscode extension and I'm already using a custom auth handler
As @lucasvbeek said this is indeed a bug as partialSuccess is being ignored

Comment on lines +366 to +378
const key = curAuth.agentCtx.currentKey();
proto.authPK(curAuth.username, key, (buf, cb) => {
curAuth.agentCtx.sign(key, buf, {}, (err, signed) => {
if (err) {
err.level = 'agent';
this.emit('error', err);
} else {
return cb(signed);
}

return tryNextAgentKey();
});
});

Choose a reason for hiding this comment

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

@lucasvbeek is this actually needed? I'm cherry picking this PR and noticed that with this USERAUTH_FAILURE gets called again, just checking for partialSuccess seems enough and I can connect successfully.

if (!partialSuccess) {
      debug && debug(`Client: Agent key #${pos + 1} failed`);
      return tryNextAgentKey();
}

Copy link
Author

Choose a reason for hiding this comment

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

Looks like it should be fine with just the check, I copied the full block from USERAUTH_PK_OK to make sure I didn't miss anything. Thanks for checking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants