Merge into linaro : linaro_offspring_add_bzr

Status:	Rejected
Rejected by:	Guilherme Salgado on 2011-11-09
Proposed branch:	lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh
Merge into:	lp:~linaro-automation/offspring/linaro
Diff against target:	616 lines (+401/-13) 13 files modified lib/offspring/build/bin/offspring-build (+4/-0) lib/offspring/build/functions/exit.sh (+3/-0) lib/offspring/build/functions/ssh.sh (+23/-0) lib/offspring/build/tests/offspring_test_key (+27/-0) lib/offspring/build/tests/offspring_test_key.pub (+1/-0) lib/offspring/build/tests/test.sh (+24/-0) lib/offspring/build/wrappers/test.sh (+52/-0) lib/offspring/master/models.py (+11/-2) lib/offspring/slave/slave.py (+58/-10) lib/offspring/slave/tests/test_private_ssh_keys.py (+110/-0) lib/offspring/web/queuemanager/admin.py (+3/-1) lib/offspring/web/queuemanager/forms.py (+78/-0) lib/offspring/web/queuemanager/models.py (+7/-0)
To merge this branch:	bzr merge lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh
Related bugs:	Link a bug report

Reviewer	Review Type	Date Requested	Status
Guilherme Salgado (community)		2011-10-05	Needs Fixing on 2011-10-13
Review via email: mp+78279@code.launchpad.net

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-09-30: Posted in a previous version of this proposal

#

Hi James,

On Thu, 2011-09-29 at 17:39 +0000, James Tunnicliffe wrote:
> Same as yesterday (nearly), now targeting the correct branch.
>
> I have an SQL migration script ready, but I will submit it on top of salgado's change that introduces the migration directory.
>
> This change adds support for adding an SSH user and private key to an Offspring project on creation. The key is written to a temporary file, the location of which is passed to the builder, which then loads it into a new instance of ssh-agent, which is killed after the build is finished. At this point the key file is deleted.
>
> 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.
>
> 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?

> + 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?

> +
> Print
> Print "OK: %s build %s completed at %s." "${PROJECT}" "${BUILDNAME}" "`date`"
> }
>
> +_AddKeys() {
> + echo AddKeys
> + INDEX=0
> + RUN_SSH_AGENT=0
> + SSH_KEYS[0]=0
> + INSERT_POINT=0
> + for ARG in "$@"
> + do
> + if [ $INDEX -gt 2 ]; then # key files...
> + let RUN_SSH_AGENT=1
> + SSH_KEYS[$INSERT_POINT]="$ARG"
> + INSERT_POINT=$((INSERT_POINT+1))

Would it be possible to simplify this a bit by checking the number of
arguments?

  if [ $# -gt 2 ]; then
    # We were passed an SSH key to use when fetching the config from LP
    SSH_KEY=$3
  fi

And later you could even use SSH_KEY to decide whether or not run run
ssh-agent?

> + fi
> + done
> +
> + if [ $RUN_SSH_AGENT = 1 ]; then
> + eval `ssh-agent`
> + for KEY in "$SSH_KEYS"

The for loop is not needed here as there can only be one SSH key
associated with a project, right?

> + do
> + ssh-add $KEY
> + done
> + fi
> +}
> +
> ##############################
> # Main
> ##############################
> @@ -19...

Hi James,

On Thu, 2011-09-29 at 17:39 +0000, James Tunnicliffe wrote:
> Same as yesterday (nearly), now targeting the correct branch.
> 
> I have an SQL migration script ready, but I will submit it on top of salgado's change that introduces the migration directory.
> 
> This change adds support for adding an SSH user and private key to an Offspring project on creation. The key is written to a temporary file, the location of which is passed to the builder, which then loads it into a new instance of ssh-agent, which is killed after the build is finished. At this point the key file is deleted.
> 
> 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.
> 
> 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?

> +        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?

> +
>      Print
>      Print "OK: %s build %s completed at %s." "${PROJECT}" "${BUILDNAME}" "`date`"
>  }
>  
> +_AddKeys() {
> +    echo AddKeys
> +    INDEX=0
> +    RUN_SSH_AGENT=0
> +    SSH_KEYS[0]=0
> +    INSERT_POINT=0
> +    for ARG in "$@"
> +    do
> +        if [ $INDEX -gt 2 ]; then # key files...
> +            let RUN_SSH_AGENT=1
> +            SSH_KEYS[$INSERT_POINT]="$ARG"
> +            INSERT_POINT=$((INSERT_POINT+1))

Would it be possible to simplify this a bit by checking the number of
arguments?

if [ $# -gt 2 ]; then
    # We were passed an SSH key to use when fetching the config from LP
    SSH_KEY=$3
  fi

And later you could even use SSH_KEY to decide whether or not run run
ssh-agent?

> +        fi
> +    done
> +
> +    if [ $RUN_SSH_AGENT = 1 ]; then
> +        eval `ssh-agent`
> +        for KEY in "$SSH_KEYS"

The for loop is not needed here as there can only be one SSH key
associated with a project, right?

> +        do
> +            ssh-add $KEY
> +        done
> +    fi
> +}
> +
>  ##############################
>  # Main
>  ##############################
> @@ -191,6 +220,8 @@
>      exit 1
>  fi
>  
> +_AddKeys $@
> +
>  # Initialize core & load settings
>  echo "Performing initialization..."
>  _Initialize
> 
> === modified file 'lib/offspring/master/models.py'
> --- lib/offspring/master/models.py	2011-09-23 14:01:26 +0000
> +++ lib/offspring/master/models.py	2011-09-29 17:38:30 +0000
> @@ -108,7 +108,10 @@
>          return (self.previous_state, self.current_state)
>      
>      def build(self, request):
> -        if self.server_proxy.startBuild(request.project.name, request.project.config_url) == LexbuilderSlaveServer.REQUEST_OKAY:
> +        if self.server_proxy.startBuild(request.project.name,
> +                                        request.project.config_url,
> +                                        request.project.lp_user,
> +                                        request.project.lp_ssh_key) == LexbuilderSlaveServer.REQUEST_OKAY:
>              self.previous_state = self.current_state
>              self.current_state = LexbuilderSlave.STATE_BUILDING
>              self.current_job = BuildResult(request, self)
> @@ -165,7 +168,13 @@
>      status = Unicode()
>      config_url = Unicode()
>      notes = Unicode()
> -    
> +
> +    # The Launchpad User and SSH key are stored per project. If we stored them
> +    # per LauncpadProject, anyone who could create a Project referencing that
> +    # LaunchpadProject could get access to the private data in it.
> +    lp_user = Unicode()
> +    lp_ssh_key = Unicode()
> +
>      def __init__(self, name, priority=40):
>          self.title = unicode(name.capitalize())
>          self.name = unicode(name)
> 
> === modified file 'lib/offspring/slave/slave.py'
> --- lib/offspring/slave/slave.py	2011-09-23 14:01:26 +0000
> +++ lib/offspring/slave/slave.py	2011-09-29 17:38:30 +0000
> @@ -1,6 +1,7 @@
>  # Copyright 2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
> +
>  __all__ = (
>      'LexbuilderSlave',
>      'LexbuilderSlaveServer',
> @@ -12,6 +13,8 @@
>  import platform
>  import time
>  import xmlrpclib
> +import re
> +import tempfile
>  
>  from offspring import config, offspring_root
>  from offspring.daemon import (
> @@ -57,9 +60,30 @@
>          self.server.stop()
>          self.server.join()
>  
> -    def startBuild(self, projectName, configBranchURL):
> +    def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):
> +        # check for lp: format URLs. Convert these to bzr+ssh ones
> +        configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
> +                                 configBranchURL)
> +
> +        # The assumption is that this function will only be called with a
> +        # bzr+ssh URL (hence the name!)
> +        assert(re.search("bzr\+ssh://", configBranchURL))
> +
> +        # Add user to URL
> +        configBranchURL = re.sub("bzr\+ssh://",
> +                                 "bzr+ssh://" + lpUser + "@", configBranchURL)
> +
> +        return configBranchURL
> +
> +    def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
> +
> +        if lpUser:
> +            # We don't want to pass the user separately, we want it to be
> +            # integrated into the configBranchURL.
> +            configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
> +
>          self.currentBuild = ProjectBuildProcess(
> -            self, projectName, configBranchURL)
> +            self, projectName, configBranchURL, lpSSHKey)
>          logging.info("Starting build: %s %s %s" % (
>              self.currentBuild.getCommandPath(), projectName, configBranchURL))
>          self.currentBuild.start()
> @@ -70,9 +94,12 @@
>              if self.currentBuild.process is not None:
>                  if self.currentBuild.process.poll() is None:
>                      self.currentBuild.process.terminate()
> -            # Give it 5 seconds before sending kill.
> -            time.sleep(int(config.slave("child_wait_timeout")))
> -            self.currentBuild.process.kill()
> +                    if self.currentBuild.process.poll() is None:
> +                        # Give it 5 seconds before sending kill.
> +                        time.sleep(int(config.slave("child_wait_timeout")))
> +                        if self.currentBuild.process.poll() is None:
> +                            self.currentBuild.process.kill()
> +            self.currentBuild.tidyUp()
>  
> 
>  class LexbuilderSlaveServer(LexbuilderXMLRPCServer):
> @@ -90,11 +117,11 @@
>          LexbuilderXMLRPCServer.__init__(
>              self, addr, requestHandler, logRequests, allow_none, encoding, request_timeout)
>  
> -    def api_startBuild(self, projectName, configBranchURL):
> +    def api_startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
>          if self.slave.currentBuild is not None:
>              if self.slave.currentBuild.state == ProjectBuildProcess.STATE_ACTIVE:
>                  return LexbuilderSlaveServer.REQUEST_ERROR
> -        self.slave.startBuild(projectName, configBranchURL)
> +        self.slave.startBuild(projectName, configBranchURL, lpUser, lpSSHKey)
>          return LexbuilderSlaveServer.REQUEST_OKAY
>  
>      def api_stopBuild(self):
> @@ -159,14 +186,32 @@
>  
>  class ProjectBuildProcess(MonitoredProcess):
>  
> -    def __init__(self, supervisor, projectName, configBranchURL):
> -        MonitoredProcess.__init__(
> -            self, supervisor, [projectName, configBranchURL],
> +    def __init__(self, supervisor, projectName, configBranchURL, lpSSHKey=None):
> +        self.args = [projectName, configBranchURL]
> +        self.ssh_key_file = None
> +        
> +        if lpSSHKey:
> +            # Write SSH key to disk. tempfile will delete it after the build
> +            # when this class goes out of scope.
> +            self.ssh_key_file = tempfile.NamedTemporaryFile()
> +            self.args.append(self.ssh_key_file.name)
> +            self.ssh_key_file.write(lpSSHKey)
> +            self.ssh_key_file.flush()

I wonder if it wouldn't make more sense to run this in preExecution()
(or maybe setup(), even).

> +
> +        MonitoredProcess.__init__(self, supervisor, self.args,
>              environment_updates={
>                  'CONFIG_SCRIPT': os.path.join(
>                      offspring_root, 'bin', 'offspring-builder-config')})
>          self.projectName = projectName
>  
> +    def __del__(self):
> +        self.tidyUp()
> +
> +    def tidyUp(self):
> +        if self.ssh_key_file:
> +            self.ssh_key_file.close()
> +            self.ssh_key_file = None
> +
>      def setup(self):
>          self.buildName = None
>  
> @@ -189,6 +234,7 @@
>          logging.debug(
>              "Setting state of slave to %s" % LexbuilderSlave.STATE_IDLE)
>          self.master.state = LexbuilderSlave.STATE_IDLE
> +        self.tidyUp()
>  
>      def getCommandPath(self):
>          return config.slave("build_command")
> 
> === added file 'lib/offspring/slave/test_private_ssh_keys.py'
> --- lib/offspring/slave/test_private_ssh_keys.py	1970-01-01 00:00:00 +0000
> +++ lib/offspring/slave/test_private_ssh_keys.py	2011-09-29 17:38:30 +0000

This should be under a tests/ directory; do you mind creating it and
moving it there?

> @@ -0,0 +1,54 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +import os
> +
> +import offspring.slave
> +import unittest
> +import xmlrpclib
> +import re
> +import os.path
> +
> +class TestLexbuilderSlavePrivateSSHKeys(unittest.TestCase):
> +
> +    def setUp(self):
> +        self.slave = offspring.slave.LexbuilderSlave(('localhost', 8760))
> +        self.slave.start()
> +        self.x = xmlrpclib.ServerProxy("http://admin:password@localhost:8760")
> +
> +    def tearDown(self):
> +        self.slave.stop()
> +
> +    def test_startBuildWithCustomSSHKey(self):
> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]
> +        expected_args = [args[0],
> +                         re.sub("lp:", "bzr+ssh://" + args[2] + "@bazaar.launchpad.net/", args[1]),
> +                         args[3]]
> +
> +        self.slave.startBuild(args[0], args[1], args[2], args[3])
> +        passed_args = self.slave.currentBuild.args
> +        
> +        self.assertEqual(expected_args, passed_args)
> +
> +    def test_startBuildWithCustomSSHKey(self):

