Skip to content
This repository was archived by the owner on Mar 20, 2024. It is now read-only.

Rework SSH key handling and configuration #76

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Rework SSH key handling and configuration #76

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions cfg.sample.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ app_client_secret = ""
#ssh_key = """
#"""

# If you're using local git, you may want to set up a ssh-config
# file specific to github.com, primarily to ensure the host key
# is configured.
# ssh_config_path = ""

# By default, Homu extracts the name+email from the Github account. However,
# you may want to use a private email for the account, and associate the commits
# with a public email address.
Expand Down
8 changes: 5 additions & 3 deletions homu/git_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
import sys
import os

SSH_KEY_FILE = os.path.join(os.path.dirname(__file__), '../cache/key')


def main():
args = ['ssh', '-i', SSH_KEY_FILE, '-S', 'none'] + sys.argv[1:]
cfgpath = os.environ.get('HOMU_SSH_CONFIG')
args = ['ssh', '-i', os.getenv('HOMU_GIT_KEY_PATH'), '-S', 'none']
if cfgpath is not None:
args.extend(['-F', cfgpath])
args.extend(sys.argv[1:])
os.execvp('ssh', args)


Expand Down
24 changes: 14 additions & 10 deletions homu/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@
from queue import Queue
import os
import subprocess
from .git_helper import SSH_KEY_FILE
import shlex
import tempfile

STATUS_TO_PRIORITY = {
'success': 0,
Expand Down Expand Up @@ -453,12 +453,6 @@ def init_local_git_cmds(repo_cfg, git_cfg):
fpath = 'cache/{}/{}'.format(repo_cfg['owner'], repo_cfg['name'])
url = '[email protected]:{}/{}.git'.format(repo_cfg['owner'], repo_cfg['name'])

if not os.path.exists(SSH_KEY_FILE):
os.makedirs(os.path.dirname(SSH_KEY_FILE), exist_ok=True)
with open(SSH_KEY_FILE, 'w') as fp:
fp.write(git_cfg['ssh_key'])
os.chmod(SSH_KEY_FILE, 0o600)

if not os.path.exists(fpath):
utils.logged_call(['git', 'init', fpath])
utils.logged_call(['git', '-C', fpath, 'remote', 'add', 'origin', url])
Expand Down Expand Up @@ -488,7 +482,6 @@ def create_merge(state, repo_cfg, branch, git_cfg, ensure_merge_equal=False):
desc = 'Merge conflict'

if git_cfg['local_git']:

git_cmd = init_local_git_cmds(repo_cfg, git_cfg)

utils.logged_call(git_cmd('fetch', 'origin', state.base_ref,
Expand Down Expand Up @@ -959,7 +952,8 @@ def fetch_mergeability(mergeable_que):
mergeable_que.task_done()


def check_timeout(states, queue_handler):
def check_timeout(states, queue_handler, tmp_ssh_key):
# This function holds a reference to tmp_ssh_key to keep it alive
while True:
try:
for repo_label, repo_states in states.items():
Expand Down Expand Up @@ -1239,13 +1233,23 @@ def queue_handler():
return process_queue(states, repos, repo_cfgs, logger, buildbot_slots, db, git_cfg)

os.environ['GIT_SSH'] = os.path.join(os.path.dirname(__file__), 'git_helper.py')
sshcfg = cfg_git.get('ssh_config_path')
if sshcfg is not None:
os.environ['HOMU_SSH_CONFIG'] = sshcfg
os.environ['GIT_EDITOR'] = 'cat'

tmp_ssh_key = None
if git_cfg['local_git']:
tmp_ssh_key = tempfile.NamedTemporaryFile(prefix='homu-sshkey')
Copy link

@aneeshusa aneeshusa Jan 17, 2017

Choose a reason for hiding this comment

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

It seems bad to not clean up a temporary file with key info, esp. since it's named. It would be nice to use a context manager to automatically close this file (if we have one i.e. if git_cfg['local_git']), and wrap the thread spawning code in a with block that uses said context manager.

tmp_ssh_key.write(git_cfg['ssh_key'].encode('utf-8'))
tmp_ssh_key.flush()
os.environ['HOMU_GIT_KEY_PATH'] = tmp_ssh_key.name
Copy link

@aneeshusa aneeshusa Jan 17, 2017

Choose a reason for hiding this comment

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

This seems like a hack. Passing the filename of the temp_ssh_key in an environment variable and then having to add a dummy parameter to check_timeout to keep it alive is likely to break down the road. Plus, I've never liked programs changing their environments, which act as superglobals. Instead, we can store tmp_ssh_key in git_cfg, which is conveniently already threaded through the code into all the places we execute git, thus automatically preserving lifetimes.

HOMU_GIT_KEY_PATH still needs to be passed as an environment variable to git_helper.py. It turns out (almost!) all calls to git are in one of two forms: utils.silent_call(git_cmd(...)) or utils.logged_call(git_cmd(...)), and all calls to utils.silent_call and utils.logged_call invoke git.

I would prefer git_cmd (aka the function returned by init_local_git_cmds) to return both the complete git command array as well as a dict representing the env vars, e.g. with a tuple or dict.
Then, update utils.silent_call and utils.logged_call to handle the new interface and pass an updated environment through to subprocess.

The same thing should be done for HOMU_SSH_CONFIG, GIT_SSH, and GIT_EDITOR.

Extra fun fact: updating os.environ in Python calls putenv, which has a flawed interface that is broken in various ways. Another reason not to modify it!


from . import server
Thread(target=server.start, args=[cfg, states, queue_handler, repo_cfgs, repos, logger, buildbot_slots, my_username, db, repo_labels, mergeable_que, gh]).start()

Thread(target=fetch_mergeability, args=[mergeable_que]).start()
Thread(target=check_timeout, args=[states, queue_handler]).start()
Thread(target=check_timeout, args=[states, queue_handler, tmp_ssh_key]).start()

queue_handler()

Expand Down