MyProxy: change private key cipher to EVP_aes_256_cbc()

[fscheiner]

As per #229 MyProxy still used an old cipher for encrypting private keys. Changes courtesy of Mischa Salle (@msalle).

Fixes #229.

[fscheiner]

@msalle :
Let's see if the CI is also happy. :-)

[msalle]

Hi @fscheiner Thanks!
Since we have the code twice, maybe we should put both in a function or macro?
Something like
#define MYPROXY_PRIVKEY_CIPHER() EVP_aes_256_cbc()
?

[fscheiner]

Hi @fscheiner Thanks! Since we have the code twice, maybe we should put both in a function or macro? Something like #define MYPROXY_PRIVKEY_CIPHER() EVP_aes_256_cbc() ?

Good idea! I also shortly thought about something like that, e.g. when in ten years or so AES256 is considered old and deprecated. ;-)

Would you place it in the header or directly into myproxy/source/ssl_utils.c? Maybe the latter is better, because one does not have to check which include includes it if need be.

[msalle]

Hi @fscheiner Thanks! Since we have the code twice, maybe we should put both in a function or macro? Something like #define MYPROXY_PRIVKEY_CIPHER() EVP_aes_256_cbc() ?

Good idea! I also shortly thought about something like that, e.g. when in ten years or so AES256 is considered old and deprecated. ;-)

currently still not yet broken and they claim it's quantum crypto safe... (famous last words?)

Would you place it in the header or directly into myproxy/source/ssl_utils.c? Maybe the latter is better, because one does not have to check which include includes it if need be.

one of the reasons I hadn't made the patch was that I hadn't come up with a decision for that. I'm thinking it might be nice in the header, since it's then part of the public API, but easier in ssl_utils.c. Probably better for now in the ssl_utils.c for reason you're giving. And we can always make it public later when needed.

[fscheiner]

Good idea! I also shortly thought about something like that, e.g. when in ten years or so AES256 is considered old and deprecated. ;-)

currently still not yet broken and they claim it's quantum crypto safe... (famous last words?)

:-))

Would you place it in the header or directly into myproxy/source/ssl_utils.c? Maybe the latter is better, because one does not have to check which include includes it if need be.

one of the reasons I hadn't made the patch was that I hadn't come up with a decision for that. I'm thinking it might be nice in the header, since it's then part of the public API, but easier in ssl_utils.c. Probably better for now in the ssl_utils.c for reason you're giving. And we can always make it public later when needed.

I can make the changes. I think we don't need another build test as it worked for the current version already, but just a positive review for merging it.

Apart from that we can still have a discussion about that change of course. CCing @ellert @maarten-litmaath. Not sure, is Jim still interested in or working on MyProxy? If yes, then we should maybe CC him, too.

[maarten-litmaath]

Hi guys,
what happens when an existing installation is upgraded to the new version?
Will the existing key continue being used without any problem?

I suspect Jim would not really want to spend time on MyProxy any more...

[fscheiner]

Hi Maarten,

Hi guys, what happens when an existing installation is upgraded to the new version? Will the existing key continue being used without any problem?

According to #229:

It reads using PEM_read_PrivateKey() in ssl_private_key_load_from_file(), ssl_utils.c lines 743-744 and PEM_read_bio_PrivateKey() in ssl_proxy_from_pem(), ssl_utils.c lines 927-928 which are generic enough to read other formats too.

...I'd say yes.

[maarten-litmaath]

So the improvement will by default only benefit new installations?
That would be fine with me.

[fscheiner]

So the improvement will by default only benefit new installations? That would be fine with me.

Most likely, but I don't know if MyProxy only stores a private key locally during initial setup. But even if not, the functions for reading it won't care if a new key is AES256 encrypted.

[msalle]

So the improvement will by default only benefit new installations? That would be fine with me.

Most likely, but I don't know if MyProxy only stores a private key locally during initial setup. ,
But even if not, the functions for reading it won't care if a new key is AES256 encrypted.

Just to clarify: for each proxy that gets delegated to myproxy it creates and stores a new private key. But changing the format even for an existing server looks fine because it can read the different ciphers. So having a mixed set in its data store in /var/lib/myproxy/ seems totally fine. As such an update would be totally transparent for any client.

[fscheiner]

Just to clarify: for each proxy that gets delegated to myproxy it creates and stores a new private key. But changing the format even for an existing server looks fine because it can read the different ciphers. So having a mixed set in its data store in /var/lib/myproxy/ seems totally fine. As such an update would be totally transparent for any client.

Thanks for the clarification. Looking good, so I'd say let's do it.

[msalle]

Other than the extra () in the #ifndef (see cmment there) I think it looks be fine. Also probably good to see whether the tests succeed.

[msalle]

Assuming the builds and tests go well, I'd say looks good to me.

[fscheiner]

Checks look good. Merging this now.