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: make PublicKey implement encoding.TextMarshaler/TextUnmarshaler using PEM #33564

Open
nulldowntime opened this issue Aug 9, 2019 · 13 comments

Comments

@nulldowntime
Copy link

@nulldowntime nulldowntime commented Aug 9, 2019

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

$ go version
go version go1.12.7 darwin/amd64

and whatever play.golang.org uses.

Does this issue reproduce with the latest release?

It is the latest release as of now. It can also be reproduced here: https://play.golang.org/p/cbsOhB8lHxe

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

go env Output
$ go env
GOARCH="amd64"
GOBIN=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/path/to/my/gocode"
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.7/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.7/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="/dev/null"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/td/yb98xptn12d60pfrjw87kkm80000gn/T/go-build432091977=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

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

package main

import (
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/rand"
	"encoding/json"
	"log"
)

func main() {

	privKey, _ := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
	jsonKey, err := json.Marshal(privKey)
	if err != nil {
		log.Fatalf("error marshalling private key: %v", err)
	}

	var retrieveKey ecdsa.PrivateKey

	if err := json.Unmarshal(jsonKey, &retrieveKey); err != nil {
		log.Fatalf("error parsing json: %s", err)
	}
}

What did you expect to see?

A key struct created from json that has just been created from the very same data structure.

The same process does succeed for RSA: https://play.golang.org/p/FFYREV4NMfv , although not on the playground, where I get "Program exited: process took too long." I did test it locally and I'm actually using it as a workaround.

What did you see instead?

error parsing json: json: cannot unmarshal object into Go struct field PrivateKey.Curve of type elliptic.Curve

I have also written the jsonKey to file and verified that it is valid json.

@odeke-em odeke-em changed the title ecdsa private key json marshalls fine but unmarshal returns an error crypto/ecdsa: Private key json marshals fine but unmarshal returns an error Aug 10, 2019
@odeke-em
Copy link
Member

@odeke-em odeke-em commented Aug 10, 2019

Thank you for this report @nulldowntime and welcome to the Go project!

So the issue here is that for ecdsa.PrivateKey we don't have custom JSON marshaler and unmarhsler methods yet the embedded field Curve is an interface as per:

https://golang.org/pkg/crypto/ecdsa/#PrivateKey
Screen Shot 2019-08-10 at 2 14 31 AM

which is an embedded field https://golang.org/pkg/crypto/ecdsa/#PublicKey
Screen Shot 2019-08-10 at 2 15 44 AM

and finally https://golang.org/pkg/crypto/elliptic/#Curve
Screen Shot 2019-08-10 at 2 16 57 AM

of which a non-nil interface is backed by a concrete type so the JSON.marshal works alright, but not the other way unless we define some custom json.Unmarshaler https://golang.org/pkg/encoding/json/#Unmarshaler methods

You can unmarshal though to the CurveParams as per https://play.golang.org/p/NfTlvFC28Zw or inlined below

package main

import (
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/rand"
	"encoding/json"
	"fmt"
	"log"
)

func main() {

	privKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
	if err != nil {
		log.Fatalf("Failed to generate privateKey: %v", err)
	}
	jsonKey, err := json.Marshal(privKey)
	if err != nil {
		log.Fatalf("error marshalling private key: %v", err)
	}
	type retrieve struct {
		CurveParams *elliptic.CurveParams `json:"Curve"`
	}

	rt := new(retrieve)
	if err := json.Unmarshal(jsonKey, rt); err != nil {
		log.Fatalf("error parsing json: %s", err)
	}
	fmt.Printf("Retrieved.CurveParams: %#v\n", rt.CurveParams)
}

which prints out successfully

Retrieved.CurveParams: &elliptic.CurveParams{
 P:115792089210356248762697446949407573530086143415290314195533631308867097853951,
 N:115792089210356248762697446949407573529996955224135760342422259061068512044369, 
 B:41058363725152142129326129780047268409114441015993725554835256314039467401291, 
 Gx:48439561293906451759052585252797914202762949526041747995844080717082404635286, 
 Gy:36134250956749795798585127919587881956611106672985015071877198253568414405109,
 BitSize:256,
 Name:"P-256"
}

and then for a sample of json.Unmarshaler https://play.golang.org/p/_JvAmJOBe5A

I don't think these structs are normally JSON serialized and unserialized as it has been almost a decade without json interfaces being implemented but I shall defer to the crypto experts @FiloSottile @agl and I shall retitle this issue as a feature request for json.Unmarshaler methods

