Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support both ECC and RSA keys during initialization #1552

Merged
merged 3 commits into from
Jul 3, 2019
Merged

Support both ECC and RSA keys during initialization #1552

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6d8d6c9
Support both ECC and RSA keys during initialization
johncrim Jun 11, 2019
753664f
Address PR comment: Change openssl use in cryptutil.py as well.
johncrim Jul 1, 2019
39e47af
Adding unittests for pvt key change
vrdmr Jul 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion bin/waagent2.0
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@johncrim johncrim Jun 29, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johncrim - I added couple of unittests. Please let me know if it looks good. Thanks.

os.rename(filename, keys[pubkey] + ".prv")
os.chmod(keys[pubkey] + ".prv", 0600)
MyDistro.setSelinuxContext( keys[pubkey] + '.prv','unconfined_u:object_r:ssh_home_t:s0')
Expand Down