Adam, you may want to look at this too. I was wondering about the location ...
10 years, 9 months ago
(2013-06-06 14:47:31 UTC)
#3
Adam, you may want to look at this too.
I was wondering about the location of the hostkey check. I think it does not any
difference, but I'd rather have a crypto expert say so.
https://codereview.appspot.com/9922043/diff/9002/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/9922043/diff/9002/ssh/client.go#newcode192 ssh/client.go:192: checker := c.config.HostKeyChecker I don't think it matters from ...
10 years, 9 months ago
(2013-06-07 16:06:55 UTC)
#4
https://codereview.appspot.com/9922043/diff/9002/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/9922043/diff/9002/ssh/client.go#newcode192
ssh/client.go:192: checker := c.config.HostKeyChecker
I don't think it matters from a security point of view. I would, personally, do
it immediately after unmarshal on the general principle that things should be
validated ASAP. In this case it would mean that we could skip the expensive
group.diffieHellman operation.
https://codereview.appspot.com/9922043/diff/9002/ssh/client.go#newcode465
ssh/client.go:465: // HostKeyChecker is called to check host keys for unexpected
// HostKeyChecker, if not nil, is called during the cryptographic handshake to
validate the server's host key. A nil HostKeyChecker implies that all host keys
are accepted.
https://codereview.appspot.com/9922043/diff/9002/ssh/client_auth.go
File ssh/client_auth.go (right):
https://codereview.appspot.com/9922043/diff/9002/ssh/client_auth.go#newcode71
ssh/client_auth.go:71: // DO NOT SUBMIT - should we pass the hostname if using
ssh.Dial?
Unless the hostname is passed then it's difficult to have much security.
Consider a user that ssh'es to two machines: a and b. If b can compromise a DNS
lookup to a then they can inject their own IP address. Then the user, who tries
to connect to a, will actually connect to b and the HostKeyChecker will think
it's ok because it'll be b's address and key.
10 years, 9 months ago
(2013-06-07 16:51:54 UTC)
#5
https://codereview.appspot.com/9922043/diff/9002/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/9922043/diff/9002/ssh/client.go#newcode192
ssh/client.go:192: checker := c.config.HostKeyChecker
On 2013/06/07 16:06:55, agl1 wrote:
> I don't think it matters from a security point of view. I would, personally,
do
> it immediately after unmarshal on the general principle that things should be
> validated ASAP. In this case it would mean that we could skip the expensive
> group.diffieHellman operation.
Done.
https://codereview.appspot.com/9922043/diff/9002/ssh/client.go#newcode465
ssh/client.go:465: // HostKeyChecker is called to check host keys for unexpected
On 2013/06/07 16:06:55, agl1 wrote:
> // HostKeyChecker, if not nil, is called during the cryptographic handshake to
> validate the server's host key. A nil HostKeyChecker implies that all host
keys
> are accepted.
Done.
https://codereview.appspot.com/9922043/diff/9002/ssh/client_auth.go
File ssh/client_auth.go (right):
https://codereview.appspot.com/9922043/diff/9002/ssh/client_auth.go#newcode71
ssh/client_auth.go:71: // DO NOT SUBMIT - should we pass the hostname if using
ssh.Dial?
On 2013/06/07 16:06:55, agl1 wrote:
> Unless the hostname is passed then it's difficult to have much security.
> Consider a user that ssh'es to two machines: a and b. If b can compromise a
DNS
> lookup to a then they can inject their own IP address. Then the user, who
tries
> to connect to a, will actually connect to b and the HostKeyChecker will think
> it's ok because it'll be b's address and key.
I'm now inserting the original argument of the Dial function, which I hope is
better.
On Fri, Jun 7, 2013 at 6:51 PM, <hanwen@google.com> wrote: >> to connect to a, ...
10 years, 9 months ago
(2013-06-08 10:26:02 UTC)
#6
On Fri, Jun 7, 2013 at 6:51 PM, <hanwen@google.com> wrote:
>> to connect to a, will actually connect to b and the HostKeyChecker
>
> will think
>>
>> it's ok because it'll be b's address and key.
>
>
> I'm now inserting the original argument of the Dial function, which I
> hope is better.
>
> https://codereview.appspot.com/9922043/
I now wonder: would it be better to pass both the original adress and
the connection, so you could check both hostname and IP address?
--
Han-Wen Nienhuys
Google Munich
hanwen@google.com
I've changed the checker function so it takes both string address and the net.Addr https://codereview.appspot.com/9922043/diff/3001/ssh/client.go ...
10 years, 9 months ago
(2013-06-10 17:12:38 UTC)
#8
I've changed the checker function so it takes both string address and the
net.Addr
https://codereview.appspot.com/9922043/diff/3001/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/9922043/diff/3001/ssh/client.go#newcode192
ssh/client.go:192: checker := c.config.HostKeyChecker
On 2013/06/10 01:15:06, dfc wrote:
> if checker := c.config.HostKeyChecker; checker != nil {
> if err := checker.Check(c.transport.RemoteAddr(), hostKeyAlgo, ... ); err
!=
> nil {
> return nil, nil, err
> }
> }
Done.
https://codereview.appspot.com/9922043/diff/15001/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/9922043/diff/15001/ssh/client.go#newcode33
ssh/client.go:33: serverAddress string
On 2013/06/10 01:15:06, dfc wrote:
> I think this should go into *ClientConfig
Why?
If it's in ClientConfig, it suggests that setting it affects what is dialed.
Is it assumed that most people use Client() or Dial() to setup a connection?
People that use Client() can still check the address by instantiating a special
HostKeyChecker for each connection. For that case, a callback rather than
interface may be a little more convenient.
https://codereview.appspot.com/9922043/diff/23001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go (left): https://codereview.appspot.com/9922043/diff/23001/ssh/test/test_unix_test.go#oldcode168 ssh/test/test_unix_test.go:168: // from that that package provide. why has all ...
10 years, 9 months ago
(2013-06-11 10:52:30 UTC)
#9
On 2013/06/10 17:12:38, hanwen wrote: > I've changed the checker function so it takes both ...
10 years, 9 months ago
(2013-06-11 10:55:38 UTC)
#10
On 2013/06/10 17:12:38, hanwen wrote:
> I've changed the checker function so it takes both string address and the
> net.Addr
>
> https://codereview.appspot.com/9922043/diff/3001/ssh/client.go
> File ssh/client.go (right):
>
> https://codereview.appspot.com/9922043/diff/3001/ssh/client.go#newcode192
> ssh/client.go:192: checker := c.config.HostKeyChecker
> On 2013/06/10 01:15:06, dfc wrote:
> > if checker := c.config.HostKeyChecker; checker != nil {
> > if err := checker.Check(c.transport.RemoteAddr(), hostKeyAlgo, ... ); err
> !=
> > nil {
> > return nil, nil, err
> > }
> > }
>
> Done.
>
> https://codereview.appspot.com/9922043/diff/15001/ssh/client.go
> File ssh/client.go (right):
>
> https://codereview.appspot.com/9922043/diff/15001/ssh/client.go#newcode33
> ssh/client.go:33: serverAddress string
> On 2013/06/10 01:15:06, dfc wrote:
> > I think this should go into *ClientConfig
>
> Why?
>
> If it's in ClientConfig, it suggests that setting it affects what is dialed.
Yes, that sounds correct.
> Is it assumed that most people use Client() or Dial() to setup a connection?
Yes, how else would they create a connection. You either have a net.Conn or an
address, as a string.
> People that use Client() can still check the address by instantiating a
special
> HostKeyChecker for each connection. For that case, a callback rather than
> interface may be a little more convenient.
It is assumed that people will be creating a new *ClientConfig for each dial as
they will be passing connection specific information like their username.
On Tue, Jun 11, 2013 at 12:55 PM, <dave@cheney.net> wrote: >> Is it assumed that ...
10 years, 9 months ago
(2013-06-11 12:24:35 UTC)
#11
On Tue, Jun 11, 2013 at 12:55 PM, <dave@cheney.net> wrote:
>> Is it assumed that most people use Client() or Dial() to setup a
>
> connection?
>
> Yes, how else would they create a connection. You either have a net.Conn
> or an address, as a string.
Sorry - I meant to say, is Client() the preferred interface, or rather Dial() ?
>> People that use Client() can still check the address by instantiating
>
> a special
>>
>> HostKeyChecker for each connection. For that case, a callback rather
>
> than
>>
>> interface may be a little more convenient.
>
>
> It is assumed that people will be creating a new *ClientConfig for each
> dial as they will be passing connection specific information like their
> username.
Fair enough; I didn't realize that. I'll have another look at this
from that perspective. Isn't it somewhat inconsistent that the address
to dial is an argument to Dial rather than a field in ClientConfig,
though?
--
Google Germany GmbH - ABC-Str. 19 - 20354 Hamburg
Registergericht und -nummer: Hamburg, HRB 86891
Sitz der Gesellschaft: Hamburg - Geschäftsführer: Graham Law, Katherine
Stephens
> Fair enough; I didn't realize that. I'll have another look at this > from ...
10 years, 9 months ago
(2013-06-11 12:56:50 UTC)
#12
> Fair enough; I didn't realize that. I'll have another look at this
> from that perspective. Isn't it somewhat inconsistent that the address
> to dial is an argument to Dial rather than a field in ClientConfig,
> though?
In retrospect maybe, but I think (not sure if agl agrees) that the ssh
package follows the lead of the crypto/tls package. So if you can find
an example of that way that package does things, that is a sound
argument to implement it in the ssh package.
PTAL moved the unresolved address into Config, and fixed the merge snafu. https://codereview.appspot.com/9922043/diff/23001/ssh/test/test_unix_test.go File ssh/test/test_unix_test.go ...
10 years, 9 months ago
(2013-06-11 13:44:12 UTC)
#13
https://codereview.appspot.com/9922043/diff/42001/ssh/client.go File ssh/client.go (right): https://codereview.appspot.com/9922043/diff/42001/ssh/client.go#newcode475 ssh/client.go:475: DialAddress string I'm sorry, one last thing. The form ...
10 years, 9 months ago
(2013-06-11 14:14:49 UTC)
#14
10 years, 9 months ago
(2013-06-11 15:02:53 UTC)
#15
https://codereview.appspot.com/9922043/diff/42001/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/9922043/diff/42001/ssh/client.go#newcode475
ssh/client.go:475: DialAddress string
On 2013/06/11 14:14:49, dfc wrote:
> I'm sorry, one last thing. The form of the comments should be
>
> // DialAddress is the original address ...
>
I fixed the form
> If I leave this empty, and HostKeyChecker empty, will the Client/Dial
functions
> still operate as before ? If so, please note that in the comment as well as
note
> that if HostKeyChecker is used, DialAddress should also be set.
That is up to the HostKeyChecker (which receives a pointer to the Config) to
implement. The current test does not look at the server address at all, for
example.
I tried tweaking the comment a bit, but not sure if it got any clearer.
Suggestions?
https://codereview.appspot.com/9922043/diff/42001/ssh/test/session_test.go
File ssh/test/session_test.go (right):
https://codereview.appspot.com/9922043/diff/42001/ssh/test/session_test.go#ne...
ssh/test/session_test.go:52: }
On 2013/06/11 14:14:49, dfc wrote:
> can you please add tests for the other permutations
>
> HostKeyChecker set, and DialAddress set
> HostKeyChecker not set, and DialAddress set
> HostKetChecker not set, and DialAddress not set
>
> by my reading, the last two should be noops.
the test doesn't address this part at all, primarily because SSHD is started
with -i, so the client is not really talking to a network address. Since we're
not even running ssh.Dial, we wouldn't be testing if it is set correctly in the
normal use case.
We could mock things out more, but IMO the test should
1. generate a RSA key
2. run sshd with that key as authorized key on a TCP/IP port
3. use ssh.Dial("localhost:PORT") to setup the connection
then we'd have a more representative test of real-life usage. I'm happy to take
this on, but I'd rather do it in a separate CL.
My main concern is this change does not break existing code. ie, if there is ...
10 years, 9 months ago
(2013-06-12 02:28:02 UTC)
#16
My main concern is this change does not break existing code. ie, if there is
existing code that does not set these two fields, will it continue to work. If
the answer is yes, the existing code will continue to work, then LGTM.
The default is a nil HostKeyChecker, which is equivalent to what we have today. On ...
10 years, 9 months ago
(2013-06-12 09:09:34 UTC)
#17
The default is a nil HostKeyChecker, which is equivalent to what we have today.
On Wed, Jun 12, 2013 at 4:28 AM, <dave@cheney.net> wrote:
> My main concern is this change does not break existing code. ie, if
> there is existing code that does not set these two fields, will it
> continue to work. If the answer is yes, the existing code will continue
> to work, then LGTM.
>
> https://codereview.appspot.com/9922043/
--
Han-Wen Nienhuys
Google Munich
hanwen@google.com
LGTM in that case. On Wed, Jun 12, 2013 at 7:09 PM, Han-Wen Nienhuys <hanwen@google.com> ...
10 years, 9 months ago
(2013-06-12 09:20:21 UTC)
#18
LGTM in that case.
On Wed, Jun 12, 2013 at 7:09 PM, Han-Wen Nienhuys <hanwen@google.com> wrote:
> The default is a nil HostKeyChecker, which is equivalent to what we have
today.
>
> On Wed, Jun 12, 2013 at 4:28 AM, <dave@cheney.net> wrote:
>> My main concern is this change does not break existing code. ie, if
>> there is existing code that does not set these two fields, will it
>> continue to work. If the answer is yes, the existing code will continue
>> to work, then LGTM.
>>
>> https://codereview.appspot.com/9922043/
>
>
>
> --
> Han-Wen Nienhuys
> Google Munich
> hanwen@google.com
10 years, 9 months ago
(2013-06-17 13:05:03 UTC)
#20
https://codereview.appspot.com/9922043/diff/48001/ssh/client.go
File ssh/client.go (right):
https://codereview.appspot.com/9922043/diff/48001/ssh/client.go#newcode444
ssh/client.go:444: config.DialAddress = addr
On 2013/06/14 19:32:03, agl1 wrote:
> The config argument cannot be mutated like this. It may be used, concurrently,
> by many connections.
I guess that brings up back to an earlier version of this patch.
(Is there much use for for concurrent use, though? Each connection would connect
as the same username, and the SSH channel system would be more efficient for
that use-case.)
I really planned to get to this today, honest. It's not quite ready to land, ...
10 years, 9 months ago
(2013-06-20 22:26:11 UTC)
#23
I really planned to get to this today, honest.
It's not quite ready to land, but I'll probably just fix the nits before
submitting to save a round trip. Tomorrow hopefully.
Issue 9922043: code review 9922043: go.crypto/ssh: add hook for host key checking.
(Closed)
Created 10 years, 10 months ago by hanwen-google
Modified 10 years, 8 months ago
Reviewers:
Base URL:
Comments: 20