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: come up with better solution for testing platform verifiers #52108

Open
rolandshoemaker opened this issue Apr 1, 2022 · 18 comments
Assignees
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@rolandshoemaker
Copy link
Member

As evidenced by #52094 and #51599, there are issues with relying on third-party services for testing the platform verifier implementations. Ideally we'd run these tests entirely locally, but this requires mutating the trust store on the systems being tested.

While we absolutely cannot start inserting arbitrary certificates into the trust stores of developers, it may be reasonable to do this on the trybots (although there will still be some gaps here, since user added roots are always going to be treated somewhat differently than roots the system chooses to trust.)

We should still have some kind of local testing that doesn't rely on trust store mutation though, perhaps just retaining the existing badssl.com based tests but gating them behind a flag?

@rolandshoemaker rolandshoemaker added the NeedsFix The path to resolution is known, but the work has not been done. label Apr 1, 2022
@gopherbot
Copy link

Change https://go.dev/cl/397694 mentions this issue: crypto/x509: local platform verifier tests on trybots

@gopherbot
Copy link

Change https://go.dev/cl/405914 mentions this issue: crypto/x509: attempt to prime windows root pool before hybrid test

gopherbot pushed a commit that referenced this issue May 12, 2022
In TestHybridPool attempt to prime to the windows root pool before
the real test actually happens. This is a bit of a band-aid, with
a better long term solution discussed in #52108.

Updates #51599

Change-Id: I406add8d9cd9e3fae37bfc20b97f5479c10a52c2
Reviewed-on: https://go-review.googlesource.com/c/go/+/405914
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Run-TryBot: Roland Shoemaker <roland@golang.org>
@bcmills
Copy link
Contributor

bcmills commented Jun 14, 2022

2022-06-06T18:37:38-fc97075/windows-amd64-longtest has another failure due to badssl.com having a cert that is bad in the wrong kind of way:

--- FAIL: TestPlatformVerifier (15.19s)
    --- FAIL: TestPlatformVerifier/wrong_host_for_leaf (15.11s)
        root_windows_test.go:109: unexpected verification error: got "x509: certificate has expired or is not yet valid: ", want "x509: certificate is valid for *.badssl.com, badssl.com, not wrong.host.badssl.com"
FAIL
FAIL	crypto/x509	32.031s

@rolandshoemaker
Copy link
Member Author

The least worst option I can think of: we insert a static certificate into the builder images for macos and windows, and use it to sign local certificates. For testing on local systems, we provide a very scary flag the user can pass that will generate an ephemeral key/cert pair and attempt to insert it into the trust store for the duration of the testing, removing it when done.

This should get us the best of both worlds, with only minor pain.

@rolandshoemaker
Copy link
Member Author

Plan is: basically above. We'll generate a highly constrained root, which will be used for testing. Rather than a flag that lets the user insert it into their own pool, we'll simply add it to the tree and allow users to insert it into their own trust pool if they wish.

We'll add a wrapper to the platform tests which decide whether or not to run them based on the detectable presence of the constraint root, this way they'll run on developers systems who want to test them, and it'll require at least some knowledge of trust stores in order to add it (rather than simply passing a flag and not really knowing what you're getting yourself into).

I'll implement the sniffing + tests etc first, and once that has landed and we have a root in the tree, we'll pass this on to the release team to add to the builder images.

@rolandshoemaker rolandshoemaker self-assigned this Apr 17, 2023
@gopherbot
Copy link

Change https://go.dev/cl/488855 mentions this issue: crypto/x509: use synthetic root for platform testing

gopherbot pushed a commit that referenced this issue Jun 14, 2023
Rather than using the external network and real-world chains for testing
the integrations with platform verifiers, use a synthetic test root.

This changes adds a constrained root and key pair to the tree, and adds
a test suite that verifies certificates issued from that root. These
tests are only executed if the root is detected in the trust store. For
reference, the script used to generate the root and key is attached to
the bottom of this commit message.

This change leaves the existing windows/darwin TestPlatformVerifier
tests in place, since the trybots do not currently have the test root in
place, and as such cannot run the suite. Once the builder images have
the root integrated, we can remove the old flaky tests, and the trybots
will begin running the new suite automatically.

Updates #52108

-- gen.go --
package main

import (
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/rand"
	"crypto/x509"
	"crypto/x509/pkix"
	"encoding/pem"
	"flag"
	"log"
	"math/big"
	"net"
	"os"
	"time"
)

