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

Allow specifying OpenPGP implementation to use for signing #3094

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

wiktor-k
Copy link

@wiktor-k wiktor-k commented Oct 2, 2024

Hi 👋

I'm pushing a first preview of SOP signing for SHA256SUMs. I've tested that locally and also added tests for rsop and old gpg (just in case) and will begin to refactor this a bit. Since I'm new to both mkosi and Python I think it's good to get feedback earlier in the process if something looks particularly wrong here.

Note that I've tried to make it as minimal as possible with no nice-to-have adjustments mentioned in #3042.

Fixes: #3042
CC: @dvzrv

@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 2 times, most recently from 88fd737 to 2f5e5db Compare October 2, 2024 10:43
@DaanDeMeyer
Copy link
Contributor

DaanDeMeyer commented Oct 2, 2024

I'm curious about hardware tokens. Will those be supported with a unified CLI interface as well?

mkosi/config.py Outdated Show resolved Hide resolved
mkosi/config.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
mkosi/__init__.py Outdated Show resolved Hide resolved
@wiktor-k
Copy link
Author

wiktor-k commented Oct 2, 2024

I'm curious about hardware tokens. Will those be supported with a unified CLI interface as well?

Yes, I actually tested that with my Yubikey and found a couple of issues I fixed. This, of course, depends on the underlying SOP implementation that's used. rsop implements it.

@wiktor-k
Copy link
Author

wiktor-k commented Oct 2, 2024

Okay folks, thanks for the great feedback! I need to install the type-checking tools locally not to spam the CI. I'll be back with the adjustments (all valid points! 🙇 )

@wiktor-k wiktor-k marked this pull request as ready for review October 3, 2024 10:41
@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 2 times, most recently from 5028264 to 49e8075 Compare October 3, 2024 11:13
@wiktor-k wiktor-k marked this pull request as draft October 3, 2024 11:35
mkosi/config.py Outdated Show resolved Hide resolved
@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 5 times, most recently from 7de844c to efc0a77 Compare October 4, 2024 13:43
@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 2 times, most recently from aa13165 to efc0a77 Compare October 7, 2024 07:53
tests/test_signing.py Outdated Show resolved Hide resolved
@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 3 times, most recently from 9231cb6 to a949723 Compare October 7, 2024 11:05
tests/test_signing.py Fixed Show fixed Hide fixed
tests/test_signing.py Fixed Show fixed Hide fixed
@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 3 times, most recently from 4d22ef7 to e8a316e Compare October 8, 2024 07:46
tests/test_signing.py Outdated Show resolved Hide resolved
@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 3 times, most recently from 54b75d1 to 08f4128 Compare October 8, 2024 07:59
tests/test_signing.py Outdated Show resolved Hide resolved
tests/test_signing.py Outdated Show resolved Hide resolved
tests/test_signing.py Outdated Show resolved Hide resolved
tests/test_signing.py Outdated Show resolved Hide resolved
@@ -4562,6 +4569,7 @@ def summary(config: Config) -> str:
Passphrase: {none_to_none(config.passphrase)}
Checksum: {yes_no(config.checksum)}
Sign: {yes_no(config.sign)}
OpenPGP Tool: ({config.openpgp_tool or "gpg"})
Copy link
Contributor

Choose a reason for hiding this comment

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

Just so we don't forget: This also needs an entry in the man page.

tests/test_signing.py Outdated Show resolved Hide resolved
pytest.skip("Needs `sqop` binary in PATH to perform sop tests.")
with tempfile.TemporaryDirectory() as path, Image(config) as image:
tmp_path = Path(path)
tmp_path.chmod(0o755)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not that it matters a lot in the test, but why does the directory holding the key need to be world readable?

tests/test_signing.py Outdated Show resolved Hide resolved
def test_signing_checksums_with_gpg(config: ImageConfig) -> None:
with tempfile.TemporaryDirectory() as path, Image(config) as image:
tmp_path = Path(path)
tmp_path.chmod(0o777)
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's really not have a world-writable thing, even in tests.

Copy link
Author

Choose a reason for hiding this comment

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

The issue here is that gpg during its operation writes temporary files to that directory and since the tests starts as root but then switch user during image.build the directory must be readable to both root and that temporary build user.

I'll try to minimize the number of chmods. With the sop case the key and cert files need to be just readable by both users but in GPG case it needs writeable access sadly :/

@wiktor-k wiktor-k force-pushed the wiktor/teach-mkosi-sop branch 2 times, most recently from 658bcbf to 63822d2 Compare October 11, 2024 11:48
This is especially useful when the test fails and it is likely that the
`stderr` contains debugging info.
@wiktor-k
Copy link
Author

Okay, I think I minimized it as much as possible. Removed all dependencies to the point where removing anything else triggers CI failures.

One thing that is consistently failing is the opensuse/arch build with a rather peculiar message:

Failed:
    dbus-1-common-1.14.10-3.1.noarch                                              
    system-group-hardware-20170617-25.4.noarch                                    
    system-group-kvm-20170617-25.4.noarch                                         
    system-user-lp-20170617-25.4.noarch                                           
  
  Cleaning up.
  Plugins were unloaded.
  
  Traceback (most recent call last):
    File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 67, in main
      return _main(base, args, cli_class, option_parser_class)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 106, in _main
      return cli_run(cli, base)
             ^^^^^^^^^^^^^^^^^^
    File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 130, in cli_run
      ret = resolving(cli, base)
            ^^^^^^^^^^^^^^^^^^^^
    File "/usr/lib/python3.12/site-packages/dnf/cli/main.py", line 183, in resolving
      base.do_transaction(display=displays)
    File "/usr/lib/python3.12/site-packages/dnf/cli/cli.py", line 272, in do_transaction
      raise dnf.exceptions.Error(_('Transaction failed'))
  dnf.exceptions.Error: Transaction failed

If you have any ideas on what to optimize here I'm all ears. Sadly the GnuPG tests require world-writable dirs (since the dir is setup before the image.build thing is executed). I could also remove that test but I think it's a good idea to see if signing works. If there are other approaches I can try, I'm happy to adjust the code.

Thanks for your time! 👋

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

Successfully merging this pull request may close these issues.

Extend OpenPGP signing support with Stateless OpenPGP
3 participants