@odeke-em odeke-em changed the title crypto/ecdsa: Private key json marshals fine but unmarshal returns an error proposal: crypto/ecdsa: make PrivateKey implement json.Unmarshaler Aug 10, 2019
@gopherbot gopherbot added this to the Proposal milestone Aug 10, 2019
@gopherbot gopherbot added the Proposal label Aug 10, 2019
@nulldowntime
Copy link
Author

@nulldowntime nulldowntime commented Aug 14, 2019

Thanks for the very detailed, and frankly awesome, answer. I'm sure being able to unmarshal ecdsa keys, in addition to rsa, would not be a bad thing :), so I naturally welcome the proposal.

My use case is somewhat arbitrary (storing ACME/Let's Encrypt account details in json format), but I can't imagine this not being useful to a lot of people as ecdsa becomes more widespread.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 15, 2019

I think implementing PublicKey.MarshalJSON and PublicKey.UnmarshalJSON so that they just use the name of the curve (for the common ones) would be nice. No need to actually encode the whole parameters.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Aug 15, 2019

Ah, also note that unmarshaling into CurveParams will force you into the generic implementation of the curve, which will be extremely slow. It's one of the issues of the Curve/CurveParams design, and why I wish we didn't have CurveParams, really.

@FiloSottile FiloSottile changed the title proposal: crypto/ecdsa: make PrivateKey implement json.Unmarshaler proposal: crypto/ecdsa: make PublicKey implement json.Unmarshaler and json.Marshaler Dec 2, 2019
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Dec 12, 2019

I just heard of another user being confused by this. I think we should do this, by calling x509.MarshalPKIXPublicKey from PublicKey.MarshalJSON and x509.ParsePKIXPublicKey from PublicKey.UnmarshalJSON, and simply encoding the []byte like encoding/json always does, as base64.

@rsc
Copy link
Contributor

@rsc rsc commented Dec 17, 2019

JSON should not be privileged. It may make more sense to implement MarshalBinary and MarshalText.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Dec 17, 2019

Yeah, even better. encoding/json unfortunately doesn't know about MarshalBinary, which feels like the most appropriate of the two, so let's do MarshalText instead? (Or do we want to open a proposal for encoding/json to use BinaryMarshaler?)

I think we should use PEM for MarshalText at this point. The newlines are kind of ugly and the malleability is unfortunate but there is no obvious justification for picking plain base64 in MarshalText.

@rsc
Copy link
Contributor

@rsc rsc commented Jan 15, 2020

OK, so it sounds like the suggestion is to implement MarshalText and use PEM as the text format.

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

Leaving open for a week for final comments.

@rsc rsc moved this from Active to Declined in Proposals Jan 15, 2020
@rsc rsc moved this from Declined to Likely Accept in Proposals Jan 15, 2020
@rsc rsc changed the title proposal: crypto/ecdsa: make PublicKey implement json.Unmarshaler and json.Marshaler proposal: crypto/ecdsa: make PublicKey implement encoding.TextMarshaler/TextUnmarshaler using PEM Jan 22, 2020
@rsc rsc moved this from Likely Accept to Accepted in Proposals Jan 22, 2020
@rsc
Copy link
Contributor

@rsc rsc commented Jan 22, 2020

No change in consensus, so accepted with title change.

@rsc rsc modified the milestones: Proposal, Backlog Jan 22, 2020
@rsc rsc changed the title proposal: crypto/ecdsa: make PublicKey implement encoding.TextMarshaler/TextUnmarshaler using PEM crypto/ecdsa: make PublicKey implement encoding.TextMarshaler/TextUnmarshaler using PEM Jan 22, 2020
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.15 Feb 4, 2020
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Feb 5, 2020

If this works out we might consider extending it to other PublicKey and maybe PrivateKey types, but I'm afraid we'll run into import loops with crypto/x509, where the encoding functions are.

@FiloSottile FiloSottile added NeedsFix and removed Proposal labels Mar 26, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 26, 2020

Change https://golang.org/cl/225638 mentions this issue: ecdsa: add marshaling support PrivateKey and PublicKey types

@d1str0
Copy link
Contributor

@d1str0 d1str0 commented Apr 14, 2020

Would love to see this implemented, especially since Lego, the most popular Let's Encrypt library for Go, uses ecdsa keys by default for ACME registration. These keys should be persisted and JSON can help this process for certain storage engines.

@odeke-em
Copy link
Member

@odeke-em odeke-em commented May 13, 2020

@odeke-em odeke-em modified the milestones: Go1.15, Go1.16 May 13, 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
7 participants
You can’t perform that action at this time.