This test is overwriting the previous one.

> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]

Will this cause startBuild() to actually fetch your config branch from
LP?  We must not do that because then we need to be able to connect to
LP if we want to run the tests.

> +        expected_args = [args[0],
> +                         re.sub("lp:", "bzr+ssh://" + args[2] + "@bazaar.launchpad.net/", args[1]),
> +                         args[3]]
> +
> +        self.slave.startBuild(args[0], args[1], args[2], args[3])
> +
> +        file_path = self.slave.currentBuild.ssh_key_file.name
> +        read_key = open(file_path).read()
> +
> +        # We should have a key written to a file for the slave to read...
> +        self.assertEqual(read_key, args[3])
> +
> +        self.slave.stop()

We don't really need to stop the slave to have the key file removed,
right?

I think it'd be nicer if we could come up with a job whose config
doesn't need to be fetched from LP and that runs instantaneously, so
that we can just wait for it to finish and show that after it finishes
the key file is removed.

> +        # Having stopped the slave, the key should have been erased
> +        self.assertFalse(os.path.exists(file_path))
> +
> +def test_suite():
> +    loader = unittest.TestLoader()
> +    return loader.loadTestsFromName(__name__)

I'm pretty sure you don't need this as we're using nose as the test
runner.

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-03: Posted in a previous version of this proposal

#

Download full text (9.5 KiB)

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.

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.

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...

Hi,

Thanks for the feedback.

On 30 September 2011 20:07, Guilherme Salgado
<guilherme.salgado@linaro.org> 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.

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.

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.
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!

