Remove raw pointers from the public interface

[reneme]

X509_Object::check_signature(const Public_Key*) did give us a run for our money yesterday. In some application code we (accidentially) switched from the overload that takes a const Public_Key& and suddenly started seeing segfaults. One might say, we should have read the documentation:

/**
* Check the signature on this data
* @param key the public key purportedly used to sign this data
*        the object will be deleted after use (this should have
*        been a std::unique_ptr<Public_Key>)
* @return true if the signature is valid, otherwise false
*/

Should we try and get such things out of the API before 3.0?

Other places where raw pointers without "clear" ownership contracts are used:

• Public_Key* X509::load_key(...)
• Public_Key* X509::copy_key(...)
• Private_Key* PKCS8::load_key(...)
  (raw pointer overloads are deprecated, but could be removed)
• bool X509_Object::check_signature(const Public_Key*)
  (takes ownership and deletes the pointer 😨)
• Public_Key* PKCS10_Request::subject_public_key()
• Private_Key* Credentials_Manager::private_key_for()
• Various semi-internal APIs in the TLS implementations
• probably more

Unfortunately, some of those APIs aren't deprecated, yet. Still, I think we should see what to do with them.

[reneme]

See also: #2677

[randombit]

For PKCS8::load_key we should absolutely remove all the deprecated versions; the ones returning unique_ptr have been there since 2.3

Unfortunately X509::load_key/copy_key for whatever reason never got the same smart pointer treatment.

check_signature we should just change and note it in the migration guide; I expect it's not that common for applications to use this directly.

PKCS10_Request::subject_public_key should get the same treatment as X509_Certificate::subject_public_key; that function just calls load_subject_public_key and releases the pointer. (We should also deprecate the versions returning a raw pointer, currently X509_Certificate::subject_public_key is not deprecated)

Credentials_Manager::private_key_for not sure if there is an easy way to transition. Maybe simpler to just change the return type and note it in the migration guide.

[randombit]

#3220 addressed all of the ones mentioned in your original comment aside from Credentials_Manager::private_key_for but there are still several also from #2677 which should be dealt with before 3.0

[randombit]

Private_Key* Credentials_Manager::private_key_for() is a tricky one, because currently the contract is that the Credentials_Manager retains ownership of the private key. That's actually rather awkward, because there is absolutely no way for the credentials manager to know when or if the TLS layer has ever "finished" using the key, and so must retain it in memory perpetually.

I can see a few possible return types

1. std::unique_ptr<Private_Key> where ownership is passed to the caller
2. std::shared_ptr<Private_Key>
3. Private_Key& where we throw an exception instead of returning nullptr to indicate a problem

I'm really not sure what the right thing is.

1. Requires copying the key, but the ownership transfer is clear
2. Maybe the best approach?
3. Has the same ownership problem as the current approach, the reference is (potentially) alive forever.

[randombit]

Proposed private_key_for change in #3258

[randombit]

I think this has been resolved now?

[reneme]

Yes. Thanks a lot!