Don't send two post-quantum initial key shares.

More than one post-quantum group is now defined so it would be possible
for two PQ groups to be 1st and 2nd preferences. In that case, we
probably don't want to send two PQ initial key shares.

(Only one PQ group is _implemented_ currently, so we can't write a test
for this.)

Change-Id: I51ff118f224153e09a0c3ee8b142aebb6b340dcb
Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/56226
Reviewed-by: David Benjamin <davidben@google.com>
Commit-Queue: Adam Langley <agl@google.com>

ssl/extensions.cc

@@ -2307,13 +2307,15 @@ bool ssl_setup_key_shares(SSL_HANDSHAKE *hs, uint16_t override_group_id) {
 
     group_id = groups[0];
 
-    if (is_post_quantum_group(group_id) && groups.size() >= 2) {
-      // CECPQ2(b) is not sent as the only initial key share. We'll include the
-      // 2nd preference group too to avoid round-trips.
-      second_group_id = groups[1];
+    // We'll try to include one post-quantum and one classical initial key
+    // share.
+    for (size_t i = 1; i < groups.size() && second_group_id == 0; i++) {
+      if (is_post_quantum_group(group_id) != is_post_quantum_group(groups[i])) {
+        second_group_id = groups[i];
         assert(second_group_id != group_id);
       }
     }
+  }
 
   CBB key_exchange;
   hs->key_shares[0] = SSLKeyShare::Create(group_id);

ssl/test/runner/runner.go

@@ -11901,13 +11901,13 @@ func addCurveTests() {
 		},
 	})
 
-	// ... but only if CECPQ2 is listed first.
+	// ... and the other way around
 	testCases = append(testCases, testCase{
-		name: "CECPQ2KeyShareNotIncludedSecond",
+		name: "CECPQ2KeyShareIncludedSecond",
 		config: Config{
 			MinVersion: VersionTLS13,
 			Bugs: ProtocolBugs{
-				ExpectedKeyShares: []CurveID{CurveX25519},
+				ExpectedKeyShares: []CurveID{CurveX25519, CurveCECPQ2},
 			},
 		},
 		flags: []string{
@@ -11917,6 +11917,25 @@ func addCurveTests() {
 		},
 	})
 
+        // ... and even if there's another curve in the middle because it's the
+        // first classical and first post-quantum "curves" that get key shares
+        // included.
+	testCases = append(testCases, testCase{
+		name: "CECPQ2KeyShareIncludedThird",
+		config: Config{
+			MinVersion: VersionTLS13,
+			Bugs: ProtocolBugs{
+				ExpectedKeyShares: []CurveID{CurveX25519, CurveCECPQ2},
+			},
+		},
+		flags: []string{
+			"-curves", strconv.Itoa(int(CurveX25519)),
+			"-curves", strconv.Itoa(int(CurveP256)),
+			"-curves", strconv.Itoa(int(CurveCECPQ2)),
+			"-expect-curve-id", strconv.Itoa(int(CurveX25519)),
+		},
+	})
+
 	// If CECPQ2 is the only configured curve, the key share is sent.
 	testCases = append(testCases, testCase{
 		name: "JustConfiguringCECPQ2Works",