[gsi] Fix some memory leaks when using Secgsi

[smithdh]

To fix issue #2021. (Draft initially).

[abh3]

Other than a code reduction comment I don't see anything inherently wrong here assuming all the frees are appropriate. Let's see what @gganis has to say.

[abh3]

This definition of sslGaurd is almost identical to the previous version. It should be trivial to define a superset of sslGaurd and only have one definition placed at file level and used where need.

[bbockelm]

Can we drop support for pre-OpenSSL 1.0? That would mean we drop support for RHEL5 (which seems OK).

[bbockelm]

If you don't want the bother of an entire class, you could do something like:

std::unique_ptr<EVP_PKEY, decltype(EVP_PKEY_free)> ekro(nullptr, &EVP_PKEY_free)

Type definition is uglier but maybe clearer to some?

[smithdh]

Thank you for the comments; I've aimed to take those on; changed to using std::unique_ptr (with a type alias to try to make it clearer); which means no repetition of the previous 'sslGuard' class. Also added a return code check on a call of ExportPrivate(). I didn't remove the pre-openssl-1.0-case call yet, although probably we don't need it.

[abh3]

Well, I suppose using unique_ptr removes a class definition but I can't say it makes it easier to understand. Yes, the introduction of type synonyms helps but still feels a bit forced. That's just an individual preference here. Otherwise, I don't see anything wrong. Let's see of @gganis has anything more to say.

[gganis]

Hi, many thanks for the detailed patch. The part on XrdCrypto looks good to me. I am not sure to fully understand what the changes in XrdSecProtocolgsi bring: a CA cert might be left unfreed at process ending, but was that causing an increasing leak?
I also agree that support for openssl < 1 can be safely drop. Maybe a separate PR?

[smithdh]

hi @gganis , thank you for taking a look. I'll try to summarise the changes for XrdSecProtocolgsi and my reasoning, below:

(i) Store an exported version of the new delegated proxy request's private key in hs->Cref->buf4, rather than a XrdCryptoRSA*. This is so that the 'buf4' destructor (~XrdSutPFBuf) can free this storage. In the previous situation, once the client signs the delegated proxy and returns it to the server, the EVP_PKEY* inside buf4's XrdCryptoRSA* gets shared with a XrdCryptoX509 object. That latter should eventually be deleted, which also takes care of the EVP_PKEY, but the XrdCryptoRSA object in buf4 leaks. (Indeed it can't be free-ed as it would also try to double free the EVP_PKEY). In case something goes wrong and the dialogue fails before the signed delegated proxy is processed, the XrdCryptoRSA and its EVP_PKEY leaks. (See bottom of comment for a case where the dialogue can fail *)

(ii) Added flag so that ~gsiHSVars() can cleanup and delete PxyChain member. I think, in the server this should only be set in the interval between delegated proxy request generation by the server and receipt of the signed version from client; but (similar to (i)) if the dialogue is interrupted it leaks. On the production service I could see indication that this does happen quite frequently. (See below *)

(iii) Added PxyChain->Cleanup(); in a few places; this was because I had added similar code in ~gsiHSVars, so did so in the few other places. I used the 'keepCA=false' default parameter, as I understand the issue of keeping the first certificate in the chain, if it is a CA, is because in the case of hs->Chain, the first certificate is a CA certificate from our cache, so shouldn't be freed. But the gsiHSVars::PxyChain (or XrdSecProtocolgsi::proxyChain, once it is moved there) doesn't share with the cache, so I don't believe the exception is needed (but indeed the first cert in the delegated proxy chain probably is not a CA, and so this makes no difference). I changed this in one existing place, in XrdSecProtocolgsi::Delete() for proxyChain->Cleanup, for the same reason.

(*) On one production server there are quite frequently message like:

230613 17:03:19 223031 secgsi_Authenticate: buffer with requested info missing :Not allowed to sign proxy requests

(which have not reported as problematic, and wasn't really the subject of the current issue, but indicated some other paths are often taken, which can also leak). On a development xrootd standalone server I could reproduce this server side error message by using an old client (e.g. v 4.9.0) and using the server's IP address rather than name while requesting delegation. e.g.

./src/XrdCl/xrdcp --tpc delegate first root://1.2.3.4//afile /dev/null