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/x509: enable deep copy of x509.CertPool #24540

Closed
frankgreco opened this issue Mar 26, 2018 · 5 comments
Closed

crypto/x509: enable deep copy of x509.CertPool #24540

frankgreco opened this issue Mar 26, 2018 · 5 comments

Comments

@frankgreco
Copy link

@frankgreco frankgreco commented Mar 26, 2018

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

go version go1.10 darwin/amd64

Does this issue reproduce with the latest release?

yes

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

GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/gre9521/Library/Caches/go-build"
GOEXE=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH="/Users/gre9521/Documents/projects/gopath"
GORACE=""
GOROOT="/usr/local/go"
GOTMPDIR=""
GOTOOLDIR="/usr/local/go/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
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/1w/rr53y3313fg626_7gcq4cprc0000gn/T/go-build284290677=/tmp/go-build -gno-record-gcc-switches -fno-common"

Feature Request

Currently, because x509.CertPool contains no exported fields, it is impossible to create a deep copy. For performance reasons, it might be desired to compute x509.SystemCertPool() only once and extract of copy of it for each request if dynamic TLS needs to be configured.

As a workaround, I could currently do one of the following:

  • copy the code in the crypto/x509 package that load the cert pool
  • use a curated root ca pool and not use the system cert pool

Ideally, I would like to be able to make a deep copy of *x509.CertPool.

@FiloSottile FiloSottile changed the title Deep Copy of x509.CertPool crypto/x509: enable deep copy of x509.CertPool Mar 26, 2018
@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Mar 26, 2018

The use case is to call x509.SystemCertPool() and then make different AddCert/AppendCertsFromPEM calls every time, correct?

We could look at this as a proposal to add (*x509.CertPool).Copy() *x509.CertPool, but maybe we should just cache x509.SystemCertPool() at the first execution, and then return copies, solving the performance issues. I wonder if there is any real use case for reloading the system cert pool.

@frankgreco
Copy link
Author

@frankgreco frankgreco commented Mar 26, 2018

@FiloSottile yes, your use case assumption is correct.

Concerning a use case, you wouldn't be reloading the cert pool, just establishing a common base for which to build different, dynamic RootCA bundles from.

If it is decided that this is okay to be implemented, i'd be willing to provide the implementation.

I think your suggestion to cache the pool is a promising one as it doesn't change the existing API. Of course, as you alluded to, it would technically be a change in behavior and so anyone that was expecting numerous calls to SystemCertPool() to return the latest modified pool would have to modify their code.

@adamdecaf
Copy link
Contributor

@adamdecaf adamdecaf commented Mar 26, 2018

Interestingly there is some caching already. (c *Certificate) Verify(opts VerifyOptions) calls into code which uses a cached system root pool. Looking at this code now I notice there's a possible race between code calling SystemCertPool() and the cache used in Verify(..).

https://github.com/golang/go/blob/go1.10/src/crypto/x509/root.go#L20-L21

if opts.Roots == nil {
opts.Roots = systemRootsPool()

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented Mar 26, 2018

Good catch. Looks like SystemCertPool intentionally exposes loading, but only as a way of solving the singleton being modified. a62ae9f Making a copy every time would be just as good.

I think it would be even more consistent to document that loading happens only once and SystemCertPool returns identical copies, it would optimize the common use case and it would simplify the code by joining the two codepaths.

Let's do that.

@andybons andybons added this to the Go1.11 milestone Mar 26, 2018
@gopherbot
Copy link

@gopherbot gopherbot commented Mar 27, 2018

Change https://golang.org/cl/102699 mentions this issue: crypto/x509: cache the result of SystemCertPool

@gopherbot gopherbot closed this in a25d0d8 Mar 27, 2018
@golang golang locked and limited conversation to collaborators Mar 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.