scripts: bind ~/.ssh over the chroot's ~/.ssh.

scripts: bind ~/.ssh over the chroot's ~/.ssh.

This also mounts the path to our ssh-agent socket (usually in /tmp) inside the
chroot so we can use our external agent.

TEST=None

BUG=None

Change-Id: I543e8b2527be9958c1158234f39ecc34fc9dd0df
Signed-Off-By: Elly Jones <ellyjones@chromium.org>;
Signed-Off-By: sosa <sosa@chromium.org>;

[Olof Johansson, 2010-08-30 21:37:30]

Please close this bug then:
http://code.google.com/p/chromium-os/issues/detail?id=6242

[Elly Fong-Jones, 2010-08-30 21:51:08]

On 2010/08/30 21:37:30, Olof Johansson wrote:
> Please close this bug then:
> http://code.google.com/p/chromium-os/issues/detail?id=6242

LGTY?

[Olof Johansson, 2010-08-31 01:04:29]

I'd prefer if someone from the build team looked LGTM'ed it.

[sosa, 2010-08-31 01:28:38]

nits

http://codereview.chromium.org/3277006/diff/1/2
File enter_chroot.sh (right):

http://codereview.chromium.org/3277006/diff/1/2#newcode108
enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
I know it's not consistent elsewhere in this code but you should decide whether
to use ${Var} or $Var.  I personally prefer ${Var} when possible.

http://codereview.chromium.org/3277006/diff/1/2#newcode108
enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
Check here also if [ -n "$SSH_AUTH_SOCK" ] and -d "${HOME}/.ssh"

http://codereview.chromium.org/3277006/diff/1/2#newcode110
enter_chroot.sh:110: mkdir "${FLAGS_chroot}/home/${USER}/.ssh"
-p?  Also can you re-use ${MOUNTED_PATH}?

http://codereview.chromium.org/3277006/diff/1/2#newcode111
enter_chroot.sh:111: sudo mount --bind $HOME/.ssh "$MOUNTED_PATH" || \
Quotes around vars like so:  "${HOME}/.ssh"

