crypto: add Equal(PublicKey) bool method to PublicKey implementations

[erahn]

What version of Go are you using (go version)?

1.9

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

Any

What did you do?

https://play.golang.org/p/DAZM324jkY

What did you expect to see?

This is a request for adding a Cmp() function to the PublicKey / PrivateKey types in the crypto library. Currently it is non-trivial to check if two public keys or two private keys are the same and requires checking the algorithms definition and manually comparing each operator. It would be much similar to have some Cmp() functions to simplify this.

What did you see instead?

This is a feature request - but not to be too cheeky - a lack of a simple way to compare two crypto keys.

[dsnet]

\cc @agl

[nhooyr]

This may be useful for the x/crypto/ssh package for public key authorization. The current approach to authorize a public key is to marshal it into the openSSH wire format and then compare those bytes against a authorized public key's (which was also marshalled into the openSSH wire format) bytes.

[rsc]

It does seem like we should do something here; it's a little hard to retrofit into the existing interfaces but we could. It seems like the operations needed are

func PrivateToPublicKey(PrivateKey) PublicKey
func PrivateKeysEqual(k1, k2 PrivateKey) bool
func PublicKeysEqual(k1, k2 PublicKey) bool

but with better names. Do I have the desired semantics correct?

[erahn]

Hello Russ, I think you have the semantics correct for the latter 2. There is a function already ( Public() ) which does the PrivateToPublicKey functionality. One thing that I wonder though is if there should be an easy function to verify if a public and private key pair match. I believe that for elliptic curves the public key is derived from the private key, but for others, such as RSA, the public key is saved alongside the private key. This can lead to issues such as: https://blog.hboeck.de/archives/888-How-I-tricked-Symantec-with-a-Fake-Private-Key.html . The problem with having Public() verify for those scenarios is that you need to verify by signing a message then verifying the signature, which may require nontrivial resources on some systems to do so. Having a separate function such as: "VerifyPublicPrivateKey( PrivateKey, PublicKey ) bool" may allow for easily resolving that issue.

…

On Mon, Oct 16, 2017 at 1:23 PM, Russ Cox ***@***.***> wrote: It does seem like we should do something here; it's a little hard to retrofit into the existing interfaces but we could. It seems like the operations needed are func PrivateToPublicKey(PrivateKey) PublicKey func PrivateKeysEqual(k1, k2 PrivateKey) bool func PublicKeysEqual(k1, k2 PublicKey) bool but with better names. Do I have the desired semantics correct? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#21704 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AX3dhsuhTVp3OUVyf-phLtNZmdoA29szks5ss7tFgaJpZM4PIHgB> .

[hanwen]

For SSH, we should have a separate conversation (if any). It basically amounts to syntactic sugar for

func Eq(a, b ssh.PublicKey) {
return 0 == bytes.Compare(a.Marshal(), b.Marshal())
}

[nhooyr]

@hanwen Wouldn't it be more efficient to compare the keys directly instead of marshalling first, assuming this issue is solved first.

[hanwen]

yes, but it would also be much more work and much more error prone.

The whole SSH protocol is marshaling data all the time to send things over the wire, so the cost of marshaling for a key comparison is neglible compared to using an SSH connection.

[nhooyr]

I'm just saying that if this issue is solved, it would also be applicable in x/crypto/ssh. It's not absolutely necessary as you have pointed out but if a function that compared two public/private keys did exist and was working well, I think it would be better to use that instead of comparing the marshalled bytes.

[ianlancetaylor]

@rsc You suggested functions above, but the crypto package today has essentially no functions. Did you mean instead something like: It could be implemented using interfaces, so that given a PrivateKey you can test whether it supports some sort of Equaler interface with an Equal method.

Does anybody want to turn this proposal into a complete suggestion, one way or another?

[erahn]

I would be interested in pursing this further. What would the next step be? I am looking at https://golang.org/doc/contribute.html and it is unclear if a full design doc is needed or if I should just submit some code to add interface functions along the lines of what @ianlancetaylor and @rsc are suggesting?

[ianlancetaylor]

@erahn The next step is a proposal for specifically how the crypto package will be changed, and how those changes will be reflected in crypto/... packages. Thanks.

[erahn]

Ok, thanks Ian.

My proposal is as follows:

In the crypto/crypto.go file update the PublicKey and PrivateKey interfaces to have an Equal() function that will accept an argument of the corresponding interface type and return a bool to indicate if they are equal or not.

