-
Notifications
You must be signed in to change notification settings - Fork 49
Rework SSH key handling and configuration #76
base: master
Are you sure you want to change the base?
Conversation
|
In #18 I talked about a generic "ssh_args" but that turned out to be annoying to implement because it'd require serializing an array into a environment variable. In general one can do basically everything with a ssh config file, so supporting that works well too. |
I'm running homu in OpenShift v3/Kubernetes where it's installed (as root) to `/usr`, but we run as non-root. This follows general best practice that apps shouldn't be able to mutate their code. However, we were trying to write the ssh key to `/usr`. Fix this by generating a tempfile. This is also more secure as it closes a prior race condition where we'd write the file with potentially non-private access permissions, then chmod it. Also rework things so that we only write the key once at startup. By using `NamedTemporaryFile`, it'll be `unlink()ed` once the object goes out of scope. To keep it alive long enough, pass it as an argument to the "main loop". Secondly, I want to have the github.com host key in a ConfigMap, distinct from the code. And I want to tweak the ssh configuration for https://stribika.github.io/2015/01/04/secure-secure-shell.html among other things. Hence, this also adds a `ssh_config_file` config option.
00e0efd to
34639b3
Compare
|
Updated 🆕 to fix flake8. |
|
r? @aneeshusa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry about the review delay, I'm not familiar with the Homu code base and it's rather spaghetti-y for my taste.
I like the increased configurability, but I'm worried about the implementation in terms of robustness,
so I've outlined an alternate implementation.
| tmp_ssh_key = tempfile.NamedTemporaryFile(prefix='homu-sshkey') | ||
| 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 |
There was a problem hiding this comment.
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!
|
|
||
| tmp_ssh_key = None | ||
| if git_cfg['local_git']: | ||
| tmp_ssh_key = tempfile.NamedTemporaryFile(prefix='homu-sshkey') |
There was a problem hiding this comment.
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.
|
☔ The latest upstream changes (presumably #84) made this pull request unmergeable. Please resolve the merge conflicts. |
I'm running homu in OpenShift v3/Kubernetes where it's installed (as root) to
/usr, but we run as non-root. This follows general best practice that appsshouldn't be able to mutate their code.
However, we were trying to write the ssh key to
/usr. Fix this bygenerating a tempfile. This is also more secure as it closes a prior
race condition where we'd write the file with potentially non-private
access permissions, then chmod it.
Also rework things so that we only write the key once at startup. By
using
NamedTemporaryFile, it'll beunlink()edonce the object goesout of scope. To keep it alive long enough, pass it as an argument to
the "main loop".
Secondly, I want to have the github.com host key in a ConfigMap,
distinct from the code. And I want to tweak the
ssh configuration for https://stribika.github.io/2015/01/04/secure-secure-shell.html
among other things.
Hence, this also adds a
ssh_config_fileconfig option.This change is