>> +
>>      Print
>>      Print "OK: %s build %s completed at %s." "${PROJECT}" "${BUILDNAME}" "`date`"
>>  }
>>
>> +_AddKeys() {
>> +    echo AddKeys
>> +    INDEX=0
>> +    RUN_SSH_AGENT=0
>> +    SSH_KEYS[0]=0
>> +    INSERT_POINT=0
>> +    for ARG in "$@"
>> +    do
>> +        if [ $INDEX -gt 2 ]; then # key files...
>> +            let RUN_SSH_AGENT=1
>> +            SSH_KEYS[$INSERT_POINT]="$ARG"
>> +            INSERT_POINT=$((INSERT_POINT+1))
>
> Would it be possible to simplify this a bit by checking the number of
> arguments?
>
>  if [ $# -gt 2 ]; then
>    # We were passed an SSH key to use when fetching the config from LP
>    SSH_KEY=$3
>  fi
>
> And later you could even use SSH_KEY to decide whether or not run run
> ssh-agent?
>
>> +        fi
>> +    done
>> +
>> +    if [ $RUN_SSH_AGENT = 1 ]; then
>> +        eval `ssh-agent`
>> +        for KEY in "$SSH_KEYS"
>
> The for loop is not needed here as there can only be one SSH key
> associated with a project, right?

Yep, I can simplify all of the above by assuming that if there is a
third argument, it is a location of an SSH key file.

<snip>
>>  class ProjectBuildProcess(MonitoredProcess):
>>
>> -    def __init__(self, supervisor, projectName, configBranchURL):
>> -        MonitoredProcess.__init__(
>> -            self, supervisor, [projectName, configBranchURL],
>> +    def __init__(self, supervisor, projectName, configBranchURL, lpSSHKey=None):
>> +        self.args = [projectName, configBranchURL]
>> +        self.ssh_key_file = None
>> +
>> +        if lpSSHKey:
>> +            # Write SSH key to disk. tempfile will delete it after the build
>> +            # when this class goes out of scope.
>> +            self.ssh_key_file = tempfile.NamedTemporaryFile()
>> +            self.args.append(self.ssh_key_file.name)
>> +            self.ssh_key_file.write(lpSSHKey)
>> +            self.ssh_key_file.flush()
>
> I wonder if it wouldn't make more sense to run this in preExecution()
> (or maybe setup(), even).

Probably. Good point.

>> === added file 'lib/offspring/slave/test_private_ssh_keys.py'
>> --- lib/offspring/slave/test_private_ssh_keys.py      1970-01-01 00:00:00 +0000
>> +++ lib/offspring/slave/test_private_ssh_keys.py      2011-09-29 17:38:30 +0000
>
> This should be under a tests/ directory; do you mind creating it and
> moving it there?

No problem. There is another test file in the slave directory, so I
just went with the existing structure, but as soon as there is a
collection a directory definitely makes sense.

>> @@ -0,0 +1,54 @@
>> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
>> +# GNU Affero General Public License version 3 (see the file LICENSE).
>> +import os
>> +
>> +import offspring.slave
>> +import unittest
>> +import xmlrpclib
>> +import re
>> +import os.path
>> +
>> +class TestLexbuilderSlavePrivateSSHKeys(unittest.TestCase):
>> +
>> +    def setUp(self):
>> +        self.slave = offspring.slave.LexbuilderSlave(('localhost', 8760))
>> +        self.slave.start()
>> +        self.x = xmlrpclib.ServerProxy("http://admin:password@localhost:8760")
>> +
>> +    def tearDown(self):
>> +        self.slave.stop()
>> +
>> +    def test_startBuildWithCustomSSHKey(self):
>> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]
>> +        expected_args = [args[0],
>> +                         re.sub("lp:", "bzr+ssh://" + args[2] + "@bazaar.launchpad.net/", args[1]),
>> +                         args[3]]
>> +
>> +        self.slave.startBuild(args[0], args[1], args[2], args[3])
>> +        passed_args = self.slave.currentBuild.args
>> +
>> +        self.assertEqual(expected_args, passed_args)
>> +
>> +    def test_startBuildWithCustomSSHKey(self):
>
> This test is overwriting the previous one.
>
>> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]
>
> Will this cause startBuild() to actually fetch your config branch from
> LP?  We must not do that because then we need to be able to connect to
> LP if we want to run the tests.

No, the config branch in the test doesn't exist, but it could do with
a comment to say so! I should also test it without an internet
connection to check to see if I have missed something, like it waiting
on a time-out or something.

>> +        expected_args = [args[0],
>> +                         re.sub("lp:", "bzr+ssh://" + args[2] + "@bazaar.launchpad.net/", args[1]),
>> +                         args[3]]
>> +
>> +        self.slave.startBuild(args[0], args[1], args[2], args[3])
>> +
>> +        file_path = self.slave.currentBuild.ssh_key_file.name
>> +        read_key = open(file_path).read()
>> +
>> +        # We should have a key written to a file for the slave to read...
>> +        self.assertEqual(read_key, args[3])
>> +
>> +        self.slave.stop()
>
> We don't really need to stop the slave to have the key file removed,
> right?

Actually no. That should be a call to stopBuild().

>> +        # Having stopped the slave, the key should have been erased
>> +        self.assertFalse(os.path.exists(file_path))
>> +
>> +def test_suite():
>> +    loader = unittest.TestLoader()
>> +    return loader.loadTestsFromName(__name__)
>
> I'm pretty sure you don't need this as we're using nose as the test
> runner.

Ah, OK. I just copied slave/tests.py. Will fix both up.

-- 
James Tunnicliffe

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-03: Posted in a previous version of this proposal

#

Download full text (5.2 KiB)

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!
> ...

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
> <guilherme.salgado@linaro.org> 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.

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-03: Posted in a previous version of this proposal

#

Download full text (5.8 KiB)

On 3 October 2011 15:40, Guilherme Salgado <email address hidden> wrote:
> 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 was mostly thinking of testing on my machine here (not wanting my
user to be logged out of lp), but it would be simple enough to set up
another user to run the slave.

>>
>> 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.

Sounds good to me.

> 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-buil...

On 3 October 2011 15:40, Guilherme Salgado <guilherme.salgado@linaro.org> wrote:
> 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
>> <guilherme.salgado@linaro.org> 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 was mostly thinking of testing on my machine here (not wanting my
user to be logged out of lp), but it would be simple enough to set up
another user to run the slave.

>>
>> 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.

Sounds good to me.

> 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.
>
>
> --
> https://code.launchpad.net/~dooferlad/offspring/linaro_offspring_add_bzr_ssh/+merge/77575
> You are the owner of lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh.
>

-- 
James Tunnicliffe

Revision history for this message

James Westby (james-w) wrote on 2011-10-03: Posted in a previous version of this proposal

#

On Mon, 03 Oct 2011 14:40:28 -0000, Guilherme Salgado <email address hidden> wrote:
> 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.

That I'm not 100% sure on. Probably worth asking Cody or IS.

Multiple builds will be serialised to the same slave, so there is
certainly sharing of homedirs there, but reliably cleaning up after a
build would avoid problems with that, if it can indeed be done reliably.

Is even having access to the build artefacts going to be an issue for
member services?

Also, while I remember, be sure to take note of the added dependencies
when requesting a rollout of this code, as if e.g. ssh-agent is missing
on a machine where it needs to be it may break existing builds.

> 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.

If this is used for building images with live-build, then that is
trivially possible.

Thanks,

James

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-04: Posted in a previous version of this proposal

#

On 3 October 2011 18:25, James Westby <email address hidden> wrote:
> On Mon, 03 Oct 2011 14:40:28 -0000, Guilherme Salgado <email address hidden> wrote:
>> 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.
>
> That I'm not 100% sure on. Probably worth asking Cody or IS.
>
> Multiple builds will be serialised to the same slave, so there is
> certainly sharing of homedirs there, but reliably cleaning up after a
> build would avoid problems with that, if it can indeed be done reliably.

I take it each offspring-slave instance runs as its own user, thus
having its own home directory then? If this is the case, then it
should solve the ssh-agent vulnerability.

> Is even having access to the build artefacts going to be an issue for
> member services?

I imagine that if the final output of the build is private, it is safe
to assume that anything to do with it should be private.

> Also, while I remember, be sure to take note of the added dependencies
> when requesting a rollout of this code, as if e.g. ssh-agent is missing
> on a machine where it needs to be it may break existing builds.

ssh-agent is installed with openssh-client, so it should be installed
already. I have put a WI down to make sure it gets into the docs
though.

>> 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.
>
> If this is used for building images with live-build, then that is
> trivially possible.

...so we are going to have to be careful to clean up after ourselves.
It doesn't sound trivial since the build process can write to ~, /tmp
and possibly some other places. Deleting all files owned by the user
from a list of locations would be one way of going about it, but I
think I would set up a VM to test it in!

--
James Tunnicliffe

On 3 October 2011 18:25, James Westby <jw+debian@jameswestby.net> wrote:
> On Mon, 03 Oct 2011 14:40:28 -0000, Guilherme Salgado <guilherme.salgado@linaro.org> wrote:
>> 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.
>
> That I'm not 100% sure on. Probably worth asking Cody or IS.
>
> Multiple builds will be serialised to the same slave, so there is
> certainly sharing of homedirs there, but reliably cleaning up after a
> build would avoid problems with that, if it can indeed be done reliably.

I take it each offspring-slave instance runs as its own user, thus
having its own home directory then? If this is the case, then it
should solve the ssh-agent vulnerability.

> Is even having access to the build artefacts going to be an issue for
> member services?

I imagine that if the final output of the build is private, it is safe
to assume that anything to do with it should be private.

> Also, while I remember, be sure to take note of the added dependencies
> when requesting a rollout of this code, as if e.g. ssh-agent is missing
> on a machine where it needs to be it may break existing builds.

ssh-agent is installed with openssh-client, so it should be installed
already. I have put a WI down to make sure it gets into the docs
though.

>> 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.
>
> If this is used for building images with live-build, then that is
> trivially possible.

...so we are going to have to be careful to clean up after ourselves.
It doesn't sound trivial since the build process can write to ~, /tmp
and possibly some other places. Deleting all files owned by the user
from a list of locations would be one way of going about it, but I
think I would set up a VM to test it in!

-- 
James Tunnicliffe

Revision history for this message

James Westby (james-w) wrote on 2011-10-04: Posted in a previous version of this proposal

#

On Tue, 4 Oct 2011 13:59:45 +0100, James Tunnicliffe <email address hidden> wrote:
> I take it each offspring-slave instance runs as its own user, thus
> having its own home directory then? If this is the case, then it
> should solve the ssh-agent vulnerability.

I imagine they run as the same username on different machines, so if
they are not sharing /home on NFS or similar it should be ok, but I
don't know if they are.

> > Is even having access to the build artefacts going to be an issue for
> > member services?
>
> I imagine that if the final output of the build is private, it is safe
> to assume that anything to do with it should be private.

Yeah, I guess it's a question of whether we can trust the people who
have access to the config to not try and take over a build and get at
artefacts from a different project.

> > Also, while I remember, be sure to take note of the added dependencies
> > when requesting a rollout of this code, as if e.g. ssh-agent is missing
> > on a machine where it needs to be it may break existing builds.
>
> ssh-agent is installed with openssh-client, so it should be installed
> already. I have put a WI down to make sure it gets into the docs
> though.

Thanks. There's also django-group-access, which likely isn't installed.

> >> 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.
> >
> > If this is used for building images with live-build, then that is
> > trivially possible.
>
> ...so we are going to have to be careful to clean up after ourselves.
> It doesn't sound trivial since the build process can write to ~, /tmp
> and possibly some other places. Deleting all files owned by the user
> from a list of locations would be one way of going about it, but I
> think I would set up a VM to test it in!

A scheme that didn't rely on such a cleanup would be good, but may not
be feasible.

Thanks,

James

On Tue, 4 Oct 2011 13:59:45 +0100, James Tunnicliffe <james.tunnicliffe@linaro.org> wrote:
> I take it each offspring-slave instance runs as its own user, thus
> having its own home directory then? If this is the case, then it
> should solve the ssh-agent vulnerability.

I imagine they run as the same username on different machines, so if
they are not sharing /home on NFS or similar it should be ok, but I
don't know if they are.

> > Is even having access to the build artefacts going to be an issue for
> > member services?
> 
> I imagine that if the final output of the build is private, it is safe
> to assume that anything to do with it should be private.

Yeah, I guess it's a question of whether we can trust the people who
have access to the config to not try and take over a build and get at
artefacts from a different project.

> > Also, while I remember, be sure to take note of the added dependencies
> > when requesting a rollout of this code, as if e.g. ssh-agent is missing
> > on a machine where it needs to be it may break existing builds.
> 
> ssh-agent is installed with openssh-client, so it should be installed
> already. I have put a WI down to make sure it gets into the docs
> though.

Thanks. There's also django-group-access, which likely isn't installed.

> >> 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.
> >
> > If this is used for building images with live-build, then that is
> > trivially possible.
> 
> ...so we are going to have to be careful to clean up after ourselves.
> It doesn't sound trivial since the build process can write to ~, /tmp
> and possibly some other places. Deleting all files owned by the user
> from a list of locations would be one way of going about it, but I
> think I would set up a VM to test it in!

A scheme that didn't rely on such a cleanup would be good, but may not
be feasible.

Thanks,

James

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-06:

#

Download full text (22.5 KiB)

On Wed, 2011-10-05 at 16:25 +0000, James Tunnicliffe wrote:
> James Tunnicliffe has proposed merging lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh into lp:~linaro-infrastructure/offspring/linaro.
>
> Requested reviews:
> Linaro Infrastructure (linaro-infrastructure)
>
> For more details, see:
> https://code.launchpad.net/~dooferlad/offspring/linaro_offspring_add_bzr_ssh/+merge/78279
>
> This merge address the comments raised about the initial one. It also
> updates the web UI so the admin interface allows the user to change
> the ssh user and key, but can't get access to the stored key.

I was expecting the ssh user/key to be editable on the project's edit
page (the project_edit() view), but I guess that wouldn't work because
people with the change_project permission would then be able to change
that for all projects, right?

Having to use the admin UI to change a project's ssh user/key means the
people in charge of every project will have to send the ssh key to
somebody with admin rights to set. I think we should check with Vicky
that this is ok.

> I have an SQL migration script ready, but I will submit it on top of
> salgado's change that introduces the migration directory.
>
> This change adds support for adding an SSH user and private key to an
> Offspring project on creation. The key is written to a temporary file,
> the location of which is passed to the builder, which then loads it
> into a new instance of ssh-agent, which is killed after the build is
> finished. At this point the key file is deleted.
>
> The SSH user is inserted into the bzr branch URL. lp: URLs are
> supported, as are bzr+ssh.

> === 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-10-05 16:24:23 +0000
> @@ -35,6 +35,7 @@
> source "${IBS_FUNCDIR}/reports.sh"
> source "${IBS_FUNCDIR}/logging.sh"
> source "${IBS_FUNCDIR}/files.sh"
> + source "${IBS_FUNCDIR}/ssh.sh"
>
> [ -n "${IBS_PROXY}" ] && export http_proxy="${IBS_PROXY}"
>
> @@ -195,6 +196,9 @@
> echo "Performing initialization..."
> _Initialize
>
> +# If any SSH keys have been passed to us, load them into ssh-agent
> +AddKeys ${3}
> +
> # Setup and start logging
> _Setup
>
>
> === modified file 'lib/offspring/build/functions/exit.sh'
> --- lib/offspring/build/functions/exit.sh 2010-11-29 08:27:24 +0000
> +++ lib/offspring/build/functions/exit.sh 2011-10-05 16:24:23 +0000
> @@ -61,6 +61,9 @@
> Action_done
> fi
>
> + # Always stop SSH agent...
> + StopSSHAgent
> +
> Action "Remvoving build statefile"
> rm ${IBS_BUILDER_STATEFILE}
> Action_done
>
> === added file 'lib/offspring/build/functions/ssh.sh'
> --- lib/offspring/build/functions/ssh.sh 1970-01-01 00:00:00 +0000
> +++ lib/offspring/build/functions/ssh.sh 2011-10-05 16:24:23 +0000
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +
> +# Copyright 2010 Canonical Ltd. This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +AddKeys() {
> + RUN_SSH_AGENT=1
> +
> + if [ -z "${1}" ]; then
> + ...

On Wed, 2011-10-05 at 16:25 +0000, James Tunnicliffe wrote:
> James Tunnicliffe has proposed merging lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh into lp:~linaro-infrastructure/offspring/linaro.
> 
> Requested reviews:
>   Linaro Infrastructure (linaro-infrastructure)
> 
> For more details, see:
> https://code.launchpad.net/~dooferlad/offspring/linaro_offspring_add_bzr_ssh/+merge/78279
> 
> This merge address the comments raised about the initial one. It also
> updates the web UI so the admin interface allows the user to change
> the ssh user and key, but can't get access to the stored key.

I was expecting the ssh user/key to be editable on the project's edit
page (the project_edit() view), but I guess that wouldn't work because
people with the change_project permission would then be able to change
that for all projects, right?

Having to use the admin UI to change a project's ssh user/key means the
people in charge of every project will have to send the ssh key to
somebody with admin rights to set.  I think we should check with Vicky
that this is ok.

> I have an SQL migration script ready, but I will submit it on top of
> salgado's change that introduces the migration directory.
> 
> This change adds support for adding an SSH user and private key to an
> Offspring project on creation. The key is written to a temporary file,
> the location of which is passed to the builder, which then loads it
> into a new instance of ssh-agent, which is killed after the build is
> finished. At this point the key file is deleted.
> 
> The SSH user is inserted into the bzr branch URL. lp: URLs are
> supported, as are bzr+ssh.

> === 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-10-05 16:24:23 +0000
> @@ -35,6 +35,7 @@
>      source "${IBS_FUNCDIR}/reports.sh"
>      source "${IBS_FUNCDIR}/logging.sh"
>      source "${IBS_FUNCDIR}/files.sh"
> +    source "${IBS_FUNCDIR}/ssh.sh"
>  
>      [ -n "${IBS_PROXY}" ] && export http_proxy="${IBS_PROXY}"
>  
> @@ -195,6 +196,9 @@
>  echo "Performing initialization..."
>  _Initialize
>  
> +# If any SSH keys have been passed to us, load them into ssh-agent
> +AddKeys ${3}
> +
>  # Setup and start logging
>  _Setup
>  
> 
> === modified file 'lib/offspring/build/functions/exit.sh'
> --- lib/offspring/build/functions/exit.sh	2010-11-29 08:27:24 +0000
> +++ lib/offspring/build/functions/exit.sh	2011-10-05 16:24:23 +0000
> @@ -61,6 +61,9 @@
>          Action_done
>      fi
>  
> +    # Always stop SSH agent...
> +    StopSSHAgent
> +
>      Action "Remvoving build statefile"
>      rm ${IBS_BUILDER_STATEFILE}
>      Action_done
> 
> === added file 'lib/offspring/build/functions/ssh.sh'
> --- lib/offspring/build/functions/ssh.sh	1970-01-01 00:00:00 +0000
> +++ lib/offspring/build/functions/ssh.sh	2011-10-05 16:24:23 +0000
> @@ -0,0 +1,23 @@
> +#!/bin/bash
> +
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +AddKeys() {
> +    RUN_SSH_AGENT=1
> +
> +    if [ -z "${1}" ]; then
> +        let RUN_SSH_AGENT=-1
> +    fi
> +
> +    if [ $RUN_SSH_AGENT = 1 ]; then
> +        eval `ssh-agent`
> +        ssh-add $1
> +    fi
> +}
> +
> +StopSSHAgent() {
> +    if [ $RUN_SSH_AGENT = 1 ]; then
> +        kill $SSH_AGENT_PID
> +    fi
> +}
> \ No newline at end of file
>

> === added file 'lib/offspring/build/tests/test.sh'
> --- lib/offspring/build/tests/test.sh	1970-01-01 00:00:00 +0000
> +++ lib/offspring/build/tests/test.sh	2011-10-05 16:24:23 +0000
> @@ -0,0 +1,24 @@
> +#!/bin/bash
> +
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +# Get the full directory path of the test script
> +DIR="$( cd "$( dirname "$0" )" && pwd )"
> +
> +# Load functions under test
> +source "${DIR}/../functions/ssh.sh"
> +
> +# Load test key
> +AddKeys "${DIR}/offspring_test_key"
> +
> +# See what keys are loaded (expect lib/offspring/build/tests/offspring_test_key)
> +echo After loading test key, keys are:
> +ssh-add -l
> +
> +# Stop the loaded ssh-agent process
> +StopSSHAgent
> +
> +# Check that no keys are listed: Expect "Could not open a connection to your authentication agent."
> +echo "After killing ssh agent, we shouldn't be able to find any keys..."
> +ssh-add -l
> 
> === added file 'lib/offspring/build/wrappers/test.sh'
> --- lib/offspring/build/wrappers/test.sh	1970-01-01 00:00:00 +0000
> +++ lib/offspring/build/wrappers/test.sh	2011-10-05 16:24:23 +0000
> @@ -0,0 +1,52 @@
> +#!/bin/bash
> +
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +Buildtool_setup() {
> +    Catch_faults permit_errors
> +
> +    TARGETFS="${IBS_WORKDIR}/${PROJECT}/chroot"
> +
> +    if [ -d "${IBS_WORKDIR}/${PROJECT}/config" ]; then
> +        rm -rf "${IBS_WORKDIR}/${PROJECT}/config" > /dev/null 2>&1 || true
> +    fi
> +
> +    Action "Copying project configuration directory into work directory for build"
> +    Export_project_config "${PROJECT}" "${IBS_WORKDIR}/${PROJECT}/config"
> +    Action_done
> +
> +    if [ "$?" != "0" ]; then
> +        Error "Unable to copy project build configuration to %s" "${IBS_WORKDIR}/${PROJECT}/config"
> +        Die
> +    fi
> +    Catch_faults disallow_errors
> +}
> +
> +Buildtool_build() {
> +    Action "Test Build - Buildtool_build"
> +    Action_done
> +}
> +
> +Buildtool_cleanup() {
> +    Action "Test Build - Buildtool_cleanup"
> +    Action_done
> +}
> +
> +_Check_directory() {
> +    if [ "${PWD}" != "${IBS_WORKDIR}/${PROJECT}" ]; then
> +        Error "Working directory is '%s'; expected '%s'" "{$PWD}" "${IBS_WORKDIR}/${PROJECT}"
> +        Die
> +    fi
> +    Debug "Directory check performed and was successful; PWD: %s" "${PWD}"
> +}
> +
> +_SaveConfig() {
> +    Action "Test Build - _SaveConfig"
> +    Action_done
> +}
> +
> +_SaveBuildWorkDirectory() {
> +    Action "Test Build - _SaveBuildWorkDirectory"
> +    Action_done
> +}
>

> === modified file 'lib/offspring/slave/slave.py'
> --- lib/offspring/slave/slave.py	2011-09-23 14:01:26 +0000
> +++ lib/offspring/slave/slave.py	2011-10-05 16:24:23 +0000
> @@ -1,6 +1,7 @@
>  # Copyright 2010 Canonical Ltd.  This software is licensed under the
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
> +
>  __all__ = (
>      'LexbuilderSlave',
>      'LexbuilderSlaveServer',
> @@ -12,6 +13,8 @@
>  import platform
>  import time
>  import xmlrpclib
> +import re
> +import tempfile
>  
>  from offspring import config, offspring_root
>  from offspring.daemon import (
> @@ -57,9 +60,30 @@
>          self.server.stop()
>          self.server.join()
>  
> -    def startBuild(self, projectName, configBranchURL):
> +    def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):
> +        # check for lp: format URLs. Convert these to bzr+ssh ones
> +        configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
> +                                 configBranchURL)

I thought you were going to use 'bzr lp-login' as it's not a problem if
we fail to cleanup ~/.bazaar/authentication.conf as long as we make sure
to kill the ssh agent.  And if we fail to kill the ssh agent then using
lp-login is not much worse than it is now.

> +
> +        # The assumption is that this function will only be called with a
> +        # bzr+ssh URL (hence the name!)
> +        assert(re.search("bzr\+ssh://", configBranchURL))
> +
> +        # Add user to URL
> +        configBranchURL = re.sub("bzr\+ssh://",
> +                                 "bzr+ssh://" + lpUser + "@", configBranchURL)
> +
> +        return configBranchURL
> +
> +    def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
> +
> +        if lpUser:
> +            # We don't want to pass the user separately, we want it to be
> +            # integrated into the configBranchURL.
> +            configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
> +
>          self.currentBuild = ProjectBuildProcess(
> -            self, projectName, configBranchURL)
> +            self, projectName, configBranchURL, lpSSHKey)
>          logging.info("Starting build: %s %s %s" % (
>              self.currentBuild.getCommandPath(), projectName, configBranchURL))
>          self.currentBuild.start()
> @@ -70,9 +94,12 @@
>              if self.currentBuild.process is not None:
>                  if self.currentBuild.process.poll() is None:
>                      self.currentBuild.process.terminate()
> -            # Give it 5 seconds before sending kill.
> -            time.sleep(int(config.slave("child_wait_timeout")))
> -            self.currentBuild.process.kill()
> +                    if self.currentBuild.process.poll() is None:
> +                        # Give it 5 seconds before sending kill.
> +                        time.sleep(int(config.slave("child_wait_timeout")))
> +                        if self.currentBuild.process.poll() is None:
> +                            self.currentBuild.process.kill()

