Deprecate EVP_PKEY_set_type This function is practically useless. It makes a typed but empty key, which is somehow distinct from an untyped and empty key. Looking for callers within our tests and externally, the patterns are: - EVP_PKEY_set_type(EVP_PKEY_X25519) followed by EVP_PKEY_set1_tls_encodedpoint. This is regrettably needed for that API to work, but we've also marked EVP_PKEY_set1_tls_encodedpoint as deprecated, I think because of this weird half-empty state. Sadly this means we can never totally remove this, and we're stuck with some of these half-empty states. - EVP_PKEY_set_type(EVP_PKEY_NONE). This is used in some tests by callers who try to test their "unknown type" codepaths. The usual pattern is they load a key and then set the type to EVP_PKEY_NONE. The result of this is an empty EVP_PKEY, identical to just calling EVP_PKEY_new. Loading the key was never useful. It's also an error! The function fails because EVP_PKEY_NONE is not a supported type. Callers were relying on a quirk that we clear the key before noticing the parameters are wrong. Write a test and update the comment so we don't forget this behavior is depended on. (Though I'm going to try to fix everyone relying on this.) - EVP_PKEY_set_type(EVP_PKEY_RSA), etc. This does not produce a valid key. It's done in some test code to avoid loading a real key. Our own tests do this too, though it's also to test that EVP_PKEY_missing_parameters works for this weird empty state, that ideally wouldn't be possible in the first place. The immediate motivation is that the work to make EVP more static-linker-friendly collides a bit with EVP_PKEY_set_type(EVP_PKEY_EC). If we're trying to treat P-256 and P-384 as distinct EVP_PKEY types, it's unclear how that operation is supposed to work. Bug: 42290409 Change-Id: Id4368e049abea698f0eaed1419cb366a5fe3a424 Reviewed-on: https://boringssl-review.googlesource.com/c/boringssl/+/81269 Auto-Submit: David Benjamin <davidben@google.com> Reviewed-by: Lily Chen <chlily@google.com> Commit-Queue: Lily Chen <chlily@google.com> Commit-Queue: David Benjamin <davidben@google.com>

diff --git a/crypto/evp/evp.cc b/crypto/evp/evp.cc
index 82a98dd..806c34c 100644
--- a/crypto/evp/evp.cc
+++ b/crypto/evp/evp.cc

@@ -210,9 +210,8 @@
 
 int EVP_PKEY_set_type(EVP_PKEY *pkey, int type) {
   if (pkey && pkey->pkey) {
-    // This isn't strictly necessary, but historically |EVP_PKEY_set_type| would
-    // clear |pkey| even if |evp_pkey_asn1_find| failed, so we preserve that
-    // behavior.
+    // Some callers rely on |pkey| getting cleared even if |type| is
+    // unsupported, usually setting |type| to |EVP_PKEY_NONE|.
     free_it(pkey);
   }
 

diff --git a/crypto/evp/evp_extra_test.cc b/crypto/evp/evp_extra_test.cc
index eac46e8..f39f493 100644
--- a/crypto/evp/evp_extra_test.cc
+++ b/crypto/evp/evp_extra_test.cc

@@ -1325,3 +1325,16 @@
   ASSERT_TRUE(EVP_PKEY_CTX_get_rsa_pss_saltlen(pctx, &salt_len));
   EXPECT_EQ(salt_len, RSA_PSS_SALTLEN_DIGEST);
 }
+
+// Test that setting a key to type none will clear it. Some calling code has
+// unit tests that rely on this, usually as part of a roundabout way to get a
+// key of the "wrong" type to test with. (In reality, |EVP_PKEY_new| will
+// produce the same object.)
+TEST(EVPExtraTest, SetNoneClearsKey) {
+  bssl::UniquePtr<EVP_PKEY> pkey = LoadExampleRSAKey();
+  ASSERT_TRUE(pkey);
+  // EVP_PKEY_NONE is not a known type, so this should fail.
+  EXPECT_FALSE(EVP_PKEY_set_type(pkey.get(), EVP_PKEY_NONE));
+  // However, it still resets the key to the initial state.
+  EXPECT_EQ(EVP_PKEY_id(pkey.get()), EVP_PKEY_NONE);
+}

diff --git a/include/openssl/evp.h b/include/openssl/evp.h
index 13bf55e..805aed6 100644
--- a/include/openssl/evp.h
+++ b/include/openssl/evp.h

@@ -139,11 +139,6 @@
 #define EVP_PKEY_HKDF NID_hkdf
 #define EVP_PKEY_DH NID_dhKeyAgreement
 
-// EVP_PKEY_set_type sets the type of |pkey| to |type|. It returns one if
-// successful or zero if the |type| argument is not one of the |EVP_PKEY_*|
-// values. If |pkey| is NULL, it simply reports whether the type is known.
-OPENSSL_EXPORT int EVP_PKEY_set_type(EVP_PKEY *pkey, int type);
-
 // EVP_PKEY_cmp_parameters compares the parameters of |a| and |b|. It returns
 // one if they match, zero if not, or a negative number of on error.
 //
@@ -903,6 +898,23 @@
 OPENSSL_EXPORT int EVP_PKEY_CTX_set_ec_param_enc(EVP_PKEY_CTX *ctx,
                                                  int encoding);
 
+// EVP_PKEY_set_type sets the type of |pkey| to |type|. It returns one if
+// successful or zero if the |type| argument is not one of the |EVP_PKEY_*|
+// values. If |pkey| is NULL, it simply reports whether the type is known.
+//
+// There are very few cases where this function is useful. Changing |pkey|'s
+// type clears any previously stored keys, so there is no benefit to loading a
+// key and then changing its type. Although |pkey| is left with a type
+// configured, it has no key, and functions which set a key, such as
+// |EVP_PKEY_set1_RSA|, will configure a type anyway. If writing unit tests that
+// are only sensitive to the type of a key, it is preferable to construct a real
+// key, so that tests are more representative of production code.
+//
+// The only API pattern which requires this function is
+// |EVP_PKEY_set1_tls_encodedpoint| with X25519, which requires a half-empty
+// |EVP_PKEY| that was first configured with |EVP_PKEY_X25519|.
+OPENSSL_EXPORT int EVP_PKEY_set_type(EVP_PKEY *pkey, int type);
+
 // EVP_PKEY_set1_tls_encodedpoint replaces |pkey| with a public key encoded by
 // |in|. It returns one on success and zero on error.
 //