func writePEM(pemType string, der []byte, path string) error {
	enc := pem.EncodeToMemory(&pem.Block{
		Type:  pemType,
		Bytes: der,
	})
	return os.WriteFile(path, enc, 0666)
}

func main() {
	certPath := flag.String("cert-path", "platform_root_cert.pem", "Path to write certificate PEM")
	keyPath := flag.String("key-path", "platform_root_key.pem", "Path to write key PEM")
	flag.Parse()

	key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
	if err != nil {
		log.Fatalf("ecdsa.GenerateKey failed: %s", err)
	}

	now := time.Now()
	tmpl := &x509.Certificate{
		SerialNumber: big.NewInt(9009),
		Subject: pkix.Name{
			CommonName: "Go platform verifier testing root",
		},
		NotBefore:                   now.Add(-time.Hour),
		NotAfter:                    now.Add(time.Hour * 24 * 365 * 5),
		IsCA:                        true,
		BasicConstraintsValid:       true,
		PermittedDNSDomainsCritical: true,
		// PermittedDNSDomains restricts the names in certificates issued from this root to *.testing.golang.invalid.
		// The .invalid TLD is, per RFC 2606, reserved for testing, and as such anything issued for this certificate
		// should never be valid in the real world.
		PermittedDNSDomains: []string{"testing.golang.invalid"},
		// ExcludedIPRanges prevents any certificate issued from this root that contains an IP address in both the full
		// IPv4 and IPv6 ranges from being considered valid.
		ExcludedIPRanges: []*net.IPNet{{IP: make([]byte, 4), Mask: make([]byte, 4)}, {IP: make([]byte, 16), Mask: make([]byte, 16)}},
		KeyUsage:         x509.KeyUsageCertSign,
		ExtKeyUsage:      []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth},
	}

	certDER, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, key.Public(), key)
	if err != nil {
		log.Fatalf("x509.CreateCertificate failed: %s", err)
	}

	keyDER, err := x509.MarshalECPrivateKey(key)
	if err != nil {
		log.Fatalf("x509.MarshalECPrivateKey failed: %s", err)
	}

	if err := writePEM("CERTIFICATE", certDER, *certPath); err != nil {
		log.Fatalf("failed to write certificate PEM: %s", err)
	}
	if err := writePEM("EC PRIVATE KEY", keyDER, *keyPath); err != nil {
		log.Fatalf("failed to write key PEM: %s", err)
	}
}

Change-Id: If7c4a9f18466662a60fea5443e603232a9260026
Reviewed-on: https://go-review.googlesource.com/c/go/+/488855
Reviewed-by: Filippo Valsorda <filippo@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@rolandshoemaker
Copy link
Member Author

