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

Show file tree
Hide file tree
Changes from all commits
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
7 changes: 4 additions & 3 deletions azurelinuxagent/common/utils/cryptutil.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@

DECRYPT_SECRET_CMD = "{0} cms -decrypt -inform DER -inkey {1} -in /dev/stdin"


class CryptUtil(object):
def __init__(self, openssl_cmd):
self.openssl_cmd = openssl_cmd
Expand All @@ -53,8 +54,8 @@ 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)
cmd = "{0} pkey -in {1} -pubout 2>/dev/null".format(self.openssl_cmd,
file_name)
pub = shellutil.run_get_output(cmd)[1]
return pub

Expand Down Expand Up @@ -117,7 +118,7 @@ def asn1_to_ssh(self, pubkey):
keydata.extend(b"\0")
keydata.extend(self.num_to_bytes(n))
keydata_base64 = base64.b64encode(bytebuffer(keydata))
return ustr(b"ssh-rsa " + keydata_base64 + b"\n",
return ustr(b"ssh-rsa " + keydata_base64 + b"\n",
encoding='utf-8')
except ImportError as e:
raise CryptError("Failed to load pyasn1.codec.der")
Expand Down
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
9 changes: 9 additions & 0 deletions tests/data/wire/trans_pub
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
-----BEGIN PUBLIC KEY-----
MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEA09wkCR3pXk16iBIqMh5N
c5YLnHMpPK4k+3hhkxVKixTSUjprTAen6DZ8/bbOtWzBb5AnPoBVaiMgSotC6ndb
IJdlO/xFRuUeciOS9f/4n8ZoubPQbknNkikQsvYLwh9AsfYiI+Ur0s5AfTRbvhYV
wrdCpwnorDwZxVp5JdPWvtdBwYyoSNxYmSkougwm/csy58T4kx1tcNQZj4+ztmJy
7wpe8E9opWxzofaOuoFLx62NdvMvKt7NNQPPjmubJEnMI7lKTamiG5iDvfBTKQBQ
9XF3svxadLKrPW/jOs5uqfAEDKivrslH+GNMF+MU693yoUaid+K/ZWfP1exgVNmx
cQIDAQAB
-----END PUBLIC KEY-----
34 changes: 16 additions & 18 deletions tests/utils/test_crypt_util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,33 +15,17 @@
# Requires Python 2.6+ and Openssl 1.0+
#

import base64
import binascii
import errno as errno
import glob
import random
import string
import subprocess
import sys
import tempfile
import uuid
import unittest

import azurelinuxagent.common.conf as conf
import azurelinuxagent.common.utils.shellutil as shellutil
from azurelinuxagent.common.future import ustr
from azurelinuxagent.common.utils.cryptutil import CryptUtil
from azurelinuxagent.common.exception import CryptError
from azurelinuxagent.common.version import PY_VERSION_MAJOR
from azurelinuxagent.common.utils.cryptutil import CryptUtil
from tests.tools import *
from subprocess import CalledProcessError


def is_python_version_26():
return sys.version_info[0] == 2 and sys.version_info[1] == 6


class TestCryptoUtilOperations(AgentTestCase):

def test_decrypt_encrypted_text(self):
encrypted_string = load_data("wire/encrypted.enc")
prv_key = os.path.join(self.tmp_dir, "TransportPrivate.pem")
Expand Down Expand Up @@ -75,6 +59,20 @@ def test_decrypt_encrypted_text_text_not_encrypted(self):
crypto = CryptUtil(conf.get_openssl_cmd())
self.assertRaises(CryptError, crypto.decrypt_secret, encrypted_string, prv_key)

def test_get_pubkey_from_crt(self):
crypto = CryptUtil(conf.get_openssl_cmd())
prv_key = os.path.join(data_dir, "wire", "trans_prv")
expected_pub_key = os.path.join(data_dir, "wire", "trans_pub")

with open(expected_pub_key) as fh:
self.assertEqual(fh.read(), crypto.get_pubkey_from_prv(prv_key))

def test_get_pubkey_from_crt_invalid_file(self):
crypto = CryptUtil(conf.get_openssl_cmd())
prv_key = os.path.join(data_dir, "wire", "trans_prv_does_not_exist")

self.assertRaises(IOError, crypto.get_pubkey_from_prv, prv_key)


if __name__ == '__main__':
unittest.main()