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>