This change has landed (🎉), the next step is for @golang/release (I think) to insert the root (https://github.com/golang/go/blob/master/src/crypto/x509/platform_root_cert.pem) into the trust stores for the windows and darwin builder images (happy to work with whoever wants to take this on how to do so, depending on how the images are built it may be as easy as dropping the file on a machine and double clicking it, or may require some command line magic).

@heschi
Copy link
Contributor

heschi commented Jun 14, 2023

Preferably you'd give us a Powershell line to add to https://cs.opensource.google/go/x/build/+/master:env/windows/startup.ps1, and instructions to add to https://cs.opensource.google/go/x/build/+/master:env/darwin/setup-notes.md.

The work to update the builders is nontrivial. Is it good enough to get it installed on one Windows and one Darwin builder? Or does it have to be on all of them? Can it wait for the LUCI migration, or would you prefer to have it sooner?

@rolandshoemaker
Copy link
Member Author

Ideally I think getting this working before we fully switch to LUCI would be ideal, but I understand if it's a relatively lower priority. The old tests are still in-tree, so we the urgency isn't incredibly high, we just have to tolerate the transient failures they introduce. I think it's fine to start with just one builder, as long as the test suite is being run somewhere.

For my own memory, I'll look into putting these in the right places:

  • macOS: security add-trusted-cert -d -r trustRoot -p ssl -k /Library/Keychains/System.keychain <certificate>
  • Windows: certutil.exe -addstore root <certificate> (there may also be a powershell thing I am unaware of, I will look)

@gopherbot
Copy link

Change https://go.dev/cl/503836 mentions this issue: env/darwin,env/windows: add platform testing root

@gopherbot
Copy link

Change https://go.dev/cl/505755 mentions this issue: crypto/x509: rename duplicated test

gopherbot pushed a commit that referenced this issue Jun 23, 2023
Rename the old TestPlatformVerifier to TestPlatformVerifierLegacy, and
add TODO about removing it once the synthetic root is widely deployed on
builders.

Updates #52108

Change-Id: I6cdba268e4738804c7f76ea18c354470b3e0318c
Reviewed-on: https://go-review.googlesource.com/c/go/+/505755
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@cagedmantis cagedmantis self-assigned this Jun 27, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jun 27, 2023
@gopherbot
Copy link

Change https://go.dev/cl/506895 mentions this issue: dashboard: udated host-windows-amd64-2016 varients

gopherbot pushed a commit to golang/build that referenced this issue Jun 28, 2023
This change updates the image used by the host-windows-amd64-2016
variants. The startup.ps1 was updated with the changes introduced
in CL 503836.

Updates golang/go#52108

Change-Id: I96d5b09309bb33b8c0181e189e6c0c36cad333ce
Reviewed-on: https://go-review.googlesource.com/c/build/+/506895
Run-TryBot: Carlos Amedee <carlos@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
Auto-Submit: Carlos Amedee <carlos@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@cagedmantis
Copy link
Contributor

The Windows amd64 images have been updated and pushed to production.

gopherbot pushed a commit to golang/build that referenced this issue Jun 30, 2023
Adds notes to env/darwin/setup-notes.md on how to install the
crypto/x509 testing root, and adds a stanza to env/windows/startup.ps1
to do the same thing.

This change assumes that the platform root has been added to the
go-builder-data GCS bucket.

Updates golang/go#52108

Change-Id: Ieb4d6461c4ee96ddc6c00c18213e5447ba9ab273
Reviewed-on: https://go-review.googlesource.com/c/build/+/503836
TryBot-Bypass: Dmitri Shuralyov <dmitshur@golang.org>
Reviewed-by: Heschi Kreinick <heschi@google.com>
@cagedmantis cagedmantis removed their assignment Jul 11, 2023
bradfitz pushed a commit to tailscale/go that referenced this issue Jul 15, 2023
Rename the old TestPlatformVerifier to TestPlatformVerifierLegacy, and
add TODO about removing it once the synthetic root is widely deployed on
builders.

Updates golang#52108

Change-Id: I6cdba268e4738804c7f76ea18c354470b3e0318c
Reviewed-on: https://go-review.googlesource.com/c/go/+/505755
Run-TryBot: Roland Shoemaker <roland@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
@gopherbot gopherbot modified the milestones: Go1.21, Go1.22 Aug 8, 2023
@bcmills
Copy link
Contributor

bcmills commented Dec 11, 2023

@rolandshoemaker, given the comment from @cagedmantis above, can we take care of the TODOS to remove the TestPlatformVerifierLegacy tests? They are still flaking periodically (see #56791).

@rolandshoemaker
Copy link
Member Author

Ah yes, I completely missed that. I'll be happy to remove those tests.

@gopherbot
Copy link

Change https://go.dev/cl/548976 mentions this issue: crypto/x509: remove TestPlatformVerifierLegacy tests

@rolandshoemaker
Copy link
Member Author

Based on the LUCI results, I am under the impression the test root has only been added to the TryBot builders, but not the LUCI builders (which show the skip message because the test root cannot be found). Is there a timeline for adding them to the LUCI ones as well?

I think we could probably can delete the old tests now, but if we are only running the LUCI builders then these tests wont be exercised at all, which is probably not ideal.

cc @cagedmantis

@dmitshur
Copy link
Contributor

As an update, test results here show the new test is running (not skipping) and passing on the recent windows/arm64 LUCI builder (#64587).

@dmitshur dmitshur added the Builders x/build issues (builders, bots, dashboards) label Dec 22, 2023
@odeke-em odeke-em modified the milestones: Go1.22, Go1.23 Jan 25, 2024
gopherbot pushed a commit that referenced this issue Feb 20, 2024
They are no longer necessary, woohoo!

Updates #52108
Fixes #56791

Change-Id: I11a4c17162da4295309f74f2f8362bab0f506f78
Reviewed-on: https://go-review.googlesource.com/c/go/+/548976
Run-TryBot: Roland Shoemaker <roland@golang.org>
Reviewed-by: Bryan Mills <bcmills@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Auto-Submit: Roland Shoemaker <roland@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Builders x/build issues (builders, bots, dashboards) NeedsFix The path to resolution is known, but the work has not been done.
Projects
Status: Planned
Development

No branches or pull requests

8 participants