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

proposal: concrete interfaces for crypto.PublicKey and crypto.PrivateKey #30140

Closed
coolaj86 opened this issue Feb 8, 2019 · 15 comments
Closed
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@coolaj86
Copy link

coolaj86 commented Feb 8, 2019

Currently crypto.PublicKey and crypto.PrivateKey are empty interfaces.

This has bitten me a number of times because errors that could easily be caught by tooling or at compile time become runtime errors (sometimes very confusing ones).

It appears that all Private Keys in the crypto package include PublicKey or Public() crypto.PublicKey. I assume that, by their very nature, any future invention of asymmetric keys will include the Public Key as part of the definition of the Private Key.

There's also a well-known and intuitive standard way to compute a key Thumbprint that would work on any existing asymmetric keys and any future asymmetric keys. I believe there are also one or more well-known ways to compute a Fingerprint (SSH, among others), but perhaps Thumbprint is more distinct in that there is only one such specification (as far as I know).

I propose that crypto.PrivateKey be changed as follows:

type PrivateKey {
    Public() crypto.PublicKey
}

And that crypto.PublicKey likewise change to something like this:

type PublicKey {
    ThumbprintSHA256() []byte
}
@gopherbot gopherbot added this to the Proposal milestone Feb 8, 2019
@FiloSottile FiloSottile added the v2 A language change or incompatible library change label Feb 8, 2019
@FiloSottile
Copy link
Contributor

Interfaces can't be changed in Go 1 due to the Go 1 compatibility promise.

@coolaj86
Copy link
Author

coolaj86 commented Feb 9, 2019

Thanks for the quick reply.

Before you tag this as Go2 only:

First Point

Adding ThumbprintSHA256() []byte to all of the crypto.PublicKey types and and Public() crypto.PublicKey to "crypto/dsa" would not break compile time compatibility at all and would make it possible for someone like myself to easily type out the interface is my own code without having to wrap the object.

It makes them interface-able. No breakage.

Second Point

Although you could technically say that changing the interface breaks compatibility, it's actually just moving run time errors to compile time errors.

However, I'm willing to concede that some weirdo, somewhere, has used crypto.PublicKey as passed in a string and somehow manages to make his code work anyway. Or, perhaps more likely, someone has some dead code somewhere which doesn't actually execute (otherwise it would panic) but that is leftover in the code.

That being said, the bigger issue is the first point. It's trivial to add an interface.

@ianlancetaylor
Copy link
Contributor

Although you could technically say that changing the interface breaks compatibility, it's actually just moving run time errors to compile time errors.

Technicalities matter. We can't make this change in Go 1.

@coolaj86
Copy link
Author

Forget the second point entirely. How about the 1st point?

@ianlancetaylor
Copy link
Contributor

@FiloSottile What do you think of the idea of adding a standard method to every PublicKey and PrivateKey implementation? Does anybody actually define their own PublicKey or PrivateKey implementation?

@ianlancetaylor
Copy link
Contributor

Ping @FiloSottile

@FiloSottile
Copy link
Contributor

I'm ok with making Public() crypto.PublicKey a convention. I don't really get why anyone would be using DSA in 2019, and I wish I could freeze and deprecate crypto/dsa, but since this came up a couple times now (#27889, /cc @Merovius) I would accept a CL making dsa.PrivateKey a crypto.Signer at this point, sure.

I had never heard of a thumbprint, and I don't want to touch JWT specs if at all possible. I'm not opposed to adding a common method to all public keys, but I can't think of a good one.

@FiloSottile FiloSottile added Proposal-Crypto Proposal related to crypto packages or other security issues and removed v2 A language change or incompatible library change labels Apr 17, 2019
@coolaj86
Copy link
Author

How about Fingerprint then? That's also well standardized, a la ssh.

@bcmills bcmills added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Apr 30, 2019
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Aug 16, 2019
@FiloSottile
Copy link
Contributor

#21704 is likely to be accepted, and it proposed adding Equal(crypto.PublicKey) bool to all PublicKeys, so that would be the way to identify public keys.

For private keys, we already have Public() crypto.PublicKey, so I think this got resolved.

Closing as duplicate of #21704.

@coolaj86
Copy link
Author

coolaj86 commented Apr 10, 2020

#21704 is likely to be accepted, and it proposed adding Equal(crypto.PublicKey) bool to all PublicKeys, so that would be the way to identify public keys.

From that it doesn't look like crypto.PublicKey is going to be updated accordingly.

That proposal allows for an better workaround for types that optionally support it, but it doesn't solve the problem that crypto.PublicKey and crypto.PrivateKey are empty marker interfaces. Also crypto/dsa.PrivateKey doesn't yet implement Public().

I'd like this to be re-opened as a non-duplicate.

@coolaj86
Copy link
Author

My understanding is that by having the go version in go.mod that it is somehow possible to tell the compiler to use a different version of the API for code, and some auto-upgrade options.

Including Public and Equal in the Private and Public key interfaces respectively would be auto-upgradable for code that is currently correct and would require a change for code that can be positively identified as having an actual bug.

@FiloSottile
Copy link
Contributor

We haven't yet used the module language version to change the standard library, so we don't know yet what the process would be, but changing an interface like this would not have a clean upgrade path because of third-party implementations of PrivateKey and PublicKey that would break.

crypto/dsa.PrivateKey does not implement Public() because if you are building something new and using the new interface, you should not be using crypto/dsa.

@coolaj86
Copy link
Author

coolaj86 commented Apr 11, 2020

third-party implementations of PrivateKey and PublicKey

I don't understand. How could anything in the crypto package use a 3rd party "implementation"?

There's nothing to implement. The interface is empty and not usable.

Unless there's some sort of private type casting in the standard library code (which I haven't dug around for), there is no way to use a 3rd party implementation with the standard library.

At the very least there is no documented way to do that.

@coolaj86
Copy link
Author

bump ^^

I'm open to that there's a use case that I'm unfamiliar with or don't properly understand and I'd like to know how it would be possible for 3rd party implementations to exist (and to break).

@Merovius
Copy link
Contributor

One example that would break if you just added a method to PublicKey is x/crypto/ed25519 if used with an old Go version (in newer versions, ed25519.PublicKey is an alias). I don't think this specifically is a reason not to do it, but it may serve as an example of how these interfaces may be used, even if they are empty. A third-party might a) provide an API taking a crypto.PublicKey, b) provide one of their own, non-stdlib types usable as such and c) only pass stdlib-provided public keys to stdlib functions, catching their own types via type-switches for specialized implementations.

I'm not saying it's the most elegant or correct way to handle a use-case like this. But there is historical precedent.

@golang golang locked and limited conversation to collaborators Apr 21, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

No branches or pull requests

6 participants