crypto/x509: SystemCertPool documentation is not clear that modifications to the cert pool supplied are isolated from other pools returned by the function

[leighmcculloch]

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

go version go1.11 linux/amd64

Does this issue reproduce with the latest release?

Yes

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

GOARCH="amd64"
GOOS="linux"

What did you do?

Read the documentation for x509.SystemCertPool(). It looks like this today:

What did you expect to see?

I expected to see really clear distinction for whether the pointer I receive is to an x509.CertPool that is shared between all callers of x509.SystemCertPool(), and whether changes I make affect other copies of the pool returned by the function.

What did you see instead?

I saw the sentence below, that points out that mutation to the returned pool are not written to disk, and that changes to the pool I'm given do not impact other pools.

Any mutations to the returned pool are not written to disk and do not affect any other pool.

I think the intention with the second part of the statement is to call out that mutations to a certificate pool returned in one call are not visible in certificate pools returned in other calls to x509.SystemCertPool() either before or after the mutation, but that isn't unequivocally clear.

The statement do not affect any other pool is ambiguous as to who the any other pools are, because pools can be created in multiple ways that have nothing to do with the system certificate pool.

Example code I used to confirm that the code behaves this way

I ran the following code to confirm for myself whether x509.CertPools returned by x509.SystemCertPool() are isolated from each other and confirmed that they are.

package main

import (
        "crypto/x509"
        "fmt"
)

var exampleDotComCert = []byte(`-----BEGIN CERTIFICATE-----
MIIDETCCAfmgAwIBAgIIayinD9scg48wDQYJKoZIhvcNAQELBQAwIDEeMBwGA1UE
AxMVbWluaWNhIHJvb3QgY2EgMzdmYjk1MCAXDTE4MDgzMDIxMDc1MVoYDzIxMDgw
ODMwMjEwNzUxWjAWMRQwEgYDVQQDEwtleGFtcGxlLmNvbTCCASIwDQYJKoZIhvcN
AQEBBQADggEPADCCAQoCggEBAMA3GbSKBJJjkpv86KGJeLMOj06xC57jyJ78YH/K
Gbt9yYwC3jC0oyVqzXdH2FQ/U0s8THkBMfQ2bEyHCox9f5olBTnoK+hxEhPMIIxE
H8LwSVN4Wj+4gSXG5z7lqol9eoBB5vLH3bY0Y6DNrF61vH4VUXAuXMatRTdfht++
wCrHEXDX8oNQPH1PUjWVDNV+2JlTDPoowV0a39PS2AJKXLQli16JCgqPJI6DHuRT
ekVsPlFi0nu3L7rmQzrCn3Afu6trd75UK1nwVwALR8lthgXi1XfxUfCbPLeHbnlq
qqjipRBVMEnAW5aXfFrXfedTIzJEJ+xiGeLKZsmRTYlCNu8CAwEAAaNXMFUwDgYD
VR0PAQH/BAQDAgWgMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAMBgNV
HRMBAf8EAjAAMBYGA1UdEQQPMA2CC2V4YW1wbGUuY29tMA0GCSqGSIb3DQEBCwUA
A4IBAQAmeIKQp8sNPhYwolbnaxBe/bCdCYceGI978DupaSH01MbIbWzNQSIbz3G+
S9pWQVHTXsnvtGYUexMVHST8rt4J3UAxmrIy8se18N8/fjCmN/d7T89FrBrmUwC/
6YkcQWwGxM0L/sAUFmMD5lkHeosLzqvk2yOaZNuJ9aLKP3XH0m+K4DIQKgZmX0IK
UejbaBVMJ7YcLKKVmGtxY7Qzr6X/xdx2poTrJG3UXbkmgUHgPKAxhdMV1kLnw0Nc
TXSpQWwb7MutD5umcDKleojDG7lYlcK/KnOsQ4ylVG3Ly5PPDAVr5ZBM2O4LgG3V
IXnjtakPSHliRFagPlh47Qt2P+p4
-----END CERTIFICATE-----
`)

func main() {
        fmt.Println("getting system certificate pool 1")
        scp1, err := x509.SystemCertPool()
        panicIfErr(err)
        fmt.Println("system certificate pool 1:", len(scp1.Subjects()))

        fmt.Println("getting system certificate pool 2")
        scp2, err := x509.SystemCertPool()
        panicIfErr(err)
        fmt.Println("system certificate pool 2:", len(scp2.Subjects()))
        fmt.Println("adding a certificate for example.com to system certificate pool 2")
        scp2.AppendCertsFromPEM(exampleDotComCert)
        fmt.Println("system certificate pool 2:", len(scp2.Subjects()))

        fmt.Println("getting system certificate pool 3")
        scp3, err := x509.SystemCertPool()
        panicIfErr(err)
        fmt.Println("system certificate pool 3:", len(scp3.Subjects()))

        fmt.Println()

        fmt.Println("system certificate pool 1:", len(scp1.Subjects()))
        fmt.Println("system certificate pool 2:", len(scp2.Subjects()))
        fmt.Println("system certificate pool 3:", len(scp3.Subjects()))
}

func panicIfErr(err error) {
        if err != nil {
                panic(err)
        }
}

Outputs:

getting system certificate pool 1
system certificate pool 1: 151
getting system certificate pool 2
system certificate pool 2: 151
adding a certificate for example.com to system certificate pool 2
system certificate pool 2: 152
getting system certificate pool 3
system certificate pool 3: 151

system certificate pool 1: 151
system certificate pool 2: 152
system certificate pool 3: 151

Proposal

Change the documentation to read:

Any mutations to the returned pool are not written to disk and do not affect any other pool returned by SystemCertPool.

[gopherbot]

Change https://golang.org/cl/132378 mentions this issue: crypto/x509: clarify docs for SystemCertPool

[FiloSottile]

I feel like "makes a copy" also helps here, but the clearer the better.

[leighmcculloch]

@FiloSottile The "makes a copy" on the first line helped too for sure, but I found it ambiguous in the context of the next line saying it returns "the" pool, and not "a" pool. I wasn't sure if "copy" meant a copy of what's on disk, or that it literally made a copy on every call.

(I forgot to mention the "the" vs "a" on this issue before I opened the CL.)

[gopherbot]

Change https://golang.org/cl/132776 mentions this issue: crypto/x509: revert the to a in SystemCertPool docs