#211 [rpmbuild] allow only https and ftps protocols for source fetch
Merged 6 years ago by clime. Opened 6 years ago by clime.

@@ -137,12 +137,17 @@ 

      return package_name

  

  

+ def string2list(string):

+     return [elem.strip() for elem in re.split(r"\s*,\s*|\s+", string) if elem]

+ 

+ 

  def read_config(config_path=None):

      config = configparser.RawConfigParser(defaults={

          "resultdir": "/var/lib/copr-rpmbuild/results",

          "lockfile": "/var/lib/copr-rpmbuild/lockfile",

          "logfile": "/var/lib/copr-rpmbuild/main.log",

          "pidfile": "/var/lib/copr-rpmbuild/pid",

+         "enabled_source_protocols": "https ftps",

      })

      config_paths = [os.path.join(path, "main.ini") for path in CONF_DIRS]

      config.read(config_path or reversed(config_paths))

@@ -1,6 +1,7 @@ 

  import os

  import logging

  import tempfile

+ from ..helpers import string2list

  

  log = logging.getLogger("__main__")

  
@@ -19,7 +20,10 @@ 

      def create_rpmmacros(self):

          path = os.path.join(self.workdir, ".rpmmacros")

          with open(path, "w") as rpmmacros:

-             rpmmacros.write("%_disable_source_fetch 0")

+             rpmmacros.write("%_disable_source_fetch 0\n")

+             enabled_protocols = string2list(self.config.get("main", "enabled_source_protocols"))

+             rpmmacros.write("%__urlhelper_localopts --proto -all,{}\n"

+                             .format(','.join(["+"+protocol for protocol in enabled_protocols])))

  

      def __exit__(self, exc_type, exc_value, traceback):

          self.cleanup()

file modified
+1
@@ -1,5 +1,6 @@ 

  [main]

  frontend_url = https://copr.fedoraproject.org

+ enabled_source_protocols = https ftps

  

  [distgit0]

  distgit_hostname_pattern = src.fedoraproject.org

@@ -0,0 +1,41 @@ 

+ import os

+ import tempfile

+ import unittest

+ 

+ from ..copr_rpmbuild import helpers

+ 

+ CONFIG = """

+ [main]

+ frontend_url = https://copr.fedoraproject.org

+ resultdir = /var/lib/copr-rpmbuild/results

+ enabled_source_protocols = https ftps

+ 

+ [distgit0]

+ distgit_hostname_pattern = src.fedoraproject.org

+ distgit_lookaside_url = https://src.fedoraproject.org/repo/pkgs

+ distgit_clone_url = https://src.fedoraproject.org

+ 

+ [distgit1]

+ distgit_hostname_pattern = copr-dist-git.fedorainfracloud.org

+ distgit_lookaside_url = http://copr-dist-git.fedorainfracloud.org/repo/pkgs

+ distgit_clone_url = http://copr-dist-git.fedorainfracloud.org/git

+ 

+ [distgit2]

+ distgit_hostname_pattern = pkgs.fedoraproject.org

+ distgit_lookaside_url = https://src.fedoraproject.org/repo/pkgs

+ distgit_clone_url = git://pkgs.fedoraproject.org

+ """

+ 

+ class TestCase(unittest.TestCase):

+     def setUp(self):

+         self.config_path, self.config = self.read_config_data(CONFIG)

+ 

+     def tearDown(self):

+         os.unlink(self.config_path)

+ 

+     def read_config_data(self, config_data):

+         fd, config_path = tempfile.mkstemp()

+         f = open(fd, 'w')

+         f.write(config_data)

+         f.close()

+         return config_path, helpers.read_config(config_path)

@@ -0,0 +1,27 @@ 

+ import mock

+ from mock import MagicMock

+ from mock import call

+ 

+ from ..copr_rpmbuild.providers.base import Provider

+ from . import TestCase

+ 

+ 

+ class TestProvider(TestCase):

+     def setUp(self):

+         super(TestProvider, self).setUp()

+         self.source_json = {}

+         self.resultdir = "/path/to/resultdir"

+ 

+     @mock.patch('builtins.open', new_callable=mock.mock_open())

+     def test_create_rpmmacros(self, mock_open):

+         provider = Provider(self.source_json, self.resultdir, self.config)

+         rpmmacros = MagicMock()

+         mock_open.return_value = rpmmacros

+         provider.create_rpmmacros()

+         mock_open.assert_called_with("{}/.rpmmacros".format(provider.workdir), "w")

+         calls = [

+             call.__enter__().write('%_disable_source_fetch 0\n'),

+             call.__enter__().write('%__urlhelper_localopts --proto -all,+https,+ftps\n'),

+         ]

