Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework of signature verification #1

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

jo-so-nx
Copy link

@jo-so-nx jo-so-nx commented Mar 5, 2024

Currently, the signature verification doesn't work, esp. the detection of inline signatures what causes the whole file can't be processed, because it's invalid. This rework joins both cases of inline and detached signatures, because the GPG API allows the passing and receive of buffers.

@@ -664,12 +664,10 @@ def _print_warning(timeout):

def read(self, size=-1):
"""
Read the data from the file or URL and and uncompress it on-the-fly if
Read the data from the file or URL and uncompress it on-the-fly if
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, thanks for working on this.
Could you please split this typo fix into its own commit?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, should it go in its own PR, or can I add the commit here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whichever is easier for you. Thanks!

@dedekind
Copy link

dedekind commented Mar 6, 2024

Sorry, I wrote something here, but I was confused that this is still the old github URL, apologies, ignore my message (I also deleted it).

@twoerner
Copy link
Collaborator

twoerner commented Mar 7, 2024

Also, please add SoB lines to your commits, thanks!

@twoerner
Copy link
Collaborator

twoerner commented Mar 7, 2024

bmaptool doesn't have a lot of tests, but what little tests it does have does seem to test the signing feature. These signing tests seem to succeed both with and without this patch. Could you provide me with a clearer example of what is not working; or, better yet, provide a test that demonstrates the problem before your patch, and how your patch fixes it?

i.e. see tests/test_CLI.py

@jo-so-nx
Copy link
Author

jo-so-nx commented Mar 7, 2024

Isn't the test test_clearsign required to fail? The return code should be 1:

self.assertEqual(completed_process.returncode, 1, completed_process.stdout)

Here is what I tried:

% g clone 'https://github.com/yoctoproject/bmaptool.git' /tmp/bmt
Cloning into '/tmp/bmt'...
remote: Enumerating objects: 3077, done.
remote: Counting objects: 100% (398/398), done.
remote: Compressing objects: 100% (28/28), done.
remote: Total 3077 (delta 376), reused 371 (delta 370), pack-reused 2679
Receiving objects: 100% (3077/3077), 673.32 KiB | 3.83 MiB/s, done.
Resolving deltas: 100% (2021/2021), done.
% cd /tmp/bmt   
% venv_init venv requirements-test.txt
Activating venv in /tmp/bmt/venv
Ignoring backports.tempfile: markers 'python_version < "3.2"' don't match your environment
Ignoring mock: markers 'python_version < "3.3"' don't match your environment
Collecting six (from -r requirements-test.txt (line 1))
  Using cached six-1.16.0-py2.py3-none-any.whl.metadata (1.8 kB)
Collecting nose (from -r requirements-test.txt (line 2))
  Using cached nose-1.3.7-py3-none-any.whl.metadata (1.7 kB)
Using cached six-1.16.0-py2.py3-none-any.whl (11 kB)
Using cached nose-1.3.7-py3-none-any.whl (154 kB)
Installing collected packages: nose, six
Successfully installed nose-1.3.7 six-1.16.0
% python setup.py install
running install
…
Processing dependencies for bmap-tools==3.7
Finished processing dependencies for bmap-tools==3.7
% bmaptool --version
bmaptool 3.7
% g apply 
diff --git i/tests/test_CLI.py w/tests/test_CLI.py
index 4683960..64946e3 100644
--- i/tests/test_CLI.py
+++ w/tests/test_CLI.py
@@ -106,11 +106,9 @@ class TestCLI(unittest.TestCase):
                 "tests/test-data/test.image.gz",
                 self.tmpfile,
             ],
-            stdout=subprocess.PIPE,
-            stderr=subprocess.STDOUT,
             check=False,
         )
-        self.assertEqual(completed_process.returncode, 1, completed_process.stdout)
+        self.assertEqual(completed_process.returncode, 0)
 
     def setUp(self):
         os.environ["GNUPGHOME"] = "tests/test-data/gnupg/"
% python -m unittest -bv tests.test_CLI.TestCLI.test_clearsign
test_clearsign (tests.test_CLI.TestCLI.test_clearsign) ... Traceback (most recent call last):
  File "/tmp/bmt/venv/lib/python3.11/site-packages/bmap_tools-3.7-py3.11.egg/bmaptools/BmapCopy.py", line 401, in _parse_bmap
  File "/usr/lib/python3.11/xml/etree/ElementTree.py", line 1219, in parse
    tree.parse(source, parser)
  File "/usr/lib/python3.11/xml/etree/ElementTree.py", line 581, in parse
    self._root = parser._parse_whole(source)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^
xml.etree.ElementTree.ParseError: syntax error: line 1, column 0

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/tmp/bmt/venv/bin/bmaptool", line 33, in <module>
    sys.exit(load_entry_point('bmap-tools==3.7', 'console_scripts', 'bmaptool')())
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/bmt/venv/lib/python3.11/site-packages/bmap_tools-3.7-py3.11.egg/bmaptools/CLI.py", line 780, in main
  File "/tmp/bmt/venv/lib/python3.11/site-packages/bmap_tools-3.7-py3.11.egg/bmaptools/CLI.py", line 490, in copy_command
  File "/tmp/bmt/venv/lib/python3.11/site-packages/bmap_tools-3.7-py3.11.egg/bmaptools/BmapCopy.py", line 287, in __init__
  File "/tmp/bmt/venv/lib/python3.11/site-packages/bmap_tools-3.7-py3.11.egg/bmaptools/BmapCopy.py", line 406, in _parse_bmap
TypeError: 'NamedFile' object is not iterable
FAIL

======================================================================
FAIL: test_clearsign (tests.test_CLI.TestCLI.test_clearsign)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/tmp/bmt/tests/test_CLI.py", line 111, in test_clearsign
    self.assertEqual(completed_process.returncode, 0)
AssertionError: 1 != 0

----------------------------------------------------------------------
Ran 1 test in 0.163s

FAILED (failures=1)

The first problem is the comparison of string clearsign_marker and the byte array from bmap_obj.read() in verify_bmap_signature which is never true, and the inline signature gets never detected.

@bnavigator
Copy link

Please also see intel#116 for a case where signature checking tests currently fail.

@twoerner
Copy link
Collaborator

twoerner commented Mar 8, 2024

Please also see intel#116 for a case where signature checking tests currently fail.

Thanks for the pointer @bnavigator
So this PR fixes something that will break after 2024-06-12?

@twoerner
Copy link
Collaborator

twoerner commented Mar 8, 2024

Isn't the test test_clearsign required to fail? The return code should be 1:

self.assertEqual(completed_process.returncode, 1, completed_process.stdout)

@jo-so-nx
Funny, I was just wondering the same thing the other day!

@bnavigator
Copy link

bnavigator commented Mar 8, 2024

So this PR fixes something that will break after 2024-06-12?

It will break after 2024-06-12 with or without the PR. The embedded key expires.

[   19s] + pytest-3.9 --ignore=_build.python39 --ignore=_build.python310 --ignore=_build.python312 --ignore=_build.python311 -v -k 'not test_is_zfs_configuration_compatible_unreadable_file'
[   19s] ============================= test session starts ==============================
[   19s] platform linux -- Python 3.9.18, pytest-7.4.4, pluggy-1.3.0 -- /usr/bin/python3.9
[   19s] cachedir: .pytest_cache
[   19s] rootdir: /home/abuild/rpmbuild/BUILD/bmaptool-3.7
[   19s] collecting ... collected 19 items / 1 deselected / 18 selected
[   19s] 
[   19s] tests/test_CLI.py::TestCLI::test_clearsign FAILED                        [  5%]
[   19s] tests/test_CLI.py::TestCLI::test_unknown_signer PASSED                   [ 11%]
[   20s] tests/test_CLI.py::TestCLI::test_valid_signature FAILED                  [ 16%]
[   20s] tests/test_CLI.py::TestCLI::test_wrong_signature PASSED                  [ 22%]
[   20s] tests/test_CLI.py::TestCLI::test_wrong_signature_uknown_signer PASSED    [ 27%]
[   89s] tests/test_api_base.py::TestCreateCopy::test PASSED                      [ 33%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_get_file_system_type PASSED [ 38%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_get_file_system_type_no_fstype_found PASSED [ 44%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_get_file_system_type_symlink PASSED [ 50%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_is_compatible_file_system_ext4 PASSED [ 55%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_is_compatible_file_system_zfs_invalid PASSED [ 61%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_is_compatible_file_system_zfs_valid PASSED [ 66%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_is_zfs_configuration_compatible_disabled PASSED [ 72%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_is_zfs_configuration_compatible_enabled PASSED [ 77%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_is_zfs_configuration_compatible_invalid_read_value PASSED [ 83%]
[   89s] tests/test_bmap_helpers.py::TestBmapHelpers::test_is_zfs_configuration_compatible_notinstalled PASSED [ 88%]
[   89s] tests/test_compat.py::TestCompat::test PASSED                            [ 94%]
[   92s] tests/test_filemap.py::TestFilemap::test PASSED                          [100%]
[   92s] 
[   92s] =================================== FAILURES ===================================
[   92s] ____________________________ TestCLI.test_clearsign ____________________________
[   92s] 
[   92s] self = <tests.test_CLI.TestCLI testMethod=test_clearsign>
[   92s] 
[   92s]     def test_clearsign(self):
[   92s]         completed_process = subprocess.run(
[   92s]             [
[   92s]                 "bmaptool",
[   92s]                 "copy",
[   92s]                 "--bmap",
[   92s]                 "tests/test-data/signatures/test.image.bmap.v2.0.asc",
[   92s]                 "tests/test-data/test.image.gz",
[   92s]                 self.tmpfile,
[   92s]             ],
[   92s]             check=False,
[   92s]         )
[   92s] >       self.assertEqual(completed_process.returncode, 0)
[   92s] E       AssertionError: 1 != 0
[   92s] 
[   92s] tests/test_CLI.py:111: AssertionError
[   92s] ----------------------------- Captured stderr call -----------------------------
[   92s] bmaptool: info: discovered inline signature
[   92s] bmaptool: ERROR: An error occurred, here is the traceback:
[   92s] Traceback (most recent call last):
[   92s]   File "/home/abuild/rpmbuild/BUILDROOT/bmap-tools-3.7-0.x86_64/usr/lib/python3.9/site-packages/bmaptools/CLI.py", line 204, in verify_bmap_signature
[   92s]     plaintext, sigs = context.verify(bmap_data, det_sig_data)
[   92s]   File "/usr/lib64/python3.9/site-packages/gpg/core.py", line 558, in verify
[   92s]     raise errors.BadSignatures(results[1], results=results)
[   92s] 
[   92s] bmaptool: ERROR: discovered a BAD GPG signature: tests/test-data/signatures/test.image.bmap.v2.0.asc
[   92s] 
[   92s] 
[   92s] _________________________ TestCLI.test_valid_signature _________________________
[   92s] 
[   92s] self = <tests.test_CLI.TestCLI testMethod=test_valid_signature>
[   92s] 
[   92s]     def test_valid_signature(self):
[   92s]         completed_process = subprocess.run(
[   92s]             [
[   92s]                 "bmaptool",
[   92s]                 "copy",
[   92s]                 "--bmap",
[   92s]                 "tests/test-data/test.image.bmap.v2.0",
[   92s]                 "--bmap-sig",
[   92s]                 "tests/test-data/signatures/test.image.bmap.v2.0.valid-sig",
[   92s]                 "tests/test-data/test.image.gz",
[   92s]                 self.tmpfile,
[   92s]             ],
[   92s]             stdout=subprocess.PIPE,
[   92s]             stderr=subprocess.STDOUT,
[   92s]             check=False,
[   92s]         )
[   92s] >       self.assertEqual(completed_process.returncode, 0, completed_process.stdout)
[   92s] E       AssertionError: 1 != 0 : b'bmaptool: \x1b[91mERROR\x1b[0m: An error occurred, here is the traceback:\nTraceback (most recent call last):\n  File "/home/abuild/rpmbuild/BUILDROOT/bmap-tools-3.7-0.x86_64/usr/lib/python3.9/site-packages/bmaptools/CLI.py", line 204, in verify_bmap_signature\n    plaintext, sigs = context.verify(bmap_data, det_sig_data)\n  File "/usr/lib64/python3.9/site-packages/gpg/core.py", line 558, in verify\n    raise errors.BadSignatures(results[1], results=results)\n\nbmaptool: \x1b[91mERROR\x1b[0m: discovered a BAD GPG signature: tests/test-data/signatures/test.image.bmap.v2.0.valid-sig\n\n\n'
[   92s] 
[   92s] tests/test_CLI.py:43: AssertionError
[   92s] =========================== short test summary info ============================
[   92s] FAILED tests/test_CLI.py::TestCLI::test_clearsign - AssertionError: 1 != 0
[   92s] FAILED tests/test_CLI.py::TestCLI::test_valid_signature - AssertionError: 1 !...
[   92s] ============ 2 failed, 16 passed, 1 deselected in 72.43s (0:01:12) =============

@bnavigator
Copy link

Extending the test key is enough for intel#116. It's unrelated to this PR. Sorry for the noise. Althought I would not know where else to discuss. There is no issue tracker in the new repo, yet.

# extend signing key expiration for reproducible builds
export GNUPGHOME=$PWD/tests/test-data/gnupg
echo 'expire
50y
key 1
expire
50y
save' | gpg --command-fd=0 --batch --edit-key 927FF9746434704C5774BE648D49DFB1163BDFB4

jo-so-nx added 5 commits March 9, 2024 21:23
Passing 0xFFFFFFFFFFFFFFFF to read causes python to complain about:

    OverflowError: cannot fit 'int' into an index-sized integer

Signed-off-by: Jörg Sommer <[email protected]>
The current tests do not take into account whether the `gpg` package has
been installed or not. If it is missing, the tests should be skipped.

Furthermore, the output of the tests must be checked in order to decide
whether tests fail due to an exception or whether the desired error message
is displayed.

Signed-off-by: Jörg Sommer <[email protected]>
The verification of PGP signatures had some flaws and didn't work, because
the Python API and the GPG interface have changed. Inline signatures were
not detected, because of a comparison of string and byte array. And even
after this the code failed, because `sig.status` is no longer available.

Signed-off-by: Jörg Sommer <[email protected]>
The gpg expires on 2024-06-12 (see [1]) which makes all tests fail using the
key. Therefore extend the expiration to 50 year as suggested in [2].

[1]: intel#116
[2]: yoctoproject#1 (comment)

Signed-off-by: Jörg Sommer <[email protected]>
@jo-so-nx
Copy link
Author

jo-so-nx commented Mar 9, 2024

@twoerner another question is if this should be added:

diff --git i/.github/workflows/ci.yml w/.github/workflows/ci.yml
index 7b61782..d2c3a20 100644
--- i/.github/workflows/ci.yml
+++ w/.github/workflows/ci.yml
@@ -30,7 +30,7 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        python-version: ["3.8", "3.9", "3.10"]
+        python-version: ["3.8", "3.9", "3.10", "3.11"]
 
     steps:
     - uses: actions/checkout@v3

And is the description in README.md about tests (bmaptool/README.md) still valid? I get the error AttributeError: module 'collections' has no attribute 'Callable' when running nosetests. Hence, I use python -m unittest -bv tests.test_CLI.TestCLI. Should this be updated?

@jo-so-nx
Copy link
Author

Hint: There's also python3.12

@moto-timo
Copy link
Collaborator

Hint: There's also python3.12

Patches and pull requests are welcome.

@josch
Copy link

josch commented Jul 16, 2024

Extending the test key is enough for intel#116. It's unrelated to this PR

@bnavigator I fixed that unrelated thing in #31 by always re-creating $GNUPGHOME. This also allows this git repo to have fewer binary blobs stored in it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants