code review 13642043: go.crypto/ssh: introduce PublicKey interface type.

go.crypto/ssh: introduce PublicKey interface type.

Public functions affected:
-AgentKey.Key
-AgentClient.SignRequest
-ClientKeyring.Key
-MarshalPublicKey
-ParsePublicKey

[hanwen-google, 2013-09-10 11:11:53]

Hello agl@golang.org, jpsugar@google.com (cc: golang-dev@googlegroups.com,
jmpittman@google.com),

I'd like you to review this change to
https://code.google.com/p/go.crypto

[hanwen-google, 2013-09-10 11:16:10]

Hi, 

I think this is a big improvement, but some caveats:

* this will affect the public API. Is that OK ?
* I did not add a PrivateKey yet, to keep the change smaller, but I could add it
with this change. 
* When we add a PrivateKey, should we change the KeyRing interface? It would
look like

  type PrivateKey interface {
    PublicKey
    Sign(rand io.Reader, data []byte) (sig []byte, err error) 
  }

and ClientKeyRing could be just 

   type ClientKeyRing interface {
     Key(i int) (PrivateKey, error)
   }

[hanwen-google, 2013-09-10 11:28:55]

Also, I just noticed that cert based user-auth is untested.

Jonathan, could you look into a test that exercises the code in certs.go?
Without tests it is hard to do any refactoring.

[jmpittman, 2013-09-10 16:08:16]

I have not fully looked over this, but I wanted to comment on your ClientKeyRing
interface change idea.  A normal ssh-agent does not reveal private keys.  So,
having a ClientKeyRing return a PrivateKey would disallow the AgentClient to
implement ClientKeyRing.  Your previous idea about ClientKeyRing returning a
Signer is better for this case IMHO.

I can look into adding some tests for certs.  Are you wanting anything in
particular or mostly showing it working with client/server side auth?

[hanwen-google, 2013-09-10 16:28:28]

On Tue, Sep 10, 2013 at 6:08 PM,  <jmpittman@google.com> wrote:
> I have not fully looked over this, but I wanted to comment on your
> ClientKeyRing interface change idea.  A normal ssh-agent does not reveal
> private keys.  So, having a ClientKeyRing return a PrivateKey would
> disallow the AgentClient to implement ClientKeyRing.

AFAICT, AgentClient today does not implement ClientKeyRing either.

>   Your previous idea
> about ClientKeyRing returning a Signer is better for this case IMHO.

But that's just naming? The Signer would need to contain the private
key, otherwise it can't sign anything.

> I can look into adding some tests for certs.  Are you wanting anything
> in particular or mostly showing it working with client/server side auth?

Code coverage for the success case is a useful start. Right now, I can
put a panic() in the middle of the cert code, and all tests will still
pass.

-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[jpsugar, 2013-09-10 16:45:05]

Despite my nitpicks, this looks really good -- it's a fantastic improvement to
the API in my opinion.

https://codereview.appspot.com/13642043/diff/1002/ssh/client.go
File ssh/client.go (left):

https://codereview.appspot.com/13642043/diff/1002/ssh/client.go#oldcode367
ssh/client.go:367: 
Random blank line deletion?

https://codereview.appspot.com/13642043/diff/1002/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/13642043/diff/1002/ssh/client_auth.go#newcode158
ssh/client_auth.go:158: // Key returns the i'th Publickey, or nil if no key
exists at
Rewrap comment please. :-)

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go#newcode198
ssh/keys.go:198: Name() string
Please change this name to be clearer. It looks like this is some kind of "key
name", not a public key system name.

Perhaps PublicKeyAlgo()?

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go#newcode202
ssh/keys.go:202: AlgoName() string
This could be PrivateKeyAlgo().

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go#newcode214
ssh/keys.go:214: Data() interface{}
RawKey()? Data() sounds like it should return a []byte.

https://codereview.appspot.com/13642043/diff/1002/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13642043/diff/1002/ssh/server.go#newcode172
ssh/server.go:172: hostKeyBytes := s.config.rsaSerialized
Can we call this hostKeySerialized or hostKeyBytes? It's not specific to RSA.

[jmpittman, 2013-09-10 16:58:58]

On 2013/09/10 16:28:28, hanwen-google wrote:
> On Tue, Sep 10, 2013 at 6:08 PM,  <mailto:jmpittman@google.com> wrote:
> > I have not fully looked over this, but I wanted to comment on your
> > ClientKeyRing interface change idea.  A normal ssh-agent does not reveal
> > private keys.  So, having a ClientKeyRing return a PrivateKey would
> > disallow the AgentClient to implement ClientKeyRing.
> 
> AFAICT, AgentClient today does not implement ClientKeyRing either.
AgentClient does not implement ClientKeyRing directly, but it can currently be
used to implement ClientKeyRing.  There is currently an unexported
implementation in client_auth.go (look for agentKeyring).  The ClientAuthAgent
function currently uses this implementation.

