Repository: skypilot-org/skypilot
Pull request: Fix git SSH agent forwarding issues
Polishing SSH and git functionality.

### Tests
```
class c1:
  name = 'cluster1'
  ip = '127.0.0.1'
  auth_config = {'ssh_user': 'ubuntu', 'ssh_private_key': '~/.ssh/id_rsa'}

class c2:
  name = 'cluster2'
  ip = '127.0.0.2'
  auth_config = {'ssh_user': 'ubuntu', 'ssh_private_key': '~/.ssh/id_rsa'}

class c3:
  name = 'cluster3'
  ip = '127.0.0.3'
  auth_config = {'ssh_user': 'ubuntu', 'ssh_private_key': '~/.ssh/id_rsa'}

class c2_overwrite:
  name = 'cluster2'
  ip = 'overwrite'
  auth_config = {'ssh_user': 'ubuntu', 'ssh_private_key': '~/.ssh/id_rsa'}

# add some clusters
backend_utils.SSHConfigHelper.add_cluster(c1.name, c1.ip, c1.auth_config)
backend_utils.SSHConfigHelper.add_cluster(c2.name, c2.ip, c2.auth_config)
backend_utils.SSHConfigHelper.add_cluster(c3.name, c3.ip, c3.auth_config)
import pdb; pdb.set_trace()

# overwrite
backend_utils.SSHConfigHelper.add_cluster(c2_overwrite.name, c2_overwrite.ip, c2_overwrite.auth_config)

# delete middle config
backend_utils.SSHConfigHelper.remove_cluster(c2_overwrite.ip, c2_overwrite.auth_config)

# remove everything
backend_utils.SSHConfigHelper.remove_cluster(c1.ip, c1.auth_config)
backend_utils.SSHConfigHelper.remove_cluster(c2.ip, c2.auth_config)
backend_utils.SSHConfigHelper.remove_cluster(c2_overwrite.ip, c2_overwrite.auth_config)
backend_utils.SSHConfigHelper.remove_cluster(c3.ip, c3.auth_config)
```

- [x] Verified that SSH config no longer has a ton of trailing lines. 
- [x] git key path exists in `ssh-add -L` locally and on server
- [x] git forward works on server

Problem statement:
git credential forwarding doesn't work for https auth
Many users use `gh auth login` to authenticate with github and cache their git credentials locally. This uses https instead of ssh for git authentication. 

Current git credential forwarding in sky supports forwarding only ssh credentials. Is there some way we can add support for forwarding https credentials (access tokens?)? @gmittal 

ssh config unexpected newlines and known_host check
The ssh config helper is very useful. I found two small problems:
1. After creating and tearing down clusters, there will be multiple unexpected newlines in the `~/.ssh/config` at the beginning and the end of the file.
2. We need to add `StrictHostKeyChecking no` after each entry we add, so as to avoid the host key conflict with the known_hosts, when the user creates, tears down, and creates again with the same cluster name.

Hints from issue discussion:
HTTPS host forwarding doesn't seem to be documented anywhere by GitHub, so for now we'll have to require that if users want git to magically work on their remote VMs they're going to need to have an SSH key set up locally. `gh auth login` supports SSH setup quite easily in their login flow, including creating a new key and registering it with a GitHub account.

Commit message: Fix remove lines

Patch to review:
diff --git a/prototype/sky/backends/backend_utils.py b/prototype/sky/backends/backend_utils.py
index 3be7672af58..eba2634e082 100644
--- a/prototype/sky/backends/backend_utils.py
+++ b/prototype/sky/backends/backend_utils.py
@@ -178,8 +178,8 @@ def _get_generated_config(cls, autogen_comment: str, host_name: str,
               IdentityFile {ssh_key_path}
               IdentitiesOnly yes
               ForwardAgent yes
-              Port 22
-              """)
+              StrictHostKeyChecking no
+              Port 22""")
         return codegen
 
     @classmethod