+         rpmmacros.assert_has_calls(calls, any_order=True)

+ 

@@ -0,0 +1,10 @@ 

+ import unittest

+ from ..copr_rpmbuild.helpers import string2list

+ 

+ class TestHelpers(unittest.TestCase):

+     def test_string2list(self):

+         self.assertEqual(string2list('foo bar baz'), ['foo', 'bar', 'baz'])

+         self.assertEqual(string2list('foo,bar,baz'), ['foo', 'bar', 'baz'])

+         self.assertEqual(string2list('  foo bar\nbaz,'), ['foo', 'bar', 'baz'])

+         self.assertEqual(string2list(',,foo, \nbar\tbaz,,'), ['foo', 'bar', 'baz'])

+         self.assertEqual(string2list(',,foo\tbar\tbaz'), ['foo', 'bar', 'baz'])

file modified
+5 -3
@@ -1,17 +1,19 @@ 

  import mock

  import unittest

  from ..copr_rpmbuild.providers.pypi import PyPIProvider

+ from . import TestCase

  

  

- class TestPyPIProvider(unittest.TestCase):

+ class TestPyPIProvider(TestCase):

      def setUp(self):

+         super(TestPyPIProvider, self).setUp()

          self.source_json = {"pypi_package_version": "1.52",

                              "pypi_package_name": "motionpaint",

                              "python_versions": [2, 3]}

          self.resultdir = "/path/to/resultdir"

  

      def test_init(self):

-        provider = PyPIProvider(self.source_json, self.resultdir)

+        provider = PyPIProvider(self.source_json, self.resultdir, self.config)

         self.assertEqual(provider.pypi_package_version, "1.52")

         self.assertEqual(provider.pypi_package_name, "motionpaint")

         self.assertEqual(provider.python_versions, [2, 3])
@@ -19,7 +21,7 @@ 

      @mock.patch("rpmbuild.copr_rpmbuild.providers.pypi.run_cmd")

      @mock.patch("builtins.open")

      def test_produce_srpm(self, mock_open, run_cmd):

-         provider = PyPIProvider(self.source_json, outdir="/some/tmp/directory")

+         provider = PyPIProvider(self.source_json, "/some/tmp/directory", self.config)

          provider.produce_srpm()

          assert_cmd = ["pyp2rpm", "motionpaint", "--srpm", "-d", "/some/tmp/directory",

                        "-b", "2", "-p", "3", "-v", "1.52"]

@@ -2,21 +2,23 @@ 

  import unittest

  from munch import Munch

  from ..copr_rpmbuild.providers.rubygems import RubyGemsProvider

+ from . import TestCase

  

  

- class TestRubyGemsProvider(unittest.TestCase):

+ class TestRubyGemsProvider(TestCase):

      def setUp(self):

+         super(TestRubyGemsProvider, self).setUp()

          self.source_json = {"gem_name": "A_123"}

          self.resultdir = "/path/to/resultdir"

  

      def test_init(self):

-         provider = RubyGemsProvider(self.source_json, self.resultdir)

+         provider = RubyGemsProvider(self.source_json, self.resultdir, self.config)

          self.assertEqual(provider.gem_name, "A_123")

  

      @mock.patch("rpmbuild.copr_rpmbuild.providers.rubygems.run_cmd")

      @mock.patch("builtins.open")

      def test_produce_srpm(self, mock_open, run_cmd):

-         provider = RubyGemsProvider(self.source_json, self.resultdir)

+         provider = RubyGemsProvider(self.source_json, self.resultdir, self.config)

          provider.produce_srpm()

          assert_cmd = ["gem2rpm", "A_123", "--srpm", "-C", "/path/to/resultdir", "--fetch"]

          run_cmd.assert_called_with(assert_cmd)
@@ -28,7 +30,7 @@ 

                    "Command failed: rpmbuild -bs --nodeps --define '_sourcedir /tmp/gem2rpm-foo-20170905-3367-c2flks'"

                    "--define '_srcrpmdir .' /tmp/gem2rpm-foo-20170905-3367-c2flks/rubygem-foo.spec")

          run_cmd.return_value = Munch({"stderr": stderr})

-         provider = RubyGemsProvider(self.source_json, self.resultdir)

+         provider = RubyGemsProvider(self.source_json, self.resultdir, self.config)

          with self.assertRaises(RuntimeError) as ex:

              provider.produce_srpm()

          self.assertIn("Not specifying a license means all rights are reserved", str(ex.exception))

file modified
+3 -27
@@ -5,38 +5,19 @@ 

  import configparser

  from ..copr_rpmbuild.providers.scm import ScmProvider

  from ..copr_rpmbuild.helpers import read_config

