Code review comment for lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh

On Mon, 2011-10-03 at 08:31 +0000, James Tunnicliffe wrote:
> Hi,
>
> Thanks for the feedback.
>
> On 30 September 2011 20:07, Guilherme Salgado
> <email address hidden> wrote:
> >> The SSH user is inserted into the bzr branch URL. lp: URLs are supported, as are bzr+ssh.
> >
> > Did you consider calling "bzr lp-login <username>" right after running
> > ssh-add instead of changing the branch URL when starting the build?
> > That would be simpler to implement and more flexible as it'd allow any
> > URL schemes, but it'd also allow the build script to fetch other
> > branches from LP using the same user.
>
> That is a very good point. I didn't consider that.
>
> I guess using bzr lp-login may be a bit irritating when testing
> because it would keep logging us out.

But it would (automatically) login again at the start of every build,
no?

>
> I am not familiar with the mechanics of how lp-login works, but it
> seems to use ~/.bazaar/authentication.conf to store a block of
> information:
>
> [Launchpad]
> host = .launchpad.net
> scheme = ssh
> user = dooferlad
>
> I can't see any way of overriding this using an environment variable.
> If several slaves were being run as the same user, sharing a home
> directory, they would all use the user set by the last process to
> start so that would cause us some problems.

But I don't think we'll ever want to share home directories across
slaves because that could allow a rogue job to taint others or have
access to private information. I think it's safe to assume the slaves
don't share anything other than an NFS mount where they put the built
artifacts.

I'm CCing James W. to confirm.

>
> http://doc.bazaar.canonical.com/latest/en/user-reference/configuration-help.html
> http://doc.bazaar.canonical.com/developers/authentication-ring.html
> http://doc.bazaar.canonical.com/latest/en/user-reference/authentication-help.html
>
> A quick skim of these says that all URL formats accept the form:
> bzr branch <scheme>://<user>@host/path
> So we are fine embedding the user in the URL in general and I change
> lp:~ to the long form as a separate step, so the current code is safe.
> I agree that a more simple solution would be nice though. Will let you
> know if I think of anything better.
>
> >> As yet I have not modified the admin interface. You can add a project through the non-admin UI and from there add a user + SSH key. On the admin interface you can't see the key (which has to be not pass phrase protected) or user name. I am planning on adding a sensible way of updating the user and key without leaking the key back out of the interface tomorrow, at which point I think this feature will be complete.
> >> differences between files attachment (review-diff.txt)
> >> === modified file 'lib/offspring/build/bin/offspring-build'
> >> --- lib/offspring/build/bin/offspring-build 2011-09-21 21:06:01 +0000
> >> +++ lib/offspring/build/bin/offspring-build 2011-09-29 17:38:30 +0000
> >> @@ -168,10 +168,39 @@
> >> Stop_logging
> >> Catch_faults disable
> >>
> >> + if [ $RUN_SSH_AGENT = 1 ]; then
> >> + echo kill
> >
> > Was this just for debugging?
>
> Oops, yes. Will delete it!
>
> >> + kill $SSH_AGENT_PID
> >> + fi
> >
> > Given the security implications of failing to kill the ssh agent, I
> > think it's really important that we have a test for this. Do you have
> > any idea how hard it'd be to write one?
>
> I think it is possible, but I am not sure how simple it will be. I
> think it will involve:
>
> 1. Start a new, clean shell (assert that no ssh-agent is running in it)
> 2. Run a job that checks that ssh-agent is running and somehow returns
> information about this to the test runner.
> 3. Check that no ssh-agent is running
>
> If the new shell is created by a python subprocess call, I can monitor
> its stdout, which will be fine for checking that things go according
> to plan.
>
> It is an interesting point about leaving ssh-agent processes running.
> It isn't as long as you trust the users on the machine. We should
> treat our users as hostile though, so the assumption would be that one
> of them could put together a checkout that just tried to connect to
> running ssh-agent processes and log into other users private branches.

That was my concern, but on a second thought, that shouldn't be possible
as Offspring should not run any commands other than what's in the build
script (IIUC). Although it might be possible to, say, craft a hwpack
config that causes linaro-hwpack-create to run something else.

I'm not sure how much we need to worry about that kind of thing, though.

> To stop this we would have to run each build in some form of sandbox
> or as separate users. Perhaps the following would work:
>
> start build wrapper on build machine
> build wrapper starts ssh-agent
> build wrapper does "ssh -A untrusted@localhost <build script>
>
> So, as always, we need to trust root, but that is OK. All commands
> from untrusted users are executed as separate logins to the user
> "untrusted" and all the ssh authentication is performed by forwarding
> the authentication agent back to a trusted user in a way that can't
> easily be hijacked.
>
> BTW, "borrowing" an existing ssh-agent is *really* easy, so we should do this!

Agreed.