Support both ECC and RSA keys during initialization #1552
Conversation
* `openssl pkey` supports both RSA and ECC private keys Fixes: #1550
Codecov Report
@@ Coverage Diff @@
## develop #1552 +/- ##
==========================================
Coverage ? 64.18%
==========================================
Files ? 78
Lines ? 11068
Branches ? 1563
==========================================
Hits ? 7104
Misses ? 3623
Partials ? 341
Continue to review full report at Codecov.
|
|
Running the change through our end-to-end testing workflow. |
| @@ -3292,7 +3292,7 @@ class Certificates(object): | |||
| index = 1 | |||
| filename = str(index) + ".prv" | |||
| while os.path.isfile(filename): | |||
| pubkey = RunGetOutput(Openssl + " rsa -in " + filename + " -pubout 2> /dev/null ")[1] | |||
| pubkey = RunGetOutput(Openssl + " pkey -in " + filename + " -pubout 2> /dev/null ")[1] | |||
There was a problem hiding this comment.
I think this is the wrong place to make the change in. We don't use waagent2.0 anymore.
You'll need to make a change in get_pubkey_from_prv method of the CryptUtil class located in the azurelinuxagent/common/utils/cryptutil.py file.
def get_pubkey_from_prv(self, file_name):
if not os.path.exists(file_name):
raise IOError(errno.ENOENT, "File not found", file_name)
else:
cmd = "{0} rsa -in {1} -pubout 2>/dev/null".format(self.openssl_cmd,
file_name)
pub = shellutil.run_get_output(cmd)[1]
return pub
Also, when you make the change in the new location, could you please add a corresponding change in tests/utils/test_crypt_util.py as well?
Thanks
There was a problem hiding this comment.
ok - I had no idea about walinuxagent not using waagent2.0 anymore - is there more info on this somewhere? I'm also not a python developer, so it might be better for someone with more python expertise to fix this if test additions are required.
Can you tell me how I can test this on an Azure VM, to make sure it works? See my comment here, perhaps my workaround for testing this was wrong. If my workaround was reasonable, then this works as is - I've tested it on our VMs using the patch method described.
There was a problem hiding this comment.
@vrdmr - I've made the additional change you requested to cryptutil.py.
I didn't find any tests in test_crypt_util.py referencing get_pubkey_from_prv(), so I didn't add a test case for a non RSA cert either. I understand the desirability of adding one, but I'm afraid my attempts at adding a test case would not be a good use of time for either of us. Otoh, this would mostly be testing openssl.
I'd be happy to help test this in Azure, if you can give me guidance on the best way to test this on a VM/vmset.
There was a problem hiding this comment.
ok - I had no idea about walinuxagent not using waagent2.0 anymore - is there more info on this somewhere?
That's our mistake. I'll add some info on making changes to waagent2.0, and update our README.md.
Can you tell me how I can test this on an Azure VM, to make sure it works?
How we test a change in the provisioning aspect of the agent, is to create a new Azure VM, make the changes in the daemon code (that's the part of agent which comes baked into the distro image, and installed in the site-packages folder), cleanup the installation and restart the service.
Example script to run after you have made the change:
export PATH=$PATH:/usr/sbin
sed -i 's/Logs.Verbose=n/Logs.Verbose=y/g' /etc/waagent.conf
sed -i '/AutoUpdate.*/s/^/#/' /etc/waagent.conf
echo 'AutoUpdate.Enabled=y' >> /etc/waagent.conf
waagent --version
service walinuxagent stop
waagent -deprovision -force
rm -rf /var/lib/waagent
service walinuxagent start
and observe `/var/log/waagent.log'
I understand the desirability of adding one, but I'm afraid my attempts at adding a test case would not be a good use of time for either of us. Otoh, this would mostly be testing openssl.
The rationale for adding the unittest here is to get sanity testing done on syntax as well as major cases. You can mock the output of the RunGetOutput in this case and just do a very basic unittest. Please let me know if you need any help with that, and I'll make some time for it. With Python, we have been bitten with minor syntax issues earlier due to not enough testing and we are putting an effort to make sure all the methods have
There was a problem hiding this comment.
@johncrim - I added couple of unittests. Please let me know if it looks good. Thanks.
* Per @vrdmr, waagent2.0 is no longer used
openssl pkeysupports both RSA and ECC private keysFixes: #1550
Description
Issue #1550
PR information
Quality of Code and Contribution Guidelines
This change is