code review 13415046: go.crypto/ssh: remove misleading marshalPrivRSA.

go.crypto/ssh: remove misleading marshalPrivRSA.

Properly capitalize publicKey throughout.

[hanwen-google, 2013-09-05 14:32:21]

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

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

[agl1, 2013-09-05 15:37:06]

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

go.crypto/ssh: remove misleading marshalPrivRSA.

Properly capitalize publicKey throughout.

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

Committer: Adam Langley <agl@golang.org>

[jmp, 2013-09-05 15:53:30]

On 2013/09/05 15:37:06, agl1 wrote:
> *** Submitted as
> https://code.google.com/p/go/source/detail?r=f318941d4fb8&repo=crypto ***
> 
> go.crypto/ssh: remove misleading marshalPrivRSA.
> 
> Properly capitalize publicKey throughout.
> 
> R=golang-dev
> CC=agl, dave, golang-dev, jpsugar
> https://codereview.appspot.com/13415046
> 
> Committer: Adam Langley <mailto:agl@golang.org>

This is confusing.  ServerConfig has this...

type ServerConfig struct {
        rsa           *rsa.PrivateKey
        rsaSerialized []byte

but it does not indicate that rsaSerialized should be the serialized form of the
public key version of ServerConfig.rsa.  If this is indeed how it should be,
please follow up a CL and add a comment to rsaSerialized indicating the
distinction.  Or I can do it in my still pending cl/13528044.  But please
confirm this is how it should be.

[hanwen-google, 2013-09-05 16:12:41]

On Thu, Sep 5, 2013 at 5:53 PM,  <jonathan.mark.pittman@gmail.com> wrote:
> On 2013/09/05 15:37:06, agl1 wrote:
>>
>> *** Submitted as
>> https://code.google.com/p/go/source/detail?r=f318941d4fb8&repo=crypto
>
> ***
>
>> go.crypto/ssh: remove misleading marshalPrivRSA.
>
>
>> Properly capitalize publicKey throughout.
>
>
>> R=golang-dev
>> CC=agl, dave, golang-dev, jpsugar
>> https://codereview.appspot.com/13415046
>
>
>> Committer: Adam Langley <mailto:agl@golang.org>
>
>
> This is confusing.  ServerConfig has this...
>
> type ServerConfig struct {
>         rsa           *rsa.PrivateKey
>         rsaSerialized []byte
>
> but it does not indicate that rsaSerialized should be the serialized
> form of the public key version of ServerConfig.rsa.  If this is indeed
> how it should be, please follow up a CL and add a comment to
> rsaSerialized indicating the distinction.  Or I can do it in my still
> pending cl/13528044.  But please confirm this is how it should be.

the RSA public key is pre-serialized, I assume to save some cycles.
The private key should never be serialized as it's not meant to go
anywhere, and reading marshalPrivRSA gave me a small heart attack when
I first saw it.

I was planning to do exactly what you did in your CL (How do I
subscribe to reviews for the SSH package?).

I wonder whether it would be a good idea to abstract the
signing/verifying into interfaces

  type Verifier interface {
     Name() // ssh-rsa
     Verify(data []byte) bool

     // Serialize public key in authorized key format
     AuthorizedKey() []byte

     // Public key in SSH wire format.
     SerializedPubKey() []byte

     PublicKey() interface{}
  }
  func ParseKnownKey(base64Bytes []byte) Verifier
  func NewRSAVerifier(*rsa.PublicKey) Verifier

for verifying, and

  type Signer interface {
    // SSH standard name for this keypair, eg. ecdsa-sha256-nistp256
    Name()

    // Sign the data, and return the signature in SSH wire format
    Sign(data []byte) []byte

    // Public key in SSH wire format.
    SerializedPubKey() []byte

    PrivateKey() interface{}
 }
  NewECDSASigner(*ecdsa.PrivateKey, rand io.Reader) Signer
  ParseECDSAPrivateKey(pemBytes []byte, rand io.Reader) Signer

the keyring could hand out Signers,

  type KeyRing interface {
    // The nth key, or nil if not available.
    GetSigner(N int) Signer
  }

and, ServerConfig would hold a map[string]Signer for the hostkeys (or
perhaps a []Signer so we can encode a preference).

I think using interfaces like this would make it easier to reuse
pub/private key code in different places.  What do you think Adam?

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

[jmp, 2013-09-05 16:58:30]

For the purpose of my original question, I will add a comment in my CL to
server.go about rsaSerialized being the public key.  As for the rest of
your response, the general idea SGTM.  Adam should definitely weigh in
though.  He has a better understanding of the overall nature of this
package.

As for subscribing to reviews for the ssh package, I have a simple, but
mostly effective filter that catches this and some extra stuff.  I am sure
it could be improved, but I use...

list:{<golang-dev.googlegroups.com> <golang-nuts.googlegroups.com>} ssh

I didn't want to use "go.crypto/ssh", in case someone just referred to it
as ssh.  Maybe adding "reply@codereview-hr.appspotmail.com" to the filter
will help for just getting reviews, but it might weed out other useful
discussion.


On Thu, Sep 5, 2013 at 12:12 PM, Han-Wen Nienhuys <hanwen@google.com> wrote:

> On Thu, Sep 5, 2013 at 5:53 PM,  <jonathan.mark.pittman@gmail.com> wrote:
> > On 2013/09/05 15:37:06, agl1 wrote:
> >>
> >> *** Submitted as
> >> https://code.google.com/p/go/source/detail?r=f318941d4fb8&repo=crypto
> >
> > ***
> >
> >> go.crypto/ssh: remove misleading marshalPrivRSA.
> >
> >
> >> Properly capitalize publicKey throughout.
> >
> >
> >> R=golang-dev
> >> CC=agl, dave, golang-dev, jpsugar
> >> https://codereview.appspot.com/13415046
> >
> >
> >> Committer: Adam Langley <mailto:agl@golang.org>
> >
> >
> > This is confusing.  ServerConfig has this...
> >
> > type ServerConfig struct {
> >         rsa           *rsa.PrivateKey
> >         rsaSerialized []byte
> >
> > but it does not indicate that rsaSerialized should be the serialized
> > form of the public key version of ServerConfig.rsa.  If this is indeed
> > how it should be, please follow up a CL and add a comment to
> > rsaSerialized indicating the distinction.  Or I can do it in my still
> > pending cl/13528044.  But please confirm this is how it should be.
>
> the RSA public key is pre-serialized, I assume to save some cycles.
> The private key should never be serialized as it's not meant to go
> anywhere, and reading marshalPrivRSA gave me a small heart attack when
> I first saw it.
>
> I was planning to do exactly what you did in your CL (How do I
> subscribe to reviews for the SSH package?).
>
> I wonder whether it would be a good idea to abstract the
> signing/verifying into interfaces
>
>   type Verifier interface {
>      Name() // ssh-rsa
>      Verify(data []byte) bool
>
>      // Serialize public key in authorized key format
>      AuthorizedKey() []byte
>
>      // Public key in SSH wire format.
>      SerializedPubKey() []byte
>
>      PublicKey() interface{}
>   }
>   func ParseKnownKey(base64Bytes []byte) Verifier
>   func NewRSAVerifier(*rsa.PublicKey) Verifier
>
> for verifying, and
>
>   type Signer interface {
>     // SSH standard name for this keypair, eg. ecdsa-sha256-nistp256
>     Name()
>
>     // Sign the data, and return the signature in SSH wire format
>     Sign(data []byte) []byte
>
>     // Public key in SSH wire format.
>     SerializedPubKey() []byte
>
>     PrivateKey() interface{}
>  }
>   NewECDSASigner(*ecdsa.PrivateKey, rand io.Reader) Signer
>   ParseECDSAPrivateKey(pemBytes []byte, rand io.Reader) Signer
>
> the keyring could hand out Signers,
>
>   type KeyRing interface {
>     // The nth key, or nil if not available.
>     GetSigner(N int) Signer
>   }
>
> and, ServerConfig would hold a map[string]Signer for the hostkeys (or
> perhaps a []Signer so we can encode a preference).
>
> I think using interfaces like this would make it easier to reuse
> pub/private key code in different places.  What do you think Adam?
>
> --
> Han-Wen Nienhuys
> Google Munich
> hanwen@google.com
>

[agl1, 2013-09-09 17:08:36]

On Thu, Sep 5, 2013 at 12:12 PM, Han-Wen Nienhuys <hanwen@google.com> wrote:
> I think using interfaces like this would make it easier to reuse
> pub/private key code in different places.  What do you think Adam?

Seems reasonable if you think that it'll help the code.


Cheers

AGL

[hanwen-google, 2013-09-26 12:08:26]

*** Abandoned ***