crypto/rsa: rsa.PrivateKey with json.Unmarshal and Go1.20 results in slow keys

[evanj]

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

$ go version
go version go1.20.3 darwin/arm64

Does this issue reproduce with the latest release?

Yes

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

go env Output

$ go env
GO111MODULE=""
GOARCH="arm64"
GOBIN=""
GOCACHE="/Users/evan.jones/Library/Caches/go-build"
GOENV="/Users/evan.jones/Library/Application Support/go/env"
GOEXE=""
GOEXPERIMENT=""
GOFLAGS=""
GOHOSTARCH="arm64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="/Users/evan.jones/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="/Users/evan.jones/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/homebrew/Cellar/go/1.20.3/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/homebrew/Cellar/go/1.20.3/libexec/pkg/tool/darwin_arm64"
GOVCS=""
GOVERSION="go1.20.3"
GCCGO="gccgo"
AR="ar"
CC="cc"
CXX="c++"
CGO_ENABLED="1"
GOMOD="/Users/evan.jones/rsajsongo120/go.mod"
GOWORK=""
CGO_CFLAGS="-O2 -g"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-O2 -g"
CGO_FFLAGS="-O2 -g"
CGO_LDFLAGS="-O2 -g"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch arm64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/g1/97d8s0r57hj4nv4_qd3fqcrm0000gp/T/go-build3310032317=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

If you create an rsa.PrivateKey, then call json.Marshal, then call json.Unmarshal, the resulting key "works" but is 3X slower for signing operations. The problem is that Go 1.20 added private fields to rsa.PrecomputedValues which do not get round-tripped. The resulting keys need to have key.Precompute() called to restore their performance. The following test fails on Go 1.20, but works on Go 1.19. The included benchmark demonstrates the performance problem.

From Issue #59442 , it seems like we may want to preserve the existing behavior: #59442 (comment)

package main

import (
	"crypto"
	"crypto/rand"
	"crypto/rsa"
	"encoding/json"
	"reflect"
	"testing"
)

func mustGenerateRSA() *rsa.PrivateKey {
	privateKey, err := rsa.GenerateKey(rand.Reader, 2048)
	if err != nil {
		panic(err)
	}
	return privateKey
}

func roundTripUnmarshal(privateKey *rsa.PrivateKey) *rsa.PrivateKey {
	jsonBytes, err := json.Marshal(privateKey)
	if err != nil {
		panic(err)
	}
	roundTripped := &rsa.PrivateKey{}
	err = json.Unmarshal(jsonBytes, roundTripped)
	if err != nil {
		panic(err)
	}
	return roundTripped
}

func TestJSONRoundTrip(t *testing.T) {
	orig := mustGenerateRSA()
	roundTripped := roundTripUnmarshal(orig)
	if !reflect.DeepEqual(orig, roundTripped) {
		t.Errorf("expected JSON Unmarshal key to be the same;\n  orig=%#v\n  roundTripped=%#v",
			orig, roundTripped)
	}
}

func BenchmarkJSONRoundTrip(b *testing.B) {
	orig := mustGenerateRSA()
	roundTripped := roundTripUnmarshal(orig)

	b.Run("orig", func(b *testing.B) {
		signBenchmark(b, orig)
	})
	b.Run("roundTripped", func(b *testing.B) {
		signBenchmark(b, roundTripped)
	})
	roundTripped.Precompute()
	b.Run("roundTripped_after_Precompute", func(b *testing.B) {
		signBenchmark(b, roundTripped)
	})
}

func signBenchmark(b *testing.B, key *rsa.PrivateKey) {
	zeroDigest := make([]byte, 32)
	for i := 0; i < b.N; i++ {
		_, err := key.Sign(rand.Reader, zeroDigest, crypto.SHA256)
		if err != nil {
			b.Fatal(err)
		}
	}
}

What did you expect to see?

The test should pass, and the benchmark should show the two cases have approximately the same performance, like on Go 1.19:

goos: darwin
goarch: arm64
pkg: example.com/rsajsongo120
BenchmarkJSONRoundTrip/orig-10         	    1179	   1022081 ns/op
BenchmarkJSONRoundTrip/roundTripped-10 	    1166	   1024418 ns/op
BenchmarkJSONRoundTrip/roundTripped_after_Precompute-10         	    1172	   1018375 ns/op
PASS

What did you see instead?

The test fails, showing that the round tripped key has nil fields:

          roundTripped=&rsa.PrivateKey{PublicKey:rsa.PublicKey{N:24...37, E:65537}, D:21..33, Primes:[]*big.Int{...}, Precomputed:rsa.PrecomputedValues{Dp:12...13, Dq:10...41, Qinv:13...24, CRTValues:[]rsa.CRTValue{}, n:(*bigmod.Modulus)(nil), p:(*bigmod.Modulus)(nil), q:(*bigmod.Modulus)(nil)}}
FAIL

The benchmark shows ~3X slower signing until calling Precompute():

goos: darwin
goarch: arm64
pkg: example.com/rsajsongo120
BenchmarkJSONRoundTrip/orig-10         	     972	   1243100 ns/op
BenchmarkJSONRoundTrip/roundTripped-10 	     285	   4164388 ns/op
BenchmarkJSONRoundTrip/roundTripped_after_Precompute-10         	     961	   1251028 ns/op
PASS
ok  	example.com/rsajsongo120	4.482s

[rolandshoemaker]

It seems like any of the obvious solutions involve satisfying the json.Unmarshaler interface, which will require adding a new API, and therefore needs to go through the proposal process. Unless @FiloSottile you had another idea?

[mateusz834]

@rolandshoemaker How about atomic.Pointer? Is it fine to use it in crypto/rsa?
Edit: Note that this issue might still affect other marshallers (yaml, ...) when only adding custom json.Unmarshaler.

[seankhliao]

What if decrypt() called Precompute() instead of falling back?
PrecomputedValues has unexported fields that could hide a sync.Once

[FiloSottile]

We discussed this at the time with @rsc and found no good solution because any write, even by a sync.Once or sync/atomic, to PrivateKey could be detected by the race detector if it happened at the same time as a copy, which so far was allowed.

The good news though is that https://go.dev/cl/632478 now mostly reuses the public precomputed values if they are available, so the penalty of missing the private result of Precompute is down to 7.5% of a signature.

goos: darwin
goarch: arm64
pkg: crypto/rsa
cpu: Apple M2
                            │      -      │
                            │   sec/op    │
SignPSS/2048-8                855.1µ ± 4%
ParsePKCS8PrivateKey/2048-8   65.77µ ± 0%

(I am using ParsePKCS8PrivateKey as an indicative benchmark of "taking a PrivateKey with public precomputed values and filling out the rest".)

I think this effectively resolves this issue.

[gopherbot]

Change https://go.dev/cl/632478 mentions this issue: crypto/x509: keep RSA CRT values in ParsePKCS1PrivateKey