@@ -237,18 +237,19 @@ def add_cluster(
         # Add (or overwrite) the new config.
         if overwrite:
             assert overwrite_begin_idx is not None
-            updated_lines = codegen.splitlines(keepends=True)
+            updated_lines = codegen.splitlines(keepends=True) + ['\n\n']
             config[overwrite_begin_idx:overwrite_begin_idx +
                    len(updated_lines)] = updated_lines
             with open(config_path, 'w') as f:
-                f.write('\n')
-                f.writelines(config)
-                f.write('\n')
+                f.write(''.join(config).strip())
         else:
             with open(config_path, 'a') as f:
-                f.write('\n')
+                f.write('\n\n')
                 f.write(codegen)
 
+        # Add git credential forwarding
+        GitCredentialsHelper.start_agent()
+
     @classmethod
     def remove_cluster(cls, ip: str, auth_config: Dict[str, str]):
         """Remove authentication information for cluster from local SSH config.
@@ -281,6 +282,12 @@ def remove_cluster(cls, ip: str, auth_config: Dict[str, str]):
         if start_line_idx is None:  # No config to remove.
             return
 
+        # Scan for end of previous config.
+        cursor = start_line_idx
+        while cursor > 0 and len(config[cursor].strip()) > 0:
+            cursor -= 1
+        prev_end_line_idx = cursor
+
         # Scan for end of the cluster config.
         end_line_idx = None
         cursor = start_line_idx + 1
@@ -293,9 +300,52 @@ def remove_cluster(cls, ip: str, auth_config: Dict[str, str]):
             cursor += 1
 
         # Remove sky-generated config and update the file.
-        config[start_line_idx:end_line_idx] = []
+        config[prev_end_line_idx:end_line_idx] = [
+            '\n'
+        ] if end_line_idx is not None else []
         with open(config_path, 'w') as f:
-            f.writelines(config)
+            f.write(''.join(config).strip())
+
+
+class GitCredentialsHelper(object):
+    """Helper for handling local git SSH credentials."""
+
+    # TODO: Add support for other remotes (e.g. Bitbucket)
+    git_remote_url = 'git@github.com'
+
+    @classmethod
+    def _get_ssh_key(cls) -> Optional[str]:
+        """Find SSH key used for git authentication."""
+        out = run(f'ssh -T {cls.git_remote_url} -v',
+                  check=False,
+                  capture_output=True)
+        ssh_log = out.stderr.decode('utf-8').splitlines()
+
+        # scan for the key
+        key_path = None
+        cursor = 0
+        while cursor < len(ssh_log):
+            line = ssh_log[cursor]
+            if 'Server accepts key' in line:
+                key_path = [e for e in line.split() if '.ssh' in e][0]
+                logger.info(f'Found git key: {key_path}')
+                break
+            if 'Permission denied (publickey)' in line:
+                logger.info('Git SSH key not configured.'
+                            'Git credential forwarding will not work over SSH.')
+                break
+            cursor += 1
+
+        return key_path
+
+    @classmethod
+    def start_agent(cls) -> None:
+        """Start SSH agent for credential forwarding."""
+        key_path = cls._get_ssh_key()
+        if key_path is not None:
+            logger.info('Starting local SSH agent and adding GitHub SSH key.')
+            run('eval "$(ssh-agent -s)" && '
+                f'ssh-add -K {key_path} || ssh-add {key_path}')
 
 
 # TODO(suquark): once we have sky on PYPI, we should directly install sky


Code review comments:
diff_hunk: @@ -293,9 +300,52 @@ def remove_cluster(cls, ip: str, auth_config: Dict[str, str]):
             cursor += 1
 
         # Remove sky-generated config and update the file.
-        config[start_line_idx:end_line_idx] = []
+        config[prev_end_line_idx:end_line_idx] = [
+            '\n'
+        ] if end_line_idx is not None else []
         with open(config_path, 'w') as f:
-            f.writelines(config)
+            f.write(''.join(config).strip())
+
+
+class GitCredentialsHelper(object):
+    """Helper for handling local git SSH credentials."""
+
+    # TODO: Add support for other remotes (e.g. Bitbucket)
+    git_remote_url = 'git@github.com'
+
+    @classmethod
+    def _get_ssh_key(cls) -> Optional[str]:
+        """Find SSH key used for git authentication."""
+        out = run(f'ssh -T {cls.git_remote_url} -v',
+                  check=False,
+                  capture_output=True)
path: prototype/sky/backends/backend_utils.py
text: @user1:
nit: How about using `stderr=subprocess.PIPE` instead, if we still want to support python 3.6? The `capture_output` is added in python 3.7.

@author:
Fixed