Why is this needed?

> +            self.currentBuild.tidyUp()
>  
> 
>  class LexbuilderSlaveServer(LexbuilderXMLRPCServer):
> @@ -90,11 +117,11 @@
>          LexbuilderXMLRPCServer.__init__(
>              self, addr, requestHandler, logRequests, allow_none, encoding, request_timeout)
>  
> -    def api_startBuild(self, projectName, configBranchURL):
> +    def api_startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
>          if self.slave.currentBuild is not None:
>              if self.slave.currentBuild.state == ProjectBuildProcess.STATE_ACTIVE:
>                  return LexbuilderSlaveServer.REQUEST_ERROR
> -        self.slave.startBuild(projectName, configBranchURL)
> +        self.slave.startBuild(projectName, configBranchURL, lpUser, lpSSHKey)
>          return LexbuilderSlaveServer.REQUEST_OKAY
>  
>      def api_stopBuild(self):
> @@ -159,14 +186,34 @@
>  
>  class ProjectBuildProcess(MonitoredProcess):
>  
> -    def __init__(self, supervisor, projectName, configBranchURL):
> -        MonitoredProcess.__init__(
> -            self, supervisor, [projectName, configBranchURL],
> +    def __init__(self, supervisor, projectName, configBranchURL, lpSSHKey=None):
> +        self.args = [projectName, configBranchURL]
> +        self.ssh_key_file = None
> +
> +        if lpSSHKey:
> +            # Write SSH key to disk. tempfile will delete it after the build
> +            # when this class goes out of scope.
> +            # Need to have this here so we can pass the temp file name
> +            # containing the key to the builder process on its command line.
> +            self.ssh_key_file = tempfile.NamedTemporaryFile()
> +            self.args.append(self.ssh_key_file.name)
> +            self.ssh_key_file.write(lpSSHKey)
> +            self.ssh_key_file.flush()
> +
> +        MonitoredProcess.__init__(self, supervisor, self.args,
>              environment_updates={
>                  'CONFIG_SCRIPT': os.path.join(
>                      offspring_root, 'bin', 'offspring-builder-config')})
>          self.projectName = projectName
>  
> +    def __del__(self):
> +        self.tidyUp()
> +
> +    def tidyUp(self):
> +        if self.ssh_key_file:
> +            self.ssh_key_file.close()
> +            self.ssh_key_file = None
> +
>      def setup(self):
>          self.buildName = None
>  
> @@ -189,6 +236,7 @@
>          logging.debug(
>              "Setting state of slave to %s" % LexbuilderSlave.STATE_IDLE)
>          self.master.state = LexbuilderSlave.STATE_IDLE
> +        self.tidyUp()
>  
>      def getCommandPath(self):
>          return config.slave("build_command")
> 
> === added directory 'lib/offspring/slave/tests'
> === added file 'lib/offspring/slave/tests/test_private_ssh_keys.py'
> --- lib/offspring/slave/tests/test_private_ssh_keys.py	1970-01-01 00:00:00 +0000
> +++ lib/offspring/slave/tests/test_private_ssh_keys.py	2011-10-05 16:24:23 +0000
> @@ -0,0 +1,88 @@
> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> +# GNU Affero General Public License version 3 (see the file LICENSE).
> +
> +import os
> +import subprocess
> +import offspring.slave
> +import unittest
> +import xmlrpclib
> +import re
> +import os.path
> +
> +class TestLexbuilderSlavePrivateSSHKeys(unittest.TestCase):
> +
> +    def nothing(self):
> +        pass
> +
> +    def setUp(self):
> +        self.slave = offspring.slave.LexbuilderSlave(('localhost', 8760))
> +        self.slave.start()
> +        self.x = xmlrpclib.ServerProxy("http://admin:password@localhost:8760")

Why do we need a ServerProxy?

> +        self.PBP_start = offspring.slave.slave.ProjectBuildProcess.start
> +
> +    def tearDown(self):
> +        offspring.slave.slave.ProjectBuildProcess.start = self.PBP_start
> +        self.slave.stop()
> +
> +    def test_startBuildWithCustomSSHKey(self):
> +        # Test doesn't want to trigger an actual build. The following line
> +        # prevents this.
> +        offspring.slave.slave.ProjectBuildProcess.start = self.nothing
> +
> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]
> +
> +        self.slave.startBuild(args[0], args[1], args[2], args[3])