> 
> >   Your previous idea
> > about ClientKeyRing returning a Signer is better for this case IMHO.
> 
> But that's just naming? The Signer would need to contain the private
> key, otherwise it can't sign anything.
> 
It is not just naming.  Returning a PrivateKey would carry an expectation that
it is the actual private key.  Returning a Signer would only carry the
expectation that the Signer has access to the private key to be used for
signing.   A Signer would carry no expectation that the private key is directly
accessible from itself.  And an ssh agent would fit this.

> > I can look into adding some tests for certs.  Are you wanting anything
> > in particular or mostly showing it working with client/server side auth?
> 
> Code coverage for the success case is a useful start. Right now, I can
> put a panic() in the middle of the cert code, and all tests will still
> pass.
> 
Understood.  We also need to add some sort of safe guard in association with
supportedHostKeyAlgos as it still only contains "ssh-rsa" and yet others get
through for client usage.  We need to add in all of the types supported in this
package and reject or prevent usage of types not supported by this package (like
v00 certs and some others).

> -- 
> Han-Wen Nienhuys
> Google Munich
> mailto:hanwen@google.com

[hanwen-google, 2013-09-10 17:08:00]

On Tue, Sep 10, 2013 at 6:58 PM,  <jmpittman@google.com> wrote:
> On 2013/09/10 16:28:28, hanwen-google wrote:
>
>> On Tue, Sep 10, 2013 at 6:08 PM,  <mailto:jmpittman@google.com> wrote:
>> > I have not fully looked over this, but I wanted to comment on your
>> > ClientKeyRing interface change idea.  A normal ssh-agent does not
>
> reveal
>>
>> > private keys.  So, having a ClientKeyRing return a PrivateKey would
>> > disallow the AgentClient to implement ClientKeyRing.
>
>
>> AFAICT, AgentClient today does not implement ClientKeyRing either.
>
> AgentClient does not implement ClientKeyRing directly, but it can
> currently be used to implement ClientKeyRing.  There is currently an
> unexported implementation in client_auth.go (look for agentKeyring).
> The ClientAuthAgent function currently uses this implementation.

Ah, right.

>> >   Your previous idea
>> > about ClientKeyRing returning a Signer is better for this case IMHO.
>
>
>> But that's just naming? The Signer would need to contain the private
>> key, otherwise it can't sign anything.
>
>
> It is not just naming.  Returning a PrivateKey would carry an
> expectation that it is the actual private key.

The interface just requires something that can sign data. You could
make a PrivateKey that carries a connection to the agent, and forwards
any Sign() call to the agent. Arguably, Signer is more Go-ish, but I
think PrivateKey conveys more information to crypto people.

>  Returning a Signer would
> only carry the expectation that the Signer has access to the private key
> to be used for signing.   A Signer would carry no expectation that the
> private key is directly accessible from itself.  And an ssh agent would
> fit this.

>> Code coverage for the success case is a useful start. Right now, I can
>> put a panic() in the middle of the cert code, and all tests will still
>> pass.
>
>
> Understood.

> We also need to add some sort of safe guard in association
> with supportedHostKeyAlgos as it still only contains "ssh-rsa" and yet
> others get through for client usage.  We need to add in all of the types
> supported in this package and reject or prevent usage of types not
> supported by this package (like v00 certs and some others).

I'll add support for proper host keys, hopefully in a next CL. The
crypto config could just carry a

  HostKeys []PrivateKey

and the PrivateKey implementations will specify what they do or do not support.

-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[hanwen-google, 2013-09-10 17:14:40]

https://codereview.appspot.com/13642043/diff/1002/ssh/client.go
File ssh/client.go (left):

https://codereview.appspot.com/13642043/diff/1002/ssh/client.go#oldcode367
ssh/client.go:367: 
On 2013/09/10 16:45:05, jpsugar wrote:
> Random blank line deletion?

Done.

https://codereview.appspot.com/13642043/diff/1002/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/13642043/diff/1002/ssh/client_auth.go#newcode158
ssh/client_auth.go:158: // Key returns the i'th Publickey, or nil if no key
exists at
On 2013/09/10 16:45:05, jpsugar wrote:
> Rewrap comment please. :-)

Done.

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go#newcode198
ssh/keys.go:198: Name() string
On 2013/09/10 16:45:05, jpsugar wrote:
> Please change this name to be clearer. It looks like this is some kind of "key
> name", not a public key system name.
> 
> Perhaps PublicKeyAlgo()?