+ from . import TestCase

  

  from mock import patch, MagicMock

  

- CONFIG = """

- [main]

- frontend_url = https://copr.fedoraproject.org

- resultdir = /var/lib/copr-rpmbuild/results

- 

- [distgit0]

- distgit_hostname_pattern = src.fedoraproject.org

- distgit_lookaside_url = https://src.fedoraproject.org/repo/pkgs

- distgit_clone_url = https://src.fedoraproject.org

- 

- [distgit1]

- distgit_hostname_pattern = copr-dist-git.fedorainfracloud.org

- distgit_lookaside_url = http://copr-dist-git.fedorainfracloud.org/repo/pkgs

- distgit_clone_url = http://copr-dist-git.fedorainfracloud.org/git

- 

- [distgit2]

- distgit_hostname_pattern = pkgs.fedoraproject.org

- distgit_lookaside_url = https://src.fedoraproject.org/repo/pkgs

- distgit_clone_url = git://pkgs.fedoraproject.org

- """

- 

  RPKG_CONF_JINJA = """

  [rpkg]

  lookaside = {{ lookaside_url }}

  anongiturl = {{ clone_url }}/%(module)s

  """

  

- class TestScmProvider(unittest.TestCase):

+ class TestScmProvider(TestCase):

      def setUp(self):

+         super(TestScmProvider, self).setUp()

          self.source_json = {

              "type": "git",

              "clone_url": "https://example.org/somerepo.git",
@@ -46,11 +27,6 @@ 

              "srpm_build_method": "rpkg",

          }

          self.resultdir = "/path/to/resultdir"

-         fd, config_path = tempfile.mkstemp()

-         f = open(fd, 'w')

-         f.write(CONFIG)

-         f.close()

-         self.config = read_config(config_path)

  

      def test_init(self):

          source_json = self.source_json.copy()

file modified
+6 -10
@@ -2,36 +2,32 @@ 

  import unittest

  import configparser

  from ..copr_rpmbuild.providers.spec import SpecUrlProvider

+ from . import TestCase

  

  

- class TestSpecUrlProvider(unittest.TestCase):

+ class TestSpecUrlProvider(TestCase):

      def setUp(self):

+         super(TestSpecUrlProvider, self).setUp()

          self.source_json = {"url": u"http://foo.ex/somepackage.spec"}

          self.resultdir = "/path/to/resultdir"

  

      def test_init(self):

-         provider = SpecUrlProvider(self.source_json, self.resultdir)

+         provider = SpecUrlProvider(self.source_json, self.resultdir, self.config)

          self.assertEqual(provider.url, "http://foo.ex/somepackage.spec")

  

      @mock.patch('requests.get')

      @mock.patch("rpmbuild.copr_rpmbuild.providers.spec.run_cmd")

      @mock.patch('builtins.open', new_callable=mock.mock_open())

      def test_produce_srpm(self, mock_open, run_cmd, mock_get):

-         provider = SpecUrlProvider(self.source_json, self.resultdir)

+         provider = SpecUrlProvider(self.source_json, self.resultdir, self.config)

          provider.produce_srpm()

          run_cmd.assert_called_with(["rpkg", "srpm", "--outdir", self.resultdir,

                                      "--spec", '{}/somepackage.spec'.format(provider.workdir)],

                                     cwd=provider.workdir)

  

-     @mock.patch('builtins.open', new_callable=mock.mock_open())

-     def test_create_rpmmacros(self, mock_open):

-         provider = SpecUrlProvider(self.source_json, self.resultdir)

-         provider.create_rpmmacros()

-         mock_open.assert_called_with("{}/.rpmmacros".format(provider.workdir), "w")

- 

      @mock.patch('requests.get')

      @mock.patch('builtins.open', new_callable=mock.mock_open())

      def test_save_spec(self, mock_open, mock_get):

-         provider = SpecUrlProvider(self.source_json, self.resultdir)

+         provider = SpecUrlProvider(self.source_json, self.resultdir, self.config)

          provider.save_spec()

          mock_open.assert_called_with("{}/somepackage.spec".format(provider.workdir), "w")

Ad. https://bugzilla.redhat.com/show_bug.cgi?id=1536846.

Currently users are open to "source attacks" (e.g. man-in-the-middle) if they are using insecure protocols for Source directive in spec file. This PR prevents using insecure protocols by default by limiting curl protocols (the tool that is used by rpmbuild for downloading sources) to "https" and "ftps". This is just a default setting. Other COPR deployments may allow usage of other protocols if they want to.

Other COPR deployments may allow usage of other protocols if they want to.

