Merge ~jugmac00/lpci:pass-in-credentials into lpci:main

[Jürgen Gmach]

Add cli option to pass in a configuration file for credentials

[Colin Watson (cjwatson)]

I don't really like the fact that this can only allow for a single set of credentials; that seems like an interface smell. I'd also like us to think ahead a bit more: CI environments typically have some way to store secrets safely and pass them into build jobs, and isn't this just a particular kind of secret? It's true that we're under some time pressure, but I don't want that to cause us to write ourselves into a corner where the syntax of `.launchpad.yaml` is concerned, so let's take some time to think through this.

My suggestion on https://code.launchpad.net/~jugmac00/lpcraft/+git/lpcraft/+merge/425829 might help a certain amount here, because then we wouldn't be confined to purely textual substitutions, but we could parse the URL properly and insert the username and password into it using the normal `urllib.parse` functions.

But more broadly, I think we need support for passing in more than one kind of secret, and it would be wise for the actual text of the secrets not to be on the lpcraft command line (as written here it might require extra code in launchpad-buildd to redact it properly, and the command line is just a very leaky sort of place for secrets generally - it's normally better to pass them via files).

How about a `--secrets` option which takes the path to a YAML file, which would contain a mapping of names to scalar values? launchpad-buildd would then take the contents of that file as an XML-RPC parameter (encoded for safe transport over XML-RPC), write it out in such a way as to avoid it showing up in the build log, and pass that option to lpcraft.

For the configuration file syntax, again, we should think ahead to the point where we'll probably want to make secrets accessible to `run` (we can't effectively hide these particular credentials from `run` anyway, since it's running as root inside a container where we've written the credentials to `/etc/apt/sources.list` or similar). I'd prefer to avoid `<...>` for that, since that means something quite different in shell. A natural approach would be to use shell variables: for instance, imagine being able to configure a repository with `MY_SECRET=value`, and then you could evaluate `$MY_SECRET` in a `run` command. We don't have to get all the way there in this branch, but I'd like this to be compatible with that sort of approach in the future.

On the other hand, expanding `$`-prefixed shell variables in a URL is a bit weird. Fortunately, if we've thought ahead enough for the secrets to be a dict mapping keys to scalar values, we have some flexibility here. How about having a secret called something like `apt:canonical.example.org`, which would contain credentials applied to all `package-repositories` entries with that hostname? You'd then write something like this (following Snapcraft's syntax):

    package-repositories:
      - type: apt
        components: [main]
        suites: [focal]
        url: https://canonical.example.org/artifactory/jammy-golang-backport

lpcraft would parse the URL, notice that has the hostname `canonical.example.org` and that it has an `apt:{hostname}` secret matching that, and fill in the credentials.

What do you think? I'm not 100% we...

review: Needs Information

[Jürgen Gmach (jugmac00)]

I had a quick look what github does:

https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#example-using-secrets

They are using Jinja-like placeholders - the placeholders are defined separately per repository - this also seems a viable way.

About URL parsing... there is not much we need to do - as we use a pydantic validator, it automatically parses the URL and it should be straightforward to use something like `url.user = "username"; url.password = "secret"` to add the credentials to the URL object, and then just use `str(url)` to build it again.

I have this morning off but I'll explore these options, but I think we need to discuss this again before implementing the one or the other option.

[Colin Watson (cjwatson)]

review: Approve

[Colin Watson (cjwatson)]

Oh, the documentation should also explain that `package-repositories.url` is rendered using Jinja2.

[Jürgen Gmach (jugmac00)]

Thanks, Colin! That is a good idea.

diff --git a/NEWS.rst b/NEWS.rst
index df71226..da3bbc7 100644
--- a/NEWS.rst
+++ b/NEWS.rst
@@ -19,6 +19,8 @@ Version history
 - Rebuild the Snap package to include updated system packages.
   See https://ubuntu.com/security/notices/USN-5495-1/.
 
+- Add new CLI option to provide secrets via a YAML-based configuration file.
+
 0.0.17 (2022-06-17)
 ===================
 
diff --git a/docs/cli-interface.rst b/docs/cli-interface.rst
index 45a3914..0c44c3f 100644
--- a/docs/cli-interface.rst
+++ b/docs/cli-interface.rst
@@ -29,6 +29,17 @@ lpcraft run optional arguments
 - ``--plugin-setting``, e.g.
   ``lpcraft run --plugin-setting="foo=bar"``
 
+- ``--secrets``, e.g.
+  ``lpcraft run --secrets="<path-to-configuration-file>"``
+
+  The configuration file should look like...
+
+  .. code::
+
+    key: secret
+    another_key: another_secret
+
+
 lpcraft run-one
 ---------------
 
@@ -52,3 +63,13 @@ lpcraft run-one optional arguments
 
 - ``--plugin-setting``, e.g.
   ``lpcraft run-one --plugin-setting="foo=bar" test 0``.
+
+- ``--secrets``, e.g.
+  ``lpcraft run-one --secrets="<path-to-configuration-file>" test 0``.
+
+  The configuration file should look like...
+
+  .. code::
+
+    key: secret
+    another_key: another_secret
diff --git a/docs/configuration.rst b/docs/configuration.rst
index b453270..6e2b84a 100644
--- a/docs/configuration.rst
+++ b/docs/configuration.rst
@@ -167,5 +167,8 @@ More properties can be implemented on demand.
     One or several of ``bionic``, ``focal``, ``jammy``.
 
 ``url`` (required)
-    Specifies the URL of the package-repository.
-    e.g. ``http://ppa.launchpad.net/snappy-dev/snapcraft-daily/ubuntu``
+    Specifies the URL of the package-repository,
+    e.g. ``http://ppa.launchpad.net/snappy-dev/snapcraft-daily/ubuntu``.
+    The URL is rendered using `Jinja2 <https://pypi.org/project/Jinja2/>`_.
+    This can be used to supply authentication details via the *secrets*
+    command line option.
diff --git a/lpcraft/commands/run.py b/lpcraft/commands/run.py
index 757dfe0..c7128ae 100644
--- a/lpcraft/commands/run.py
+++ b/lpcraft/commands/run.py
@@ -12,10 +12,12 @@ from pathlib import Path, PurePath
 from tempfile import NamedTemporaryFile
 from typing import Dict, List, Optional, Set
 
+import yaml
 from craft_cli import BaseCommand, EmitterMode, emit
 from craft_providers import Executor, lxd
 from craft_providers.actions.snap_installer import install_from_store
 from dotenv import dotenv_values
+from jinja2 import BaseLoader, Environment
 from pluggy import PluginManager
 
 from lpcraft import env
@@ -229,6 +231,7 @@ def _install_apt_packages(
     apt_replacement_repositories: Optional[List[str]],
     additional_apt_repositories: Optional[List[PackageRepository]],
     environment: Optional[Dict[str, Optional[str]]],
+    secrets: Optional[Dict[str, str]],
 ) -> None:
     if apt_replacement_repositories or additional_apt_repositories:
         sources_list_path = "/etc/apt/sources.list"
@@ -247,9 +250,14 @@ def _install_apt_packages(
             sources = "\n".join(apt_replacement_repositories) + "\n"
         if additional_apt_repositories:
             for repository in additional_apt_repositories:
-                sources += (
-                    "\n" + "\n".join(repository.sources_list_lines()) + "\n"
+                sources += "\n" + "\n".join(repository.sources_list_lines())
+            if secrets:
+                template = Environment(loader=BaseLoader()).from_string(
+                    sources
                 )
+                sources = template.render(**secrets)
+            sources += "\n"
+
         with emit.open_stream("Replacing /etc/apt/sources.list") as stream:
             instance.push_file_io(
                 destination=PurePath(sources_list_path),
@@ -339,6 +347,7 @@ def _run_job(
     additional_apt_repositories: Optional[List[PackageRepository]] = None,
     env_from_cli: Optional[List[str]] = None,
     plugin_settings: Optional[List[str]] = None,
+    secrets: Optional[Dict[str, str]] = None,
 ) -> None:
     """Run a single job."""
     # XXX jugmac00 2022-04-27: we should create a configuration object to be
@@ -429,6 +438,7 @@ def _run_job(
                 apt_replacement_repositories=apt_replacement_repositories,
                 additional_apt_repositories=additional_apt_repositories,
                 environment=environment,
+                secrets=secrets,
             )
         for cmd in (pre_run_command, run_command, post_run_command):
             if cmd:
@@ -509,6 +519,14 @@ class RunCommand(BaseCommand):
             action="append",
             help="Set an environment variable.",
         )
+        parser.add_argument(
+            "--secrets",
+            dest="secrets_file",
+            metavar="file",
+            default=None,
+            type=Path,
+            help="Pass in a YAML-based configuration file for secrets.",
+        )
 
     def run(self, args: Namespace) -> int:
         """Run the command."""
@@ -518,6 +536,12 @@ class RunCommand(BaseCommand):
         provider.ensure_provider_is_available()
         launched_instances = []
 
+        secrets = {}
+        if args.secrets_file:
+            with open(args.secrets_file) as f:
+                content = f.read()
+            secrets = yaml.safe_load(content)
+
         try:
             for stage in config.pipeline:
                 stage_failed = False
@@ -543,6 +567,7 @@ class RunCommand(BaseCommand):
                                 additional_apt_repositories=job.package_repositories,  # noqa: E501
                                 env_from_cli=args.set_env,
                                 plugin_settings=args.plugin_setting,
+                                secrets=secrets,
                             )
                     except CommandError as e:
                         if len(stage) == 1:
@@ -624,6 +649,14 @@ class RunOneCommand(BaseCommand):
             action="append",
             help="Set an environment variable.",
         )
+        parser.add_argument(
+            "--secrets",
+            dest="secrets_file",
+            metavar="file",
+            default=None,
+            type=Path,
+            help="Pass in a YAML-based configuration file for secrets.",
+        )
 
     def run(self, args: Namespace) -> int:
         """Run the command."""
@@ -641,6 +674,11 @@ class RunOneCommand(BaseCommand):
         provider = get_provider()
         provider.ensure_provider_is_available()
 
+        secrets = {}
+        if args.secrets_file:
+            with open(args.secrets_file) as f:
+                content = f.read()
+            secrets = yaml.safe_load(content)
         try:
             _run_job(
                 args.job,
@@ -651,6 +689,7 @@ class RunOneCommand(BaseCommand):
                 additional_apt_repositories=job.package_repositories,
                 env_from_cli=args.set_env,
                 plugin_settings=args.plugin_setting,
+                secrets=secrets,
             )
         finally:
             if args.clean:
diff --git a/lpcraft/commands/tests/test_run.py b/lpcraft/commands/tests/test_run.py
index 0da675a..ddb521b 100644
--- a/lpcraft/commands/tests/test_run.py
+++ b/lpcraft/commands/tests/test_run.py
@@ -2056,6 +2056,101 @@ class TestRun(RunBaseTestCase):
             file_contents,
         )
 
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_run_with_additional_apt_repositories_with_secrets(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        existing_repositories = [
+            "deb http://archive.ubuntu.com/ubuntu/ focal main restricted",
+            "deb-src http://archive.ubuntu.com/ubuntu/ focal main restricted",
+        ]
+
+        def fake_pull_file(source: Path, destination: Path) -> None:
+            destination.write_text("\n".join(existing_repositories))
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.return_value = subprocess.CompletedProcess([], 0)
+        launcher.return_value.pull_file.side_effect = fake_pull_file
+
+        config = dedent(
+            """
+            pipeline:
+                - test
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: ls -la
+                    packages: [git]
+                    package-repositories:
+                        - type: apt
+                          formats: [deb]
+                          components: [main, universe]
+                          suites: [focal]
+                          url: "https://{{auth}}@canonical.example.org/artifactory/jammy-golang-backport"
+            """  # noqa: E501
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        credentials = dedent('auth: "user:pass"')
+        Path(".launchpad-secrets.yaml").write_text(credentials)
+
+        result = self.run_command(
+            "run",
+            "--secrets",
+            ".launchpad-secrets.yaml",
+        )
+
+        self.assertEqual(0, result.exit_code)
+        self.assertEqual(
+            [
+                call(
+                    ["apt", "update"],
+                    cwd=Path("/root/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+                call(
+                    ["apt", "install", "-y", "git"],
+                    cwd=Path("/root/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+                call(
+                    ["bash", "--noprofile", "--norc", "-ec", "ls -la"],
+                    cwd=Path("/root/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+            ],
+            execute_run.call_args_list,
+        )
+        mock_info = launcher.return_value.push_file_io.call_args_list
+
+        self.assertEqual(
+            Path("/etc/apt/sources.list"), mock_info[0][1]["destination"]
+        )
+
+        file_contents = mock_info[0][1]["content"].read().decode()
+
+        self.assertEqual(
+            dedent(
+                """\
+            deb http://archive.ubuntu.com/ubuntu/ focal main restricted
+            deb-src http://archive.ubuntu.com/ubuntu/ focal main restricted
+            deb https://user:[e-mail]/artifactory/jammy-golang-backport focal main universe
+            """  # noqa: E501
+            ),
+            file_contents,
+        )
+
 
 class TestRunOne(RunBaseTestCase):
     def test_config_file_not_under_project_directory(self):
@@ -2804,3 +2899,138 @@ class TestRunOne(RunBaseTestCase):
                 exit_code=1, errors=[CommandError("File not found", retcode=1)]
             ),
         )
+
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_provide_secrets_file_via_cli(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.return_value = subprocess.CompletedProcess([], 0)
+        config = dedent(
+            """
+            pipeline:
+                - test
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: tox
+            """
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        credentials = dedent('auth: "user:pass"')
+        Path(".launchpad-secrets.yaml").write_text(credentials)
+
+        result = self.run_command(
+            "run-one",
+            "--secrets",
+            ".launchpad-secrets.yaml",
+            "test",
+            "0",
+        )
+        self.assertEqual(0, result.exit_code)
+        self.assertIn(
+            "'--secrets', '.launchpad-secrets.yaml'", result.trace[0]
+        )
+
+    @patch("lpcraft.commands.run.get_provider")
+    @patch("lpcraft.commands.run.get_host_architecture", return_value="amd64")
+    def test_run_with_additional_apt_repositories_with_secrets(
+        self, mock_get_host_architecture, mock_get_provider
+    ):
+        existing_repositories = [
+            "deb http://archive.ubuntu.com/ubuntu/ focal main restricted",
+            "deb-src http://archive.ubuntu.com/ubuntu/ focal main restricted",
+        ]
+
+        def fake_pull_file(source: Path, destination: Path) -> None:
+            destination.write_text("\n".join(existing_repositories))
+
+        launcher = Mock(spec=launch)
+        provider = makeLXDProvider(lxd_launcher=launcher)
+        mock_get_provider.return_value = provider
+        execute_run = launcher.return_value.execute_run
+        execute_run.return_value = subprocess.CompletedProcess([], 0)
+        launcher.return_value.pull_file.side_effect = fake_pull_file
+
+        config = dedent(
+            """
+            pipeline:
+                - test
+            jobs:
+                test:
+                    series: focal
+                    architectures: amd64
+                    run: ls -la
+                    packages: [git]
+                    package-repositories:
+                        - type: apt
+                          formats: [deb]
+                          components: [main, universe]
+                          suites: [focal]
+                          url: https://canonical.example.org/artifactory/jammy-golang-backport
+            """  # noqa: E501
+        )
+        Path(".launchpad.yaml").write_text(config)
+
+        credentials = dedent('auth: "user:pass"')
+        Path(".launchpad-secrets.yaml").write_text(credentials)
+
+        result = self.run_command(
+            "run-one",
+            "--secrets",
+            ".launchpad-secrets.yaml",
+            "test",
+            "0",
+        )
+
+        self.assertEqual(0, result.exit_code)
+        self.assertEqual(
+            [
+                call(
+                    ["apt", "update"],
+                    cwd=Path("/root/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+                call(
+                    ["apt", "install", "-y", "git"],
+                    cwd=Path("/root/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+                call(
+                    ["bash", "--noprofile", "--norc", "-ec", "ls -la"],
+                    cwd=Path("/root/lpcraft/project"),
+                    env={},
+                    stdout=ANY,
+                    stderr=ANY,
+                ),
+            ],
+            execute_run.call_args_list,
+        )
+        mock_info = launcher.return_value.push_file_io.call_args_list
+
+        self.assertEqual(
+            Path("/etc/apt/sources.list"), mock_info[0][1]["destination"]
+        )
+
+        file_contents = mock_info[0][1]["content"].read().decode()
+
+        self.assertEqual(
+            dedent(
+                """\
+                deb http://archive.ubuntu.com/ubuntu/ focal main restricted
+                deb-src http://archive.ubuntu.com/ubuntu/ focal main restricted
+                deb https://canonical.example.org/artifactory/jammy-golang-backport focal main universe
+                """  # noqa: E501
+            ),
+            file_contents,
+        )
diff --git a/setup.cfg b/setup.cfg
index 11d2a98..69db240 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -25,6 +25,7 @@ install_requires =
     PyYAML
     craft-cli
     craft-providers
+    jinja2
     pluggy
     pydantic
     python-dotenv
diff --git a/tox.ini b/tox.ini
index 71f524e..1d9be16 100644
--- a/tox.ini
+++ b/tox.ini
@@ -6,6 +6,8 @@ envlist =
     py39
     py310
     coverage
+    docs
+
 skip_missing_interpreters =
     true