ugorji / go

idiomatic codec and rpc lib for msgpack, cbor, json, etc. msgpack.org[Go]
MIT License
1.86k stars 295 forks source link

Extension encfn gets sent zero value unexpectedly #321

Closed jim-minter closed 4 years ago

jim-minter commented 5 years ago

I'm using github.com/ugorji/go/codec to deserialise a JSON object to a struct containing a *rsa.PrivateKey field. In the raw JSON, the private key appears as a base64 encoded string containing the DER bytes.

I can use either of codec.JsonHandle.AddExt or codec.JsonHandle.SetInterfaceExt to customise serialisation/deserialisation to handle the above case. When I encode (from struct to JSON), I find that the encoder gets called as expected. However, I find that when I decode, the encoder unexpectedly gets called with a &rsa.PrivateKey{}, then the decoder is called as expected.

This is problematic in the *rsa.PrivateKey case as the zero value rsa.PrivateKey is invalid and x509.MarshalPKCS1PrivateKey panics when attempting to marshal it. More widely, I wonder if it's a logic error for github.com/ugorji/go/codec to call the encoder on decode?

The following shows the issue. The section commented // Why is this needed? is a workaround:

package main

import (
    "bytes"
    "crypto/rand"
    "crypto/rsa"
    "crypto/x509"
    "fmt"
    "log"
    "reflect"

    "github.com/ugorji/go/codec"
)

type JSONKey struct {
    Key *rsa.PrivateKey `json:"key,omitempty"`
}

func run() error {
    h := &codec.JsonHandle{}

    err := h.AddExt(reflect.TypeOf(&rsa.PrivateKey{}), 0,
        func(v reflect.Value) ([]byte, error) {
            log.Println("encoding...")
            if reflect.DeepEqual(v.Elem().Interface(), rsa.PrivateKey{}) { // Why is this needed?
                log.Println("found zero value!")
                return nil, nil
            }
            return x509.MarshalPKCS1PrivateKey(v.Interface().(*rsa.PrivateKey)), nil
        },
        func(v reflect.Value, b []byte) error {
            log.Println("decoding...")
            key, err := x509.ParsePKCS1PrivateKey(b)
            if err != nil {
                return err
            }
            v.Elem().Set(reflect.ValueOf(key).Elem())
            return nil
        },
    )
    if err != nil {
        return err
    }

    buf := &bytes.Buffer{}

    {
        var s JSONKey
        s.Key, err = rsa.GenerateKey(rand.Reader, 2048)
        if err != nil {
            return err
        }

        e := codec.NewEncoder(buf, h)
        err = e.Encode(s)
        if err != nil {
            return err
        }

        fmt.Println(buf.String())
    }

    {
        var s JSONKey

        d := codec.NewDecoder(buf, h)
        err = d.Decode(&s)
        if err != nil {
            return err
        }

        fmt.Printf("%#v\n", *s.Key)
    }

    return nil
}

func main() {
    if err := run(); err != nil {
        panic(err)
    }
}

Sample output is:

2019/10/14 15:28:59 encoding...
{"key":"MII..."}
2019/10/14 15:28:59 encoding...
2019/10/14 15:28:59 found zero value!
2019/10/14 15:28:59 decoding...
rsa.PrivateKey{...}

In the above output, I would expect to see the text encoding... once only.

ugorji commented 4 years ago

AddExt is deprecated.

Please use SetInterfaceExt instead.

Also, what you have are not the encoder being called twice. But one of the callbacks is being called as part of the machination. I think it gets confusing if you are thinking of a callback as exclusively for encoding or decoding.

For understanding, see https://github.com/ugorji/go/blob/codec/v1.1.7/codec/decode.go#L1812

ugorji commented 4 years ago

Please reply and tag me if you still see an issue here.