I find the names confusing, but if what you're suggesting is correct, wouldn't
this one be the PrivateKeyAlgo, and the other the PublicKeyAlgo? (I'm not sure.)

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go#newcode214
ssh/keys.go:214: Data() interface{}
On 2013/09/10 16:45:05, jpsugar wrote:
> RawKey()? Data() sounds like it should return a []byte.

Done. Only now I noticed that I forgot to add the only caller, which is in a
test file I forgot to add.

https://codereview.appspot.com/13642043/diff/1002/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13642043/diff/1002/ssh/server.go#newcode172
ssh/server.go:172: hostKeyBytes := s.config.rsaSerialized
On 2013/09/10 16:45:05, jpsugar wrote:
> Can we call this hostKeySerialized or hostKeyBytes? It's not specific to RSA.

This part will need to be twiddled anyway, because a server can have multiple
hostkeys (one for each key algo.)

[jpsugar, 2013-09-10 17:16:31]

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go#newcode198
ssh/keys.go:198: Name() string
On 2013/09/10 17:14:40, hanwen-google wrote:
> I find the names confusing, but if what you're suggesting is correct, wouldn't
> this one be the PrivateKeyAlgo, and the other the PublicKeyAlgo? (I'm not
sure.)

Erm, quite possibly. I'm not entirely clear on the usage. Whichever one returns
a cert algo should be the PublicKeyAlgo.

[hanwen-google, 2013-09-10 17:40:26]

On Tue, Sep 10, 2013 at 7:16 PM,  <jpsugar@google.com> wrote:
>
> https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go
> File ssh/keys.go (right):
>
> https://codereview.appspot.com/13642043/diff/1002/ssh/keys.go#newcode198
> ssh/keys.go:198: Name() string
> On 2013/09/10 17:14:40, hanwen-google wrote:
>>
>> I find the names confusing, but if what you're suggesting is correct,
>
> wouldn't
>>
>> this one be the PrivateKeyAlgo, and the other the PublicKeyAlgo? (I'm
>
> not sure.)
>
> Erm, quite possibly. I'm not entirely clear on the usage. Whichever one
> returns a cert algo should be the PublicKeyAlgo.

Done.

-- 
Han-Wen Nienhuys
Google Munich
hanwen@google.com

[jpsugar, 2013-09-10 18:07:39]

LGTM

https://codereview.appspot.com/13642043/diff/26001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/13642043/diff/26001/ssh/client_auth.go#newcode227
ssh/client_auth.go:227: s := serializeSignature(key.PrivateKeyAlgo(), sign)
Is this a bug you are fixing? The original code used algoname in both places.

https://codereview.appspot.com/13642043/diff/26001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13642043/diff/26001/ssh/keys.go#newcode197
ssh/keys.go:197: // Name returns the name of the public key system
Update comment please.

https://codereview.appspot.com/13642043/diff/26001/ssh/keys.go#newcode213
ssh/keys.go:213: // Data returns the underlying object, eg. *rsa.PublicKey.
Update comment please.

[hanwen-google, 2013-09-11 11:11:44]

also added keys_test.go.

Adam, any comments?

https://codereview.appspot.com/13642043/diff/26001/ssh/client_auth.go
File ssh/client_auth.go (right):

https://codereview.appspot.com/13642043/diff/26001/ssh/client_auth.go#newcode227
ssh/client_auth.go:227: s := serializeSignature(key.PrivateKeyAlgo(), sign)
On 2013/09/10 18:07:39, jpsugar wrote:
> Is this a bug you are fixing? The original code used algoname in both places.

Done. Since the cert code has no tests, it's hard to be sure, though.

https://codereview.appspot.com/13642043/diff/26001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13642043/diff/26001/ssh/keys.go#newcode197
ssh/keys.go:197: // Name returns the name of the public key system
On 2013/09/10 18:07:39, jpsugar wrote:
> Update comment please.

Done.

https://codereview.appspot.com/13642043/diff/26001/ssh/keys.go#newcode213
ssh/keys.go:213: // Data returns the underlying object, eg. *rsa.PublicKey.
On 2013/09/10 18:07:39, jpsugar wrote:
> Update comment please.

Done.

[agl1, 2013-09-13 18:24:11]

LGTM

Changing the public API is acceptable for a good reason, and this seems like a
nice cleanup such that it hopefully won't affect many users of the package.

In the future, if there's a PrivateKey interface, but which only actually
implements signing (i.e. by talking to an agent), then I think Signer is a
better name.

[agl1, 2013-09-13 18:25:29]

*** Submitted as
https://code.google.com/p/go/source/detail?r=84f83fa76088&repo=crypto ***

go.crypto/ssh: introduce PublicKey interface type.

Public functions affected:
-AgentKey.Key
-AgentClient.SignRequest
-ClientKeyring.Key
-MarshalPublicKey
-ParsePublicKey

R=agl, jpsugar, jmpittman
CC=golang-dev
https://codereview.appspot.com/13642043

Committer: Adam Langley <agl@golang.org>

[hanwen-google, 2013-09-16 12:18:51]

*** Abandoned ***