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: use platform verifier on Windows, macOS and iOS #46287

Closed
FiloSottile opened this issue May 20, 2021 · 10 comments
Closed

crypto/x509: use platform verifier on Windows, macOS and iOS #46287

FiloSottile opened this issue May 20, 2021 · 10 comments
Labels
Milestone

Comments

@FiloSottile
Copy link
Contributor

Background

crypto/x509 comes with a pretty good chain builder and verifier, but it doesn’t ship a root store, the set of root CA certificates that leaf certificates need to chain to. Instead, x509.Verify either takes an explicit CertPool, or uses “the system roots”.

“The system roots” are mostly a lie (#39540). The idea is that the platform should be in charge of trust decisions, and that you’d want a Go CLI tool to work like the browser on the same machine (including trusting any “enterprise” or development roots). In practice no modern root store can be captured by a static list of roots: many have additional rules such as “this root can only sign certificates that end in .fr” or “this root can only sign this specific set of certificates” or “this root can only sign certificates logged to CT”. Moreover, platforms also have space for CA certificates that are not trusted, but can be used to build chains.

Our current API leaks the idea that system roots are just a list of trusted CAs: x509.SystemCertPool returns a CertPool that can be used with Verify like a CertPool full of custom certificates. It has no visible space for custom logic or untrusted intermediates.

On macOS, we go to great lengths to apply the complex root store logic to extract the list of certificates that are unconditionally trusted, by invoking the Security.framework APIs. This is fickle and complicated logic (golang.org/cl/227037, #38888, #42414), prone to security-relevant errors, slow (#19561), memory intensive (#26731), with no support for reloading (#41888, #35887), and with no support for untrusted intermediates (#35631).

On iOS, we don’t have access to the root store, so we hardcode a copy of it, which we do our best to keep in sync (#38843), but which has even harder to avoid memory usage issues.

On Windows, x509.SystemCertPool is not supported (#16736) and a Verify with a nil Roots invokes the platform verifier through the platform APIs.

On UNIX, there isn’t really a consistent platform root store story, and roots are mostly a list of certificates you can find in one of many somewhat arbitrary paths. Our logic for finding roots kind of grew organically and can be improved (#38869), but this is the platform for which the API is a better fit. There is no platform verifier.

Proposal

We’d like to move to using the platform verifier where available, so on macOS, iOS, and Windows. When Verify is invoked with a nil Roots, we will pass the rest of the VerifyOptions to the platform, and return the chain the platform built, if any. If Roots is set to a custom CertPool, we will keep using our chain builder.

In order not to break existing code that invokes x509.SystemCertPool and then adds extra custom certificates to it, we’ll have Verify perform two verifications in that case, one with the platform and one with the custom roots, and return chains from both.

This will resolve the loading time issue, the memory issue, the risk of mistakes in root extraction code, the need for reloading the root store, the lack of support for untrusted intermediates, and will more correctly apply the custom platform logic.

Open questions

Alignment

There are a few verification rules that we want to apply regardless of the platform:

  1. Name Constraints are applied to all names on the certificate. This is critical because VerifyHostname can only see the leaf, so it needs to be able to trust that if Verify returned it, the certificate is authorized to claim all those names.

  2. Extended Key Usage values are enforced nested down a chain.

  3. IP addresses and certain invalid DNS names are supported, like in VerifyHostname.

If the platform verifier does not align with these rules, it might be hard to compensate for it, because these rules influence chain building itself.

SystemCertPool().Subjects()

The CertPool returned by SystemCertPool() will be just a placeholder that says “use the platform verifier”. That doesn’t work with the Subjects method, though, because the list of roots is not in fact available.

We have two options: 1) we keep all the complicated, slow, inaccurate, and out-of-sync macOS and iOS code to extract roots and only use it to populate Subjects, and leave SystemCertPool unsupported on Windows; or 2) we deprecate Subjects, making CertPool fully opaque.

(2) lets us easily implement Equal (#46057) and Copy (#35044), but not Contains (#39179).

/cc @rolandshoemaker @golang/security

@FiloSottile FiloSottile added the Proposal-Crypto Proposal related to crypto packages or other security issues label May 20, 2021
@gopherbot gopherbot added this to the Proposal milestone May 20, 2021
@jfesler
Copy link

jfesler commented May 21, 2021

I would greatly appreciate using the system verifier - but not if this requires using the Xcode linker.

The background for this comment: Less about x509, and more about DNS resolution. Darwin builds, built from linux containers, have no access to the system's resolver. And on that platform, DNS is not simply /etc/resolv.conf - the operating system has a complex set of rules that support routing dns queries by domain to different services. Tools built from linux build farms end up reporting problems like "host not found" for internal domains.

I'd hate for us to repeat that for x509 as well.

An additional important use case for not breaking x509.SystemCertPool, is for docker containers built from scratch, just the single go binary. I suspect I'm not the only one who's learned how to bundle enterprise roots into apps.

@AlekSi
Copy link
Contributor

AlekSi commented May 21, 2021

And on that platform, DNS is not simply /etc/resolv.conf

I like the analogy with DNS. Arguably, "DNS is not simply /etc/resolv.conf" on any platform: https://tailscale.com/blog/sisyphean-dns-client-linux/ (check second to the last diagram). X.509 handling has a similar complexity.

@rsc
Copy link
Contributor

rsc commented May 26, 2021

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@FiloSottile
Copy link
Contributor Author

I would greatly appreciate using the system verifier - but not if this requires using the Xcode linker.

It won't. We'll use the same cgo-less linking technique we use to get the roots. I am actually surprised DNS resolution doesn't, since we've been linking System internally for a few releases now.

An additional important use case for not breaking x509.SystemCertPool, is for docker containers built from scratch, just the single go binary. I suspect I'm not the only one who's learned how to bundle enterprise roots into apps.

How does that involve x509.SystemCertPool? Also, Docker mostly runs Linux, which is out of scope of this proposal.

@jfesler
Copy link

jfesler commented May 29, 2021

I think you covered my concerns already (after I reread the proposal a few times); in particular:

In order not to break existing code that invokes x509.SystemCertPool and then adds extra custom certificates to it, we’ll have Verify perform two verifications in that case, one with the platform and one with the custom roots, and return chains from both.

Thank you.

@rsc
Copy link
Contributor

rsc commented Jun 2, 2021

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@rsc
Copy link
Contributor

rsc commented Jun 9, 2021

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: crypto/x509: use platform verifier on Windows, macOS and iOS crypto/x509: use platform verifier on Windows, macOS and iOS Jun 9, 2021
@rsc rsc modified the milestones: Proposal, Backlog Jun 9, 2021
@FiloSottile FiloSottile modified the milestones: Backlog, Go1.18 Sep 23, 2021
praveenkumar added a commit to praveenkumar/proxy that referenced this issue Sep 28, 2021
As per https://golang.org/src/crypto/x509/cert_pool.go looks like there
is no implementation of  `SystemCertPool` for windows platform and it
just return the error.
```
func SystemCertPool() (*CertPool, error) {
	if runtime.GOOS == "windows" {
		// Issue 16736, 18609:
		return nil, errors.New("crypto/x509: system root pool is
not available on Windows")
	}
....
```

- golang/go#16736
- golang/go#46287
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353132 mentions this issue: crypto/x509: use platform verifier on darwin

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353403 mentions this issue: crypto/x509: use the platform verifier on iOS

@gopherbot
Copy link
Contributor

Change https://golang.org/cl/353589 mentions this issue: crypto/x509: verification with system and custom roots

gopherbot pushed a commit that referenced this issue Nov 5, 2021
When VerifyOptions.Roots is nil, default to using the platform X.509
certificate verification APIs on darwin, rather than using the Go
verifier. Since our oldest supported version of macOS is 10.12, we are
able to use the modern verification APIs, and don't need to resort to
the complex chain building trickery employed by chromium et al.

Unfortunately there is not a clean way to programmatically add test
roots to the system trust store that the builders would tolerate. The
most obvious solution, using 'security add-trusted-cert' requires human
interaction for authorization. We could also manually add anchors to
the constructed SecTrustRef, but that would require adding a whole
bunch of plumbing for test functionality, and would mean we weren't
really testing the actual non-test path. The path I've chosen here is
to just utilize existing valid, and purposefully invalid, trusted
chains, from google.com and the badssl.com test suite. This requires
external network access, but most accurately reflects real world
contexts.

This change removes the x509.SystemCertPool() functionality, which will
be ammended in a follow-up change which supports the suggested hybrid
pool approach described in #46287.

Updates #46287
Fixes #42414
Fixes #38888
Fixes #35631
Fixes #19561

Change-Id: I17f0d6c5cb3ef8a1f2731ce3296478b28d30df46
Reviewed-on: https://go-review.googlesource.com/c/go/+/353132
Trust: Roland Shoemaker <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
TryBot-Result: Go Bot <[email protected]>
gopherbot pushed a commit that referenced this issue Nov 6, 2021
Use the same certificate verification APIs on iOS as on macOS (they
share the same APIs, so we should be able to transparently use them
on both.)

Updates #46287
Fixes #38843

Change-Id: If70f99b0823dd5fa747c42ff4f20c3b625605327
Reviewed-on: https://go-review.googlesource.com/c/go/+/353403
Trust: Roland Shoemaker <[email protected]>
Reviewed-by: Filippo Valsorda <[email protected]>
Run-TryBot: Roland Shoemaker <[email protected]>
@rsc rsc added this to Proposals Aug 10, 2022
@rsc rsc moved this to Accepted in Proposals Aug 10, 2022
@rsc rsc removed this from Proposals Nov 9, 2022
ronensc added a commit to ronensc/flowlogs-pipeline that referenced this issue Feb 16, 2023
Subjects() has been deprecated in Go 1.18:
golang/go#46287
ronensc added a commit to ronensc/flowlogs-pipeline that referenced this issue Feb 20, 2023
Subjects() has been deprecated in Go 1.18:
golang/go#46287
ronensc added a commit to netobserv/flowlogs-pipeline that referenced this issue Mar 1, 2023
* Update ebpf-agent dependency

go get github.com/netobserv/netobserv-ebpf-agent@latest
go get github.com/netobserv/flowlogs-pipeline/pkg/pipeline
go mod vendor

* Fix Generic related build issues

* Add Flags fields to decode_protobuf

* Rename test funcions

* Handle FIN_ACK

* Add a test for MoveToFront

* Validate TCPFlags field name is not empty

* Add correct direction

* Add test case for mismatch of AB field count

* Add operational metric for tcp flags

* Rename CorrectDirection -> SwapAB

* Change test

* Update README

* Add json tag to conntrack api

* Update docs

* Rename variable

* Make linter happy

* Make linter happy

Subjects() has been deprecated in Go 1.18:
golang/go#46287

* Enable SwapAB only when the feature flag is set

* Fix rebase errors

* NETOBSERV-838 fix IsDuplicate

* Add missing 'omitempty'

* Add parenthesis for clarity

* Add tests for IsDuplicate()

---------

Co-authored-by: Julien Pinsonneau <[email protected]>
@golang golang locked and limited conversation to collaborators Jun 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

6 participants