This test is showing that startBuild() does the right thing when given a
user and key, but I think it should do so via XML-RPC because that is
how the user/key are going to be passed in from the master to the slave.
What do you think?

> +
> +        passed_args = self.slave.currentBuild.args

Instead of storing the passed arguments as an instance variable in
ProjectBuildProcess, you could just mock its __init__() here and check
that it was passed the correct arguments. That way we don't need to
pollute production code with stuff that only exists to aid testing
(.args).  It's not a big deal, but it'd be good to at least have a
comment there stating that .args is there to make testing easier.

> +
> +        bzr_path = re.sub("lp:",
> +                          "bzr+ssh://" + args[2] + "@bazaar.launchpad.net/",
> +                          args[1])
> +
> +        expected_args = [args[0], bzr_path,
> +                         self.slave.currentBuild.ssh_key_file.name]
> +
> +        self.assertEqual(expected_args, passed_args)
> +
> +    def test_checkSSHKeyDeletedWhenBuildFinished(self):
> +        # Test doesn't want to trigger an actual build. The following line
> +        # prevents this.
> +        offspring.slave.slave.ProjectBuildProcess.start = self.nothing
> +        
> +        args = ["project_name", "lp:~dooferlad/offspring/config", "user", "key"]
> +
> +        self.slave.startBuild(args[0], args[1], args[2], args[3])
> +
> +        file_path = self.slave.currentBuild.ssh_key_file.name
> +        read_key = open(file_path).read()
> +
> +        # We should have a key written to a file for the slave to read...
> +        self.assertEqual(read_key, args[3])
> +
> +        self.slave.stopBuild()
> +        # Having stopped the slave, the key should have been erased
> +        self.assertFalse(os.path.exists(file_path))
> +
> +    def test_ssh_agent_killed_when_build_terminates(self):
> +        # 1. Start job
> +        # 2. Wait for completion
> +        # 3. test no ssh-agent process running
> +        pass

Oops

> +
> +    def test_checkSSHAgent(self):
> +        # This is a wrapper around a shell script that runs the commands and
> +        # we just check the output.
> +
> +        dir_of_this_file = os.path.dirname(os.path.abspath(__file__))
> +        test_script = os.path.join(dir_of_this_file, "../../build/tests/test.sh")
> +
> +        p = subprocess.Popen(test_script, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
> +
> +        stdout = p.communicate()[0]
> +
> +        lines = stdout.split("\n")
> +
> +        self.assertTrue(re.search("Agent pid \d+", lines[0]))
> +        self.assertTrue(re.search("Identity added:.*?offspring_test_key", lines[1]))
> +        self.assertTrue(re.search("offspring_test_key $RSA$", lines[3]))
> +        self.assertTrue(re.search("Could not open a connection to your authentication agent\.", lines[5]))
> \ No newline at end of file

If any of those asserts fail, the developer will be left with no clue as
to why because the error message will be something like 'False is not
True'.  One thing you can do to improve that is to pass the line you're
trying to match against as the error message to assertTrue().

Also, the test would be easier to read if you unpacked stdout.split()
into named variables.

> 
> === renamed file 'lib/offspring/slave/tests.py' => 'lib/offspring/slave/tests/tests.py'
> === modified file 'lib/offspring/web/queuemanager/admin.py'
> --- lib/offspring/web/queuemanager/admin.py	2011-02-24 05:08:42 +0000
> +++ lib/offspring/web/queuemanager/admin.py	2011-10-05 16:24:23 +0000
> @@ -2,6 +2,7 @@
>  # GNU Affero General Public License version 3 (see the file LICENSE).
>  
>  from django.contrib import admin
> +from django import forms
>  
>  from offspring.web.queuemanager.forms import ProjectBaseForm
>  from offspring.web.queuemanager.models import (
> @@ -43,7 +44,8 @@
>  
> 
>  class ProjectAdmin(admin.ModelAdmin):
> -    fields = ['title', 'name', 'arch', 'project_group', 'launchpad_project', 'suite', 'series', 'priority', 'status', 'is_active', 'config_url', 'notes']
> +    fields = ['title', 'name', 'arch', 'project_group', 'launchpad_project', 'suite', 'series', 'priority',
> +              'status', 'is_active','config_url', 'lp_user', 'lp_ssh_key_input', 'notes']
>      list_display = ('display_name', 'arch', 'series', 'project_group', 'launchpad_project', 'is_active', 'status', 'priority', 'config_url')
>      list_filter = ['arch', 'series', 'is_active', 'project_group', 'status']
>      search_fields = ['title', 'name', 'arch', 'notes']
> 
> === modified file 'lib/offspring/web/queuemanager/forms.py'
> --- lib/offspring/web/queuemanager/forms.py	2011-03-03 00:06:58 +0000
> +++ lib/offspring/web/queuemanager/forms.py	2011-10-05 16:24:23 +0000
> @@ -4,6 +4,7 @@
>  from django.db import models
>  from django.forms import ModelChoiceField, ModelForm, Textarea, TextInput
>  from django.forms import fields
> +from django import forms
>  
>  from offspring.web.queuemanager.models import (
>      Project,
> @@ -12,11 +13,87 @@
>  )
>  
>  from offspring.web.queuemanager.widgets import SelectWithAddNew
> +import re
> +
>  
>  class ProjectBaseForm(ModelForm):
>      status = fields.CharField(max_length=200,
>          widget=fields.Select(choices=Project.STATUS_CHOICES), required=True)
>  
> +    ssh_help = ("Enter a private SSH ASCII key block, complete with begin "+
> +                "and end markers. Saved keys are not shown. If you wish to "+
> +                "erase a saved key (rather than replacing it) enter the "+
> +                "string \"ERASE\".")
> +
> +    lp_ssh_key_input = forms.CharField(label="Launchpad User's SSH key",
> +                                       required=False,
> +                                       widget=forms.Textarea(
> +                                               attrs={'cols': 73, 'rows' : 4}),
> +                                       help_text=ssh_help)

Does this mean it's also possible to change a project's ssh key on its
edit view?  IIUC that means anybody with change_project rights can then
do so for all projects?  I guess that's why you've made it so that an
existing ssh key is not displayed?

> +
> +    def __init__(self, *args, **kwargs):
> +        super(ProjectBaseForm, self).__init__(*args, **kwargs)
> +
> +        # Make sure lp_ssh_key_input turns up next to lp_user. This is needed
> +        # because I insert a special lp_ssh_key_input field in this class
> +        # (above) and without doing this it will show up last.
> +        field_order = self.fields.keyOrder
> +        field_order.pop(field_order.index('lp_ssh_key_input'))
> +        lp_user_pos = field_order.index('lp_user')
> +        field_order.insert(lp_user_pos + 1, 'lp_ssh_key_input')
> +
> +        self.ui_messages = {"ssh set":
> +                             "An SSH key has already been saved for this "+
> +                             "project. To update it paste a new one here.",
> +                            "ssh clear":
> +                             "No SSH key has been saved for this project. "+
> +                             "Paste one here to set."}
> +
> +        # Set the form fields based on the model object
> +        if kwargs.has_key('instance'):
> +            instance = kwargs['instance']
> +            if instance.lp_ssh_key and instance.lp_ssh_key != "":
> +                self.initial['lp_ssh_key_input'] = self.ui_messages["ssh set"]
> +            else:
> +                self.initial['lp_ssh_key_input'] = self.ui_messages["ssh clear"]
> +
> +    def clean_lp_ssh_key_input(self):
> +        new_key = self.cleaned_data['lp_ssh_key_input']
> +
> +        key_search_regexp = (r"-----BEGIN \w+ PRIVATE KEY-----"+
> +                             r".*"+
> +                             r"-----END \w+ PRIVATE KEY-----")
> +        is_key = re.search(key_search_regexp, new_key, re.DOTALL | re.MULTILINE)
> +        is_message = new_key in self.ui_messages.values()
> +        
> +        if not(is_key or new_key == "" or is_message or new_key == "ERASE"):
> +            raise forms.ValidationError(
> +                            "Key block doesn't look valid - not saved.")
> +
> +        # Always return the cleaned data, whether you have changed it or not.
> +        return new_key
> +
> +    def save(self, commit=True):
> +        model = super(ProjectBaseForm, self).save(commit=False)
> +
> +        # Save the SSH key
> +        new_key = self.cleaned_data['lp_ssh_key_input']
> +        is_message = new_key in self.ui_messages.values()
> +        
> +        if new_key == "" or is_message:
> +            pass # No new key entered
> +
> +        elif new_key == "ERASE":
> +            model.lp_ssh_key = "" # Erase stored key
> +
> +        else: # Save new key (validated in clean_lp_ssh_key_input)
> +            model.lp_ssh_key = new_key
> +
> +        if commit:
> +            model.save()
> +
> +        return model
> +
>      class Meta:
>          model = Project
>          widgets = {
> @@ -24,6 +101,7 @@
>              'series' : TextInput(attrs={'style': 'text-transform: lowercase;'}),
>              'config_url': TextInput(attrs={'size': 50}),
>              'notes' : Textarea(attrs={'cols': 73, 'rows' : 4}),
> +            'lp_ssh_key': Textarea(attrs={'cols': 73, 'rows' : 4}),
>          }
>  
>      def clean_name(self):
>

-- 
Guilherme Salgado <https://launchpad.net/~salgado>

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-06:

#

Download full text (8.9 KiB)

Hi Guilherme,

Thanks for another review. Seem to be getting close now.

On 6 October 2011 14:28, Guilherme Salgado <email address hidden> wrote:
> On Wed, 2011-10-05 at 16:25 +0000, James Tunnicliffe wrote:
>> James Tunnicliffe has proposed merging lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh into lp:~linaro-infrastructure/offspring/linaro.

>> This merge address the comments raised about the initial one. It also
>> updates the web UI so the admin interface allows the user to change
>> the ssh user and key, but can't get access to the stored key.
>
> I was expecting the ssh user/key to be editable on the project's edit
> page (the project_edit() view), but I guess that wouldn't work because
> people with the change_project permission would then be able to change
> that for all projects, right?
>
> Having to use the admin UI to change a project's ssh user/key means the
> people in charge of every project will have to send the ssh key to
> somebody with admin rights to set. I think we should check with Vicky
> that this is ok.

Ah, sorry, that comment was wrong - anyone can paste a new SSH key
into the project page, it isn't just the admin page.

Am I right in thinking that the code you are putting in will restrict
access to all the project pages to specified users? If so, I think
this is OK.

<snip>

>> === modified file 'lib/offspring/slave/slave.py'
>> - def startBuild(self, projectName, configBranchURL):
>> + def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):
>> + # check for lp: format URLs. Convert these to bzr+ssh ones
>> + configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
>> + configBranchURL)
>
> I thought you were going to use 'bzr lp-login' as it's not a problem if
> we fail to cleanup ~/.bazaar/authentication.conf as long as we make sure
> to kill the ssh agent. And if we fail to kill the ssh agent then using
> lp-login is not much worse than it is now.

