Issue 6546051: code review 6546051: go.crypto/ssh: use new error message type when authenti...
Descriptiongo.crypto/ssh: use new error message type when authentication fails.
Fixes issue 3995.
Patch Set 1 #Patch Set 2 : diff -r ad8a96e038bb http://code.google.com/p/go.crypto #Patch Set 3 : diff -r ad8a96e038bb http://code.google.com/p/go.crypto #
Total comments: 1
Patch Set 4 : diff -r ad8a96e038bb http://code.google.com/p/go.crypto #Patch Set 5 : diff -r ad8a96e038bb http://code.google.com/p/go.crypto #Patch Set 6 : diff -r ad8a96e038bb http://code.google.com/p/go.crypto #MessagesTotal messages: 13
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), I'd like you to review this change to http://code.google.com/p/go.crypto
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6546051/diff/3/ssh/common.go File ssh/common.go (right): https://codereview.appspot.com/6546051/diff/3/ssh/common.go#newcode92 ssh/common.go:92: // AuthenticationFailedError results when a client disconnects rather then "rather than attempting another authentication" Is this an error that I expect to see as a server? As a return value from Handshake? Probably worth mentioning.
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
On 2012/09/21 12:55:41, agl1 wrote:
> LGTM
NOT LGTM.
var AuthenticationFailedError = errors.New("ssh: authentication failed")
Sign in to reply to this message.
Hello agl@golang.org, dave@cheney.net (cc: golang-dev@googlegroups.com), Please take another look.
Sign in to reply to this message.
On 2012/09/21 15:42:33, kardia wrote: > Hello mailto:agl@golang.org, mailto:dave@cheney.net (cc: mailto:golang-dev@googlegroups.com), > > Please take another look. I'm not sure that you can infer msgDisconnect == AuthFailure in this case, can you add a test to expose this behaviour ?
Sign in to reply to this message.
On 2012/09/21 15:49:32, dfc wrote: > On 2012/09/21 15:42:33, kardia wrote: > > Hello mailto:agl@golang.org, mailto:dave@cheney.net (cc: > mailto:golang-dev@googlegroups.com), > > > > Please take another look. > > I'm not sure that you can infer msgDisconnect == AuthFailure in this case, can > you add a test to expose this behaviour ? I have tested this against other ssh clients, and this is the practical behavior for an operational ssh server. However, after inspecting the go ssh client code, it appears that the Client.Close() closes the TCP layer, but doesn't send the disconnect message (inspection was short, I could have missed it.) Because we are not unmarshaling the disconnect message, we also don't have access to the disconnect reason code (RFC 4253 11.1). There may be some messages, such as disconnect, that we may ALWAYs want to parse, then return that message in the UnexpectedMessageError struct. ... This change is an improvement for my own code. I would prefer to make this change now, then file an issue to think about a solution that would test the disconnect code as well. ... The changes for Client to support sending a disconnect message would probably mean sending the disconnect message in ClientConn.handshake(). Without this change, it would be difficult to test.
Sign in to reply to this message.
> I have tested this against other ssh clients, and this is the practical behavior > for an operational ssh server. > > However, after inspecting the go ssh client code, it appears that the > Client.Close() closes the TCP layer, but doesn't send the disconnect message > (inspection was short, I could have missed it.) Because we are not unmarshaling > the disconnect message, we also don't have access to the disconnect reason code > (RFC 4253 11.1). The TCP close signal can come from many sources, it may be that the client is not following the proper disconnection protocol, but I don't think that is the issue here. > This change is an improvement for my own code. I would prefer to make this > change now, then file an issue to think about a solution that would test the > disconnect code as well. > ... > > The changes for Client to support sending a disconnect message would probably > mean sending the disconnect message in ClientConn.handshake(). Without this > change, it would be difficult to test. Rereading 4253, it is arguable that our client is not doing the right thing, ie, it should be sending an SSH_DISCONNECT message before handing up. This makes me less keen to accept this patch as it patching the server to work around a problem in the client. The proper fix should be in client.go, trasport.go or common.go.
Sign in to reply to this message.
On Sat, Sep 22, 2012 at 4:57 AM, <dave@cheney.net> wrote: >> I have tested this against other ssh clients, and this is the > > practical behavior >> >> for an operational ssh server. > > >> However, after inspecting the go ssh client code, it appears that the >> Client.Close() closes the TCP layer, but doesn't send the disconnect > > message >> >> (inspection was short, I could have missed it.) Because we are not > > unmarshaling >> >> the disconnect message, we also don't have access to the disconnect > > reason code >> >> (RFC 4253 11.1). > > > The TCP close signal can come from many sources, it may be that the > client is not following the proper disconnection protocol, but I don't > think that is the issue here. > > >> This change is an improvement for my own code. I would prefer to make > > this >> >> change now, then file an issue to think about a solution that would > > test the >> >> disconnect code as well. >> ... > > >> The changes for Client to support sending a disconnect message would > > probably >> >> mean sending the disconnect message in ClientConn.handshake(). Without > > this >> >> change, it would be difficult to test. > > > Rereading 4253, it is arguable that our client is not doing the right > thing, ie, it should be sending an SSH_DISCONNECT message before handing > up. This makes me less keen to accept this patch as it patching the > server to work around a problem in the client. The proper fix should be > in client.go, trasport.go or common.go. > > https://codereview.appspot.com/6546051/ I think you are misunderstanding the problem here. This patch has nothing to do with patching the client. The only reason the Go ssh client is involved is for your request for a test. My response is, in the current state of things, that can't be done. This patch fixes a problem with the server. A non-Go SSH Client will send a disconnect message when the Go server currently tries to unmarshal an auth attempt message. This usually happens when the client runs out of authentication methods. I don't see how the code can be in a different place.
Sign in to reply to this message.
It is very probable that I don't understand the problem. Maybe if you proposed a CL that decoded the disconnect method. Sorry for the briefness of my reply, there is only a wafer of Wifi available. On Sun, Sep 23, 2012 at 12:54 AM, Daniel Theophanes <kardianos@gmail.com> wrote: > On Sat, Sep 22, 2012 at 4:57 AM, <dave@cheney.net> wrote: >>> I have tested this against other ssh clients, and this is the >> >> practical behavior >>> >>> for an operational ssh server. >> >> >>> However, after inspecting the go ssh client code, it appears that the >>> Client.Close() closes the TCP layer, but doesn't send the disconnect >> >> message >>> >>> (inspection was short, I could have missed it.) Because we are not >> >> unmarshaling >>> >>> the disconnect message, we also don't have access to the disconnect >> >> reason code >>> >>> (RFC 4253 11.1). >> >> >> The TCP close signal can come from many sources, it may be that the >> client is not following the proper disconnection protocol, but I don't >> think that is the issue here. >> >> >>> This change is an improvement for my own code. I would prefer to make >> >> this >>> >>> change now, then file an issue to think about a solution that would >> >> test the >>> >>> disconnect code as well. >>> ... >> >> >>> The changes for Client to support sending a disconnect message would >> >> probably >>> >>> mean sending the disconnect message in ClientConn.handshake(). Without >> >> this >>> >>> change, it would be difficult to test. >> >> >> Rereading 4253, it is arguable that our client is not doing the right >> thing, ie, it should be sending an SSH_DISCONNECT message before handing >> up. This makes me less keen to accept this patch as it patching the >> server to work around a problem in the client. The proper fix should be >> in client.go, trasport.go or common.go. >> >> https://codereview.appspot.com/6546051/ > > I think you are misunderstanding the problem here. This patch has > nothing to do with patching the client. The only reason the Go ssh > client is involved is for your request for a test. My response is, in > the current state of things, that can't be done. This patch fixes a > problem with the server. A non-Go SSH Client will send a disconnect > message when the Go server currently tries to unmarshal an auth > attempt message. This usually happens when the client runs out of > authentication methods. I don't see how the code can be in a > different place.
Sign in to reply to this message.
Hello, I would like to apologise for my previous comments on this issue. At best I don't understand the problem as stated so I am in a poor position to judge the proposed solution. I think that it is appropriate that I recuse myself, and leave agl to have the final decision on this proposal. Cheers Dave
Sign in to reply to this message.
Thanks Dave, I think we did cross wires, but your questioning was ultimately useful. Although I think this change by itself is incrementally fine, it feels like the proper solution might be a bit deeper then what is here right now. Still thinking, probably be back in a day or so. On 2012/10/03 11:06:23, dfc wrote: > Hello, > > I would like to apologise for my previous comments on this issue. At best I > don't understand the problem as stated so I am in a poor position to judge the > proposed solution. > > I think that it is appropriate that I recuse myself, and leave agl to have the > final decision on this proposal. > > Cheers > > Dave
Sign in to reply to this message.
|
