Unwind legacy SSL_PRIVATE_KEY_METHOD hooks.

After much procrastinating, we finally moved Chromium to the new stuff.
We can now delete this. This is a breaking change for
SSL_PRIVATE_KEY_METHOD consumers, but it should be trivial (remove some
unused fields in the struct). I've bumped BORINGSSL_API_VERSION to ease
any multi-sided changes that may be needed.

Change-Id: I9fe562590ad938bcb4fcf9af0fadeff1d48745fb
Reviewed-on: https://boringssl-review.googlesource.com/23224
Commit-Queue: David Benjamin <davidben@google.com>
CQ-Verified: CQ bot account: commit-bot@chromium.org <commit-bot@chromium.org>
Reviewed-by: Steven Valdez <svaldez@google.com>

7 files changed, 12 insertions, 184 deletions

diff --git a/include/openssl/base.h b/include/openssl/base.h
index adb5047..9445556 100644
--- a/include/openssl/base.h
+++ b/include/openssl/base.h
@@ -151,7 +151,7 @@ extern "C" {
 // A consumer may use this symbol in the preprocessor to temporarily build
 // against multiple revisions of BoringSSL at the same time. It is not
 // recommended to do so for longer than is necessary.
-#define BORINGSSL_API_VERSION 4
+#define BORINGSSL_API_VERSION 5
 
 #if defined(BORINGSSL_SHARED_LIBRARY)
 
diff --git a/include/openssl/ssl.h b/include/openssl/ssl.h
index eba2fa3..53a8eb5 100644
--- a/include/openssl/ssl.h
+++ b/include/openssl/ssl.h
@@ -1125,16 +1125,7 @@ enum ssl_private_key_result_t {
 // key hooks. This is used to off-load signing operations to a custom,
 // potentially asynchronous, backend. Metadata about the key such as the type
 // and size are parsed out of the certificate.
-//
-// TODO(davidben): This API has a number of legacy hooks. Remove the last
-// consumer of |sign_digest| and trim it.
 struct ssl_private_key_method_st {
-  // type is ignored and should be NULL.
-  int (*type)(SSL *ssl);
-
-  // max_signature_len is ignored and should be NULL.
-  size_t (*max_signature_len)(SSL *ssl);
-
   // sign signs the message |in| in using the specified signature algorithm. On
   // success, it returns |ssl_private_key_success| and writes at most |max_out|
   // bytes of signature data to |out| and sets |*out_len| to the number of bytes
@@ -1156,30 +1147,6 @@ struct ssl_private_key_method_st {
                                         uint16_t signature_algorithm,
                                         const uint8_t *in, size_t in_len);
 
-  // sign_digest signs |in_len| bytes of digest from |in|. |md| is the hash
-  // function used to calculate |in|. On success, it returns
-  // |ssl_private_key_success| and writes at most |max_out| bytes of signature
-  // data to |out|. On failure, it returns |ssl_private_key_failure|. If the
-  // operation has not completed, it returns |ssl_private_key_retry|. |sign|
-  // should arrange for the high-level operation on |ssl| to be retried when the
-  // operation is completed. This will result in a call to |complete|.
-  //
-  // If the key is an RSA key, implementations must use PKCS#1 padding. |in| is
-  // the digest itself, so the DigestInfo prefix, if any, must be prepended by
-  // |sign|. If |md| is |EVP_md5_sha1|, there is no prefix.
-  //
-  // It is an error to call |sign_digest| while another private key operation is
-  // in progress on |ssl|.
-  //
-  // This function is deprecated. Implement |sign| instead.
-  //
-  // TODO(davidben): Remove this function.
-  enum ssl_private_key_result_t (*sign_digest)(SSL *ssl, uint8_t *out,
-                                               size_t *out_len, size_t max_out,
-                                               const EVP_MD *md,
-                                               const uint8_t *in,
-                                               size_t in_len);
-
   // decrypt decrypts |in_len| bytes of encrypted data from |in|. On success it
   // returns |ssl_private_key_success|, writes at most |max_out| bytes of
   // decrypted data to |out| and sets |*out_len| to the actual number of bytes
@@ -3978,18 +3945,6 @@ OPENSSL_EXPORT int SSL_set_tmp_ecdh(SSL *ssl, const EC_KEY *ec_key);
 OPENSSL_EXPORT int SSL_add_dir_cert_subjects_to_stack(STACK_OF(X509_NAME) *out,
                                                       const char *dir);
 
-// SSL_set_private_key_digest_prefs copies |num_digests| NIDs from |digest_nids|
-// into |ssl|. These digests will be used, in decreasing order of preference,
-// when signing with |ssl|'s private key. It returns one on success and zero on
-// error.
-//
-// Use |SSL_set_signing_algorithm_prefs| instead.
-//
-// TODO(davidben): Remove this API when callers have been updated.
-OPENSSL_EXPORT int SSL_set_private_key_digest_prefs(SSL *ssl,
-                                                    const int *digest_nids,
-                                                    size_t num_digests);
-
 // SSL_set_verify_result calls |abort| unless |result| is |X509_V_OK|.
 //
 // TODO(davidben): Remove this function once it has been removed from
diff --git a/ssl/ssl_privkey.cc b/ssl/ssl_privkey.cc
index e9990af..134ad56 100644
--- a/ssl/ssl_privkey.cc
+++ b/ssl/ssl_privkey.cc
@@ -193,33 +193,6 @@ static int setup_ctx(SSL *ssl, EVP_MD_CTX *ctx, EVP_PKEY *pkey, uint16_t sigalg,
   return 1;
 }
 
-static int legacy_sign_digest_supported(const SSL_SIGNATURE_ALGORITHM *alg) {
-  return (alg->pkey_type == EVP_PKEY_EC || alg->pkey_type == EVP_PKEY_RSA) &&
-         !alg->is_rsa_pss;
-}
-
-static enum ssl_private_key_result_t legacy_sign(
-    SSL *ssl, uint8_t *out, size_t *out_len, size_t max_out, uint16_t sigalg,
-    const uint8_t *in, size_t in_len) {
-  // TODO(davidben): Remove support for |sign_digest|-only
-  // |SSL_PRIVATE_KEY_METHOD|s.
-  const SSL_SIGNATURE_ALGORITHM *alg = get_signature_algorithm(sigalg);
-  if (alg == NULL || !legacy_sign_digest_supported(alg)) {
-    OPENSSL_PUT_ERROR(SSL, SSL_R_UNSUPPORTED_PROTOCOL_FOR_CUSTOM_KEY);
-    return ssl_private_key_failure;
-  }
-
-  const EVP_MD *md = alg->digest_func();
-  uint8_t hash[EVP_MAX_MD_SIZE];
-  unsigned hash_len;
-  if (!EVP_Digest(in, in_len, hash, &hash_len, md, NULL)) {
-    return ssl_private_key_failure;
-  }
-
-  return ssl->cert->key_method->sign_digest(ssl, out, out_len, max_out, md,
-                                            hash, hash_len);
-}
-
 enum ssl_private_key_result_t ssl_private_key_sign(
     SSL_HANDSHAKE *hs, uint8_t *out, size_t *out_len, size_t max_out,
     uint16_t sigalg, Span<const uint8_t> in) {
@@ -229,9 +202,8 @@ enum ssl_private_key_result_t ssl_private_key_sign(
     if (hs->pending_private_key_op) {
       ret = ssl->cert->key_method->complete(ssl, out, out_len, max_out);
     } else {
-      ret = (ssl->cert->key_method->sign != NULL ? ssl->cert->key_method->sign
-                                                 : legacy_sign)(
-          ssl, out, out_len, max_out, sigalg, in.data(), in.size());
+      ret = ssl->cert->key_method->sign(ssl, out, out_len, max_out, sigalg,
+                                        in.data(), in.size());
     }
     hs->pending_private_key_op = ret == ssl_private_key_retry;
     return ret;
@@ -308,14 +280,6 @@ bool ssl_private_key_supports_signature_algorithm(SSL_HANDSHAKE *hs,
     return false;
   }
 
-  // Newer algorithms require message-based private keys.
-  // TODO(davidben): Remove this check when sign_digest is gone.
-  if (ssl->cert->key_method != NULL &&
-      ssl->cert->key_method->sign == NULL &&
-      !legacy_sign_digest_supported(alg)) {
-    return false;
-  }
-
   return true;
 }
 
@@ -511,7 +475,6 @@ int SSL_CTX_set_signing_algorithm_prefs(SSL_CTX *ctx, const uint16_t *prefs,
                              prefs, num_prefs);
 }
 
-
 int SSL_set_signing_algorithm_prefs(SSL *ssl, const uint16_t *prefs,
                                     size_t num_prefs) {
   return set_algorithm_prefs(&ssl->cert->sigalgs, &ssl->cert->num_sigalgs,
@@ -523,52 +486,3 @@ int SSL_CTX_set_verify_algorithm_prefs(SSL_CTX *ctx, const uint16_t *prefs,
   return set_algorithm_prefs(&ctx->verify_sigalgs, &ctx->num_verify_sigalgs,
                              prefs, num_prefs);
 }
-
-int SSL_set_private_key_digest_prefs(SSL *ssl, const int *digest_nids,
-                                     size_t num_digests) {
-  OPENSSL_free(ssl->cert->sigalgs);
-
-  static_assert(sizeof(int) >= 2 * sizeof(uint16_t),
-                "sigalgs allocation may overflow");
-
-  ssl->cert->num_sigalgs = 0;
-  ssl->cert->sigalgs =
-      (uint16_t *)OPENSSL_malloc(sizeof(uint16_t) * 2 * num_digests);
-  if (ssl->cert->sigalgs == NULL) {
-    OPENSSL_PUT_ERROR(SSL, ERR_R_MALLOC_FAILURE);
-    return 0;
-  }
-
-  // Convert the digest list to a signature algorithms list.
-  //
-  // TODO(davidben): Replace this API with one that can express RSA-PSS, etc.
-  for (size_t i = 0; i < num_digests; i++) {
-    switch (digest_nids[i]) {
-      case NID_sha1:
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA1;
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] = SSL_SIGN_ECDSA_SHA1;
-        ssl->cert->num_sigalgs += 2;
-        break;
-      case NID_sha256:
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA256;
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] =
-            SSL_SIGN_ECDSA_SECP256R1_SHA256;
-        ssl->cert->num_sigalgs += 2;
-        break;
-      case NID_sha384:
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA384;
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] =
-            SSL_SIGN_ECDSA_SECP384R1_SHA384;
-        ssl->cert->num_sigalgs += 2;
-        break;
-      case NID_sha512:
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs] = SSL_SIGN_RSA_PKCS1_SHA512;
-        ssl->cert->sigalgs[ssl->cert->num_sigalgs + 1] =
-            SSL_SIGN_ECDSA_SECP521R1_SHA512;
-        ssl->cert->num_sigalgs += 2;
-        break;
-    }
-  }
-
-  return 1;
-}
diff --git a/ssl/test/bssl_shim.cc b/ssl/test/bssl_shim.cc
index 7eca21d..8acabd7 100644
--- a/ssl/test/bssl_shim.cc
+++ b/ssl/test/bssl_shim.cc
@@ -433,10 +433,7 @@ static ssl_private_key_result_t AsyncPrivateKeyComplete(
 }
 
 static const SSL_PRIVATE_KEY_METHOD g_async_private_key_method = {
-    nullptr /* type */,
-    nullptr /* max_signature_len */,
     AsyncPrivateKeySign,
-    nullptr /* sign_digest */,
     AsyncPrivateKeyDecrypt,
     AsyncPrivateKeyComplete,
 };
@@ -453,27 +450,6 @@ static bool GetCertificate(SSL *ssl, bssl::UniquePtr<X509> *out_x509,
                            bssl::UniquePtr<EVP_PKEY> *out_pkey) {
   const TestConfig *config = GetTestConfig(ssl);
 
-  if (!config->digest_prefs.empty()) {
-    bssl::UniquePtr<char> digest_prefs(
-        OPENSSL_strdup(config->digest_prefs.c_str()));
-    std::vector<int> digest_list;
-
-    for (;;) {
-      char *token =
-          strtok(digest_list.empty() ? digest_prefs.get() : nullptr, ",");
-      if (token == nullptr) {
-        break;
-      }
-
-      digest_list.push_back(EVP_MD_type(EVP_get_digestbyname(token)));
-    }
-
-    if (!SSL_set_private_key_digest_prefs(ssl, digest_list.data(),
-                                          digest_list.size())) {
-      return false;
-    }
-  }
-
   if (!config->signing_prefs.empty()) {
     std::vector<uint16_t> u16s(config->signing_prefs.begin(),
                                config->signing_prefs.end());
diff --git a/ssl/test/runner/runner.go b/ssl/test/runner/runner.go
index 7d02c15..8700af2 100644
--- a/ssl/test/runner/runner.go
+++ b/ssl/test/runner/runner.go
@@ -8306,8 +8306,8 @@ func addSignatureAlgorithmTests() {
 		expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:",
 	})
 
-	// Test that hash preferences are enforced. BoringSSL does not implement
-	// MD5 signatures.
+	// Test that signature preferences are enforced. BoringSSL does not
+	// implement MD5 signatures.
 	testCases = append(testCases, testCase{
 		testType: serverTest,
 		name:     "ClientAuth-Enforced",
@@ -8376,26 +8376,8 @@ func addSignatureAlgorithmTests() {
 		expectedError: ":WRONG_SIGNATURE_TYPE:",
 	})
 
-	// Test that the agreed upon digest respects the client preferences and
-	// the server digests.
-	testCases = append(testCases, testCase{
-		name: "NoCommonAlgorithms-Digests",
-		config: Config{
-			MaxVersion: VersionTLS12,
-			ClientAuth: RequireAnyClientCert,
-			VerifySignatureAlgorithms: []signatureAlgorithm{
-				signatureRSAPKCS1WithSHA512,
-				signatureRSAPKCS1WithSHA1,
-			},
-		},
-		flags: []string{
-			"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
-			"-key-file", path.Join(*resourceDir, rsaKeyFile),
-			"-digest-prefs", "SHA256",
-		},
-		shouldFail:    true,
-		expectedError: ":NO_COMMON_SIGNATURE_ALGORITHMS:",
-	})
+	// Test that the negotiated signature algorithm respects the client and
+	// server preferences.
 	testCases = append(testCases, testCase{
 		name: "NoCommonAlgorithms",
 		config: Config{
@@ -8445,7 +8427,8 @@ func addSignatureAlgorithmTests() {
 		flags: []string{
 			"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
 			"-key-file", path.Join(*resourceDir, rsaKeyFile),
-			"-digest-prefs", "SHA256,SHA1",
+			"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA256)),
+			"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA1)),
 		},
 		expectedPeerSignatureAlgorithm: signatureRSAPKCS1WithSHA256,
 	})
@@ -8461,7 +8444,9 @@ func addSignatureAlgorithmTests() {
 		flags: []string{
 			"-cert-file", path.Join(*resourceDir, rsaCertificateFile),
 			"-key-file", path.Join(*resourceDir, rsaKeyFile),
-			"-digest-prefs", "SHA512,SHA256,SHA1",
+			"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA512)),
+			"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA256)),
+			"-signing-prefs", strconv.Itoa(int(signatureRSAPKCS1WithSHA1)),
 		},
 		expectedPeerSignatureAlgorithm: signatureRSAPKCS1WithSHA1,
 	})
diff --git a/ssl/test/test_config.cc b/ssl/test/test_config.cc
index 2d9f725..a5ce5a1 100644
--- a/ssl/test/test_config.cc
+++ b/ssl/test/test_config.cc
@@ -132,7 +132,6 @@ const Flag<bool> kBoolFlags[] = {
 
 const Flag<std::string> kStringFlags[] = {
   { "-write-settings", &TestConfig::write_settings },
-  { "-digest-prefs", &TestConfig::digest_prefs },
   { "-key-file", &TestConfig::key_file },
   { "-cert-file", &TestConfig::cert_file },
   { "-expect-server-name", &TestConfig::expected_server_name },
diff --git a/ssl/test/test_config.h b/ssl/test/test_config.h
index b742f94..ea12d34 100644
--- a/ssl/test/test_config.h
+++ b/ssl/test/test_config.h
@@ -26,7 +26,6 @@ struct TestConfig {
   int resume_count = 0;
   std::string write_settings;
   bool fallback_scsv = false;
-  std::string digest_prefs;
   std::vector<int> signing_prefs;
   std::vector<int> verify_prefs;
   std::string key_file;