Add a function Public to the PrivateKey interface that will return the corresponding PublicKey to the PrivateKey. I realize after looking at the code that this wasn't implemented everywhere nor is it guaranteed to exist.

I would like to have a function to verify that a private key corresponds to a public key by encrypting some data with the private key then decrypting it with the public, but I don't see a clean place to add that.

How is that for a proposal?

Thanks!

[ianlancetaylor]

Thanks. Unfortunately any change has to follow the Go 1 compatibility guarantee (https://golang.org/doc/go1compat), and I don't think that will let us add a method to an existing interface type. That would break any existing program that writes a type that is meant to implement the interface.

[erahn]

@ianlancetaylor thanks for the link and read, I had not considered that. Is there an accepted way in the golang community to do these sort of changes ( i.e. add functions to do the above ) that I could leverage?

Adding the functions in crypto/crypto.go doesn't seem like the correct way to go since I would not be able to implement them without doing type casting. Implementing each function in the appropriate library, i.e. crypto/ecdsa, crypto/rsa, etc would mean that new algorithms could skip over implementing this nor would support for it be evident by looking at the top level crypto library.

[nhooyr]

@erahn

Add the equals method to a new interface in the top level crypto library and then add a global Equals function that takes two keys and checks if their two types are the same and both implement the interface and then calls that method on one of the keys to check if they are equal. The global equals method should maybe also return an ok bool to indicate whether one of the keys does not support the interface.

[erahn]

@nhooyr Thanks for the suggestion. One thing that I do not understand is - What is the value of having a separate top level interface that other new key types could just skip implementing? Why not just implement the Equal() and Public() functions for each existing key type?

[nhooyr]

Great question, I was thinking it would make the contract explicit and allow easy comparison between keys of different types for consumers. Other new key types could skip implementing but that is unlikely given it would be in the godoc and someone would eventually complain. Thats my thinking anyway. Maybe we could just add a Equals() method to each existing key type.

[erahn]

@nhooyr I like the idea, but I don't think that anything that doesn't force someone to implement the functions would be a good solution.

@ianlancetaylor : Here is my new proposal. I will add new Equals functions to the RSA, ECDSA, and DSA keys. I will also add a Public() function to the DSA key that will do the same thing as the RSA and ECDSA Public() functions.

[nhooyr]

But that proposal does not force someone to implement the functions either and it also does not document the contract.

[erahn]

I could make an interface to document the contract in the top level crypto/crypto.go file, but that wouldn't force someone to implement it in future keys. Anything that would force someone to do so would break the compatibility guarantee would it not?

[nhooyr]

Yes, so you cannot force it.

[erahn]

Okay, I can understand the importance of documenting the contract, even if it is not enforced. So I can make my new proposal:

I will add new Equals functions to the RSA, ECDSA, and DSA keys. I will also add a Public() function to the DSA key that will do the same thing as the RSA and ECDSA Public() functions. These will be captured in 2 top level interfaces "ComparablePublicKey" with the Equals function and "ComparablePrivateKey" with the Public and Equals functions.

[FiloSottile]

Sounds like the most useful thing we could do here is add Equal(crypto.PublicKey) bool methods to *rsa.PublicKey, *ecdsa.PublicKey and ed25519.PublicKey. (Not including DSA because ideally we'd be dropping support for DSA keys, not encouraging wiring them into new code.) Then users can just do an interface upgrade on the private key for Public() crypto.PublicKey and on the public key for Equal(crypto.PublicKey) bool.

This would also be compatible with github.com/google/go-cmp/cmp:

"(T) Equal(I) bool" where T is assignable to I

https://pkg.go.dev/github.com/google/go-cmp/cmp#Equal

[rsc]

It sounds like the current proposal is to standardize an optional but recommended method for public keys:

Equal(crypto.PublicKey) bool

Is that correct? Does anyone object to this approach?

[rsc]

No change in consensus, so accepted.

[gopherbot]

Change https://golang.org/cl/223754 mentions this issue: crypto/rsa,crypto/ecdsa,crypto/ed25519: implement PublicKey.Equal

[bcmills]

Reopening because this is likely to be reverted in CL 225077 due to failing tests. (It can then be resubmitted and re-closed once the tests are fixed.)

[gopherbot]

Change https://golang.org/cl/225460 mentions this issue: crypto/rsa,crypto/ecdsa,crypto/ed25519: implement PublicKey.Equal