From 5b401fd1e89e61b483a3c06f5cf021209caab906 Mon Sep 17 00:00:00 2001
From: Timothee 'TTimo' Besset <ttimo@ttimo.net>
Date: Tue, 13 Jun 2023 10:05:59 -0600
Subject: [PATCH] Improve permission management of the devkit ssh key.

---
 ChangeLog                        |  4 ++
 client/devkit_client/__init__.py | 73 ++++++++++++++++++--------------
 2 files changed, 45 insertions(+), 32 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index a79191b..1ad3829 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,7 @@
+2023-06-13 Timothee Besset <ttimo@valvesoftware.com>
+
+	* Release v.0.20230613.0
+	* Improve permission management of the devkit ssh key.
 	* Windows: migrate to Python 3.11
 
 2023-04-09 Timothee Besset <ttimo@valvesoftware.com>
diff --git a/client/devkit_client/__init__.py b/client/devkit_client/__init__.py
index 7bdf302..c395714 100644
--- a/client/devkit_client/__init__.py
+++ b/client/devkit_client/__init__.py
@@ -460,6 +460,40 @@ def get_public_key(key):
     return public_key
 
 
+def _fix_key_permissions(key_path, pubkey_path):
+    if platform.system() == 'Linux':
+        # enforce/fix file permissions
+        os.chmod(key_path, 0o400)
+        os.chmod(pubkey_path, 0o400)
+    else:
+        # fix permissions for private keys the windows way, keep ssh happy
+        # do not rely on get_username here, use the full domain\name of the current user - some systems fail if you just give username
+        # also icacls.exe docs suggest this should work with the SID, but that will fail with "No mapping between account names and security IDs was done."
+        # I really hate everything about this ...
+        username = windows_get_domain_and_name()
+        for cmd in (
+            ['icacls.exe', key_path, '/Reset'],
+            ['icacls.exe', key_path, '/Inheritance:r'],
+            ['icacls.exe', key_path, '/Grant:r', f'{username}:(R)'],
+            # for diagnostics
+            ['icacls.exe', key_path],
+        ):
+            cp = subprocess.run(
+                cmd,
+                stdin=subprocess.DEVNULL,
+                stdout=subprocess.PIPE,
+                stderr=subprocess.STDOUT,
+                creationflags=SUBPROCESS_CREATION_FLAGS,
+                text=True
+            )
+            if cp.returncode == 0:
+                logger.debug(' '.join(cmd))
+                logger.debug(cp.stdout.strip('\n'))
+            else:
+                logger.warning(' '.join(cmd))
+                logger.warning(cp.stdout.strip('\n'))
+
+
 def ensure_devkit_key():
     # this is called from threads and needs to be marshalled due to possible fs and permissions manipulations
     global g_lock
@@ -469,7 +503,12 @@ def ensure_devkit_key():
         pubkey_path = os.path.join(key_folder, 'devkit_rsa.pub')
         try:
             key = paramiko.RSAKey.from_private_key_file(key_path)
-        except FileNotFoundError:
+        except PermissionError as e:
+            logger.warning(e)
+            logger.warning('Fix permissions and try paramiko key load again.')
+            _fix_key_permissions(key_path, pubkey_path)
+            key = paramiko.RSAKey.from_private_key_file(key_path)
+        except FileNotFoundError as e:
             # first time setup
             logger.warning(f'{key_path} not found - generating a new passwordless devkit key')
             os.makedirs(key_folder, exist_ok=True)
@@ -481,37 +520,7 @@ def ensure_devkit_key():
             key.write_private_key_file(key_path)
             o = open(pubkey_path, 'w')
             o.write(get_public_key(key))
-        if platform.system() == 'Linux':
-            # enforce/fix file permissions
-            os.chmod(key_path, 0o400)
-            os.chmod(pubkey_path, 0o400)
-        else:
-            # fix permissions for private keys the windows way, keep ssh happy
-            # do not rely on get_username here, use the full domain\name of the current user - some systems fail if you just give username
-            # also icacls.exe docs suggest this should work with the SID, but that will fail with "No mapping between account names and security IDs was done."
-            # I really hate everything about this ...
-            username = windows_get_domain_and_name()
-            for cmd in (
-                ['icacls.exe', key_path, '/Reset'],
-                ['icacls.exe', key_path, '/Inheritance:r'],
-                ['icacls.exe', key_path, '/Grant:r', f'{username}:(R)'],
-                # for diagnostics
-                ['icacls.exe', key_path],
-            ):
-                cp = subprocess.run(
-                    cmd,
-                    stdin=subprocess.DEVNULL,
-                    stdout=subprocess.PIPE,
-                    stderr=subprocess.STDOUT,
-                    creationflags=SUBPROCESS_CREATION_FLAGS,
-                    text=True
-                )
-                if cp.returncode == 0:
-                    logger.debug(' '.join(cmd))
-                    logger.debug(cp.stdout.strip('\n'))
-                else:
-                    logger.warning(' '.join(cmd))
-                    logger.warning(cp.stdout.strip('\n'))
+        _fix_key_permissions(key_path, pubkey_path)
         return (key, key_path, pubkey_path)
 
 
-- 
GitLab