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: Can 'CertPool.contains()' be made public? #39179

Open
stephen-fox opened this issue May 20, 2020 · 7 comments
Open

crypto/x509: Can 'CertPool.contains()' be made public? #39179

stephen-fox opened this issue May 20, 2020 · 7 comments
Milestone

Comments

@stephen-fox
Copy link

@stephen-fox stephen-fox commented May 20, 2020

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

$ go version
go version go1.13.7 darwin/amd64

Does this issue reproduce with the latest release?

Yes. The x509.CertPool related source code in release-branch.go1.14 does not appear to have changed, and is not public.

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

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE=""
GOENV=""
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY=""
GONOSUMDB=""
GOOS="darwin"
GOPATH="go"
GOPRIVATE=""
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/opt/pkg/go113"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/opt/pkg/go113/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
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/zz/l8crt7256_vfh01b32jqp7yc0000gn/T/go-build669682046=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Searched the go source code for alternative paths to CertPool.contains(). I have not found an alternative yet.

What did you expect to see?

A public equivalent to CertPool.contains(), or another code path that accesses the method by taking a CertPool as input (e.g.,x509.IsCertIn(*Certificate, *CertPool) bool).

What did you see instead?

There does not appear to be any public code to determine if a CertPool contains a given certificate.

Notes

I would like to make x509.CertPool.contains() public. Before submitting any changes, I figured I would ask :) I do not see any (obvious) reasons why it would be kept private in the source code, or in git blame.

Thank you for reading.

  • Stephen
@icholy
Copy link

@icholy icholy commented May 20, 2020

@stephen-fox what's your use-case?

@stephen-fox
Copy link
Author

@stephen-fox stephen-fox commented May 21, 2020

@icholy, I am working on a library that is utilized by a TLS server checking tool. One of the data points I would like to examine is whether a certificate provided by a TLS server is already in the CertPool containing the trusted CAs.

@cagedmantis cagedmantis added this to the Backlog milestone May 21, 2020
@cagedmantis
Copy link
Contributor

@cagedmantis cagedmantis commented May 21, 2020

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 21, 2020

You can do a map lookup of RawSubject against CertPool.Subjects(). Does that solve your use case?

@stephen-fox
Copy link
Author

@stephen-fox stephen-fox commented May 21, 2020

Thank you for the suggestion. I initially thought the same thing. However, that only provides the subject field of each certificate in the CertPool.

In addition to the map lookup by subject, the CertPool.contains() method appears to use the Certificate.Equals() method to check certificate equality, which is a lot safer than only examining the subject. Leveraging the existing code would be less error prone, and more efficient than anything I could write outside of the x509 library.

@FiloSottile
Copy link
Member

@FiloSottile FiloSottile commented May 21, 2020

For your purposes (checking if the server sent a root) the Subject should be enough: the only case I can think of in which a server would want to send a certificate with a Subject matching a trusted root is if it's a compatibility cross-cert, and you can simply check for that by verifying it's not self-signed.

Exposing contains would force us to retain full representations of the roots in memory, while we have pending CLs that lazy load them for performance, relying on the limited API, and we might not even have the full certificate for the system pool on some platforms (like Windows).

@stephen-fox
Copy link
Author

@stephen-fox stephen-fox commented May 22, 2020

Interesting. You will have to forgive my naïve understanding of the x509 library - for my use case, there are two paths to initializing a CertPool:

  • Creating an empty CertPool and then loading a PEM file / directory full of PEM files using the AppendCertsFromPEM() method
  • Calling x509.SystemCertPool() (which does not appear to support Windows)

If I am understanding you correctly, the CertPool may not contain a full representation of a certificate depending how the CertPool was initialized?

My concern with using the Subjects() method is that a certificate provided by a TLS server may have the same subject as one in the pool, but might be a completely different certificate. My use case is a bit different than a standard TLS client in that my library can connect to TLS servers with validation disabled (tls.Config.InsecureSkipVerify = true). The purpose of this is falling back to collect certificates offered by the server for examination in the case of TLS validation / handshake failure. I wonder what would happen if a certificate in the CertPool expired, and the remote copy was replaced with a new certificate containing the same subject?

A few questions too - what do you mean by "CLs"? Also, when you say "relying on a limited API", are you referring to the CA trust store APIs offered by operating systems? I have been following your trials and tribulations with macOS, so that is my first guess :)

Thank you for bearing with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

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