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.