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/ecdsa: add SignASN1, VerifyASN1 #20544

Open
jyasskin opened this issue May 31, 2017 · 8 comments
Open

crypto/ecdsa: add SignASN1, VerifyASN1 #20544

jyasskin opened this issue May 31, 2017 · 8 comments

Comments

@jyasskin
Copy link

@jyasskin jyasskin commented May 31, 2017

The crypto.Signer implementation provided by crypto.ecdsa encodes its signature into bytes using the ASN.1 format that's used by TLS, but it doesn't provide a matching verification routine that takes a []byte instead of a pair of big.Ints. It's straightforward to implement one, and both crypto.tls and crypto.x509 have done so, but it seems silly to make every consumer independently implement a standardized signature format.

@mikioh mikioh changed the title Feature: ecdsa should provide an ASN1/DER verification routine proposal: crypto/ecdsa: should provide an ASN1/DER verification routine Jun 1, 2017
@mikioh mikioh added the Proposal label Jun 1, 2017
@gopherbot gopherbot added this to the Proposal milestone Jun 1, 2017
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jun 5, 2017

/cc @agl

@agl agl self-assigned this Jun 5, 2017
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Aug 14, 2017

I ran into this oddness during BoringCrypto work too. It does seem like there should be a

package ecdsa
func VerifyBytes(pub *PublicKey, hash, sig []byte) bool

to match ecdsa.PrivateKey.Sign. It would be tempting to add SignBytes that returns []byte instead of two big.Int, but that name doesn't seem right. So maybe instead:

package ecdsa
func SignASN1(io.Reader, priv *PrivateKey, hash []byte) (sig []byte, err error)
func VerifyASN1(pub *PublicKey, hash, sig []byte) bool

?

@agl, I'm happy to write these if you sign off (ha) on the names.

@FiloSottile FiloSottile assigned katiehockman and unassigned agl Nov 18, 2019
@rsc rsc changed the title proposal: crypto/ecdsa: should provide an ASN1/DER verification routine proposal: crypto/ecdsa: add SignASN1, VerifyASN1 Nov 19, 2019
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 19, 2019

This came up during @katiehockman's work on Wycheproof tests, and it seems like a good idea, it looks like we are forcing everyone to use encoding/asn1 when they just want to deal with ECDSA. I'm in favor of adding these APIs.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 20, 2019

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

Leaving open for a week for final comments.

@agl

This comment has been minimized.

Copy link
Contributor

@agl agl commented Nov 20, 2019

Exposing a public signature interface with big-ints was dumb(*). The interface should always have been byte-based.

(* happy to use strong language here because it was my screw-up.)

@rsc rsc added this to Final Comment in Proposals Nov 27, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

No change in consensus (agl strongly agrees!), so accepting.

@rsc rsc changed the title proposal: crypto/ecdsa: add SignASN1, VerifyASN1 crypto/ecdsa: add SignASN1, VerifyASN1 Nov 27, 2019
@rsc rsc modified the milestones: Proposal, Go1.15 Nov 27, 2019
@katiehockman

This comment has been minimized.

Copy link
Member

@katiehockman katiehockman commented Nov 27, 2019

Great, thanks! I have a change ready for this, so I'll pass it along shortly (though it won't go in until 1.15)

@erincandescent

This comment has been minimized.

Copy link

@erincandescent erincandescent commented Dec 7, 2019

I wonder if there should be a crypto.Verifier interface which is symmetric with crypto.Signer, which public key types should implement?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.