http://codereview.chromium.org/3277006/diff/1/2#newcode112
enter_chroot.sh:112: warn "Could not mount $MOUNTED_PATH"
use die instead of warn.  should work well with additional checks (i.e. it's
pretty bad if they can't bind mount a directory that exists.

http://codereview.chromium.org/3277006/diff/1/2#newcode116
enter_chroot.sh:116: warn "Count not mount $ASOCK"
same ... warn vs. die

[Elly Fong-Jones, 2010-08-31 12:52:56]

On 2010/08/31 01:28:38, sosa wrote:
> nits
> 
> http://codereview.chromium.org/3277006/diff/1/2
> File enter_chroot.sh (right):
> 
> http://codereview.chromium.org/3277006/diff/1/2#newcode108
> enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> I know it's not consistent elsewhere in this code but you should decide
whether
> to use ${Var} or $Var.  I personally prefer ${Var} when possible.

Done.

> http://codereview.chromium.org/3277006/diff/1/2#newcode108
> enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> Check here also if [ -n "$SSH_AUTH_SOCK" ] and -d "${HOME}/.ssh"

Done.

> http://codereview.chromium.org/3277006/diff/1/2#newcode110
> enter_chroot.sh:110: mkdir "${FLAGS_chroot}/home/${USER}/.ssh"
> -p?  Also can you re-use ${MOUNTED_PATH}?

Done and done.

> http://codereview.chromium.org/3277006/diff/1/2#newcode111
> enter_chroot.sh:111: sudo mount --bind $HOME/.ssh "$MOUNTED_PATH" || \
> Quotes around vars like so:  "${HOME}/.ssh"

Done.

> http://codereview.chromium.org/3277006/diff/1/2#newcode112
> enter_chroot.sh:112: warn "Could not mount $MOUNTED_PATH"
> use die instead of warn.  should work well with additional checks (i.e. it's
> pretty bad if they can't bind mount a directory that exists.

Done.

> http://codereview.chromium.org/3277006/diff/1/2#newcode116
> enter_chroot.sh:116: warn "Count not mount $ASOCK"
> same ... warn vs. die

Done.

[seano, 2010-08-31 14:17:24]

Please please do not bind mount everyone's private ~/.ssh dirs* into a build
chroot where untrusted/third-party/untested code and scripst run (as root)

*often mounted via NFS :)

Probably a better idea (from IRC just now) is to specifically copy ~/.ssh/config
to a tmpfile and bind/copy that into the chroot, as well as SSH_AUTH_SOCK.

On 2010/08/31 12:52:56, Elly Jones wrote:
> On 2010/08/31 01:28:38, sosa wrote:
> > nits
> > 
> > http://codereview.chromium.org/3277006/diff/1/2
> > File enter_chroot.sh (right):
> > 
> > http://codereview.chromium.org/3277006/diff/1/2#newcode108
> > enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> > I know it's not consistent elsewhere in this code but you should decide
> whether
> > to use ${Var} or $Var.  I personally prefer ${Var} when possible.
> 
> Done.
> 
> > http://codereview.chromium.org/3277006/diff/1/2#newcode108
> > enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> > Check here also if [ -n "$SSH_AUTH_SOCK" ] and -d "${HOME}/.ssh"
> 
> Done.
> 
> > http://codereview.chromium.org/3277006/diff/1/2#newcode110
> > enter_chroot.sh:110: mkdir "${FLAGS_chroot}/home/${USER}/.ssh"
> > -p?  Also can you re-use ${MOUNTED_PATH}?
> 
> Done and done.
> 
> > http://codereview.chromium.org/3277006/diff/1/2#newcode111
> > enter_chroot.sh:111: sudo mount --bind $HOME/.ssh "$MOUNTED_PATH" || \
> > Quotes around vars like so:  "${HOME}/.ssh"
> 
> Done.
> 
> > http://codereview.chromium.org/3277006/diff/1/2#newcode112
> > enter_chroot.sh:112: warn "Could not mount $MOUNTED_PATH"
> > use die instead of warn.  should work well with additional checks (i.e. it's
> > pretty bad if they can't bind mount a directory that exists.
> 
> Done.
> 
> > http://codereview.chromium.org/3277006/diff/1/2#newcode116
> > enter_chroot.sh:116: warn "Count not mount $ASOCK"
> > same ... warn vs. die
> 
> Done.

[seano, 2010-08-31 14:20:00]

My other concern is that this breaks the ssh agent that gets started anywhere
that uses scripts/remote_access.sh:remote_access_init().. will they play nicely
together?

On 2010/08/31 14:17:24, seano wrote:
> Please please do not bind mount everyone's private ~/.ssh dirs* into a build
> chroot where untrusted/third-party/untested code and scripst run (as root)
> 
> *often mounted via NFS :)
> 
> Probably a better idea (from IRC just now) is to specifically copy
~/.ssh/config
> to a tmpfile and bind/copy that into the chroot, as well as SSH_AUTH_SOCK.
> 
> On 2010/08/31 12:52:56, Elly Jones wrote:
> > On 2010/08/31 01:28:38, sosa wrote:
> > > nits
> > > 
> > > http://codereview.chromium.org/3277006/diff/1/2
> > > File enter_chroot.sh (right):
> > > 
> > > http://codereview.chromium.org/3277006/diff/1/2#newcode108
> > > enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> > > I know it's not consistent elsewhere in this code but you should decide
> > whether
> > > to use ${Var} or $Var.  I personally prefer ${Var} when possible.
> > 
> > Done.
> > 
> > > http://codereview.chromium.org/3277006/diff/1/2#newcode108
> > > enter_chroot.sh:108: if [ -z "$(mount | grep -F "on $MOUNTED_PATH ")" ]
> > > Check here also if [ -n "$SSH_AUTH_SOCK" ] and -d "${HOME}/.ssh"
> > 
> > Done.
> > 
> > > http://codereview.chromium.org/3277006/diff/1/2#newcode110
> > > enter_chroot.sh:110: mkdir "${FLAGS_chroot}/home/${USER}/.ssh"
> > > -p?  Also can you re-use ${MOUNTED_PATH}?
> > 
> > Done and done.
> > 
> > > http://codereview.chromium.org/3277006/diff/1/2#newcode111
> > > enter_chroot.sh:111: sudo mount --bind $HOME/.ssh "$MOUNTED_PATH" || \
> > > Quotes around vars like so:  "${HOME}/.ssh"
> > 
> > Done.
> > 
> > > http://codereview.chromium.org/3277006/diff/1/2#newcode112
> > > enter_chroot.sh:112: warn "Could not mount $MOUNTED_PATH"
> > > use die instead of warn.  should work well with additional checks (i.e.
it's
> > > pretty bad if they can't bind mount a directory that exists.
> > 
> > Done.
> > 
> > > http://codereview.chromium.org/3277006/diff/1/2#newcode116
> > > enter_chroot.sh:116: warn "Count not mount $ASOCK"
> > > same ... warn vs. die
> > 
> > Done.

[sosa, 2010-08-31 18:45:02]

LGTM.  You might want to consider hiding this behind a flag ... i.e. --bindssh

[Elly Fong-Jones, 2010-08-31 18:52:47]

On 2010/08/31 18:45:02, sosa wrote:
> LGTM.  You might want to consider hiding this behind a flag ... i.e. --bindssh

Updated a bit: only copy known_hosts (the file we need) in; bind the agent
socket and export its pid and socket name into the chroot so we can still use
it.

[sosa, 2010-08-31 18:57:34]

http://codereview.chromium.org/3277006/diff/2002/15001
File enter_chroot.sh (right):

http://codereview.chromium.org/3277006/diff/2002/15001#newcode113
enter_chroot.sh:113: cp -r "${HOME}/.ssh/known_hosts" "${MOUNTED_PATH}"
You can mount a file (just have to touch it first).  You may want to do that to
keep them in sync.

[Elly Fong-Jones, 2010-08-31 18:59:20]

On 2010/08/31 18:57:34, sosa wrote:
> http://codereview.chromium.org/3277006/diff/2002/15001
> File enter_chroot.sh (right):
> 
> http://codereview.chromium.org/3277006/diff/2002/15001#newcode113
> enter_chroot.sh:113: cp -r "${HOME}/.ssh/known_hosts" "${MOUNTED_PATH}"
> You can mount a file (just have to touch it first).  You may want to do that
to
> keep them in sync.

I don't think keeping them in sync is a win, unless we're gaining new known
hosts from the outside system (?). We don't want the dev environment writing
entries into the host's known_hosts.

[kliegs1, 2010-08-31 19:12:09]

On 2010/08/31 18:59:20, Elly Jones wrote:
> On 2010/08/31 18:57:34, sosa wrote:
> > http://codereview.chromium.org/3277006/diff/2002/15001
> > File enter_chroot.sh (right):
> > 
> > http://codereview.chromium.org/3277006/diff/2002/15001#newcode113
> > enter_chroot.sh:113: cp -r "${HOME}/.ssh/known_hosts" "${MOUNTED_PATH}"
> > You can mount a file (just have to touch it first).  You may want to do that
> to
> > keep them in sync.
> 
> I don't think keeping them in sync is a win, unless we're gaining new known
> hosts from the outside system (?). We don't want the dev environment writing
> entries into the host's known_hosts.

Keeping them split also ensures you don't accidentally clobber a file.  

I also agree with sosa about adding a flag to protect it. Although defaulting
the flag to on makes sense.

[sosa, 2010-08-31 20:06:01]

http://codereview.chromium.org/3277006/diff/2002/15001
File enter_chroot.sh (right):

http://codereview.chromium.org/3277006/diff/2002/15001#newcode107
enter_chroot.sh:107: MOUNTED_PATH="$(readlink -f
"${FLAGS_chroot}/home/${USER}/.ssh")"
If you don't mount then this is inaccurate and the mount | grep is invalid.  Can
you change the var names and check if it exists rather than as a mount?

[rochberg, 2010-09-02 02:07:37]

On 2010/08/30 21:24:01, Elly Jones wrote:
> 

This is nice, but it doesn't yet get me through build_packages with private
repositories.   At least one issue is that build_packages is calling sudo
without -E (so we're losing the ssh agent variables).    I'll talk to you about
this separate CL tomorrow

[Elly Fong-Jones, 2010-09-02 13:21:36]

On 2010/08/31 20:06:01, sosa wrote:
> http://codereview.chromium.org/3277006/diff/2002/15001
> File enter_chroot.sh (right):
> 
> http://codereview.chromium.org/3277006/diff/2002/15001#newcode107
> enter_chroot.sh:107: MOUNTED_PATH="$(readlink -f
> "${FLAGS_chroot}/home/${USER}/.ssh")"
> If you don't mount then this is inaccurate and the mount | grep is invalid. 
Can
> you change the var names and check if it exists rather than as a mount?

Fixed.

[Elly Fong-Jones, 2010-09-02 13:21:55]

On 2010/08/31 19:12:09, kliegs1 wrote:
> On 2010/08/31 18:59:20, Elly Jones wrote:
> > On 2010/08/31 18:57:34, sosa wrote:
> > > http://codereview.chromium.org/3277006/diff/2002/15001
> > > File enter_chroot.sh (right):
> > > 
> > > http://codereview.chromium.org/3277006/diff/2002/15001#newcode113
> > > enter_chroot.sh:113: cp -r "${HOME}/.ssh/known_hosts" "${MOUNTED_PATH}"
> > > You can mount a file (just have to touch it first).  You may want to do
that
> > to
> > > keep them in sync.
> > 
> > I don't think keeping them in sync is a win, unless we're gaining new known
> > hosts from the outside system (?). We don't want the dev environment writing
> > entries into the host's known_hosts.
> 
> Keeping them split also ensures you don't accidentally clobber a file.  
> 
> I also agree with sosa about adding a flag to protect it. Although defaulting
> the flag to on makes sense.

Flag added and defaulted to on.