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/rsa: rsa.PrivateKey with json.Unmarshal and Go1.20 results in slow keys #59695

Open
Tracked by #57752
evanj opened this issue Apr 18, 2023 · 3 comments
Open
Tracked by #57752
Assignees
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@evanj
Copy link
Contributor

evanj commented Apr 18, 2023

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 rolandshoemaker added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 18, 2023
@FiloSottile FiloSottile self-assigned this Apr 19, 2023
@FiloSottile FiloSottile added this to the Go1.21 milestone Apr 19, 2023
@rolandshoemaker
Copy link
Member

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
Copy link
Member

mateusz834 commented Apr 22, 2023

@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
Copy link
Member

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

@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
@gopherbot gopherbot modified the milestones: Go1.22, Go1.23 Feb 6, 2024
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. Performance
Projects
None yet
Development

No branches or pull requests

6 participants