crypto/x509: CertPools do not equal each other in 1.16

[pcman312]

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

$ go version
go version go1.16.3 darwin/amd64

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="amd64"
GOBIN=""
GOCACHE="$HOME/Library/Caches/go-build"
GOENV="$HOME/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOINSECURE=""
GOMODCACHE="$HOME/go/pkg/mod"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="$HOME/go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="$HOME/dev/go-bin/go"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="$HOME/dev/go-bin/go/pkg/tool/darwin_amd64"
GOVCS=""
GOVERSION="go1.16.3"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD="$HOME/go/src/github.com/hashicorp/vault/go.mod"
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -arch x86_64 -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/_x/qxg_n2l15nzblrs1_sm_5_qw0000gp/T/go-build3017105958=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Create two *x509.CertPool objects from the same PEM []byte and then compare them with reflect.DeepEqual:
https://play.golang.com/p/ktVmOf0Qo9W

What did you expect to see?

They should equal each other

What did you see instead?

They did not equal each other

Additional notes

I ran this against Go 1.15.11 and they came out as equal, but in 1.16.3 (and 1.16.2) they are not equal. I also got the same results when wrapping the CertPool in a *tls.Config then comparing against a Cloned config.

[seankhliao]

cc @FiloSottile and @bradfitz for CL 230025

[FiloSottile]

I think reflect.DeepEqual is firmly outside the Go 1 Compatibility Promise. The supported way to do this would be to compare the Subjects().

[cherrymui]

Thanks @FiloSottile . Closing as working as intended. Thanks.

[pcman312]

I think reflect.DeepEqual is firmly outside the Go 1 Compatibility Promise.

Where is this documented? I can't seem to find this exception.

[cherrymui]

In general, types that contains unexported fields are not supposed to be compared by value, because it compares internal fields, which are internal implementation details. DeepEqual is just worse.

[dnephin]

What do you think about adding an Equal(other CertPool) bool method to x509.CertPool to support more complete comparison. I believe go-cmp will use that automatically once it is added.

I'm happy to open a separate issue for discussion if that seems like it might be an option.

[pcman312]

@FiloSottile I don't think that comparing Subjects() is a sufficient solution to this problem. Multiple certificates can have the same subject, but otherwise different data (key, serial number, expiration date, etc.) but will be seen as identical when comparing just using Subjects(). See example: https://play.golang.com/p/pOvfLP6_O1X. This comparison doesn't allow for comparison of expiration dates or any other values within the certificates in the pool. I agree with @dnephin 's suggestion that an alternative Equals function or some way of doing introspection on the certificates in the pool is a better option.

I have also not found any documentation before now stating that reflect.DeepEquals (or the reflect package as a whole) does not adhere to the Go 1 compatibility promise. If this is true, it should be discussed and documented.

[cespare]

I have also not found any documentation before now stating that reflect.DeepEquals (or the reflect package as a whole) does not adhere to the Go 1 compatibility promise. If this is true, it should be discussed and documented.

reflect.DeepEqual itself does adhere to the Go 1 compatibility promise. (It has not changed.)

The question is whether equality under reflect.DeepEqual is a property preserved by the Go 1 compatibility promise. The answer must be that it is not: if it were, that would preclude changes to unexported struct fields. The Go 1 compatibility document doesn't explicitly say that changes to unexported fields are allowed (perhaps it ought to), but note that it does provide for the addition of new exported fields which can also change DeepEqual equality.

[dnephin]

I opened a new issue about adding the Equal method: #46057.