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

Jump to bottom

crypto/ed25519,x/crypto/ed25519: add ValidPrivateKey() method #33923

Closed
kevinburke1 opened this issue Aug 29, 2019 · 4 comments
Closed

crypto/ed25519,x/crypto/ed25519: add ValidPrivateKey() method #33923

kevinburke1 opened this issue Aug 29, 2019 · 4 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.

Comments

@kevinburke1
Copy link

kevinburke1 commented Aug 29, 2019 •
edited

I mistakenly used 32 bytes of random as a ed25519.PrivateKey and was extremely confused why signatures were not validating, was frantically triple checking the data I was attempting to sign/verify was the same on both ends, etc. I am an experienced user and if I can get tripped up by this, less experienced users could as well.

Adding a method for users to check whether a private key is valid could help alleviate this by suggesting a different source of error, and also helping users ensure that they are using valid keys to sign data. Roughly it could do:

// ValidPrivateKey reports whether in is a valid ed25519 private key.
func ValidPrivateKey(in []byte) bool {
        origKey := copy(in)
	in[0] &= 248
	in[31] &= 127
	in[31] |= 64
        return bytes.Equal(origKey, in)
}

You could also do func (p *PrivateKey) Valid() bool though I worry about letting people create a PrivateKey object and then determine whether that is valid or not, because they might try to use it later to sign stuff.
kevinburke added a commit to kevinburke/nacl that referenced this issue Aug 29, 2019
@kevinburke
cmd: add command for generating keypairs
f3ed23d 
See issue golang/go#33923, I had mistakenly used `openssl rand -hex
64` to generate a private key, which did not work for a pretty subtle
reason - usually the data you are attempting to sign has been subtly
changed between when you signed the message and when you verified it
but here the key was the problem.

Attempt to fix this by making it really easy for users to generate
keys for use with this library.
@julieqiu julieqiu added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 29, 2019
@julieqiu
Copy link
Member

/cc @FiloSottile

@FiloSottile
Copy link
Contributor

That's not what ValidPrivateKey would look like: PrivateKey is 32 bytes of random seed (which is not clamped) followed by the public key. Instead, you'd want to check that NewKeyFromSeed(pk.Seed()) == pk.

Sign checks the PrivateKey length, so are you sure you were using 32 bytes of random?

@kevinburke1
Copy link
Author

Ah. I diagnosed the problem incorrectly. I assumed priv.Public() generated a public key from the private key using the same ScalarMult() functions that Keypair() does. However, it just returns priv[32:], the last 32 bytes.

The last 32 bytes of the private key I was using - the public key - was also read from crypto/rand so the public key returned by priv.Public() had no relationship to the private key.

I suppose if the private key is 64 bytes and the last 32 bytes are the public key, a function could also validate that relationship.

@ston1th ston1th mentioned this issue Aug 31, 2019
Closed
@FiloSottile
Copy link
Contributor

I don't think we want to encourage manually creating private keys by adding a validation function. Keys should only be generated by GenerateKey or NewKeyFromSeed.

I kind of regret not making GenerateKey return only (PrivateKey, error), so that it would get associated in the godoc, but too late.

@golang golang locked and limited conversation to collaborators Dec 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@FiloSottile @julieqiu @gopherbot @kevinburke1