The last comment James W made about home directories was that they may
be shared between builders, so we don't want to transport the
launchpad user using ~/.bazaar/authentication.conf to the build script
because it could change before it is used.

<snip>

>> + def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
>> +
>> + if lpUser:
>> + # We don't want to pass the user separately, we want it to be
>> + # integrated into the configBranchURL.
>> + configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
>> +
>> self.currentBuild = ProjectBuildProcess(
>> - self, projectName, configBranchURL)
>> + self, projectName, configBranchURL, lpSSHKey)
>> logging.info("Starting build: %s %s %s" % (
>> self.currentBuild.getCommandPath(), projectName, configBranchURL))
>> self.currentBuild.start()
>> @@ -70,9 +94,12 @@
>> if self.currentBuild.process is not None:
>> if self.currentBuild.process.poll() is None:
>> self.currentBuild.process.terminate()
>> - # Give it 5 seconds before sending kill.
...

Hi Guilherme,

Thanks for another review. Seem to be getting close now.

On 6 October 2011 14:28, Guilherme Salgado <guilherme.salgado@linaro.org> wrote:
> On Wed, 2011-10-05 at 16:25 +0000, James Tunnicliffe wrote:
>> James Tunnicliffe has proposed merging lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh into lp:~linaro-infrastructure/offspring/linaro.

>> This merge address the comments raised about the initial one. It also
>> updates the web UI so the admin interface allows the user to change
>> the ssh user and key, but can't get access to the stored key.
>
> I was expecting the ssh user/key to be editable on the project's edit
> page (the project_edit() view), but I guess that wouldn't work because
> people with the change_project permission would then be able to change
> that for all projects, right?
>
> Having to use the admin UI to change a project's ssh user/key means the
> people in charge of every project will have to send the ssh key to
> somebody with admin rights to set.  I think we should check with Vicky
> that this is ok.

Ah, sorry, that comment was wrong - anyone can paste a new SSH key
into the project page, it isn't just the admin page.

Am I right in thinking that the code you are putting in will restrict
access to all the project pages to specified users? If so, I think
this is OK.

<snip>

>> === modified file 'lib/offspring/slave/slave.py'
>> -    def startBuild(self, projectName, configBranchURL):
>> +    def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):
>> +        # check for lp: format URLs. Convert these to bzr+ssh ones
>> +        configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
>> +                                 configBranchURL)
>
> I thought you were going to use 'bzr lp-login' as it's not a problem if
> we fail to cleanup ~/.bazaar/authentication.conf as long as we make sure
> to kill the ssh agent.  And if we fail to kill the ssh agent then using
> lp-login is not much worse than it is now.

The last comment James W made about home directories was that they may
be shared between builders, so we don't want to transport the
launchpad user using ~/.bazaar/authentication.conf to the build script
because it could change before it is used.

<snip>

>> +    def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
>> +
>> +        if lpUser:
>> +            # We don't want to pass the user separately, we want it to be
>> +            # integrated into the configBranchURL.
>> +            configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
>> +
>>          self.currentBuild = ProjectBuildProcess(
>> -            self, projectName, configBranchURL)
>> +            self, projectName, configBranchURL, lpSSHKey)
>>          logging.info("Starting build: %s %s %s" % (
>>              self.currentBuild.getCommandPath(), projectName, configBranchURL))
>>          self.currentBuild.start()
>> @@ -70,9 +94,12 @@
>>              if self.currentBuild.process is not None:
>>                  if self.currentBuild.process.poll() is None:
>>                      self.currentBuild.process.terminate()
>> -            # Give it 5 seconds before sending kill.
>> -            time.sleep(int(config.slave("child_wait_timeout")))
>> -            self.currentBuild.process.kill()
>> +                    if self.currentBuild.process.poll() is None:
>> +                        # Give it 5 seconds before sending kill.
>> +                        time.sleep(int(config.slave("child_wait_timeout")))
>> +                        if self.currentBuild.process.poll() is None:
>> +                            self.currentBuild.process.kill()
>
> Why is this needed?

I found when running tests that since I was creating and destroying
the slave, but not starting a build process, the old code would just
call self.currentBuild.process.kill() and self.currentBuild.process
didn't exist, causing an error. This code only does exactly the same
as the old code (ask process to end gracefully, wait 5 seconds, send
kill signal), but only performing each step as needed.

>> === added directory 'lib/offspring/slave/tests'
>> === added file 'lib/offspring/slave/tests/test_private_ssh_keys.py'
>> --- lib/offspring/slave/tests/test_private_ssh_keys.py        1970-01-01 00:00:00 +0000
>> +++ lib/offspring/slave/tests/test_private_ssh_keys.py        2011-10-05 16:24:23 +0000
>> @@ -0,0 +1,88 @@
>> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
>> +# GNU Affero General Public License version 3 (see the file LICENSE).
>> +
>> +import os
>> +import subprocess
>> +import offspring.slave
>> +import unittest
>> +import xmlrpclib
>> +import re
>> +import os.path
>> +
>> +class TestLexbuilderSlavePrivateSSHKeys(unittest.TestCase):
>> +
>> +    def nothing(self):
>> +        pass
>> +
>> +    def setUp(self):
>> +        self.slave = offspring.slave.LexbuilderSlave(('localhost', 8760))
>> +        self.slave.start()
>> +        self.x = xmlrpclib.ServerProxy("http://admin:password@localhost:8760")
>
> Why do we need a ServerProxy?

<snip>

> This test is showing that startBuild() does the right thing when given a
> user and key, but I think it should do so via XML-RPC because that is
> how the user/key are going to be passed in from the master to the slave.
> What do you think?

OK. the ServerProxy stays. I have modified the tests to use it where
they can and commented as to why in the places they can't.

>> +
>> +        passed_args = self.slave.currentBuild.args
>
> Instead of storing the passed arguments as an instance variable in
> ProjectBuildProcess, you could just mock its __init__() here and check
> that it was passed the correct arguments. That way we don't need to
> pollute production code with stuff that only exists to aid testing
> (.args).  It's not a big deal, but it'd be good to at least have a
> comment there stating that .args is there to make testing easier.

ProjectBuildProcess.args is used locally in __init__ to allow us to
pass the SSH key location to the build script. It could just be a
variable local to __init__ if I mocked up MonitoredProcess.__init__ -
will do that.

>> +    def test_ssh_agent_killed_when_build_terminates(self):
>> +        # 1. Start job
>> +        # 2. Wait for completion
>> +        # 3. test no ssh-agent process running
>> +        pass
>
> Oops

Doh!

>> +
>> +    def test_checkSSHAgent(self):
>> +        # This is a wrapper around a shell script that runs the commands and
>> +        # we just check the output.
>> +
>> +        dir_of_this_file = os.path.dirname(os.path.abspath(__file__))
>> +        test_script = os.path.join(dir_of_this_file, "../../build/tests/test.sh")
>> +
>> +        p = subprocess.Popen(test_script, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
>> +
>> +        stdout = p.communicate()[0]
>> +
>> +        lines = stdout.split("\n")
>> +
>> +        self.assertTrue(re.search("Agent pid \d+", lines[0]))
>> +        self.assertTrue(re.search("Identity added:.*?offspring_test_key", lines[1]))
>> +        self.assertTrue(re.search("offspring_test_key $RSA$", lines[3]))
>> +        self.assertTrue(re.search("Could not open a connection to your authentication agent\.", lines[5]))

> If any of those asserts fail, the developer will be left with no clue as
> to why because the error message will be something like 'False is not
> True'.  One thing you can do to improve that is to pass the line you're
> trying to match against as the error message to assertTrue().
>
> Also, the test would be easier to read if you unpacked stdout.split()
> into named variables.

Yea, good point.

>> === modified file 'lib/offspring/web/queuemanager/forms.py'
>> --- lib/offspring/web/queuemanager/forms.py   2011-03-03 00:06:58 +0000
>> +++ lib/offspring/web/queuemanager/forms.py   2011-10-05 16:24:23 +0000
<snip>
>>  class ProjectBaseForm(ModelForm):
>>      status = fields.CharField(max_length=200,
>>          widget=fields.Select(choices=Project.STATUS_CHOICES), required=True)
>>
>> +    ssh_help = ("Enter a private SSH ASCII key block, complete with begin "+
>> +                "and end markers. Saved keys are not shown. If you wish to "+
>> +                "erase a saved key (rather than replacing it) enter the "+
>> +                "string \"ERASE\".")
>> +
>> +    lp_ssh_key_input = forms.CharField(label="Launchpad User's SSH key",
>> +                                       required=False,
>> +                                       widget=forms.Textarea(
>> +                                               attrs={'cols': 73, 'rows' : 4}),
>> +                                       help_text=ssh_help)
>
> Does this mean it's also possible to change a project's ssh key on its
> edit view?  IIUC that means anybody with change_project rights can then
> do so for all projects?  I guess that's why you've made it so that an
> existing ssh key is not displayed?

Yes, anyone who can change the project can change the SSH key. Should
be simple enough to keep it just on the admin page though.

-- 
James Tunnicliffe

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-06:

#

Download full text (9.1 KiB)

On Thu, 2011-10-06 at 14:37 +0000, James Tunnicliffe wrote:
> Hi Guilherme,
>
> Thanks for another review. Seem to be getting close now.

Indeed, if it wasn't for the permission issue we just seem to have
uncovered... :(

> On 6 October 2011 14:28, Guilherme Salgado <email address hidden> wrote:
> > On Wed, 2011-10-05 at 16:25 +0000, James Tunnicliffe wrote:
> >> James Tunnicliffe has proposed merging lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh into lp:~linaro-infrastructure/offspring/linaro.
>
> >> This merge address the comments raised about the initial one. It also
> >> updates the web UI so the admin interface allows the user to change
> >> the ssh user and key, but can't get access to the stored key.
> >
> > I was expecting the ssh user/key to be editable on the project's edit
> > page (the project_edit() view), but I guess that wouldn't work because
> > people with the change_project permission would then be able to change
> > that for all projects, right?
> >
> > Having to use the admin UI to change a project's ssh user/key means the
> > people in charge of every project will have to send the ssh key to
> > somebody with admin rights to set. I think we should check with Vicky
> > that this is ok.
>
> Ah, sorry, that comment was wrong - anyone can paste a new SSH key
> into the project page, it isn't just the admin page.
>
> Am I right in thinking that the code you are putting in will restrict
> access to all the project pages to specified users? If so, I think
> this is OK.

Only access to private projects will be restricted to specified users.

> >> === modified file 'lib/offspring/slave/slave.py'
> >> - def startBuild(self, projectName, configBranchURL):
> >> + def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):
> >> + # check for lp: format URLs. Convert these to bzr+ssh ones
> >> + configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
> >> + configBranchURL)
> >
> > I thought you were going to use 'bzr lp-login' as it's not a problem if
> > we fail to cleanup ~/.bazaar/authentication.conf as long as we make sure
> > to kill the ssh agent. And if we fail to kill the ssh agent then using
> > lp-login is not much worse than it is now.
>
> The last comment James W made about home directories was that they may
> be shared between builders, so we don't want to transport the
> launchpad user using ~/.bazaar/authentication.conf to the build script
> because it could change before it is used.

