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

Conversation

@cgwalters
Copy link

I'm trying to run homu in a container 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, then chown 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".


This change is Reviewable

I'm trying to run homu in a container 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, then chown 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".
@cgwalters
Copy link
Author

Migrated from barosl#136

@cgwalters
Copy link
Author

Anyone want to take a look at this one? It's currently a hard requirement for my work on a Docker container for Homu.

(Although I am considering not embedding the ssh key in the config, but having it as a separate state volume)

@cgwalters
Copy link
Author

I've been thinking about simply having ssh_args as a config option rather than embedding the ssh key in the config. In practice, users already need to retain state for the known_hosts file, so having them manage the -i sshkey seems not too onerous. Thoughts?

@cgwalters
Copy link
Author

Regardless though, a review of this PR would be nice.

@Manishearth
Copy link
Member

Why is the NamedTemporaryFile necessary?

Perhaps it would be easier to just specify a keyfile path in the config?

@cgwalters
Copy link
Author

Yeah, that's where I was going with the ssh_args option. I'll take a crack at that soon if no one thinks of anything better.

I'm wondering whether it's worth keeping backwards compatibility with the sshkey option? I don't have a good sense of Homu users or how they upgrade.

@cbrewster cbrewster assigned cbrewster and unassigned cbrewster May 15, 2016
@Manishearth
Copy link
Member

Backcompat is good, yes.

@bors-servo
Copy link

☔ The latest upstream changes (presumably #54) made this pull request unmergeable. Please resolve the merge conflicts.

@cgwalters
Copy link
Author

Obsoleted by #76

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants