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 · 8 comments
Open

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

stephen-fox opened this issue May 20, 2020 · 8 comments
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone

Comments

@stephen-fox
Copy link

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 commented May 20, 2020

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

@stephen-fox
Copy link
Author

@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 the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label May 21, 2020
@cagedmantis cagedmantis added this to the Backlog milestone May 21, 2020
@cagedmantis
Copy link
Contributor

/cc @FiloSottile

@FiloSottile
Copy link
Contributor

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

@stephen-fox
Copy link
Author

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
Contributor

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

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.

@rossigee
Copy link

rossigee commented Oct 1, 2023

Subjects has been deprecated anyway, so not worth considering that. The depracation documentation seems to be lacking a good explanation of the reason, and nothing has been provided to replace it 🤦 All the tickets that were discussing these have since been 'FrozenDueToAge'.

Looks to me like this fundamental gap that's been opened up in the API has caused people to have to fork x509 all over the place 🙄 And now it looks like I might have to also 😭

FTR, my use case is simply to be able to count or iterate the certificates in a pool that has been created and passed to my function, so I can log a debug line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Development

No branches or pull requests

5 participants