My understanding is that they are only shared between builds on a
certain builder, which is not a problem as they're serialized and we're
killing ssh-agent between builds.

> >> + def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
> >> +
> >> + if lpUser:
> >> + # We don't want to pass the user separately, we want it to be
> >> + # integrated into the configBranchURL.
> >> + configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
> >> +
> >> self.currentBuild = ProjectBuildProcess(
> >> - self, projectName, configBranchURL)
> >> + self, pr...

On Thu, 2011-10-06 at 14:37 +0000, James Tunnicliffe wrote:
> Hi Guilherme,
> 
> Thanks for another review. Seem to be getting close now.

Indeed, if it wasn't for the permission issue we just seem to have
uncovered... :(

> On 6 October 2011 14:28, Guilherme Salgado <guilherme.salgado@linaro.org> wrote:
> > On Wed, 2011-10-05 at 16:25 +0000, James Tunnicliffe wrote:
> >> James Tunnicliffe has proposed merging lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh into lp:~linaro-infrastructure/offspring/linaro.
> 
> >> This merge address the comments raised about the initial one. It also
> >> updates the web UI so the admin interface allows the user to change
> >> the ssh user and key, but can't get access to the stored key.
> >
> > I was expecting the ssh user/key to be editable on the project's edit
> > page (the project_edit() view), but I guess that wouldn't work because
> > people with the change_project permission would then be able to change
> > that for all projects, right?
> >
> > Having to use the admin UI to change a project's ssh user/key means the
> > people in charge of every project will have to send the ssh key to
> > somebody with admin rights to set.  I think we should check with Vicky
> > that this is ok.
> 
> Ah, sorry, that comment was wrong - anyone can paste a new SSH key
> into the project page, it isn't just the admin page.
> 
> Am I right in thinking that the code you are putting in will restrict
> access to all the project pages to specified users? If so, I think
> this is OK.

Only access to private projects will be restricted to specified users.

> >> === modified file 'lib/offspring/slave/slave.py'
> >> -    def startBuild(self, projectName, configBranchURL):
> >> +    def add_user_to_bzr_ssh_url(self, configBranchURL, lpUser):
> >> +        # check for lp: format URLs. Convert these to bzr+ssh ones
> >> +        configBranchURL = re.sub("lp:", "bzr+ssh://bazaar.launchpad.net/",
> >> +                                 configBranchURL)
> >
> > I thought you were going to use 'bzr lp-login' as it's not a problem if
> > we fail to cleanup ~/.bazaar/authentication.conf as long as we make sure
> > to kill the ssh agent.  And if we fail to kill the ssh agent then using
> > lp-login is not much worse than it is now.
> 
> The last comment James W made about home directories was that they may
> be shared between builders, so we don't want to transport the
> launchpad user using ~/.bazaar/authentication.conf to the build script
> because it could change before it is used.

My understanding is that they are only shared between builds on a
certain builder, which is not a problem as they're serialized and we're
killing ssh-agent between builds.

> >> +    def startBuild(self, projectName, configBranchURL, lpUser=None, lpSSHKey=None):
> >> +
> >> +        if lpUser:
> >> +            # We don't want to pass the user separately, we want it to be
> >> +            # integrated into the configBranchURL.
> >> +            configBranchURL = self.add_user_to_bzr_ssh_url(configBranchURL, lpUser)
> >> +
> >>          self.currentBuild = ProjectBuildProcess(
> >> -            self, projectName, configBranchURL)
> >> +            self, projectName, configBranchURL, lpSSHKey)
> >>          logging.info("Starting build: %s %s %s" % (
> >>              self.currentBuild.getCommandPath(), projectName, configBranchURL))
> >>          self.currentBuild.start()
> >> @@ -70,9 +94,12 @@
> >>              if self.currentBuild.process is not None:
> >>                  if self.currentBuild.process.poll() is None:
> >>                      self.currentBuild.process.terminate()
> >> -            # Give it 5 seconds before sending kill.
> >> -            time.sleep(int(config.slave("child_wait_timeout")))
> >> -            self.currentBuild.process.kill()
> >> +                    if self.currentBuild.process.poll() is None:
> >> +                        # Give it 5 seconds before sending kill.
> >> +                        time.sleep(int(config.slave("child_wait_timeout")))
> >> +                        if self.currentBuild.process.poll() is None:
> >> +                            self.currentBuild.process.kill()
> >
> > Why is this needed?
> 
> I found when running tests that since I was creating and destroying
> the slave, but not starting a build process, the old code would just
> call self.currentBuild.process.kill() and self.currentBuild.process
> didn't exist, causing an error. This code only does exactly the same
> as the old code (ask process to end gracefully, wait 5 seconds, send
> kill signal), but only performing each step as needed.

Ok, I asked because it seemed to be there just to please the tests but
on second thought I think it makes sense because we don't want to call
self.currentBuild.process.kill() unconditionally, as the if block above
it implies it may be None.

> 
> >> === added directory 'lib/offspring/slave/tests'
> >> === added file 'lib/offspring/slave/tests/test_private_ssh_keys.py'
> >> --- lib/offspring/slave/tests/test_private_ssh_keys.py        1970-01-01 00:00:00 +0000
> >> +++ lib/offspring/slave/tests/test_private_ssh_keys.py        2011-10-05 16:24:23 +0000
> >> @@ -0,0 +1,88 @@
> >> +# Copyright 2010 Canonical Ltd.  This software is licensed under the
> >> +# GNU Affero General Public License version 3 (see the file LICENSE).
> >> +
> >> +import os
> >> +import subprocess
> >> +import offspring.slave
> >> +import unittest
> >> +import xmlrpclib
> >> +import re
> >> +import os.path
> >> +
> >> +class TestLexbuilderSlavePrivateSSHKeys(unittest.TestCase):
> >> +
> >> +    def nothing(self):
> >> +        pass
> >> +
> >> +    def setUp(self):
> >> +        self.slave = offspring.slave.LexbuilderSlave(('localhost', 8760))
> >> +        self.slave.start()
> >> +        self.x = xmlrpclib.ServerProxy("http://admin:password@localhost:8760")
> >
> > Why do we need a ServerProxy?
> 
> <snip>
> 
> > This test is showing that startBuild() does the right thing when given a
> > user and key, but I think it should do so via XML-RPC because that is
> > how the user/key are going to be passed in from the master to the slave.
> > What do you think?
> 
> OK. the ServerProxy stays. I have modified the tests to use it where
> they can and commented as to why in the places they can't.

Cool!

> 
> >> +
> >> +        passed_args = self.slave.currentBuild.args
> >
> > Instead of storing the passed arguments as an instance variable in
> > ProjectBuildProcess, you could just mock its __init__() here and check
> > that it was passed the correct arguments. That way we don't need to
> > pollute production code with stuff that only exists to aid testing
> > (.args).  It's not a big deal, but it'd be good to at least have a
> > comment there stating that .args is there to make testing easier.
> 
> ProjectBuildProcess.args is used locally in __init__ to allow us to
> pass the SSH key location to the build script. It could just be a
> variable local to __init__ if I mocked up MonitoredProcess.__init__ -
> will do that.

Right, that was what I thought. :)

> >> === modified file 'lib/offspring/web/queuemanager/forms.py'
> >> --- lib/offspring/web/queuemanager/forms.py   2011-03-03 00:06:58 +0000
> >> +++ lib/offspring/web/queuemanager/forms.py   2011-10-05 16:24:23 +0000
> <snip>
> >>  class ProjectBaseForm(ModelForm):
> >>      status = fields.CharField(max_length=200,
> >>          widget=fields.Select(choices=Project.STATUS_CHOICES), required=True)
> >>
> >> +    ssh_help = ("Enter a private SSH ASCII key block, complete with begin "+
> >> +                "and end markers. Saved keys are not shown. If you wish to "+
> >> +                "erase a saved key (rather than replacing it) enter the "+
> >> +                "string \"ERASE\".")
> >> +
> >> +    lp_ssh_key_input = forms.CharField(label="Launchpad User's SSH key",
> >> +                                       required=False,
> >> +                                       widget=forms.Textarea(
> >> +                                               attrs={'cols': 73, 'rows' : 4}),
> >> +                                       help_text=ssh_help)
> >
> > Does this mean it's also possible to change a project's ssh key on its
> > edit view?  IIUC that means anybody with change_project rights can then
> > do so for all projects?  I guess that's why you've made it so that an
> > existing ssh key is not displayed?
> 
> Yes, anyone who can change the project can change the SSH key. Should
> be simple enough to keep it just on the admin page though.

The problem is that there are no per-project permissions in Offspring;
if a user can change one project they can change all of them (modulo the
private ones, after our work is complete).

If we keep it only in the admin UI we'd have to give the ssh keys to
someone with admin rights, and this someone would then be given the ssh
keys of all landing teams.

Or maybe we could have one member of each LT as an admin in Offspring so
that they can do it themselves.  With your changes to not show the
existing ssh key that means they'd only be able to enter new ssh keys
for projects but not see existing ones.  I don't think it is a good idea
to have one person from each LT with admin rights though.

-- 
Guilherme Salgado <https://launchpad.net/~salgado>

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-06:

#

On 6 October 2011 17:08, Guilherme Salgado <email address hidden> wrote:
>> The last comment James W made about home directories was that they may
>> be shared between builders, so we don't want to transport the
>> launchpad user using ~/.bazaar/authentication.conf to the build script
>> because it could change before it is used.
>
> My understanding is that they are only shared between builds on a
> certain builder, which is not a problem as they're serialized and we're
> killing ssh-agent between builds.

I guess I am worried that a builder could be a multi-core machine and
all instances of a user could share 1 home directory on that machine.
Basically, the current solution is always safe, no matter what the
disk situation is and using lp-login is with the current set up, but
may not be in the future. I would just rather go with something that
is more robust.

> The problem is that there are no per-project permissions in Offspring;
> if a user can change one project they can change all of them (modulo the
> private ones, after our work is complete).
>
> If we keep it only in the admin UI we'd have to give the ssh keys to
> someone with admin rights, and this someone would then be given the ssh
> keys of all landing teams.
>
> Or maybe we could have one member of each LT as an admin in Offspring so
> that they can do it themselves. With your changes to not show the
> existing ssh key that means they'd only be able to enter new ssh keys
> for projects but not see existing ones. I don't think it is a good idea
> to have one person from each LT with admin rights though.

How about:
* When creating a project, you can enter an SSH key and user
* Once a project is created...
- If it is private, you can change the SSH key (current interface)
- If it is public, you can't change the SSH key
* The admin interface always allows you to change the SSH key

Another option would be disabling SSH users + keys for all public
projects and leave them as they are for private ones once both our
patches are working together.

The only other option In can think of is recording the user that
created the project and, if it is public, only letting that user
change the SSH user and key. For private projects we could give the
project group permission.

What do you think?

--
James Tunnicliffe

On 6 October 2011 17:08, Guilherme Salgado <guilherme.salgado@linaro.org> wrote:
>> The last comment James W made about home directories was that they may
>> be shared between builders, so we don't want to transport the
>> launchpad user using ~/.bazaar/authentication.conf to the build script
>> because it could change before it is used.
>
> My understanding is that they are only shared between builds on a
> certain builder, which is not a problem as they're serialized and we're
> killing ssh-agent between builds.

