crypto/tls: better bogo test output handling

Use the bogo JSON output format to make the test output more readable.

Cq-Include-Trybots: luci.golang.try:gotip-linux-amd64-longtest
Change-Id: Ie1a67c6a031bc1d5d8b2cdfaf78d094a0967bc2a
Reviewed-on: https://go-review.googlesource.com/c/go/+/587955
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Reviewed-by: Filippo Valsorda <filippo@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

diff --git a/src/crypto/tls/bogo_config.json b/src/crypto/tls/bogo_config.json
index 8e4cec24aa629559c795383d7ec636dda970c3af..2363dd5d659accd9560c560b97bbd48bfd92b4d2 100644 (file)
--- a/src/crypto/tls/bogo_config.json
+++ b/src/crypto/tls/bogo_config.json
@@ -30,13 +30,6 @@
         "TLS-ECH-Client-NoSupportedConfigs": "We don't support fallback to cleartext when there are no valid ECH configs",
         "TLS-ECH-Client-SkipInvalidPublicName": "We don't support fallback to cleartext when there are no valid ECH configs",
 
-        "TLS-ECH-Client-Reject-RandomHRRExtension": "TODO: bogo test cases have mismatching public certificates and public names in ECH configs. Can be removed once bogo fixed",
-        "TLS-ECH-Client-Reject-UnsupportedRetryConfigs": "TODO: bogo test cases have mismatching public certificates and public names in ECH configs. Can be removed once bogo fixed",
-        "TLS-ECH-Client-Reject-NoRetryConfigs": "TODO: bogo test cases have mismatching public certificates and public names in ECH configs. Can be removed once bogo fixed",
-        "TLS-ECH-Client-Reject": "TODO: bogo test cases have mismatching public certificates and public names in ECH configs. Can be removed once bogo fixed",
-        "TLS-ECH-Client-Reject-HelloRetryRequest": "TODO: bogo test cases have mismatching public certificates and public names in ECH configs. Can be removed once bogo fixed",
-        "TLS-ECH-Client-Reject-NoClientCertificate-TLS13": "TODO: bogo test cases have mismatching public certificates and public names in ECH configs. Can be removed once bogo fixed",
-        "TLS-ECH-Client-Reject-OverrideName-TLS13": "TODO: bogo test cases have mismatching public certificates and public names in ECH configs. Can be removed once bogo fixed",
 
         "*ECH-Server*": "no ECH server support",
         "SendV2ClientHello*": "We don't support SSLv2",

