Skip to content

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