Closed fscheiner closed 1 month ago
@msalle : Let's see if the CI is also happy. :-)
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()
?
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.
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.
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.
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...
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.
So the improvement will by default only benefit new installations? That would be fine with me.
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.
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.
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.
Checks look good. Merging this now.
As per #229 MyProxy still used an old cipher for encrypting private keys. Changes courtesy of Mischa Salle (@msalle).
Fixes #229.