Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
erahn opened this issue Aug 30, 2017 · 26 comments
Open

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

erahn opened this issue Aug 30, 2017 · 26 comments

Comments

@erahn
Copy link

@erahn erahn commented Aug 30, 2017

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 dsnet changed the title It would be nice to have a Cmp() function for crypto/PrivateKey and crypto/PublicKey proposal: crypto: add function to compare PrivateKey and PublicKey Aug 30, 2017
@gopherbot gopherbot added this to the Proposal milestone Aug 30, 2017
@gopherbot gopherbot added the Proposal label Aug 30, 2017
@dsnet

This comment has been minimized.

Copy link
Member

@dsnet dsnet commented Aug 30, 2017

\cc @agl

@nhooyr

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Sep 2, 2017

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

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Oct 16, 2017

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

This comment has been minimized.

Copy link
Author

@erahn erahn commented Oct 16, 2017

@hanwen

This comment has been minimized.

Copy link
Contributor

@hanwen hanwen commented Nov 8, 2017

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

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Nov 8, 2017

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

@hanwen

This comment has been minimized.

Copy link
Contributor

@hanwen hanwen commented Nov 8, 2017

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

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Nov 8, 2017

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 19, 2018

@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

This comment has been minimized.

Copy link
Author

@erahn erahn commented Mar 21, 2018

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 21, 2018

@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

This comment has been minimized.

Copy link
Author

@erahn erahn commented Mar 25, 2018

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

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Mar 26, 2018

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

This comment has been minimized.

Copy link
Author

@erahn erahn commented Mar 26, 2018

@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

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Mar 26, 2018

@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

This comment has been minimized.

Copy link
Author

@erahn erahn commented Apr 6, 2018

@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

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Apr 6, 2018

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

This comment has been minimized.

Copy link
Author

@erahn erahn commented Apr 11, 2018

@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

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Apr 11, 2018

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

@erahn

This comment has been minimized.

Copy link
Author

@erahn erahn commented Apr 11, 2018

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

This comment has been minimized.

Copy link
Contributor

@nhooyr nhooyr commented Apr 12, 2018

Yes, so you cannot force it.

@erahn

This comment has been minimized.

Copy link
Author

@erahn erahn commented Apr 12, 2018

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

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jan 21, 2020

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 rsc added this to Incoming in Proposals Jan 22, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jan 22, 2020

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 rsc changed the title proposal: crypto: add function to compare PrivateKey and PublicKey proposal: crypto: add Equal(PublicKey) bool method to PublicKey implementations Jan 22, 2020
@rsc rsc moved this from Incoming to Active in Proposals Jan 22, 2020
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 5, 2020

Based on the discussion above, this sounds like a likely accept.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Feb 12, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals Feb 12, 2020
@rsc rsc changed the title proposal: crypto: add Equal(PublicKey) bool method to PublicKey implementations crypto: add Equal(PublicKey) bool method to PublicKey implementations Feb 12, 2020
@rsc rsc modified the milestones: Proposal, Backlog Feb 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.