Seems like users can affect the behavior themselves, by defining appropriate macro..? In that case, it is not that important what we pick as default per-copr-instance. Looks good otherwise.

Seems like users can affect the behavior themselves, by defining appropriate macro..? In that case, it is not that important what we pick as default per-copr-instance. Looks good otherwise.

It's quite important, I think. It's exactly for the same reason, browsers started to forbid http://. For protecting users themselves.

Hms, looking again - setting for every provider the _disable_source_fetch 0 is a bad default (I did not notice this before). This should be used only for the methods which need that, for limited period of time... and only by --define, touching .rpmmacros is not really nice.

As always, if you force the tooling to generate some configuration file -- it just complicates reproducing for the end-user. Instead of re-trying particular command with different options, you need to re-create the config file all the time...

Hms, looking again - setting for every provider the _disable_source_fetch 0 is a bad default (I did not notice this before). This should be used only for the methods which need that, for limited period of time... and only by --define, touching .rpmmacros is not really nice.
As always, if you force the tooling to generate some configuration file -- it just complicates reproducing for the end-user. Instead of re-trying particular command with different options, you need to re-create the config file all the time...

Ye, I agree that this would be better placed on a command line.

Hms, looking again - setting for every provider the _disable_source_fetch 0 is a bad default (I did not notice this before). This should be used only for the methods which need that, for limited period of time... and only by --define, touching .rpmmacros is not really nice.

Actually, all the providers that run directly on host (potentially) need that: spec, pypi, gem, scm => those use this settings. (e.g. scm might not needed but also might needed depending on the provided input). And those methods that run inside a mock chroot (Custom, make_srpm) do not care.

Btw. if it is inappropriate in your deployment to download external sources, we might offer a copr-rpmbuild option to disable that completely (or you can set "enabled_source_protocols" to an empty string).

-1 while it is quite easy to setup https server nowdays, it is still not uncommon to use https. if someone needs to use it, then it is his choice (e.g. think about our dev machine).

-1 while it is quite easy to setup https server nowdays, it is still not uncommon to use https. if someone needs to use it, then it is his choice (e.g. think about our dev machine).

Not sure how it is related to our dev machines. While at least some are on http://, we don't download sources (by using Source0) from there. This pretty clear case where we should improve protection.

That sometimes person does not care about security. The fact that build pass is enough (as consideration for PR merge etc.).

That sometimes person does not care about security. The fact that build pass is enough (as consideration for PR merge etc.).

Allowing http in spec file basically means that we don't care as COPR about security, which is not a good message for users. This PR was really just waiting for unit tests. Any discussion whether this is correct or not seems to be totally ridiculous to me (we actually should have had this setting from the beginning).

Only allowing HTTPS/FTPS is an improvement over the status quo, where Copr makes it super easy to create insecure packages, by default. Perhaps it's just me, but such defaults likely hurt the reputation of the Copr service, as a whole.

Ideally, I would like to see a .spec-file-format extension where it's possible to specify a cryptographic checksum next to the source url (cf. https://bugzilla.redhat.com/show_bug.cgi?id=1536846#c8).

With such an extension the following default logic would be fair and secure:

  1. If source URL is protected by a checksum then both secure (e.g. HTTPS) and insecure (e.g. HTTP) protocols are allowed for auto-downloading.
  2. If source URL isn't protected by a checksum then only HTTPS is possible.

2 new commits added

  • [rpmbuild] add test for create_rpmmacros + refactoring
  • [rpmbuild] fix unittests
6 years ago

Only allowing HTTPS/FTPS is an improvement over the status quo, where Copr makes it super easy to create insecure packages, by default. Perhaps it's just me, but such defaults likely hurt the reputation of the Copr service, as a whole.
Ideally, I would like to see a .spec-file-format extension where it's possible to specify a cryptographic checksum next to the source url (cf. https://bugzilla.redhat.com/show_bug.cgi?id=1536846#c8).

If you want this functionality, you can open an issue at https://github.com/rpm-software-management/rpm.

Pull-Request has been merged by clime

6 years ago

Only allowing HTTPS/FTPS is an improvement over the status quo, where Copr makes it super easy to create insecure packages, by default. Perhaps it's just me, but such defaults likely hurt the reputation of the Copr service, as a whole.

The fact that we allow http:// urls in Source statements doesn't affect reputation of copr (yes, youre report and this PR represents your POV). If you really cared about security, you would request us to drop _disable_source_fetch 0 hack. -1 for this PR, +1 for fixing it properly.

+1 for fixing it properly.

By this I mean making this opt-in, per-package/per-build (or drop entirely, since this feature ever was opt-in). The fact that .rpmmacros file is created globally and affects all the builds out there and all methods is not good.