I guess I am worried that a builder could be a multi-core machine and
all instances of a user could share 1 home directory on that machine.
Basically, the current solution is always safe, no matter what the
disk situation is and using lp-login is with the current set up, but
may not be in the future. I would just rather go with something that
is more robust.

> The problem is that there are no per-project permissions in Offspring;
> if a user can change one project they can change all of them (modulo the
> private ones, after our work is complete).
>
> If we keep it only in the admin UI we'd have to give the ssh keys to
> someone with admin rights, and this someone would then be given the ssh
> keys of all landing teams.
>
> Or maybe we could have one member of each LT as an admin in Offspring so
> that they can do it themselves.  With your changes to not show the
> existing ssh key that means they'd only be able to enter new ssh keys
> for projects but not see existing ones.  I don't think it is a good idea
> to have one person from each LT with admin rights though.

How about:
* When creating a project, you can enter an SSH key and user
* Once a project is created...
  - If it is private, you can change the SSH key (current interface)
  - If it is public, you can't change the SSH key
* The admin interface always allows you to change the SSH key

Another option would be disabling SSH users + keys for all public
projects and leave them as they are for private ones once both our
patches are working together.

The only other option In can think of is recording the user that
created the project and, if it is public, only letting that user
change the SSH user and key. For private projects we could give the
project group permission.

What do you think?

-- 
James Tunnicliffe

lp:~dooferlad/offspring/linaro_offspring_add_bzr_ssh updated on 2011-10-06

66. By James Tunnicliffe on 2011-10-06: Made ProjectBuildProcess.args into a local variable for __init__ (not required for testing)
Removed null test from test_private_ssh_keys.py
67. By James Tunnicliffe on 2011-10-06: Made failures in test_checkSSHAgent easier to debug by providing sensible error messages.

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-06:

#

On Thu, 2011-10-06 at 17:31 +0000, James Tunnicliffe wrote:
> On 6 October 2011 17:08, Guilherme Salgado <email address hidden> wrote:
> >> The last comment James W made about home directories was that they may
> >> be shared between builders, so we don't want to transport the
> >> launchpad user using ~/.bazaar/authentication.conf to the build script
> >> because it could change before it is used.
> >
> > My understanding is that they are only shared between builds on a
> > certain builder, which is not a problem as they're serialized and we're
> > killing ssh-agent between builds.
>
> I guess I am worried that a builder could be a multi-core machine and
> all instances of a user could share 1 home directory on that machine.
> Basically, the current solution is always safe, no matter what the
> disk situation is and using lp-login is with the current set up, but
> may not be in the future. I would just rather go with something that
> is more robust.

Fair enough

>
> > The problem is that there are no per-project permissions in Offspring;
> > if a user can change one project they can change all of them (modulo the
> > private ones, after our work is complete).
> >
> > If we keep it only in the admin UI we'd have to give the ssh keys to
> > someone with admin rights, and this someone would then be given the ssh
> > keys of all landing teams.
> >
> > Or maybe we could have one member of each LT as an admin in Offspring so
> > that they can do it themselves. With your changes to not show the
> > existing ssh key that means they'd only be able to enter new ssh keys
> > for projects but not see existing ones. I don't think it is a good idea
> > to have one person from each LT with admin rights though.
>
> How about:
> * When creating a project, you can enter an SSH key and user
> * Once a project is created...
> - If it is private, you can change the SSH key (current interface)
> - If it is public, you can't change the SSH key
> * The admin interface always allows you to change the SSH key

Sounds good to me, and maybe we can even allow editing ssh keys of
public projects... Changing the ssh key of an existing project won't
give an attacker access to anything they don't already have access to
anyway.

>
> Another option would be disabling SSH users + keys for all public
> projects and leave them as they are for private ones once both our
> patches are working together.
>
> The only other option In can think of is recording the user that
> created the project and, if it is public, only letting that user
> change the SSH user and key. For private projects we could give the
> project group permission.
>
> What do you think?
>

--
Guilherme Salgado <https://launchpad.net/~salgado>

On Thu, 2011-10-06 at 17:31 +0000, James Tunnicliffe wrote:
> On 6 October 2011 17:08, Guilherme Salgado <guilherme.salgado@linaro.org> wrote:
> >> The last comment James W made about home directories was that they may
> >> be shared between builders, so we don't want to transport the
> >> launchpad user using ~/.bazaar/authentication.conf to the build script
> >> because it could change before it is used.
> >
> > My understanding is that they are only shared between builds on a
> > certain builder, which is not a problem as they're serialized and we're
> > killing ssh-agent between builds.
> 
> I guess I am worried that a builder could be a multi-core machine and
> all instances of a user could share 1 home directory on that machine.
> Basically, the current solution is always safe, no matter what the
> disk situation is and using lp-login is with the current set up, but
> may not be in the future. I would just rather go with something that
> is more robust.

Fair enough

> 
> > The problem is that there are no per-project permissions in Offspring;
> > if a user can change one project they can change all of them (modulo the
> > private ones, after our work is complete).
> >
> > If we keep it only in the admin UI we'd have to give the ssh keys to
> > someone with admin rights, and this someone would then be given the ssh
> > keys of all landing teams.
> >
> > Or maybe we could have one member of each LT as an admin in Offspring so
> > that they can do it themselves.  With your changes to not show the
> > existing ssh key that means they'd only be able to enter new ssh keys
> > for projects but not see existing ones.  I don't think it is a good idea
> > to have one person from each LT with admin rights though.
> 
> How about:
> * When creating a project, you can enter an SSH key and user
> * Once a project is created...
>   - If it is private, you can change the SSH key (current interface)
>   - If it is public, you can't change the SSH key
> * The admin interface always allows you to change the SSH key

Sounds good to me, and maybe we can even allow editing ssh keys of
public projects... Changing the ssh key of an existing project won't
give an attacker access to anything they don't already have access to
anyway.

> 
> Another option would be disabling SSH users + keys for all public
> projects and leave them as they are for private ones once both our
> patches are working together.
> 
> The only other option In can think of is recording the user that
> created the project and, if it is public, only letting that user
> change the SSH user and key. For private projects we could give the
> project group permission.
> 
> What do you think?
>

-- 
Guilherme Salgado <https://launchpad.net/~salgado>

Revision history for this message

James Tunnicliffe (dooferlad) wrote on 2011-10-13:

#

On 7 October 2011 00:43, Guilherme Salgado <email address hidden> wrote:
> On Thu, 2011-10-06 at 17:31 +0000, James Tunnicliffe wrote:

>> How about:
>> * When creating a project, you can enter an SSH key and user
>> * Once a project is created...
>> - If it is private, you can change the SSH key (current interface)
>> - If it is public, you can't change the SSH key
>> * The admin interface always allows you to change the SSH key
>
> Sounds good to me, and maybe we can even allow editing ssh keys of
> public projects... Changing the ssh key of an existing project won't
> give an attacker access to anything they don't already have access to
> anyway.

OK, so I think if we are going to do this, we should merge in my
current changes then add in logic using your private projects stuff to
allow the customisation of the project interface. As it is, the pages
are safe (SSH key never shown) and with the private project controls
we can disable editing SSH credentials for public projects.

Having reviewed your code I see that you have the concept of a project
owner, which seems to be the person who created the project. Perhaps
we can always give the project owner the ability to change SSH keys
for public projects?

I am not entirely sure if we need to worry about locking down the
ability to change SSH keys and users for public projects though -
anyone with access to the Offspring instance can already come along
and mess up a project's settings after all. The only difference with
SSH keys is it may be more difficult to revert any changes because the
private key may be a more closely guarded secret than the rather
public config branch URL and other settings.

--
James Tunnicliffe

Revision history for this message

Guilherme Salgado (salgado) wrote on 2011-10-13:

#

On Thu, 2011-10-13 at 13:01 +0000, James Tunnicliffe wrote:
> On 7 October 2011 00:43, Guilherme Salgado <email address hidden> wrote:
> > On Thu, 2011-10-06 at 17:31 +0000, James Tunnicliffe wrote:
>
> >> How about:
> >> * When creating a project, you can enter an SSH key and user
> >> * Once a project is created...
> >> - If it is private, you can change the SSH key (current interface)
> >> - If it is public, you can't change the SSH key
> >> * The admin interface always allows you to change the SSH key
> >
> > Sounds good to me, and maybe we can even allow editing ssh keys of
> > public projects... Changing the ssh key of an existing project won't
> > give an attacker access to anything they don't already have access to
> > anyway.
>
> OK, so I think if we are going to do this, we should merge in my
> current changes then add in logic using your private projects stuff to
> allow the customisation of the project interface. As it is, the pages
> are safe (SSH key never shown) and with the private project controls
> we can disable editing SSH credentials for public projects.

We don't necessarily need to merge your changes into trunk -- you could
just branch off of my private-projects branch, merge your changes and
then work on the new stuff. That way we never need to block on some
changes landing into trunk.

It is important that you start a new branch for those extra changes
because then we can discuss and review them separately. In fact, it'd
have been nice if you'd used a separate branch for the UI changes you
already have on this branch. ;)

> Having reviewed your code I see that you have the concept of a project
> owner, which seems to be the person who created the project. Perhaps

Right, that's necessary because otherwise it'd be more complicated to
ensure there's always someone who can edit the ACLs of the project.

> we can always give the project owner the ability to change SSH keys
> for public projects?

I'm not sure we need to restrict it only to the project owner (if that's
what you're suggesting?), because there's not any real damage one can do
by changing the SSH key of a project. The only problem is if they were
allowed to see the existing ssh key, but that you've already taken care
of.

> I am not entirely sure if we need to worry about locking down the
> ability to change SSH keys and users for public projects though -
> anyone with access to the Offspring instance can already come along
> and mess up a project's settings after all. The only difference with

In fact it's only those users with the change_project permission that
can do that...

> SSH keys is it may be more difficult to revert any changes because the
> private key may be a more closely guarded secret than the rather
> public config branch URL and other settings.

Agreed.

I'll do another review because I haven't looked at the UI changes yet.

On Thu, 2011-10-13 at 13:01 +0000, James Tunnicliffe wrote:
> On 7 October 2011 00:43, Guilherme Salgado <guilherme.salgado@linaro.org> wrote:
> > On Thu, 2011-10-06 at 17:31 +0000, James Tunnicliffe wrote:
> 
> >> How about:
> >> * When creating a project, you can enter an SSH key and user
> >> * Once a project is created...
> >>   - If it is private, you can change the SSH key (current interface)
> >>   - If it is public, you can't change the SSH key
> >> * The admin interface always allows you to change the SSH key
> >
> > Sounds good to me, and maybe we can even allow editing ssh keys of
> > public projects... Changing the ssh key of an existing project won't
> > give an attacker access to anything they don't already have access to
> > anyway.
> 
> OK, so I think if we are going to do this, we should merge in my
> current changes then add in logic using your private projects stuff to
> allow the customisation of the project interface. As it is, the pages
> are safe (SSH key never shown) and with the private project controls
> we can disable editing SSH credentials for public projects.