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: SystemCertPool documentation is not clear that modifications to the cert pool supplied are isolated from other pools returned by the function #27385

Closed
leighmcculloch opened this issue Aug 30, 2018 · 4 comments
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@leighmcculloch
Copy link
Contributor

leighmcculloch commented Aug 30, 2018

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:
screen shot 2018-08-30 at 3 21 15 pm

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.

@leighmcculloch leighmcculloch changed the title crypto/x509: SystemCertPool documentation is not clear that the cert pool supplied is a copy and modifications to a pool returned are isolated from other pools returned by the function crypto/x509: SystemCertPool documentation is not clear that modifications to the cert pool supplied are isolated from other pools returned by the function Aug 30, 2018
@gopherbot
Copy link

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

@FiloSottile
Copy link
Contributor

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

@FiloSottile FiloSottile added the NeedsFix The path to resolution is known, but the work has not been done. label Aug 30, 2018
@FiloSottile FiloSottile added this to the Go1.12 milestone Aug 30, 2018
@leighmcculloch
Copy link
Contributor Author

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

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

gopherbot pushed a commit that referenced this issue Sep 1, 2018
The words 'the returned' were changed to 'a returned' in
8201b92 when referring to the value
returned by SystemCertPool. Brad Fitz pointed out after that commit was
merged that it makes the wording of this function doc inconsistent with
rest of the stdlib since 'a returned' is not used anywhere, but 'the
returned' is frequently used.

Fixes #27385

Change-Id: I289b533a5a0b5c63eaf0abb6dec0085388ecf76b
GitHub-Last-Rev: 6c83b80
GitHub-Pull-Request: #27438
Reviewed-on: https://go-review.googlesource.com/132776
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@golang golang locked and limited conversation to collaborators Sep 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Documentation FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

3 participants