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 · 17 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Unfortunate
Milestone

Comments

@nulldowntime
Copy link

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

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
@nulldowntime
Copy link
Author

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
Contributor

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
Contributor

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 added the Proposal-Crypto Proposal related to crypto packages or other security issues label Nov 22, 2019
@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
Contributor

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 commented Dec 17, 2019

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

@FiloSottile
Copy link
Contributor

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 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 (old) Jan 15, 2020
@rsc rsc moved this from Declined to Likely Accept in Proposals (old) 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 (old) Jan 22, 2020
@rsc
Copy link
Contributor

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
Contributor

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 The path to resolution is known, but the work has not been done. and removed Proposal labels Mar 26, 2020
@gopherbot
Copy link

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

@d1str0
Copy link
Contributor

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

@katiehockman
Copy link
Contributor

This is unfortunate.
I'm not seeing a way to do this with the current APIs of crypto/x509 and crypto/ecdsa. To take marshaling as an example, in order to implement crypto/ecdsa.PublicKey.MarshalText, crypto/ecdsa would need to import crypto/x509.MarshalPKIXPublicKey, which would lead to a circular dependency. There are several layers of interwoven dependencies between the two packages, which make this more complicated. The only way I can see this working is if we could export something like crypto/ecdsa.MarshalPublicKey which can then be imported by crypto/x509. But even with that implementation (which would require a proposal for a new exported function), we would still need to copy a decent amount of code from crypto/x509 to encode it correctly.

I'm definitely open to suggestions from anyone else with familiarity of the codebase, but based on my investigation, we would need to copy ~200-250 lines of code from crypto/x509 into crypto/ecdsa with how the APIs are built today. I don't think the technical debt associated with that is worth the benefit of adding a TextMarshaler/TextUnmarshaler for ecdsa public and private keys.

@odeke-em
Copy link
Member

odeke-em commented Mar 7, 2021

@katiehockman perhaps we could use an internal package of sorts?

@katiehockman
Copy link
Contributor

@odeke-em Can you explain the rationale for making an internal package for this? I'm not sure how that would help the reporter in this case.

@RmbRT
Copy link

RmbRT commented Jan 4, 2023

I think the idea was to extract the functionality that would lead to cyclical imports into an internal package which is then shared by both packages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done. Proposal-Accepted Proposal-Crypto Proposal related to crypto packages or other security issues Unfortunate
Projects
Status: Accepted
Development

No branches or pull requests

8 participants