crypto/x509: come up with better solution for testing platform verifiers #52108
Comments
rolandshoemaker
added
the
NeedsFix
|
Change https://go.dev/cl/397694 mentions this issue: |
|
Change https://go.dev/cl/405914 mentions this issue: |
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 <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Run-TryBot: Roland Shoemaker <[email protected]>
|
2022-06-06T18:37:38-fc97075/windows-amd64-longtest has another failure due to |
|
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. |
|
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. |
|
Change https://go.dev/cl/488855 mentions this issue: |
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 <[email protected]> Auto-Submit: Roland Shoemaker <[email protected]> Reviewed-by: Bryan Mills <[email protected]> Run-TryBot: Roland Shoemaker <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
|
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). |
|
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? |
|
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:
|
|
Change https://go.dev/cl/503836 mentions this issue: |
|
Change https://go.dev/cl/505755 mentions this issue: |
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 <[email protected]> Auto-Submit: Roland Shoemaker <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Bryan Mills <[email protected]>
|
Change https://go.dev/cl/506895 mentions this issue: |
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 <[email protected]> Reviewed-by: Heschi Kreinick <[email protected]> Auto-Submit: Carlos Amedee <[email protected]> TryBot-Result: Gopher Robot <[email protected]>
|
The Windows amd64 images have been updated and pushed to production. |
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 <[email protected]> Reviewed-by: Heschi Kreinick <[email protected]>
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 <[email protected]> Auto-Submit: Roland Shoemaker <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Reviewed-by: Bryan Mills <[email protected]>
|
@rolandshoemaker, given the comment from @cagedmantis above, can we take care of the TODOS to remove the |
|
Ah yes, I completely missed that. I'll be happy to remove those tests. |
|
Change https://go.dev/cl/548976 mentions this issue: |
|
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 |
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 <[email protected]> Reviewed-by: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Roland Shoemaker <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
|
Change https://go.dev/cl/586215 mentions this issue: |
|
Change https://go.dev/cl/586235 mentions this issue: |
…y tests They are no longer necessary, woohoo! Updates #52108 Fixes #56791 Fixes #67351 Change-Id: I11a4c17162da4295309f74f2f8362bab0f506f78 Reviewed-on: https://go-review.googlesource.com/c/go/+/548976 Run-TryBot: Roland Shoemaker <[email protected]> Reviewed-by: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Roland Shoemaker <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> (cherry picked from commit c1828fb) Reviewed-on: https://go-review.googlesource.com/c/go/+/586215 Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Auto-Submit: Dmitri Shuralyov <[email protected]>
…y tests They are no longer necessary, woohoo! Updates #52108 Fixes #56791 Fixes #67352 Change-Id: I11a4c17162da4295309f74f2f8362bab0f506f78 Reviewed-on: https://go-review.googlesource.com/c/go/+/548976 Run-TryBot: Roland Shoemaker <[email protected]> Reviewed-by: Bryan Mills <[email protected]> TryBot-Result: Gopher Robot <[email protected]> Auto-Submit: Roland Shoemaker <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]> (cherry picked from commit c1828fb) Reviewed-on: https://go-review.googlesource.com/c/go/+/586235 Auto-Submit: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]> Reviewed-by: Dmitri Shuralyov <[email protected]>
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?
The text was updated successfully, but these errors were encountered: