Skip to content

Ssh key handling #136

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

Open
cgwalters wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Ssh key handling #136

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
aab6018
utils: Log stderr of failed processes
cgwalters Mar 11, 2016
3b46d3a
Rework SSH key handling
cgwalters Mar 11, 2016
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: 1 addition & 4 deletions homu/git_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
#!/usr/bin/env python3

import sys
import subprocess
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:]
args = ['ssh', '-i', os.getenv('HOMU_GIT_KEY_PATH'), '-S', 'none'] + sys.argv[1:]
os.execvp('ssh', args)

if __name__ == '__main__':
Expand Down
19 changes: 11 additions & 8 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

@ashcrow ashcrow Mar 18, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 for the use of tempfile!

import sys

STATUS_TO_PRIORITY = {
Expand Down Expand Up @@ -454,11 +454,6 @@ def create_merge(state, repo_cfg, branch, git_cfg):
fpath = 'cache/{}/{}'.format(repo_cfg['owner'], repo_cfg['name'])
url = 'git@github.com:{}/{}.git'.format(repo_cfg['owner'], repo_cfg['name'])

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 @@ -744,7 +739,8 @@ def fetch_mergeability(mergeable_que):
finally:
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 @@ -1007,11 +1003,18 @@ def queue_handler():
os.environ['GIT_SSH'] = os.path.join(os.path.dirname(__file__), 'git_helper.py')
os.environ['GIT_EDITOR'] = 'cat'

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

@ashcrow ashcrow Mar 18, 2016

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I didn't see any explicit close of the NamedTemporaryFile but it still might be worth adding delete=False to ensure the file isn't deleted if garbage collection decides it's no longer in use.

Example:

import os
import tempfile

def make_temp_file():
    n = tempfile.NamedTemporaryFile()
    n.write(b'ok')
    n.flush()
    return n.name

name = make_temp_file()
# Garbage collection has already closed the file.
print(os.stat(name))

@cgwalters cgwalters Mar 24, 2016

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This commit was intentionally tying the tmpfile lifecycle to Python GC, so that it goes away if the process exits etc. That's why I had to do the hack of passing it as an argument to the "mainloop" just so it was kept alive.

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

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
2 changes: 1 addition & 1 deletion homu/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ def lazy_debug(logger, f):
logger.debug(f())

def logged_call(args):
try: subprocess.check_call(args, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL)
try: subprocess.check_call(args)
except subprocess.CalledProcessError as e:
print('* Failed to execute command: {}'.format(args))
raise
Expand Down