diff --git a/src/crypto/tls/bogo_shim_test.go b/src/crypto/tls/bogo_shim_test.go
index ad5195cce32ecfeeae437aa4a2031a117da76502..b9db73de81603a3e7c171b52582f7b2bfc221b3b 100644 (file)
--- a/src/crypto/tls/bogo_shim_test.go
+++ b/src/crypto/tls/bogo_shim_test.go
@@ -305,8 +305,8 @@ func TestBogoSuite(t *testing.T) {
        if *bogoLocalDir != "" {
                bogoDir = *bogoLocalDir
        } else {
-               const boringsslModVer = "v0.0.0-20240517213134-ba62c812f01f"
-               output, err := exec.Command("go", "mod", "download", "-json", "github.com/google/boringssl@"+boringsslModVer).CombinedOutput()
+               const boringsslModVer = "v0.0.0-20240523173554-273a920f84e8"
+               output, err := exec.Command("go", "mod", "download", "-json", "boringssl.googlesource.com/boringssl.git@"+boringsslModVer).CombinedOutput()
                if err != nil {
                        t.Fatalf("failed to download boringssl: %s", err)
                }
@@ -324,6 +324,8 @@ func TestBogoSuite(t *testing.T) {
                t.Fatal(err)
        }
 
+       resultsFile := filepath.Join(t.TempDir(), "results.json")
+
        args := []string{
                "test",
                ".",
@@ -332,8 +334,7 @@ func TestBogoSuite(t *testing.T) {
                "-shim-extra-flags=-bogo-mode",
                "-allow-unimplemented",
                "-loose-errors", // TODO(roland): this should be removed eventually
-               "-pipe",
-               "-v",
+               fmt.Sprintf("-json-output=%s", resultsFile),
        }
        if *bogoFilter != "" {
                args = append(args, fmt.Sprintf("-test=%s", *bogoFilter))
@@ -345,21 +346,72 @@ func TestBogoSuite(t *testing.T) {
        }
        cmd := exec.Command(goCmd, args...)
        out := &strings.Builder{}
-       cmd.Stdout, cmd.Stderr = io.MultiWriter(os.Stdout, out), os.Stderr
+       cmd.Stderr = out
        cmd.Dir = filepath.Join(bogoDir, "ssl/test/runner")
        err = cmd.Run()
-       if err != nil {
-               t.Fatalf("bogo failed: %s", err)
+       // NOTE: we don't immediately check the error, because the failure could be either because
+       // the runner failed for some unexpected reason, or because a test case failed, and we
+       // cannot easily differentiate these cases. We check if the JSON results file was written,
+       // which should only happen if the failure was because of a test failure, and use that
+       // to determine the failure mode.
+
+       resultsJSON, jsonErr := os.ReadFile(resultsFile)
+       if jsonErr != nil {
+               if err != nil {
+                       t.Fatalf("bogo failed: %s\n%s", err, out)
+               }
+               t.Fatalf("failed to read results JSON file: %s", err)
        }
 
-       if *bogoFilter == "" {
-               assertPass := func(t *testing.T, name string) {
-                       t.Helper()
-                       if !strings.Contains(out.String(), "PASSED ("+name+")\n") {
-                               t.Errorf("Expected test %s did not run", name)
+       var results bogoResults
+       if err := json.Unmarshal(resultsJSON, &results); err != nil {
+               t.Fatalf("failed to parse results JSON: %s", err)
+       }
+
+       // assertResults contains test results we want to make sure
+       // are present in the output. They are only checked if -bogo-filter
+       // was not passed.
+       assertResults := map[string]string{
+               "CurveTest-Client-Kyber-TLS13": "PASS",
+               "CurveTest-Server-Kyber-TLS13": "PASS",
+       }
+
+       for name, result := range results.Tests {
+               // This is not really the intended way to do this... but... it works?
+               t.Run(name, func(t *testing.T) {
+                       if result.Actual == "FAIL" && result.IsUnexpected {
+                               t.Fatal(result.Error)
+                       }
+                       if expectedResult, ok := assertResults[name]; ok && expectedResult != result.Actual {
+                               t.Fatalf("unexpected result: got %s, want %s", result.Actual, assertResults[name])
                        }
+                       delete(assertResults, name)
+                       if result.Actual == "SKIP" {
+                               t.Skip()
+                       }
+               })
+       }
+       if *bogoFilter == "" {
+               // Anything still in assertResults did not show up in the results, so we should fail
+               for name, expectedResult := range assertResults {
+                       t.Run(name, func(t *testing.T) {
+                               t.Fatalf("expected test to run with result %s, but it was not present in the test results", expectedResult)
+                       })
                }
-               assertPass(t, "CurveTest-Client-Kyber-TLS13")
-               assertPass(t, "CurveTest-Server-Kyber-TLS13")
        }
 }
+
+// bogoResults is a copy of boringssl.googlesource.com/boringssl/testresults.Results
+type bogoResults struct {
+       Version           int            `json:"version"`
+       Interrupted       bool           `json:"interrupted"`
+       PathDelimiter     string         `json:"path_delimiter"`
+       SecondsSinceEpoch float64        `json:"seconds_since_epoch"`
+       NumFailuresByType map[string]int `json:"num_failures_by_type"`
+       Tests             map[string]struct {
+               Actual       string `json:"actual"`
+               Expected     string `json:"expected"`
+               IsUnexpected bool   `json:"is_unexpected"`
+               Error        string `json:"error,omitempty"`
+       } `json:"tests"`
+}