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

Gracefully fail `gss-with-mic` to allow fallback to next authentication #652

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@jamercee
Contributor

jamercee commented Dec 31, 2015

While researching issue #651, I discovered what I think is a bug in _parse_userauth_request(). If the authentication method is gssapi-with-mic and the ptype is not MSG_USERAUTH_GSSAPI_TOKEN, then the function raises an exception. I believe the correct response is to fail the request to allow the client to fallback to alternate methods.

I've applied and tested the patch associated with this pull request and it now works as expected.

I've also attached a copy of the Putty log to this message if you'd like to see the specifics of how it attempts authentication.

putty.txt

@bitprophet bitprophet added the Bug label Dec 31, 2015

@bitprophet

This comment has been minimized.

Member

bitprophet commented Dec 31, 2015

FTR the current behavior was added in 3e1f9f0 (via #4 / by @SebastianDeiss). If Sebastian is active I wouldn't mind a +1 from him on this since he's the original author of this part of the GSSAPI support.

Sans that input, I'd probably merge this since it's in an uncommonly-used part of the codebase and "let the client try other auth types" does seem like a more useful approach than excepting. Thanks @jamercee :)

@SebastianDeiss

This comment has been minimized.

Contributor

SebastianDeiss commented Dec 31, 2015

👍

Yes, that's definitely a bug!
Of course the client can always abort the current authentication by sending a SSH_MSG_USERAUTH_REQUEST packet, so raising an exception is not a good idea. :-)

Responding with AUTH_FAILED to allow fallback to other auth methods is the correct behavior.
Thanks for the patch @jamercee.

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 1, 2016

Thanks @SebastianDeiss !

bitprophet added a commit that referenced this pull request Jan 1, 2016

@bitprophet

This comment has been minimized.

Member

bitprophet commented Jan 1, 2016

Grumble grumble github cherry pick grumble. This has been ported to 1.15.x and above. Thanks @jamercee :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment