#51 Add XML signing support
Merged 3 years ago by abompard. Opened 4 years ago by demiobenour.
demiobenour/robosignatory master  into  master

Add XML signing support
Demi Marie Obenour • 3 years ago  
Fix typo in atomic signer
Demi Marie Obenour • 3 years ago  
Add some files to .gitignore
Demi Marie Obenour • 3 years ago  
file modified
+5
@@ -5,3 +5,8 @@ 

  .coverage

  coverage.xml

  htmlcov/

+ .*.sw[po]

+ .#*

+ \#*#

+ /build

+ /dist

Can you split this into its own commit?

file modified
+1
@@ -20,6 +20,7 @@ 

      "org.fedoraproject.*.pungi.compose.ostree",

      "org.fedoraproject.*.coreos.build.request.artifacts-sign",

      "org.fedoraproject.*.coreos.build.request.ostree-sign",

+     "org.fedoraproject.*.robosignatory.xml-sign",

      "org.fedoraproject.*.buildsys.tag",

  ]

  

file modified
+1 -1
@@ -16,7 +16,7 @@ 

          self.refs = {}

          for ref, val in self.config['ostree_refs'].items():

              if 'ref_to' in val:

-                 raise ValueError('ref_to in %s found. This is depricated' %

+                 raise ValueError('ref_to in %s found. This is deprecated' %

Can you split this out into its own commit?

                                   ref)

              self.refs[ref] = val

  

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

  from .tag import TagSigner

  from .atomic import AtomicSigner

  from .coreos import CoreOSSigner

+ from .xml import XMLSigner

  

  

  log = logging.getLogger('robosignatory')
@@ -21,6 +22,7 @@ 

          self.tag_handler = TagSigner(self.config)

          self.atomic_handler = AtomicSigner(self.config)

          self.coreos_handler = CoreOSSigner(self.config)

+         self.xml_handler = XMLSigner(self.config)

  

      def __call__(self, msg):

          """
@@ -49,6 +51,9 @@ 

                    or msg.topic.endswith('.coreos.build.request.ostree-sign')):

                  log.debug('Passing message to the CoreOS handler')

                  self.coreos_handler.consume(msg)

+             elif msg.topic.endswith('.robosignatory.xml-sign'):

+                 log.debug('Passing message to the Text handler')

+                 self.xml_handler.consume(msg)

          except Exception as e:

              error_msg = '{e}: Unable to handle message: {msg}'.format(e=e, msg=msg)

              log.exception(error_msg)

file modified
+18 -4
@@ -90,6 +90,10 @@ 

      def build_coreos_cmdline(self, *args):

          pass

  

+     @abc.abstractmethod

+     def build_xml_cmdline(self, *args):

+         pass

+ 

  

  class EchoHelper(BaseSigningHelper):

      """ A dummy "hello world" helper, used for debugging. """
@@ -116,6 +120,11 @@ 

          log.info(result)

          return result

  

+     def build_xml_cmdline(self, *args, **kwargs):

+         result = ['echo', ' '.join(['build_xml_cmdline:', str(args), str(kwargs)])]

+         log.info(result)

+         return result

+ 

  

  class SigulHelper(BaseSigningHelper):

      def __init__(self, user, passphrase_file, config_file=None,
@@ -164,11 +173,16 @@ 

          return command + rpms

  

      def build_atomic_cmdline(self, key, checksum, input_file, output_file):

-         command = self.build_cmdline('sign-ostree', key, checksum, input_file,

-                                      '--output', output_file)

+         command = self.build_cmdline('sign-ostree', '--output', output_file,

+                                      '--', key, checksum, input_file)

          return command

  

      def build_coreos_cmdline(self, key, input_file, output_file):

-         command = self.build_cmdline('sign-data', key, input_file,

-                                      '--output', output_file)

+         command = self.build_cmdline('sign-data', '--output', output_file, '--',

+                                      key, input_file)

+         return command

+ 

+     def build_xml_cmdline(self, key, input_file, output_file):

+         command = self.build_cmdline('sign-data', '--output', output_file, '--',

+                                      key, input_file)

          return command

file added
+87
@@ -0,0 +1,87 @@ 

+ from __future__ import unicode_literals, absolute_import

+ 

+ import os

+ import stat

+ import logging

+ import shutil

+ import tempfile

+ 

+ import robosignatory.utils as utils

+ from fedora_messaging.api import Message, publish

+ 

+ 

+ log = logging.getLogger(__name__)

+ 

+ class XMLSigner(object):

+ 

+     __slots__ = ('_signer', '_tmpdir', '_key')

+ 

+     def __init__(self, config):

+         self._signer = utils.get_signing_helper(**config['signing'])

+         self._tmpdir = '/tmp'

+         for k, v in config['xml'].items():

+             if k == 'tmpdir' and type(v) is str:

+                 self._tmpdir = v

+             elif k == 'key' and type(v) is str:

+                 self._key = v

+             else:

+                 raise Exception('Bad config entry {!r}'.format((k, v)))

+         if not hasattr(self, '_key'):

+             raise Exception('Key must be specified')

+         log.info('XMLSigner ready for service')

+ 

+     def _sign_object(self, msg, tmpdir):

+         input_file = os.path.join(tmpdir, 'input')

+         output_file = os.path.join(tmpdir, 'output')

+         with open(input_file, 'wb') as f:

+             f.write(msg.encode('ascii', 'strict'))

+         cmdline = self._signer.build_xml_cmdline(self._key, input_file, output_file)

+         ret, stdout, stderr = utils.run_command(cmdline)

+         if ret != 0:

+             raise Exception('Error signing! Signing output: %s, stdout: '

+                       '%r, stderr: %r' % (ret, stdout, stderr))

+         with open(output_file, 'rb') as f:

+             signature = f.read().decode('ascii', 'strict')

+         log.info('XML file was successfully signed')

+         return signature

+ 

+     def _error(self, msg, error):

+         publish(Message(

+             topic="{}.finished".format(msg.topic),

+             # respond with the same body

+             body={'body': msg.body, 'error': str(error)}

+         ))

+ 

+ 

+     def consume(self, msg):

+         log.info('Punji and/or bodhi wants to sign an XML file')

+ 

+         if type(msg.body) is not str:

+             return self._error(msg, 'Refusing to sign non-string')

+         # Security check: we must use the same key for signing packages and

+         # metadata, but being authorized to sign metadata does not necessarily

+         # grant authorization to sign packages.  This check enforces this.

+         #

+         # RPM packages have two possible signatures: one over just the main

+         # header, and one over both the main header and the payload.  RPM checks

+         # that the main header header begins with

+         # b"\x8e\xad\xe8\x01\x00\x00\x00\x00", so we’re in the clear: it is

+         # not possible to abuse this endpoint to sign an RPM.

+         if not msg.body.startswith('<?xml version="1.0" '):

+             return self._error(msg, 'Refusing to sign object that does not '

+                                     'start with XML declaration')

+         tmpdir = tempfile.mkdtemp(dir=self._tmpdir,

+                                   prefix="robosignatory-")

+         try:

+             signature = self._sign_object(msg.body, tmpdir)

+         except Exception as e:

+             log.error(e)

+             return self._error(msg, 'Signing failed')

+         finally:

+             shutil.rmtree(tmpdir)

+         publish(Message(

+             topic="{}.finished".format(msg.topic),

+             # respond with the same body

+             body={'body': msg.body, 'signature': signature}

+         ))

+ 

file modified
+3
@@ -22,6 +22,9 @@ 

              "region": "us-east-1",

          }

      },

+     "xml": {

+         "key": "testing",

+     }

  }

  

  

file added
+85
@@ -0,0 +1,85 @@ 

+ import os

+ import unittest

+ import copy

+ from collections import namedtuple

+ 

+ import mock

+ from fedora_messaging.api import Message

+ from fedora_messaging.testing import mock_sends

+ 

+ from robosignatory.xml import XMLSigner

+ 

+ 

+ TEST_CONFIG = {

+     "signing": {

+         "backend": "echo",

+     },

+     "koji_instances": {},

+     "ostree_refs": {},

+     "xml": {

+         'key': 'testing',

+     },

+ }

+ 

+ XML_MESSAGE = Message(

+     topic="org.fedoraproject.prod.robosignatory.xml-sign",

+     body='<?xml version="1.0" encoding="UTF-8"?>')

+ 

+ INVALID_MESSAGE = Message(

+     topic="org.fedoraproject.prod.robosignatory.xml-sign",

+     body='\0<?xml version="1.0" encoding="UTF-8"?>')

+ 

+ class TestXML(unittest.TestCase):

+ 

+     def setUp(self):

+         self.consumer = XMLSigner(TEST_CONFIG)

+ 

+     def _get_response_message(self, source_msg, failed=False, failure_msg="Signing failed", sig=""):

+         body= {

+             'body': source_msg.body,

+             'error': failure_msg,

+         } if failed else {

+             'body': source_msg.body,

+             'signature': sig,

+         }

+         return Message(

+             topic=source_msg.topic + ".finished",

+             body=body

+         )

+ 

+     @mock.patch('robosignatory.xml.utils.run_command')

+     def test_xml_sign(self, run_command):

+         run_command.return_value = 0, "", ""

+         expected_response = self._get_response_message(XML_MESSAGE, failed=True)

+         with mock_sends(expected_response):

+             self.consumer.consume(XML_MESSAGE)

+         run_command.assert_called()

+ 

+     @mock.patch('robosignatory.xml.utils.run_command')

+     def test_wrong_xml(self, run_command):

+         run_command.return_value = 0, "", ""

+         expected_response = self._get_response_message(INVALID_MESSAGE,

+                 failed=True, failure_msg='Refusing to sign object that does not '

+                                     'start with XML declaration')

+         with mock_sends(expected_response):

+             self.consumer.consume(INVALID_MESSAGE)

+         run_command.assert_not_called()

+ 

+     @mock.patch('robosignatory.coreos.utils.run_command')

+     def test_signing_failed(self, run_command):

+         run_command.return_value = 1, "stdout", "stderr"

+         expected_response = self._get_response_message(XML_MESSAGE, failed=True)

+         with mock_sends(expected_response):

+             self.consumer.consume(XML_MESSAGE)

+ 

+         run_command.assert_called()

+ 

+     @mock.patch('robosignatory.coreos.utils.run_command')

+     def test_no_signature(self, run_command):

+         run_command.return_value = 0, "stdout", "stderr"

+         expected_response = self._get_response_message(XML_MESSAGE, failed=True)

+ 

+         with mock_sends(expected_response):

+             self.consumer.consume(XML_MESSAGE)

+ 

+         run_command.assert_called()

This adds support for signing XML files.

Fixes #14

Signed-off-by: Demi Marie Obenour demi@invisiblethingslab.com

rebased onto 4ac4b2e

4 years ago

Can you split this out into its own commit?

Can you split this into its own commit?

rebased onto b18fa8c

3 years ago

Did I get the access control correct? My assumption here is that access control is enforced by Fedora Messaging. If this assumption is erroneous, then this code contains a security vulnerability.

I don't think Fedora Messaging enforces any access control at the moment, besides being allowed to connect to the bus or not. Any publisher inside the infra can publish to any topic (we want to change that but it hasn't been done).

@abompard Does that mean that any fedmsg publisher can cause any package to be signed, or is there additional authentication? I could add a check that the metadata signing request was somehow signed by koji.

I think so. This issue is mitigated by the fact that only Fedora Infra applications can publish to the bus (you need a certificate that we provide).

I planned to improve this situation by adding topic authorization, but the ticket has been blocked for a while. I'll try to get it going, but doing this before the end of the year would be very unadvisable as I'm sure we don't have the exhaustive list of what topics apps are publishing to.

In that case I think this PR is good to go. It has a security check to ensure that it cannot be abused to sign packages.

Can you please rebase the PR so we can get it merged?

rebased onto d574d44

3 years ago

rebased onto d56f666

3 years ago

@abompard Can you please merge this?

Now that everything looks good, it should be good to merge. :thumbsup:

Pull-Request has been merged by abompard

3 years ago