{Role} Secure _create_self_signed_cert

src/azure-cli/azure/cli/command_modules/role/custom.py

@@ -1534,22 +1534,30 @@ def _create_self_signed_cert(start_date, end_date):  # pylint: disable=too-many-
     from OpenSSL import crypto
     from datetime import timedelta
 
-    _, cert_file = tempfile.mkstemp()
-    _, key_file = tempfile.mkstemp()
-
-    # create a file with both cert & key so users can use to login
-    # leverage tempfile ot produce a random file name
-    _, temp_file = tempfile.mkstemp()
-    creds_file = path.join(path.expanduser("~"), path.basename(temp_file) + '.pem')
-
-    # create a key pair
+    # Create a PEM file ~/tmpxxxxxxxx.pem with both PRIVATE KEY & CERTIFICATE so users can use to log in.
+    # The PEM file looks like
+    # -----BEGIN PRIVATE KEY-----
+    # MIIEv...
+    # -----END PRIVATE KEY-----
+    # -----BEGIN CERTIFICATE-----
+    # MIICo...
+    # -----END CERTIFICATE-----
+
+    # Leverage tempfile to produce a random file name. The temp file itself is automatically deleted.
+    # There doesn't seem to be a good way to create a random file name without creating the file itself:
+    # https://stackoverflow.com/questions/26541416/generate-temporary-file-names-without-creating-actual-file-in-python
+    with tempfile.NamedTemporaryFile() as f:
+        temp_file_name = f.name
+    creds_file = path.join(path.expanduser("~"), path.basename(temp_file_name) + '.pem')
+
+    # Create a key pair
     k = crypto.PKey()
     k.generate_key(crypto.TYPE_RSA, 2048)
 
-    # create a self-signed cert
+    # Create a self-signed cert
     cert = crypto.X509()
     subject = cert.get_subject()
-    # as long it works, we skip fileds C, ST, L, O, OU, which we have no reasonable defaults for
+    # As long as it works, we skip fields C, ST, L, O, OU, which we have no reasonable defaults for
     subject.CN = 'CLI-Login'
     cert.set_serial_number(1000)
     asn1_format = '%Y%m%d%H%M%SZ'
@@ -1561,23 +1569,16 @@ def _create_self_signed_cert(start_date, end_date):  # pylint: disable=too-many-
     cert.set_pubkey(k)
     cert.sign(k, 'sha1')
 
-    with open(cert_file, "wt") as f:
-        f.write(crypto.dump_certificate(crypto.FILETYPE_PEM, cert).decode())
-    with open(key_file, "wt") as f:
-        f.write(crypto.dump_privatekey(crypto.FILETYPE_PEM, k).decode())
-
-    cert_string = None
-    with open(creds_file, 'wt') as cf:
-        with open(key_file, 'rt') as f:
-            cf.write(f.read())
-        with open(cert_file, "rt") as f:
-            cert_string = f.read()
-            cf.write(cert_string)
-    os.chmod(creds_file, 0o600)  # make the file readable/writable only for current user
-
-    # get rid of the header and tails for upload to AAD: ----BEGIN CERT....----
-    cert_string = re.sub(r'\-+[A-z\s]+\-+', '', cert_string).strip()
-    return (cert_string, creds_file, cert_start_date, cert_end_date)
+    cert_string = crypto.dump_certificate(crypto.FILETYPE_PEM, cert).decode()
+    key_string = crypto.dump_privatekey(crypto.FILETYPE_PEM, k).decode()
+
+    with os.fdopen(_open(creds_file), 'w+') as cf:
+        cf.write(key_string)
+        cf.write(cert_string)
+
+    # Get rid of the header and tail for uploading to AAD: -----BEGIN CERTIFICATE-----, -----END CERTIFICATE-----
+    cert_string = re.sub(r'-+[A-z\s]+-+', '', cert_string).strip()
+    return cert_string, creds_file, cert_start_date, cert_end_date
 
 
 def _create_self_signed_cert_with_keyvault(cli_ctx, years, keyvault, keyvault_cert_name):  # pylint: disable=too-many-locals
@@ -1870,3 +1871,9 @@ def _random_password(length):
 
     password = first_character + ''.join(password_list)
     return password
+
+
+def _open(location):
+    """Open a file that only the current user can access."""
+    # The 600 seems no-op on Windows, and that is fine.
+    return os.open(location, os.O_RDWR | os.O_CREAT | os.O_TRUNC, 0o600)

src/azure-cli/azure/cli/command_modules/role/tests/latest/test_role.py

@@ -6,6 +6,7 @@
 # AZURE CLI RBAC TEST DEFINITIONS
 import json
 import os
+import sys
 import tempfile
 import time
 import datetime
@@ -118,10 +119,22 @@ def test_create_for_rbac_with_cert_no_assignment(self, resource_group):
                 self.kwargs['app_id'] = result['appId']
 
                 self.assertTrue(result['fileWithCertAndPrivateKey'].endswith('.pem'))
+
+                # On Linux or MacOS, check the cert file is a regular file (S_IFREG 0100000) with permission 600
+                # https://manpages.ubuntu.com/manpages/focal/man7/inode.7.html
+                # Windows doesn't have the Linux permission concept.
+                if sys.platform != 'win32':
+                    assert os.stat(result['fileWithCertAndPrivateKey']).st_mode == 0o100600
+
                 os.remove(result['fileWithCertAndPrivateKey'])
+
                 result = self.cmd('ad sp credential reset -n {app_id} --create-cert',
                                   checks=self.check('name', '{app_id}')).get_output_in_json()
                 self.assertTrue(result['fileWithCertAndPrivateKey'].endswith('.pem'))
+
+                if sys.platform != 'win32':
+                    assert os.stat(result['fileWithCertAndPrivateKey']).st_mode == 0o100600
+
                 os.remove(result['fileWithCertAndPrivateKey'])
         finally:
             self.cmd('ad app delete --id {app_id}',