|
|
Created:
13 years, 11 months ago by agl1 Modified:
13 years, 11 months ago Reviewers:
CC:
bradfitz, borman, dave_cheney.net, gustavo_niemeyer.net, dsymonds, r, adg, rsc, rog, lvd, kevlar, raul.san, golang-dev Visibility:
Public. |
Descriptionexp/ssh: new package.
The typical UNIX method for controlling long running process is to
send the process signals. Since this doesn't get you very far, various
ad-hoc, remote-control protocols have been used over time by programs
like Apache and BIND.
Implementing an SSH server means that Go code will have a standard,
secure way to do this in the future.
Patch Set 1 #Patch Set 2 : diff -r 1b800cd636c0 https://go.googlecode.com/hg/ #Patch Set 3 : diff -r 1b800cd636c0 https://go.googlecode.com/hg/ #Patch Set 4 : diff -r 1b800cd636c0 https://go.googlecode.com/hg/ #
Total comments: 14
Patch Set 5 : diff -r 1b800cd636c0 https://go.googlecode.com/hg/ #
Total comments: 34
Patch Set 6 : diff -r 1b800cd636c0 https://go.googlecode.com/hg/ #
Total comments: 2
Patch Set 7 : diff -r a15d18698352 https://go.googlecode.com/hg/ #
MessagesTotal messages: 39
Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg/
Sign in to reply to this message.
The package name "ssh" is misleading as this package does not implement an ssh client, only the server.
Sign in to reply to this message.
Awesome! I was looking at the old (broken) ssh.go package last night. I would be highly supportive of this package being integrated into the std lib On Fri, Sep 9, 2011 at 9:33 AM, <agl@golang.org> wrote: > Reviewers: bradfitz, > > Message: > Hello bradfitz@golang.org (cc: golang-dev@googlegroups.com), > > I'd like you to review this change to > https://go.googlecode.com/hg/ > > > Description: > net/ssh: new package. > > The typical UNIX method for controlling long running process is to > send the process signals. Since this doesn't get you very far, various > ad-hoc, remote-control protocols have been used over time by programs > like Apache and BIND. > > Implementing an SSH server means that Go code will have a standard, > secure way to do this in the future. > > Please review this at http://codereview.appspot.com/4962064/ > > Affected files: > A src/pkg/net/ssh/Makefile > A src/pkg/net/ssh/channel.go > A src/pkg/net/ssh/common.go > A src/pkg/net/ssh/doc.go > A src/pkg/net/ssh/messages.go > A src/pkg/net/ssh/messages_test.go > A src/pkg/net/ssh/server.go > A src/pkg/net/ssh/server_shell.go > A src/pkg/net/ssh/server_shell_test.go > A src/pkg/net/ssh/transport.go > > >
Sign in to reply to this message.
I'm sure the client code isn't far behind. On Fri, Sep 9, 2011 at 9:39 AM, <borman@google.com> wrote: > The package name "ssh" is misleading as this package does not implement > an ssh client, only the server. > > http://codereview.appspot.com/4962064/ >
Sign in to reply to this message.
> The package name "ssh" is misleading as this package does not implement > an ssh client, only the server. Isn't it just a matter of time? There's a difference for sure, but the protocol is pretty symmetric overall. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I never filed a patent.
Sign in to reply to this message.
I don't think this belongs in the standard library. It should live in an external repo that can be goinstalled.
Sign in to reply to this message.
On Thu, Sep 8, 2011 at 7:43 PM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: > Isn't it just a matter of time? There's a difference for sure, but > the protocol is pretty symmetric overall. I seems that the only time that I can get for Go these days is on planes and I don't have any more flights planned for the rest of the year :) But, assuming that Rob doesn't decide that I've bloated the standard library too much with this, I'm happy to review changes. The code is pretty basic as is. Cheers AGL
Sign in to reply to this message.
>> Isn't it just a matter of time? There's a difference for sure, but >> the protocol is pretty symmetric overall. > > I seems that the only time that I can get for Go these days is on > planes and I don't have any more flights planned for the rest of the > year :) Hey, I can easily fix that for you. Do you want to present Go at the Ubuntu Developer Summit in November? :-) > But, assuming that Rob doesn't decide that I've bloated the standard > library too much with this, I'm happy to review changes. The code is > pretty basic as is. It'd indeed be fantastic to have a reasonable ssh client/server implementation integrated. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I never filed a patent.
Sign in to reply to this message.
I would favor doing this in a goinstallable repository. It seems a perfect candidate. That is not in any way a criticism of the package - I think it would be great to have - but it's so self-contained and well-defined I don't see why it needs to be put into every Go installation. Goinstall is there and we should use it. -rob
Sign in to reply to this message.
I'm torn. I like goinstall but the fact that this is crypto makes me nervous about having it off somewhere else. I think as a user I'd be much more comfortable using standard library 'blessed' crypto code, rather than one of potentially n maybe incomplete implementations hosted elsewhere. By having crypto in the standard library it focuses usage and (security) eyeballs on one canonical implementation, rather than letting people compete elsewhere. Witness, for example, how many people avoid using GnuTLS (despite having a nicer API) because of the fear that it's not as audited as OpenSSL. I suppose that's an argument in either direction, but I think I'm leaning slightly towards it being in the standard library, along with the rest of the crypto. *shrug* On Thu, Sep 8, 2011 at 8:18 PM, Rob 'Commander' Pike <r@golang.org> wrote: > I would favor doing this in a goinstallable repository. It seems a > perfect candidate. > > That is not in any way a criticism of the package - I think it would > be great to have - but it's so self-contained and well-defined I don't > see why it needs to be put into every Go installation. Goinstall is > there and we should use it. > > -rob >
Sign in to reply to this message.
On 9 September 2011 13:18, Rob 'Commander' Pike <r@golang.org> wrote: > I would favor doing this in a goinstallable repository. It seems a > perfect candidate. > > That is not in any way a criticism of the package - I think it would > be great to have - but it's so self-contained and well-defined I don't > see why it needs to be put into every Go installation. Goinstall is > there and we should use it. +1 The community needs more high-quality external libraries, and this is a great candidate. Can't wait to start using it. I'm sure golang-dev is still be happy to review the code - just set up the code review extension for a separate hg repository at googlecode the same way we have done for various other external libraries. Andrew
Sign in to reply to this message.
Given Brad's argument, if it is in the standard library I would like to see it the crypto subdirectory. Do we have any crypto code that is not in the crypto directory? But I am on the side of having it external. Ideally it would be vetted/audited by some known organization. Putting it in the standard library does really resolve the auditing situation, just gives you warm fuzzies. On Thu, Sep 8, 2011 at 8:25 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > I'm torn. > > I like goinstall but the fact that this is crypto makes me nervous about > having it off somewhere else. > > I think as a user I'd be much more comfortable using standard library > 'blessed' crypto code, rather than one of potentially n maybe incomplete > implementations hosted elsewhere. By having crypto in the standard library > it focuses usage and (security) eyeballs on one canonical implementation, > rather than letting people compete elsewhere. > > Witness, for example, how many people avoid using GnuTLS (despite having a > nicer API) because of the fear that it's not as audited as OpenSSL. I > suppose that's an argument in either direction, but I think I'm leaning > slightly towards it being in the standard library, along with the rest of > the crypto. > > *shrug* > > > On Thu, Sep 8, 2011 at 8:18 PM, Rob 'Commander' Pike <r@golang.org> wrote: > >> I would favor doing this in a goinstallable repository. It seems a >> perfect candidate. >> >> That is not in any way a criticism of the package - I think it would >> be great to have - but it's so self-contained and well-defined I don't >> see why it needs to be put into every Go installation. Goinstall is >> there and we should use it. >> >> -rob >> > >
Sign in to reply to this message.
I think that having an integrated ssh facility in the standard library would be a unique* feature for Go and integrates well with the current crop of crypto libraries and the exec facility. Yes, it could just as easily be supported in an external package, but I feel that ssh, tls and http are fundamental protocols, and that should justify their inclusion in the standard library. Cheers Dave * I'm pretty sure this is true for languages like Ruby, Python, Java, C; well written 3rd party libraries exist to provide ssh (usually wrapping openssh). On Fri, Sep 9, 2011 at 1:25 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote: > I'm torn. > I like goinstall but the fact that this is crypto makes me nervous about > having it off somewhere else. > I think as a user I'd be much more comfortable using standard library > 'blessed' crypto code, rather than one of potentially n maybe incomplete > implementations hosted elsewhere. By having crypto in the standard library > it focuses usage and (security) eyeballs on one canonical implementation, > rather than letting people compete elsewhere. > Witness, for example, how many people avoid using GnuTLS (despite having a > nicer API) because of the fear that it's not as audited as OpenSSL. I > suppose that's an argument in either direction, but I think I'm leaning > slightly towards it being in the standard library, along with the rest of > the crypto. > *shrug* > > On Thu, Sep 8, 2011 at 8:18 PM, Rob 'Commander' Pike <r@golang.org> wrote: >> >> I would favor doing this in a goinstallable repository. It seems a >> perfect candidate. >> >> That is not in any way a criticism of the package - I think it would >> be great to have - but it's so self-contained and well-defined I don't >> see why it needs to be put into every Go installation. Goinstall is >> there and we should use it. >> >> -rob > >
Sign in to reply to this message.
On 9 September 2011 13:25, Brad Fitzpatrick <bradfitz@golang.org> wrote: > I'm torn. > I like goinstall but the fact that this is crypto makes me nervous about > having it off somewhere else. > I think as a user I'd be much more comfortable using standard library > 'blessed' crypto code, rather than one of potentially n maybe incomplete > implementations hosted elsewhere. By having crypto in the standard library > it focuses usage and (security) eyeballs on one canonical implementation, > rather than letting people compete elsewhere. > Witness, for example, how many people avoid using GnuTLS (despite having a > nicer API) because of the fear that it's not as audited as OpenSSL. I > suppose that's an argument in either direction, but I think I'm leaning > slightly towards it being in the standard library, along with the rest of > the crypto. > *shrug* A while back we talked about moving many of the crypto packages into a separate repository. That way they could still have the well-respected Go (and agl) seal of approval, but not bloat up the Go standard library. Of course, we'd have to keep the ones used by standard library packages around. What are those? adg:~/go/src$ grep -r '"crypto' . | grep -v "^./pkg/crypto" | awk '{ print $2 }' | sort | uniq -c 4 "crypto/md5" 6 "crypto/rand" 3 "crypto/sha1" 9 "crypto/tls" Hmm. And what are their dependencies? adg:~/go/src$ grep -r '"crypto' pkg/crypto/{md5,rand,sha1,tls} | awk '{ print $2 }' | sort | uniq "crypto" "crypto/aes" "crypto/cipher" "crypto/elliptic" "crypto/hmac" "crypto/md5" "crypto/rand" "crypto/rc4" "crypto/rsa" "crypto/sha1" "crypto/subtle" "crypto/x509" "crypto/x509/pkix" Hmm... And theirs? adg:~/go/src$ grep -r '"crypto' pkg/crypto/{md5,rand,sha1,tls} | awk '{ print $2 }' | sort | uniq | xargs -Ifn grep -r '"crypto' pkg/fn | awk '{ print $2 }' | sort | uniq "crypto" "crypto/aes" "crypto/cast5" "crypto/cipher" "crypto/dsa" "crypto/elliptic" "crypto/hmac" "crypto/md5" "crypto/openpgp/armor" "crypto/openpgp/elgamal" "crypto/openpgp/error" "crypto/openpgp/packet" "crypto/openpgp/s2k" "crypto/rand" "crypto/rc4" "crypto/rsa" "crypto/sha1" "crypto/sha256" "crypto/subtle" "crypto/x509" "crypto/x509/pkix" Does it go deeper? Maybe. The crypto packages _not_ mentioned so far: crypto/blowfish crypto/des crypto/dsa crypto/ecdsa crypto/md4 crypto/ocsp crypto/rc4 crypto/rsa crypto/sha512 crypto/twofish crypto/xtea Should those be in a separate repo? Maybe. Is it worth the extra maintenance burden, though? Unless other parts of the standard library will depend on SSH for something, I don't see why it should be included in the standard library. If users have trust issues with external repositories, we should fix that problem. (A "gopher of approval" icon on the dashboard?) Andrew
Sign in to reply to this message.
> I think that having an integrated ssh facility in the standard library > would be a unique* feature for Go and integrates well with the current > crop of crypto libraries and the exec facility. I agree.. but I know I also don't have a good argument about this. Maybe it's just that standard packages tend to be well maintained and stay up-to-date more often.. but I agree with Andrew that this is a perception (or reality) to be fixed. -- Gustavo Niemeyer http://niemeyer.net http://niemeyer.net/plus http://niemeyer.net/twitter http://niemeyer.net/blog -- I never filed a patent.
Sign in to reply to this message.
I don't think that holding up Adam's code while we figure out whether or not to build another repository is fair to Adam or to us. At some point we will have to start splitting up the library, but I don't think we're there yet, and I think the costs involved are usually overlooked or at least underestimated when suggesting it. Goinstall the reality is not yet goinstall the ideal. We can discuss that under a different subject line, not attached to this CL. I think that Adam's suggested use for this is exciting: it would be a significant boon for exactly the kinds of programs - network servers - that Go is intended for. It could easily be as important as a template system or xml. I'd like to see this move forward rather than get bogged down in discussions about placement. I propose to put this in exp/ssh for now, and we can figure out final placement once we have more experience about how important it is, just like we did with exp/draw and exp/eval. Russ
Sign in to reply to this message.
+1 achievement unlocked: reasonable compromise. > I propose to put this in exp/ssh for now, and we can > figure out final placement once we have more experience > about how important it is, just like we did with exp/draw > and exp/eval. > > Russ >
Sign in to reply to this message.
On 9 September 2011 14:24, Russ Cox <rsc@golang.org> wrote: > I propose to put this in exp/ssh for now, and we can > figure out final placement once we have more experience > about how important it is, just like we did with exp/draw > and exp/eval. SGTM. I never intended to hold up the progress of this CL.
Sign in to reply to this message.
http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/channel.go File src/pkg/net/ssh/channel.go (right): http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/channel.go#ne... src/pkg/net/ssh/channel.go:12: // A Channel is an ordered, reliable, duplux stream that is multiplexed over an s/plux/plex/ http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/channel.go#ne... src/pkg/net/ssh/channel.go:16: Confirm() os.Error s/Confirm/Accept/ ? http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server.go File src/pkg/net/ssh/server.go (right): http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server.go#new... src/pkg/net/ssh/server.go:41: // PubKeyCallback, if non-nil, is called when a client attempts public Why use callbacks here instead of interface values? It's a little awkward using the boolean to give the function two uses. What about PubKeyAuth KeyAuthenticator type KeyAuthenticator interface { AuthenticateKey(user, algo string, pubkey []byte) bool CanAuthenticateKey(algo string) bool } ? http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server.go#new... src/pkg/net/ssh/server.go:42: // key authentication. It must return true iff the given public key is s/iff/if/ http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.go File src/pkg/net/ssh/server_shell.go (right): http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.... src/pkg/net/ssh/server_shell.go:11: // ServerShell contains the state for running a VT100 terminal that is capable I wonder if this should be in a separate package, and take an io.ReadWriter instead of an ssh.Channel ? http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.... src/pkg/net/ssh/server_shell.go:372: switch req := err.(type) { This might as well be if req, ok := err.(ChannelRequest); ok { http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.... src/pkg/net/ssh/server_shell.go:392: ss.c.AckRequest(ok) If you made c an io.ReadWriter, this could be ss.c.(ssh.Channel).AckRequest(ok)
Sign in to reply to this message.
Moved to exp/ssh and have replied to comments. http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/channel.go File src/pkg/net/ssh/channel.go (right): http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/channel.go#ne... src/pkg/net/ssh/channel.go:12: // A Channel is an ordered, reliable, duplux stream that is multiplexed over an On 2011/09/09 05:38:15, adg wrote: > s/plux/plex/ Done. http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/channel.go#ne... src/pkg/net/ssh/channel.go:16: Confirm() os.Error On 2011/09/09 05:38:15, adg wrote: > s/Confirm/Accept/ ? Done. http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server.go File src/pkg/net/ssh/server.go (right): http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server.go#new... src/pkg/net/ssh/server.go:41: // PubKeyCallback, if non-nil, is called when a client attempts public On 2011/09/09 05:38:15, adg wrote: > Why use callbacks here instead of interface values? I don't feel too strongly here, but your example doesn't quite work. Both AuthenticateKey and CanAuthenticateKey would need to take the same arguments. It's not testing for algorithm compat, it's the full acceptance test, twice. Which, actually, is probably silly. It shouldn't be the package user's job to deal with this quirk of SSH. So I've rewritten it so that the Server code caches the answers to PubKeyCallback and eliminated |isQuery| http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server.go#new... src/pkg/net/ssh/server.go:42: // key authentication. It must return true iff the given public key is On 2011/09/09 05:38:15, adg wrote: > s/iff/if/ iff isn't a typo: http://en.wikipedia.org/wiki/If_and_only_if However, if iff is confusing people, I should remove it. http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.go File src/pkg/net/ssh/server_shell.go (right): http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.... src/pkg/net/ssh/server_shell.go:11: // ServerShell contains the state for running a VT100 terminal that is capable On 2011/09/09 05:38:15, adg wrote: > I wonder if this should be in a separate package, and take an io.ReadWriter > instead of an ssh.Channel ? In time it might be the basis for a readline package. However, it wouldn't ever be able to cope with just a ReadWriter. A terminal needs to be able to get metadata, like the terminal size. http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.... src/pkg/net/ssh/server_shell.go:372: switch req := err.(type) { On 2011/09/09 05:38:15, adg wrote: > This might as well be > > if req, ok := err.(ChannelRequest); ok { Done. http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server_shell.... src/pkg/net/ssh/server_shell.go:392: ss.c.AckRequest(ok) On 2011/09/09 05:38:15, adg wrote: > If you made c an io.ReadWriter, this could be > > ss.c.(ssh.Channel).AckRequest(ok) I think the API cut should be different; such that this code can be a readline like package. I'd like to leave that for the future however.
Sign in to reply to this message.
http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go File src/pkg/exp/ssh/channel.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:18: // other methods on the Channel may be called. worth saying what happens? error? panic? stone silence? http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:27: // nack is sent, no other methods on the Channel may be called. ditto http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:58: RejectResourceShortage RejectionReason = 4 as these names come from the RFC, OK. but they're fiendishly long. since you have the type to protect you, you could consider dropping or at least shortening the Reject part. (this is a detail in which Go improves over C: you can't pass integer type Foo to a function expecting integer type Bar) you can improve the const layout using iota and elide all but the first type and value. RejectAdministrativelyProhibited RejectionReason = iota+1 RejectConnectionFailed (or make value 0 something useful, if that make sense) etc. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:91: return nil nil? not the error? http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:108: return nil ditto http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:151: println("window overrun") avoid println. how about log? perhaps a custom logger that tells you it's ssh that's complaining, since the output is sure to arise in scripts if anywhere http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:274: return nil nil error again. please look at them all, or explain why this is what you want. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go File src/pkg/exp/ssh/doc.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:14: An SSH server is represented by a Server, which handles manages a number s/handles // http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:30: Once a Server has been setup, connections can be attached. s/setup/set up/ http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:52: be calling it otherwise no messages will forwarded to the channels. rewrite but at least s/it/it; / http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:55: the case of a shell, the type is 'session' and ServerShell may be used to s/'/"/g http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go File src/pkg/exp/ssh/messages.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go#... src/pkg/exp/ssh/messages.go:384: func ensure(out, o []byte, needed int) ([]byte, []byte) { experience has taught us that with recent compiler optimizations this sort of thing is roundly defeated by calls to append http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/server_shell.go File src/pkg/exp/ssh/server_shell.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/server_shell... src/pkg/exp/ssh/server_shell.go:70: if b[0] != 27 { 27 deserves a name. it's an esc, right? http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/server_shell... src/pkg/exp/ssh/server_shell.go:74: if len(b) >= 3 && b[0] == 27 && b[1] == '[' { really? we need to implement VT-100 escapes? i didn't realize this was 1971. not sure i can stomach this being in our standard repo. i'm serious
Sign in to reply to this message.
On 13 September 2011 23:04, <agl@golang.org> wrote: > Which, actually, is probably silly. It shouldn't be the package user's > job to deal with this quirk of SSH. So I've rewritten it so that the > Server code caches the answers to PubKeyCallback and eliminated > |isQuery| Even better! :-) http://codereview.appspot.com/4962064/diff/8001/src/pkg/net/ssh/server.go#new... > src/pkg/net/ssh/server.go:42: // key authentication. It must return true > iff the given public key is > On 2011/09/09 05:38:15, adg wrote: >> >> s/iff/if/ > > iff isn't a typo: http://en.wikipedia.org/wiki/If_and_only_if > > However, if iff is confusing people, I should remove it. Leave it. I just learned something. Thanks.
Sign in to reply to this message.
Looks good. A few suggestions based on the output of 8g -s -m http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go File src/pkg/exp/ssh/messages.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go#... src/pkg/exp/ssh/messages.go:563: } -s -m says that lengthBytes leaks onto the heap. func writeString(w io.Writer, s []byte) { lengthBytes := []byte{ byte(len(s) >> 24), byte(len(s) >> 16), byte(len(s) >> 8), byte(len(s)), } w.Write(lengthBytes) w.Write(s) } avoids the leak http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/transport.go File src/pkg/exp/ssh/transport.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/transport.go... src/pkg/exp/ssh/transport.go:41: var lengthBytes = make([]byte, 5) can avoid heap allocation http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/transport.go... src/pkg/exp/ssh/transport.go:60: hc.mac.Write(lengthBytes[:]) var seqNumBytes = []byte{ byte(hc.seqNum >> 24), byte(hc.seqNum >> 16), byte(hc.seqNum >> 8), byte(hc.seqNum), } hc.mac.Write(seqNumBytes) can avoid heap allocation
Sign in to reply to this message.
http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go File src/pkg/exp/ssh/messages.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go#... src/pkg/exp/ssh/messages.go:563: } that's interesting. the second form is clearer, but i don't see how the compiler can avoid allocating lengthBytes on the heap - it can't know that w.Write doesn't store its buffer somewhere else. BTW what do the -s and -m flags do? flags to 6g or 6l?
Sign in to reply to this message.
http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go File src/pkg/exp/ssh/messages.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go#... src/pkg/exp/ssh/messages.go:563: } On 2011/09/14 11:46:17, rog wrote: > that's interesting. the second form is clearer, but i don't see how the compiler > can avoid allocating lengthBytes on the heap - it can't know that w.Write > doesn't store its buffer somewhere else. Write's output buffer is probably heap allocated, but there is no reason for the source that you pass to Write to be heap allocated. I think by making a slice literal, the compiler can avoid allocating, then zeroing the slice on the heap only to pass it to Write. > BTW what do the -s and -m flags do? flags to 6g or 6l? -s turns off (I think) escape analysis, -m turns on debugging for the things which _do_ escape onto the heap. rsc can probably correct me.
Sign in to reply to this message.
On Wed, Sep 14, 2011 at 13:15, <dave@cheney.net> wrote: > Looks good. A few suggestions based on the output of 8g -s -m > > > > http://codereview.appspot.com/**4962064/diff/13001/src/pkg/** > exp/ssh/messages.go<http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go> > File src/pkg/exp/ssh/messages.go (right): > > http://codereview.appspot.com/**4962064/diff/13001/src/pkg/** > exp/ssh/messages.go#newcode563<http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go#newcode563> > src/pkg/exp/ssh/messages.go:**563: } > -s -m says that lengthBytes leaks onto the heap. > > -s /disables/ escape analysis and falls back to the older logic, which should cause /more/ heap allocations. check with just -m. > func writeString(w io.Writer, s []byte) { > lengthBytes := []byte{ > byte(len(s) >> 24), > byte(len(s) >> 16), > byte(len(s) >> 8), > byte(len(s)), > } > w.Write(lengthBytes) > w.Write(s) > } > > avoids the leak > > http://codereview.appspot.com/**4962064/diff/13001/src/pkg/** > exp/ssh/transport.go<http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/transport.go> > File src/pkg/exp/ssh/transport.go (right): > > http://codereview.appspot.com/**4962064/diff/13001/src/pkg/** > exp/ssh/transport.go#newcode41<http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/transport.go#newcode41> > src/pkg/exp/ssh/transport.go:**41: > var lengthBytes = make([]byte, 5) can avoid heap allocation > > http://codereview.appspot.com/**4962064/diff/13001/src/pkg/** > exp/ssh/transport.go#newcode60<http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/transport.go#newcode60> > src/pkg/exp/ssh/transport.go:**60: hc.mac.Write(lengthBytes[:]) > var seqNumBytes = []byte{ > byte(hc.seqNum >> 24), > byte(hc.seqNum >> 16), > byte(hc.seqNum >> 8), > byte(hc.seqNum), > } > hc.mac.Write(seqNumBytes) > > can avoid heap allocation > > > http://codereview.appspot.com/**4962064/<http://codereview.appspot.com/4962064/> >
Sign in to reply to this message.
http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go File src/pkg/exp/ssh/channel.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:18: // other methods on the Channel may be called. On 2011/09/13 17:47:26, r wrote: > worth saying what happens? error? panic? stone silence? Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:27: // nack is sent, no other methods on the Channel may be called. On 2011/09/13 17:47:26, r wrote: > ditto This was actually a confusion on my part. Other channel messages can be sent after nacking a request. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:58: RejectResourceShortage RejectionReason = 4 On 2011/09/13 17:47:26, r wrote: > RejectAdministrativelyProhibited RejectionReason = iota+1 > RejectConnectionFailed Done, and have given the names a haircut too. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:91: return nil On 2011/09/13 17:47:26, r wrote: > nil? not the error? Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:108: return nil On 2011/09/13 17:47:26, r wrote: > ditto Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:151: println("window overrun") On 2011/09/13 17:47:26, r wrote: > avoid println. how about log? perhaps a custom logger that tells you it's ssh > that's complaining, since the output is sure to arise in scripts if anywhere The println was debugging that I've left in. I've replaced it with a TODO for now since the best behaviour is to tear down the connection with a protocol error. That's what other servers will do and so we don't have to worry about gracefully handling buggy clients. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:274: return nil On 2011/09/13 17:47:26, r wrote: > nil error again. please look at them all, or explain why this is what you want. The thought at the time was that the user will find out when they loop around and Read in any case. But, if that was what I was aiming for then these functions shouldn't return an os.Error at all. I've kept them returning os.Error and replaced all instances of "return nil" in this file with returning actual errors. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go File src/pkg/exp/ssh/doc.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:14: An SSH server is represented by a Server, which handles manages a number On 2011/09/13 17:47:26, r wrote: > s/handles // Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:30: Once a Server has been setup, connections can be attached. On 2011/09/13 17:47:26, r wrote: > s/setup/set up/ Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:52: be calling it otherwise no messages will forwarded to the channels. On 2011/09/13 17:47:26, r wrote: > rewrite but at least s/it/it; / Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:55: the case of a shell, the type is 'session' and ServerShell may be used to On 2011/09/13 17:47:26, r wrote: > s/'/"/g Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go File src/pkg/exp/ssh/messages.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go#... src/pkg/exp/ssh/messages.go:384: func ensure(out, o []byte, needed int) ([]byte, []byte) { On 2011/09/13 17:47:26, r wrote: > experience has taught us that with recent compiler optimizations this sort of > thing is roundly defeated by calls to append Have removed this and used append() throughout. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/messages.go#... src/pkg/exp/ssh/messages.go:563: } On 2011/09/14 11:52:53, dfc wrote: > Write's output buffer is probably heap allocated, but there is no reason for the > source that you pass to Write to be heap allocated. I think by making a slice > literal, the compiler can avoid allocating, then zeroing the slice on the heap > only to pass it to Write. (Ignoring this for now. If someone can assertively say that form $x is better I'll change it) http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/server_shell.go File src/pkg/exp/ssh/server_shell.go (right): http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/server_shell... src/pkg/exp/ssh/server_shell.go:70: if b[0] != 27 { On 2011/09/13 17:47:26, r wrote: > 27 deserves a name. it's an esc, right? Done. http://codereview.appspot.com/4962064/diff/13001/src/pkg/exp/ssh/server_shell... src/pkg/exp/ssh/server_shell.go:74: if len(b) >= 3 && b[0] == 27 && b[1] == '[' { On 2011/09/13 17:47:26, r wrote: > really? we need to implement VT-100 escapes? i didn't realize this was 1971. not > sure i can stomach this being in our standard repo. i'm serious It's not ssh specifically: I'm afraid that basically all terminals are VT100 these days; xterm, OS X Terminal etc. Without dirtying oneself with VT100 we can't even have programs that take input from the terminal. (Unless you accept the default behaviour which doesn't even handle left and right keys.) If this makes Gopher cry then I can keep ssh in a separate repo.
Sign in to reply to this message.
On Wed, Sep 14, 2011 at 10:10, <agl@golang.org> wrote: > It's not ssh specifically: I'm afraid that basically all terminals are > VT100 these days; xterm, OS X Terminal etc. Without dirtying oneself > with VT100 we can't even have programs that take input from the > terminal. (Unless you accept the default behaviour which doesn't even > handle left and right keys.) I think this is okay. We already have XML and ASN.1 packages. Knowing the de facto standard encoding of left/right/up/down arrow keys seems okay to me. It's not like this code is implementing blinking text or even colored output. Also, this is on *input*, not output: we have to deal with what's going to be sent. Russ
Sign in to reply to this message.
Can you explain why the ssh secure network protocol has to understand cursor up and cursor down? I wasn't just complaining about its existence; I simply cannot understand how it's possible. I don't use a VT-100 and ssh works fine for me. -rob
Sign in to reply to this message.
On Wed, Sep 14, 2011 at 11:40, Rob 'Commander' Pike <r@golang.org> wrote: > Can you explain why the ssh secure network protocol has to understand > cursor up and cursor down? I wasn't just complaining about its > existence; I simply cannot understand how it's possible. I don't use a > VT-100 and ssh works fine for me. Yes, but you are an ssh client, not an ssh server. Ideas like the size of your terminal window are part of the ssh protocol, because it started as the telnet protocol; the server has to process those messages somehow. It may be possible - I haven't thought enough about it - that we can arrange an interface that the ssh server can ask of things that are trying to use the exp/ssh package to be a server and let those details be processed by the importing code, not exp/ssh itself. Russ
Sign in to reply to this message.
On Wed, Sep 14, 2011 at 11:40 AM, Rob 'Commander' Pike <r@golang.org> wrote: > Can you explain why the ssh secure network protocol has to understand > cursor up and cursor down? I wasn't just complaining about its > existence; I simply cannot understand how it's possible. I don't use a > VT-100 and ssh works fine for me. But you do use a VT-100. Terminal emulators (xterm, etc) still send cursor movements etc as VT-100 escape sequences. SSH itself just reads from the terminal and, normally, the SSH server on the other end forwards them on to bash, which handles them. In this code we don't have bash as we're not aiming to replace sshd, but to provide a way of interacting with Go programs. But we're still going to be getting VT-100 codes over the wire. As Russ mentions, it might be a plan in the future to split to server_shell.go out of ssh and make it a separate package. It could be used for interactions directly with a local terminal with a bit of work. But just having the SSH protocol without some code to handle the terminal interactions means that everyone will have to try and implement VT-100 themselves. Cheers AGL
Sign in to reply to this message.
Why is this done by the ssh server? Normally programs do this by using termcap, terminfo, or some other database (and they can support more than just ANSI). There should be a different package which is not tied to ssh that handles terminal specific stuff (and probably one that is not hard coded). I am sure virtually every program that uses the SSH protocol will also need some sort of formatted print, but that doesn't mean we should roll fmt into ssh. On Wed, Sep 14, 2011 at 9:10 AM, Adam Langley <agl@golang.org> wrote: > On Wed, Sep 14, 2011 at 11:40 AM, Rob 'Commander' Pike <r@golang.org> > wrote: > > Can you explain why the ssh secure network protocol has to understand > > cursor up and cursor down? I wasn't just complaining about its > > existence; I simply cannot understand how it's possible. I don't use a > > VT-100 and ssh works fine for me. > > But you do use a VT-100. Terminal emulators (xterm, etc) still send > cursor movements etc as VT-100 escape sequences. SSH itself just reads > from the terminal and, normally, the SSH server on the other end > forwards them on to bash, which handles them. > > In this code we don't have bash as we're not aiming to replace sshd, > but to provide a way of interacting with Go programs. But we're still > going to be getting VT-100 codes over the wire. > > As Russ mentions, it might be a plan in the future to split to > server_shell.go out of ssh and make it a separate package. It could be > used for interactions directly with a local terminal with a bit of > work. But just having the SSH protocol without some code to handle the > terminal interactions means that everyone will have to try and > implement VT-100 themselves. > > > Cheers > > AGL >
Sign in to reply to this message.
Are you trying to tell me that ssh must be a combination TTY driver and cursor-addressing handler? And is this because bash runs in raw mode now? I cry myself to sleep. -rob
Sign in to reply to this message.
I can see that the ssh server may need to expose WINCH signals, but that is unrelated to keyboard handling. Just let the keystrokes flow. The terminal handling package will take care of it. On Wed, Sep 14, 2011 at 8:46 AM, Russ Cox <rsc@golang.org> wrote: > On Wed, Sep 14, 2011 at 11:40, Rob 'Commander' Pike <r@golang.org> wrote: > > Can you explain why the ssh secure network protocol has to understand > > cursor up and cursor down? I wasn't just complaining about its > > existence; I simply cannot understand how it's possible. I don't use a > > VT-100 and ssh works fine for me. > > Yes, but you are an ssh client, not an ssh server. > > Ideas like the size of your terminal window are > part of the ssh protocol, because it started as > the telnet protocol; the server has to process those > messages somehow. It may be possible - I haven't > thought enough about it - that we can arrange an > interface that the ssh server can ask of things that > are trying to use the exp/ssh package to be a server > and let those details be processed by the importing > code, not exp/ssh itself. > > Russ >
Sign in to reply to this message.
On Wed, Sep 14, 2011 at 12:33 PM, Paul Borman <borman@google.com> wrote: > I can see that the ssh server may need to expose WINCH signals, but that is > unrelated to keyboard handling. Just let the keystrokes flow. The terminal > handling package will take care of it. We don't *have* a terminal handling package. One may grow out of this and split off from ssh. That would be a good thing. The code, as is, is just about sufficient to make a nice demo from which people can work, and that's all I was aiming for in a first version. Cheers AGL
Sign in to reply to this message.
Then I would suggest writing a separate terminal package to be used in conjunction with the ssh server package. fmt and log are also separate packages. On Wed, Sep 14, 2011 at 9:56 AM, Adam Langley <agl@golang.org> wrote: > On Wed, Sep 14, 2011 at 12:33 PM, Paul Borman <borman@google.com> wrote: > > I can see that the ssh server may need to expose WINCH signals, but that > is > > unrelated to keyboard handling. Just let the keystrokes flow. The > terminal > > handling package will take care of it. > > We don't *have* a terminal handling package. One may grow out of this > and split off from ssh. That would be a good thing. The code, as is, > is just about sufficient to make a nice demo from which people can > work, and that's all I was aiming for in a first version. > > > Cheers > > AGL >
Sign in to reply to this message.
http://codereview.appspot.com/4962064/diff/24001/src/pkg/exp/ssh/channel.go File src/pkg/exp/ssh/channel.go (right): http://codereview.appspot.com/4962064/diff/24001/src/pkg/exp/ssh/channel.go#n... src/pkg/exp/ssh/channel.go:23: Read(data []byte) (int, os.Error) I would suggest that Read ignore out-of-band data. If you had a server goroutine that was handling data, it could queue up OOB data separately. In this case, you could have something like: ReadSSH(data []byte) (n int, oob_waiting bool, err os.Error) which would be called by clients who know that they want out-of-band data and can distinguish what it is. If there was OOB data, you could return 0, true, nil, or you could return data along with it if there is some. http://codereview.appspot.com/4962064/diff/24001/src/pkg/exp/ssh/doc.go File src/pkg/exp/ssh/doc.go (right): http://codereview.appspot.com/4962064/diff/24001/src/pkg/exp/ssh/doc.go#newco... src/pkg/exp/ssh/doc.go:54: channels. This seems like odd behavior. Couldn't the server have a goroutine that's doing this in the background and queueing up data or something? It seems to me that small bugs could accidentally hang up this accepting goroutine and stall *every* channel.
Sign in to reply to this message.
On 2011/09/14 17:12:53, borman wrote: > Then I would suggest writing a separate terminal package to be used in > conjunction with the ssh server package. fmt and log are also separate > packages. > > On Wed, Sep 14, 2011 at 9:56 AM, Adam Langley <mailto:agl@golang.org> wrote: > > > On Wed, Sep 14, 2011 at 12:33 PM, Paul Borman <mailto:borman@google.com> wrote: > > > I can see that the ssh server may need to expose WINCH signals, but that > > is > > > unrelated to keyboard handling. Just let the keystrokes flow. The > > terminal > > > handling package will take care of it. > > > > We don't *have* a terminal handling package. One may grow out of this > > and split off from ssh. That would be a good thing. The code, as is, > > is just about sufficient to make a nice demo from which people can > > work, and that's all I was aiming for in a first version. > > > > > > Cheers > > > > AGL > > To have low level access at terminal in Unix, it could be used this package, Go-Term[1]. [1]: https://github.com/kless/Go-Term
Sign in to reply to this message.
*** Submitted as http://code.google.com/p/go/source/detail?r=2c36e4180313 *** exp/ssh: new package. The typical UNIX method for controlling long running process is to send the process signals. Since this doesn't get you very far, various ad-hoc, remote-control protocols have been used over time by programs like Apache and BIND. Implementing an SSH server means that Go code will have a standard, secure way to do this in the future. R=bradfitz, borman, dave, gustavo, dsymonds, r, adg, rsc, rogpeppe, lvd, kevlar, raul.san CC=golang-dev http://codereview.appspot.com/4962064
Sign in to reply to this message.
|