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

x/crypto/ssh: add support for multi-step authentication #17889

Open
thinxer opened this issue Nov 11, 2016 · 12 comments
Open

x/crypto/ssh: add support for multi-step authentication #17889

thinxer opened this issue Nov 11, 2016 · 12 comments
Labels
Milestone

Comments

@thinxer
Copy link

@thinxer thinxer commented Nov 11, 2016

Previous thread here: https://groups.google.com/forum/#!topic/golang-nuts/rxrYhntkQtI.

Currently the x/crypto/ssh package uses callbacks in ServerConfig to do the autentication. Supported callbacks including "password,publickey,keyboard-interactive", and the client authenticates successfully with any of the callbacks.

This makes it impossible to implement multi-step authentication correctly. An example multi-step authentication process is to do publickey first, then keyboard-interactive to verify OTP tokens. When a client attempts to login, the server must first respond with only publickey available. When the client successfully completes the first stage, the server will respond with an authentication error with PartialSuccess set, and with the next available method keyboard-interactive. The client then knows it can continue with the second stage.

I'd propose to add a NextAuthMethodsCallback to ssh.ServerConfig. An example implementation is here: https://gist.github.com/thinxer/637acd43480174fede118704f27530a6#file-authmethods-patch.

If this change looks good, I will add tests and submit a patch for code review.

@thinxer thinxer changed the title x/crypto/ssh: does not support multi-step authentication x/crypto/ssh: add support for multi-step authentication Nov 11, 2016
@quentinmit quentinmit added this to the Unreleased milestone Nov 14, 2016
@hanwen
Copy link
Contributor

@hanwen hanwen commented Jan 9, 2017

the FR seems valid in principle. The 1st approach seems sane, but I need a code review to look at it in more detail.

@danp
Copy link
Contributor

@danp danp commented Jan 13, 2017

We are doing publickey + keyboard-interactive currently with a workaround:

We generate a ServerConfig per client with callbacks to methods on the client. Then as KeyboardInteractiveCallback and PublicKeyCallback are called we note what has succeeded so far, with either callback returning success if everything has been satisfied.

Probably would be nicer to explicitly start with publickey, get that taken care of, then explicitly move to keyboard-interactive.

@hanwen
Copy link
Contributor

@hanwen hanwen commented Jan 17, 2017

what danp is doing is OK, but nothing sets PartialSuccess in the return message. I think we could have a specialized error type that an auth callback could hand back that causes PartialSuccess to be set. Possibly that should also error should also contain the next desired auth method.

@ibuler
Copy link

@ibuler ibuler commented May 8, 2019

Any process now ?

@liuzheng
Copy link

@liuzheng liuzheng commented Sep 30, 2019

I think this is a user side question, see this solution golang/crypto#89 (comment)

@drakkan
Copy link

@drakkan drakkan commented Mar 31, 2020

I think this is a user side question, see this solution golang/crypto#89 (comment)

Hi,

based on RFC 4252, multi step authentication requires to return partial success, I don't see a partial success response in the code you linked, am I missing something? Thanks

@liuzheng
Copy link

@liuzheng liuzheng commented Apr 1, 2020

@drakkan I'm not to do that, my way is you can define those multi step authentication requires as questions, and use question checker to verify it. If verified, keep the connection, if not, close it.

WHY

there are too many authentication methods in the world, no way to support it by a frame work, so I think use that question way is better.

@drakkan
Copy link

@drakkan drakkan commented Apr 1, 2020

@drakkan I'm not to do that, my way is you can define those multi step authentication requires as questions, and use question checker to verify it. If verified, keep the connection, if not, close it.

WHY

there are too many authentication methods in the world, no way to support it by a frame work, so I think use that question way is better.

Well, a more cleaner way to ask for multiple questions is to use keyboard interactive authentication, it works well.

This issue seems focused on multi step authentication (please take a look at the forum link in the initial post). Multi step authentication, based on RFC 4252, requires to return a partial success after each step, so you are providing a workaround and not a proper solution, am I wrong?

I could be interested to send a pull request and/or to improve the existing one, but I don't want to waste my time if it cannot be merged, thanks

@liuzheng
Copy link

@liuzheng liuzheng commented Apr 1, 2020

@drakkan
Copy link

@drakkan drakkan commented Apr 9, 2020

Here is my attempt. My patch should be RFC compliant and allows to define per-user partial authentication methods.

@crazed
Copy link

@crazed crazed commented Aug 20, 2020

@drakkan thank you for your PR, do you think this will get merged soon here?

@drakkan
Copy link

@drakkan drakkan commented Aug 21, 2020

@drakkan thank you for your PR, do you think this will get merged soon here?

I don't know. This patch is included in SFTPGo 1.0.0 and allows to me to enable multi-step auth per-user (and not globally). It seems to work fine but only after a proper review I can understand if I missed something

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

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.