crypto/x509: come up with better solution for testing platform verifiers

[rolandshoemaker]

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?

[gopherbot]

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

[gopherbot]

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

[bcmills]

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]

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]

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.

[gopherbot]

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

[rolandshoemaker]

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]

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]

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]

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

[gopherbot]

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

[gopherbot]

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

[cagedmantis]

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

[bcmills]

@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]

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

[gopherbot]

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

[rolandshoemaker]

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]

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

[gopherbot]

Change https://go.dev/cl/586215 mentions this issue: [release-branch.go1.21] crypto/x509: remove TestPlatformVerifierLegacy tests

[gopherbot]

Change https://go.dev/cl/586235 mentions this issue: [release-branch.go1.22] crypto/x509: remove TestPlatformVerifierLegacy tests