code review 13338044: go.crypto/ssh: introduce PrivateKey method.

go.crypto/ssh: introduce Signer method, an abstraction of
private keys.

[hanwen-google, 2013-09-17 16:39:52]

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

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

[jpsugar, 2013-09-17 16:57:21]

https://codereview.appspot.com/13338044/diff/6001/ssh/client_auth_test.go
File ssh/client_auth_test.go (right):

https://codereview.appspot.com/13338044/diff/6001/ssh/client_auth_test.go#new...
ssh/client_auth_test.go:120: key := clientKeychain.keys[0].PublicKey()
clientKeychain.Key(0)

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode65
ssh/server.go:65: // AddHostKey adds a private key as a host key. If an existing
host
existing host *key*

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode69
ssh/server.go:69: if k.PublicKey().PrivateKeyAlgo() ==
key.PublicKey().PrivateKeyAlgo() {
This prevents the presentation of both a certificate and a regular key at the
same time. Is that desirable?

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode289
ssh/server.go:289: sig, err := MarshaledSignature(priv, s.config.rand(), H)
MarshaledSignature takes unhashed data, doesn't it? Might want to add a note
here if KEX signs hashes instead of raw data.

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode310
ssh/server.go:310: // MarshaledSignature signs the data with the appropriate
algorithm,
MarshalSignature, perhaps, to accord with the other Marshal* functions?

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode379
ssh/server.go:379: serverKexInit.ServerHostKeyAlgos,
k.PublicKey().PrivateKeyAlgo())
Are certificate algorithms not acceptable here?

[hanwen-google, 2013-09-17 19:13:25]

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode65
ssh/server.go:65: // AddHostKey adds a private key as a host key. If an existing
host
On 2013/09/17 16:57:21, jpsugar wrote:
> existing host *key*

Done.

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode69
ssh/server.go:69: if k.PublicKey().PrivateKeyAlgo() ==
key.PublicKey().PrivateKeyAlgo() {
On 2013/09/17 16:57:21, jpsugar wrote:
> This prevents the presentation of both a certificate and a regular key at the
> same time. Is that desirable?

I'm not sure if that's even in the SSH RFC. Do you know?

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode289
ssh/server.go:289: sig, err := MarshaledSignature(priv, s.config.rand(), H)
On 2013/09/17 16:57:21, jpsugar wrote:
> MarshaledSignature takes unhashed data, doesn't it? Might want to add a note
> here if KEX signs hashes instead of raw data.

Done.

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode310
ssh/server.go:310: // MarshaledSignature signs the data with the appropriate
algorithm,
On 2013/09/17 16:57:21, jpsugar wrote:
> MarshalSignature, perhaps, to accord with the other Marshal* functions?

I would say SignAndMarshal, but I don't like method names that have "and" in
them. Suggestions?

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode379
ssh/server.go:379: serverKexInit.ServerHostKeyAlgos,
k.PublicKey().PrivateKeyAlgo())
On 2013/09/17 16:57:21, jpsugar wrote:
> Are certificate algorithms not acceptable here?

see above.

[jpsugar, 2013-09-17 19:23:06]

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/6001/ssh/server.go#newcode69
ssh/server.go:69: if k.PublicKey().PrivateKeyAlgo() ==
key.PublicKey().PrivateKeyAlgo() {
On 2013/09/17 19:13:25, hanwen-google wrote:
> On 2013/09/17 16:57:21, jpsugar wrote:
> > This prevents the presentation of both a certificate and a regular key at
the
> > same time. Is that desirable?
> 
> I'm not sure if that's even in the SSH RFC. Do you know?

Certificates as a whole are not in the RFC. I think we should support them
unless we have a compelling reason not to. It's just
s/PrivateKeyAlgo/PublicKeyAlgo/.

[jmp, 2013-09-18 04:07:50]

https://codereview.appspot.com/13338044/diff/21001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/21001/ssh/keys.go#newcode231
ssh/keys.go:231: Sign(rand io.Reader, data []byte) ([]byte, error)
I like the new interface.

I have had this thought about the Sign method for a while.  If data is being
signed by a private key, I would like to expect to have this return a *signature
with the format string already in place (or just bytes with the signature format
already serialized with the signature bytes).  From there we can marshal that in
the usual way as opposed to the separate functions that serialize the signature
bytes and add the signature format (algorithm) string.  The PrivateKey ought to
be self aware and add the signature format itself.  This would benefit an ssh
agent.  The data coming out of the agent is already formatted with the signature
format attached.  All we would have to do is unmarshal it into a *signature or
pass along the existing bytes.  Right now, we do this awkwardly awful thing of
removing the format string from what the agent gives us only so we can add it
back ourselves.

https://codereview.appspot.com/13338044/diff/21001/ssh/keys.go#newcode590
ssh/keys.go:590: // TODO(hanwen): find doc for format and implement PEM parsing
For ECDSA, this will already do what you want
http://golang.org/pkg/crypto/x509/#ParseECPrivateKey.

case "EC PRIVATE KEY":
        ec, err := x509.ParseECPrivateKey(block.Bytes)
        if err != nil {
                return nil, err
        }
        rawkey = ec

[hanwen-google, 2013-09-18 11:05:45]

On Wed, Sep 18, 2013 at 6:07 AM,  <jonathan.mark.pittman@gmail.com> wrote:
>
> https://codereview.appspot.com/13338044/diff/21001/ssh/keys.go
> File ssh/keys.go (right):
>
> https://codereview.appspot.com/13338044/diff/21001/ssh/keys.go#newcode231
> ssh/keys.go:231: Sign(rand io.Reader, data []byte) ([]byte, error)
> I like the new interface.

thanks

> I have had this thought about the Sign method for a while.  If data is
> being signed by a private key, I would like to expect to have this
> return a *signature with the format string already in place (or just
> bytes with the signature format already serialized with the signature
> bytes).

I'm not sure I agree. We'd then have to make signature type public,
expose its (de)serialization methods, and duplicate the logic to
prefix the algorithm name for every key type. Also, it would loose the
symmetry between PublicKey.Verify and PrivateKey.Sign.

> a *signature or pass along the existing bytes.  Right now, we do this
> awkwardly awful thing of removing the format string from what the agent
> gives us only so we can add it back ourselves.
>
> https://codereview.appspot.com/13338044/diff/21001/ssh/keys.go#newcode590
> ssh/keys.go:590: // TODO(hanwen): find doc for format and implement PEM
> parsing
> For ECDSA, this will already do what you want
> http://golang.org/pkg/crypto/x509/#ParseECPrivateKey.
>
> case "EC PRIVATE KEY":
>         ec, err := x509.ParseECPrivateKey(block.Bytes)
>         if err != nil {
>                 return nil, err
>         }
>         rawkey = ec

Neat. I'll look into that.

> https://codereview.appspot.com/13338044/



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

[agl1, 2013-09-18 14:11:10]

https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go#newcode227
ssh/keys.go:227: // RawKey returns the underlying object, eg. *rsa.PrivateKey.
Why support this? Makes it unclear what to do about keys where we only have
oracle access (i.e. agents).

https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go#newcode589
ssh/keys.go:589: // PKCS#1, RSA private keys are supported.
This comment appears to be incorrect - looks like it supports EC keys.

https://codereview.appspot.com/13338044/diff/35001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/35001/ssh/server.go#newcode316
ssh/server.go:316: func MarshaledSignature(k PrivateKey, rand io.Reader, data
[]byte) ([]byte, error) {
Why public?

https://codereview.appspot.com/13338044/diff/35001/ssh/server.go#newcode414
ssh/server.go:414: hostKey := hostKeys[hostKeyAlgo]
it would seem more efficient not to create this map every time, but just to
range over c.config.hostKeys again.

[hanwen-google, 2013-09-18 14:31:41]

https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go#newcode227
ssh/keys.go:227: // RawKey returns the underlying object, eg. *rsa.PrivateKey.
On 2013/09/18 14:11:11, agl1 wrote:
> Why support this? Makes it unclear what to do about keys where we only have
> oracle access (i.e. agents).

removed. turns out I only needed it for tests.

Do you want to rephrase the Keyring as 

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

it would be a little neater, but it breaks clients.

https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go#newcode589
ssh/keys.go:589: // PKCS#1, RSA private keys are supported.
On 2013/09/18 14:11:11, agl1 wrote:
> This comment appears to be incorrect - looks like it supports EC keys.

Done. Do you know how SSH stores DSA keys? I tried reading OpenSSH code, but
it's hard to follow with all the indirections and ifdefs.

https://codereview.appspot.com/13338044/diff/35001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/35001/ssh/server.go#newcode316
ssh/server.go:316: func MarshaledSignature(k PrivateKey, rand io.Reader, data
[]byte) ([]byte, error) {
On 2013/09/18 14:11:11, agl1 wrote:
> Why public?

Done.

https://codereview.appspot.com/13338044/diff/35001/ssh/server.go#newcode414
ssh/server.go:414: hostKey := hostKeys[hostKeyAlgo]
On 2013/09/18 14:11:11, agl1 wrote:
> it would seem more efficient not to create this map every time, but just to
> range over c.config.hostKeys again.

Done.

[hanwen-google, 2013-09-18 14:35:48]

also, what about SetRSAPrivateKey ?

It's now a wrapper for ParsePrivateKey + AddHostKey. We could maintain it to not
break callers, or we could drop it to trim the API.

[jmp, 2013-09-18 15:09:49]

> I'm not sure I agree. We'd then have to make signature type public,
> expose its (de)serialization methods

Is there anything inherently wrong or not secure about exposing signature?  Why
would exposing the signature type require exposing (de)serialization methods?

> , and duplicate the logic to
> prefix the algorithm name for every key type.

The logic to prefix the algorithm could be kept in place and just used within
the Private Key implementations that are being added by this CL.  External
implementations of PrivateKey or a Signer would need to return a *Signature with
the Format field.  In the case of an ssh agent, we would deserialize the result
into a *Signature.  This would keep both the format and signature intact from
what the agent gave us.

> Also, it would loose the
> symmetry between PublicKey.Verify and PrivateKey.Sign.

PublicKey is a new API addition.  And this is not the go standard library.  We
can change PublicKey.Verify to take a *Signature if we want.  And potentially,
the internal logic of a PublicKey.Verify method can check the format as a part
of the verification.

[hanwen-google, 2013-09-18 15:21:26]

On Wed, Sep 18, 2013 at 5:09 PM,  <jonathan.mark.pittman@gmail.com> wrote:
>> I'm not sure I agree. We'd then have to make signature type public,
>> expose its (de)serialization methods
>
>
> Is there anything inherently wrong or not secure about exposing
> signature?  Why would exposing the signature type require exposing
> (de)serialization methods?

The more you expose, the more you paint yourself in a corner when you
want to make changes later on, so it's always a safe choice to not
expose something. Unpacking and repacking the signature for the agent
is a little odd, but since it's done internally, it's not a big
problem.

>> , and duplicate the logic to
>> prefix the algorithm name for every key type.
>
>
> The logic to prefix the algorithm could be kept in place and just used
> within the Private Key implementations that are being added by this CL.
> External implementations of PrivateKey or a Signer would need to return
> a *Signature with the Format field.  In the case of an ssh agent, we
> would deserialize the result into a *Signature.  This would keep both
> the format and signature intact from what the agent gave us.
>
>
>> Also, it would loose the
>> symmetry between PublicKey.Verify and PrivateKey.Sign.
>
>
> PublicKey is a new API addition.  And this is not the go standard
> library.  We can change PublicKey.Verify to take a *Signature if we
> want.  And potentially, the internal logic of a PublicKey.Verify method
> can check the format as a part of the verification.

Every key type would have to do the check individually, which is
exactly the sort of repetitive code I was trying to get rid of.

> https://codereview.appspot.com/13338044/

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

[hanwen-google, 2013-09-18 15:31:30]

On Wed, Sep 18, 2013 at 4:31 PM,  <hanwen@google.com> wrote:
>
> https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go
> File ssh/keys.go (right):
>
> https://codereview.appspot.com/13338044/diff/35001/ssh/keys.go#newcode227
> ssh/keys.go:227: // RawKey returns the underlying object, eg.
> *rsa.PrivateKey.
> On 2013/09/18 14:11:11, agl1 wrote:
>>
>> Why support this? Makes it unclear what to do about keys where we only
>
> have
>>
>> oracle access (i.e. agents).
>
>
> removed. turns out I only needed it for tests.
>
> Do you want to rephrase the Keyring as
>
>   type ClientKeyring interface {
>      Key(int i) (PrivateKey, error)
>   }
>
> it would be a little neater, but it breaks clients.

Come to think of it, do we need PublicKey.RawKey() ? It's only used for tests.

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

[jpsugar, 2013-09-18 17:47:20]

On 2013/09/18 15:31:30, hanwen-google wrote:
> Come to think of it, do we need PublicKey.RawKey() ? It's only used for tests.

I think no. Test-only methods can (should?) be exposed through separate
interfaces.

[jpsugar, 2013-09-18 19:16:59]

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode223
ssh/keys.go:223: type PrivateKey interface {
So is this Signer now?

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode287
ssh/keys.go:287: type rsaPublicKey rsa.PublicKey
This should probably be moved back to before its methods.

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode322
ssh/server.go:322: return serializeSignature(k.PublicKey().PublicKeyAlgo(),
sig), nil
It's not clear from the spec if the signature of a cert-based algo should use
the cert algo id or the underlying algo id. :(

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode381
ssh/server.go:381: serverKexInit.ServerHostKeyAlgos,
k.PublicKey().PrivateKeyAlgo())
PublicKeyAlgo?

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode413
ssh/server.go:413: if hostKeyAlgo == k.PublicKey().PrivateKeyAlgo() {
PublicKeyAlgo?

[jmp, 2013-09-18 20:25:07]

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode322
ssh/server.go:322: return serializeSignature(k.PublicKey().PublicKeyAlgo(),
sig), nil
The private key is used to do the signing.  And the private key has no direct
knowledge of the cert like it does the underlying public key.  So, it gets the
underlying public key's algo.

[jpsugar, 2013-09-18 21:23:14]

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode322
ssh/server.go:322: return serializeSignature(k.PublicKey().PublicKeyAlgo(),
sig), nil
On 2013/09/18 20:25:08, jmp wrote:
> The private key is used to do the signing.  And the private key has no direct
> knowledge of the cert like it does the underlying public key.  So, it gets the
> underlying public key's algo.

Okay, so this should say PrivateKeyAlgo, in case k.PublicKey() is a cert for
some weird reason.

[hanwen-google, 2013-09-19 12:01:34]

On 2013/09/18 17:47:20, jpsugar wrote:
> On 2013/09/18 15:31:30, hanwen-google wrote:
> > Come to think of it, do we need PublicKey.RawKey() ? It's only used for
tests.
> 
> I think no. Test-only methods can (should?) be exposed through separate
> interfaces.

of course. I was just wondering if I missed any use-case for access to the
underlying key. 

I removed it now.

[hanwen-google, 2013-09-19 12:24:47]

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode223
ssh/keys.go:223: type PrivateKey interface {
On 2013/09/18 19:17:00, jpsugar wrote:
> So is this Signer now?

Do you think this is a better name? It still has a PublicKey method, so it looks
strange, and together with PublicKey , PrivateKey makes more sense. We could
rename PublicKey to Verifier, but it has more than just the Verify method.

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode287
ssh/keys.go:287: type rsaPublicKey rsa.PublicKey
On 2013/09/18 19:17:00, jpsugar wrote:
> This should probably be moved back to before its methods.

Done.

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode322
ssh/server.go:322: return serializeSignature(k.PublicKey().PublicKeyAlgo(),
sig), nil
On 2013/09/18 21:23:14, jpsugar wrote:
> On 2013/09/18 20:25:08, jmp wrote:
> > The private key is used to do the signing.  And the private key has no
direct
> > knowledge of the cert like it does the underlying public key.  So, it gets
the
> > underlying public key's algo.
> 
> Okay, so this should say PrivateKeyAlgo, in case k.PublicKey() is a cert for
> some weird reason.

it would be really nice if we had an end-to-end test against sshd to settle
this. Whatever the right answer, it is likely to break next time somebody
touches this.

Also, the whole PrivateKeyAlgo() vs PublicKeyAlgo() thing is because of certs,
but since we don't have proper verification of certs (ie. verifying the
signature of the cert, and checking the timestamp) we have no way of knowing if
this is the right abstraction.

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode381
ssh/server.go:381: serverKexInit.ServerHostKeyAlgos,
k.PublicKey().PrivateKeyAlgo())
On 2013/09/18 19:17:00, jpsugar wrote:
> PublicKeyAlgo?

Done.

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode413
ssh/server.go:413: if hostKeyAlgo == k.PublicKey().PrivateKeyAlgo() {
On 2013/09/18 19:17:00, jpsugar wrote:
> PublicKeyAlgo?

Done.

[jmp, 2013-09-19 13:58:27]

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode223
ssh/keys.go:223: type PrivateKey interface {
On 2013/09/19 12:24:48, hanwen-google wrote:
> On 2013/09/18 19:17:00, jpsugar wrote:
> > So is this Signer now?
> 
> Do you think this is a better name? It still has a PublicKey method, so it
looks
> strange, and together with PublicKey , PrivateKey makes more sense. We could
> rename PublicKey to Verifier, but it has more than just the Verify method.

I would like to eventually see separate interfaces for PrivateKey, PublicKey,
Signer and Verifier where PrivateKey implements Signer and PublicKey implements
Verifier.  Also add a Marshal() method, put back the RawKey() method, or add
something like EncodePem() to PrivateKey.

One day, I would like to be able to use this package to generate actual private
and public keys as well as generate and sign certificates.  Having PrivateKey
represent a real private key could let us do that.  The same goes for PublicKey.
 And in addition, these "Keys" would also implement the Signer and Verifier. 
Having Signer and Verifier be separate interfaces could allow for external
entities to implement just those methods without being forced to implement the
exposing information about the keys.

When I was suggesting to export Signature (requiring the PrivateKey
implementation to add the Format itself), I was thinking of both signing for
authentication as well as signing a certificate.  Certificates can be signed by
a private key of the same or different algorithm.  And the only way to know the
format is for the Signer to return the format.  That could be in the currently
returned []byte, in a *Signature, or have an extra method for Signer to return
its algorithm type.

There may be better ways.  This is all in my head.  Maybe it is time for a
design doc to discuss the plan for this package?

[hanwen-google, 2013-09-19 14:07:14]

On Thu, Sep 19, 2013 at 3:58 PM,  <jonathan.mark.pittman@gmail.com> wrote:
>
> https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go
> File ssh/keys.go (right):
>
> https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode223
> ssh/keys.go:223: type PrivateKey interface {
> On 2013/09/19 12:24:48, hanwen-google wrote:
>>
>> On 2013/09/18 19:17:00, jpsugar wrote:
>> > So is this Signer now?
>
>
>> Do you think this is a better name? It still has a PublicKey method,
>
> so it looks
>>
>> strange, and together with PublicKey , PrivateKey makes more sense. We
>
> could
>>
>> rename PublicKey to Verifier, but it has more than just the Verify
>
> method.
>
> I would like to eventually see separate interfaces for PrivateKey,
> PublicKey, Signer and Verifier where PrivateKey implements Signer and
> PublicKey implements Verifier.  Also add a Marshal() method, put back
> the RawKey() method, or add something like EncodePem() to PrivateKey.
>
> One day, I would like to be able to use this package to generate actual
> private and public keys as well as generate and sign certificates.
> Having PrivateKey represent a real private key could let us do that.
> The same goes for PublicKey.  And in addition, these "Keys" would also
> implement the Signer and Verifier.  Having Signer and Verifier be
> separate interfaces could allow for external entities to implement just
> those methods without being forced to implement the exposing information
> about the keys.
>
> When I was suggesting to export Signature (requiring the PrivateKey
> implementation to add the Format itself), I was thinking of both signing
> for authentication as well as signing a certificate.  Certificates can
> be signed by a private key of the same or different algorithm.  And the
> only way to know the format is for the Signer to return the format.
> That could be in the currently returned []byte, in a *Signature, or have
> an extra method for Signer to return its algorithm type.
>
> There may be better ways.  This is all in my head.  Maybe it is time for
> a design doc to discuss the plan for this package?

I think these are valid plans, but before we build this code, there is
little point in trying to design abstractions for it. Chances that we
build the right abstraction for something that does not yet exist are
slim.

Also, generating keypairs is already provided by the various crypto
packages, eg.

 priv, err := rsa.GenerateKey(rand.Reader, 2048)

so there is no need for the SSH package to do it.

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

[jmp, 2013-09-19 17:14:27]

On 2013/09/19 14:07:14, hanwen-google wrote:
> On Thu, Sep 19, 2013 at 3:58 PM,  <mailto:jonathan.mark.pittman@gmail.com>
wrote:
> >
> > https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go
> > File ssh/keys.go (right):
> >
> > https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode223
> > ssh/keys.go:223: type PrivateKey interface {
> > On 2013/09/19 12:24:48, hanwen-google wrote:
> >>
> >> On 2013/09/18 19:17:00, jpsugar wrote:
> >> > So is this Signer now?
> >
> >
> >> Do you think this is a better name? It still has a PublicKey method,
> >
> > so it looks
> >>
> >> strange, and together with PublicKey , PrivateKey makes more sense. We
> >
> > could
> >>
> >> rename PublicKey to Verifier, but it has more than just the Verify
> >
> > method.
> >
> > I would like to eventually see separate interfaces for PrivateKey,
> > PublicKey, Signer and Verifier where PrivateKey implements Signer and
> > PublicKey implements Verifier.  Also add a Marshal() method, put back
> > the RawKey() method, or add something like EncodePem() to PrivateKey.
> >
> > One day, I would like to be able to use this package to generate actual
> > private and public keys as well as generate and sign certificates.
> > Having PrivateKey represent a real private key could let us do that.
> > The same goes for PublicKey.  And in addition, these "Keys" would also
> > implement the Signer and Verifier.  Having Signer and Verifier be
> > separate interfaces could allow for external entities to implement just
> > those methods without being forced to implement the exposing information
> > about the keys.
> >
> > When I was suggesting to export Signature (requiring the PrivateKey
> > implementation to add the Format itself), I was thinking of both signing
> > for authentication as well as signing a certificate.  Certificates can
> > be signed by a private key of the same or different algorithm.  And the
> > only way to know the format is for the Signer to return the format.
> > That could be in the currently returned []byte, in a *Signature, or have
> > an extra method for Signer to return its algorithm type.
> >
> > There may be better ways.  This is all in my head.  Maybe it is time for
> > a design doc to discuss the plan for this package?
> 
> I think these are valid plans, but before we build this code, there is
> little point in trying to design abstractions for it. Chances that we
> build the right abstraction for something that does not yet exist are
> slim.
> 
> Also, generating keypairs is already provided by the various crypto
> packages, eg.
> 
>  priv, err := rsa.GenerateKey(rand.Reader, 2048)
> 
> so there is no need for the SSH package to do it.
> 
> -- 
> Han-Wen Nienhuys
> Google Munich
> mailto:hanwen@google.com

Those crypto packages do not include the ssh wrappings.  I was thinking we would
use those packages to generate the keys and then wrap them properly within
go.crypto/ssh (i.e. openssh vs ssh2 key formats) for easy output.  Think... 
"openssl genrsa" vs "ssh-keygen -t rsa"

[hanwen-google, 2013-09-19 17:25:36]

On Thu, Sep 19, 2013 at 7:14 PM,  <jonathan.mark.pittman@gmail.com> wrote:
> On 2013/09/19 14:07:14, hanwen-google wrote:
>> I think these are valid plans, but before we build this code, there is
>> little point in trying to design abstractions for it. Chances that we
>> build the right abstraction for something that does not yet exist are
>> slim.
>
>
>> Also, generating keypairs is already provided by the various crypto
>> packages, eg.
>
>
>>   priv, err := rsa.GenerateKey(rand.Reader, 2048)
>
>
>> so there is no need for the SSH package to do it.
>
> Those crypto packages do not include the ssh wrappings.  I was thinking
> we would use those packages to generate the keys and then wrap them
> properly within go.crypto/ssh (i.e. openssh vs ssh2 key formats) for
> easy output.  Think...  "openssl genrsa" vs "ssh-keygen -t rsa"

The ssh "wrapping" is actually fairly trivial (x509, then pem, then
base64), but we could certainly add utility functions for this.

> https://codereview.appspot.com/13338044/



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

[jpsugar, 2013-09-19 17:32:09]

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode223
ssh/keys.go:223: type PrivateKey interface {
On 2013/09/19 13:58:27, jmp wrote:
> There may be better ways.  This is all in my head.  Maybe it is time for a
> design doc to discuss the plan for this package?

Yes. In the mean time, it's a bikeshed not worth painting. I have a preference
for Signer (the presence of the PublicKey method seems fine to me), but this is
fine too.

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode322
ssh/server.go:322: return serializeSignature(k.PublicKey().PublicKeyAlgo(),
sig), nil
On 2013/09/19 12:24:48, hanwen-google wrote:
> it would be really nice if we had an end-to-end test against sshd to settle
> this. Whatever the right answer, it is likely to break next time somebody
> touches this.

Agreed. For now we can change this to k.PublicKey().PrivateKeyAlgo() and
proceed. I have manually verified that signatures work this way in openssh.

[hanwen-google, 2013-09-19 17:51:39]

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go
File ssh/keys.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/keys.go#newcode223
ssh/keys.go:223: type PrivateKey interface {
On 2013/09/19 17:32:09, jpsugar wrote:
> On 2013/09/19 13:58:27, jmp wrote:
> > There may be better ways.  This is all in my head.  Maybe it is time for a
> > design doc to discuss the plan for this package?
> 
> Yes. In the mean time, it's a bikeshed not worth painting. I have a preference
> for Signer (the presence of the PublicKey method seems fine to me), but this
is
> fine too.

since Adam had a pref for Signer too, I've changed it.

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go
File ssh/server.go (right):

https://codereview.appspot.com/13338044/diff/31001/ssh/server.go#newcode322
ssh/server.go:322: return serializeSignature(k.PublicKey().PublicKeyAlgo(),
sig), nil
On 2013/09/19 17:32:09, jpsugar wrote:
> On 2013/09/19 12:24:48, hanwen-google wrote:
> > it would be really nice if we had an end-to-end test against sshd to settle
> > this. Whatever the right answer, it is likely to break next time somebody
> > touches this.
> 
> Agreed. For now we can change this to k.PublicKey().PrivateKeyAlgo() and
> proceed. I have manually verified that signatures work this way in openssh.

Done.

[jpsugar, 2013-09-19 18:30:20]

LGTM

[agl1, 2013-09-19 18:45:51]

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

go.crypto/ssh: introduce Signer method, an abstraction of
private keys.

R=agl, jpsugar, jonathan.mark.pittman
CC=golang-dev
https://codereview.appspot.com/13338044

Committer: Adam Langley <agl@golang.org>

[hanwen-google, 2013-09-20 11:55:25]

*** Abandoned ***