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

Jump to bottom

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
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Milestone
Go1.11

Comments

@frankgreco
Copy link

frankgreco commented Mar 26, 2018 •
edited

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 FiloSottile added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Mar 26, 2018
@FiloSottile
Copy link
Contributor

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

@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 commented Mar 26, 2018 •
edited

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

go/src/crypto/x509/verify.go

Lines 827 to 828 in 4b1d704

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

@FiloSottile
Copy link
Contributor

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

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

@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.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made.
Projects
None yet
Milestone
Go1.11
Development

No branches or pull requests
5 participants
@adamdecaf @andybons @FiloSottile @gopherbot @frankgreco