-
Notifications
You must be signed in to change notification settings - Fork 380
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
OpenPGP Backend based on Sequoia #1978
Comments
That's putting it very politely I think 😆 The current "API" is ad-hoc stuff added over the course of 20 years, by people with not much OpenPGP experience. I'm sure it shows. The advantage of building around the current API is that transition is smoother, eg this looks like it could be provided as an alternative to the existing version (at least for a time). That said, nobody will shed a tear over the existing API if replaced by something coherent. A soname bump is planned in rpm 4.19 (in 2023) so that'd be the first opportunity for such changes . Allowing for bigger changes in the PGP area is one of the reasons for that plan, and this is a very good time to start thinking about that. Haven't been able to actually test-drive this yet as my attempt to build on F35 is failing, stuff like
|
Oh and to make it absolutely clear, this effort is very much appreciated. A couple of weeks ago I toyed around to get basically a "hello world" in Rust invoked from rpm, but getting from that to a state the test-suite passes would've been a long, long, long and painful road for me. |
Thanks for reporting this. I created a smaller reproducer and opened an issue against rustc. I was using 1.56.1 from |
Yup, I'll investigate my options. I have to say though that this doesn't exactly fill my heart with confidence on betting rpm foundations on Rust 😆 We'll need to plan for switchable implementation, like with the other crypto stuff, to avoid a hard dependency on Rust. Rust is an immature language/toolchain from the rpm perspective anyhow, and we can't require latest x, y and z to get rpm up and running. |
Further investigation suggests that 1.56.1 is the last release that doesn't exhibit this ICE (rust-lang/rust#95406 (comment)) |
This is indeed a poor first impression :/. FWIW, I just asked @teythoon, and in the nearly five years that we've worked on Sequoia we only remember encountering one other ICE. |
Yup. Stuff happens 😄 The bigger issue is that Rust is a significant build-dependency (dragging in LLVM and version interdependencies and whatnot) which matters for bootstrapping, and may be difficult sell at all in some use-scenarios. Planning for build-time switchable implementation is (relatively) cheap insurance for that. |
I fully agree with having an interface that can be easily implemented by different backends. This means the backends "compete" on features rather than rely on vendor lock in. And different systems, of course, have different requirements, which may be better fulfilled by different backends. |
I think it shouldn't be a big deal to support the current API and a new API for a time. I'm happy to help design a new API. That should probably be done in a new issue. I suspect that you probably have opinions so perhaps we should use a high bandwidth communication channel to first synchronize, and then switch to written communication. |
Actually, I have no clue what a good API for this would look like 😄 So if you do, by all means propose something like a rough outline and we can go from there. I guess I mostly care that it doesn't look too outlandish in the rpm codebase, but considering what an hodge-podge the rpm API is, minor deviances get lost in the noise. |
What if there was a barebones implementation (based on the current one or a C++ version I wrote) that had either no or trivial third-party dependencies, and a Sequoia backend that offered more features? The barebones backend would focus on handling 99+% of use-cases that arise in practice and on failing secure in the cases it does not handle, but it would not be very user-friendly or give good error messages. The Sequoia backend would implement basically the entire OpenPGP standard and give helpful error messages, but would require more build-time and runtime dependencies. |
I don't want to comment on whether that is acceptable: that's the maintainer's call, not mine. I have two concerns about the proposal. First, Sequoia's certificate handling code is about 10k SLOC:
I don't think it is possible to significantly simplify this while maintaining the same semantics. @vanitasvitae, the author of pgpainless, recently documented some of the complexity of certificate canonicalization. @teythoon's OpenPGP test suite provides a different view of the complexity. (To be clear: there are some bits of OpenPGP that are unnecessarily complex, but I'd argue that most of the complexity is needed to handle real use cases; a robust PKI is hairy.) So, I think you could perhaps get down to half the SLOC, but I don't think you'll be able to reduce the SLOC by an order of magnitude, which is what would you'd need, I think, to really create a barebones implementation. Of course, certificate canonicalization is only one part of the puzzle. You still need to parse OpenPGP messages and certificates, and verify signatures. The other concern that I'd have is that your barebones library sounds like a point solution for rpm. If no one else is using your library, then it is going to see much less testing, which would worry me.
What runtime dependencies are you referring to? The only external run-time dependency that my port of rpm to Sequoia has is on libnettle. rpm depends on other C libraries so I don't think this is noteworthy. The rpm port to sequoia also does not fork and exec a program, like gpg, or communicate with any daemons, like gpg-agent. In terms of build dependencies, it's true that Sequoia has a few. For someone coming from C, this probably looks like a lot. But in Rust, libraries are often structured differently. For instance, the cipher crate, which Sequoia depends on, defines an abstract interface that block ciphers and stream ciphers like aes implement. In the C world, this interface and the implementations would normally all be combined into a single library. Rust makes it easy to publish multiple crates from a single repository so the dependencies are more fine grained. Perhaps your concern is just the dependency on rustc. My understanding is that many other core pieces of infrastructure are moving towards using Rust including the kernel. So, adding a rust dependency would indeed complicate the bootstrapping process for rpm, but this is shared by other components. I was recently talking to someone who works on RHEL's security team about this exact issue, and he said to not worry about the rust dependency: "rust is available in fedora and rhel. It's not a big deal. Besides I project a raising usage of rust, no point in running from it. As long as we have a compiler for it on all our arches it is as good as C for all I care." |
While Linux is the main use-case of course, rpm is used in all manner of environments where Rust availability is not at all given, and Rust is still a rather volatile element from rpm point of view at this point in time. Rpm is an old dinosaur which likes its feet firmly on ground 😄 Also, having central piece of rpm implemented in another language entirely introduces build and debugging complexities, etc. But this is all besides the point in this ticket. This ticket is about adding an OpenPGP backend based on Sequoia, with an understanding that the backend will be compile-time switchable. Any other "exit strategy" discussion belongs to #1935. Thanks. |
@nwalfield: to be clear: I am not the one who has concerns about Rust. Others do, however. |
@DemiMarie: I have concerns about Rust 😄 (I also have concerns about C, C++, ...). |
One thing to keep in mind is that such an abstraction could also be useful to https://github.com/rpm-software-management/librepo and https://github.com/ostreedev/ostree Both the latter two are GLib-C LGPLv2+ too. I guess really what we want is s/gpgme/gpgsequioame/ and hopefully the "me" part is true 😄 |
Hi colin, I wasn't aware of ostree. Thanks for pointing it out. I see that there is already an issue about replacing gpgme with sequoia. I'll keep that in mind, but I won't be able to lead such a port right now, although I'd be happy to help. (Based on the issue, it sounds like it would be straightforward.)
This is tricky. We've observed that there are at least two types of applications. There are daemons that have their own private keyring, which is often stored in a database. And there are user agents, which should use a shared keyring and need to interact with the user ("please insert token X", "please enter a password", "please select the right key", etc.). Defining an "easy" high-level API is hard given the different functionality that these types of applications require. But, even if we were to create two high-level APIs, there is still a large amount of variation between applications in each of these classes. Happily, the low-level and mid-level APIs defined by In short, I'd encourage you to look at the exact functionality that you need and see if a 100 line shim wouldn't be adequate. |
The following still needs to be done:
There are four stubs:
Modulo Note: Modulo the above issues, I'd appreciate a review regarding my approach. Is it acceptable? |
Would it be possible to open-code the macro output to avoid that error? |
The printing code is unreachable in practice. I suggest not implementing it. |
Sure. |
Are you saying the output is just an internal debugging tool, and its format is not part of the API contract? |
I don’t think the printing code can be turned on. It’s either optimized out by the compiler or is unreachable code in the binary. |
👍
Oh... AFAIK the only user of these is PackageKit which uses them because back then it was all there was, and nobody has bothered to change what appears to be working. We'll rip this stuff out from 4.19, don't bother with them. We'll be releasing 4.18 alpha shortly, and after that is branched I'll start by purging all these old obsolete APIs.
The printing thing only ever was a hidden debugging aid that indeed seems gone entirely unreachable. There are better tools for dumping PGP packet data now, don't worry about it.
I haven't been able to test yet due to the rustc issue (I'll try to find time to do so soon though). I'm not going to be able to review the Rust code which is all Hebrew to me, but then that's kinda ideal to me, because all I really want is a blackbox which deals with "this PGP stuff" 😅 |
I might be able to help with that; I’ve written Rust code (including an OpenPGP signature parser) before. |
I'm happy to report that I have committed a non-ugly workaround for the ICE, and tested with 1.59.0. I haven't tested with Fedora's rustc, but I expect that this change should be sufficient for it too. |
The patch is now tiny. It changes I'd like to better document the build instructures, but it seems like the README is not the right place for that. Where should I mention The test suite fails in two ways for me:
267 fails this way:
The Sequoia backend returns the f00650f8 subkey, which the internal parser does not:
This is because the Sequoia backend returns all subkeys and checks for validity when checking a signature. 269 fails in a similar way:
Again, the Sequoia backend returns an invalid subkey, which it would refuse to use to verify a message. Note: this approach is sensible as some of the validity checks depend on the current time. By checking these variants at signature verification time, we avoid a potential TOCTOU bug. |
This change adds support for using Sequoia as an alternative to the internal OpenPGP backend. To use this backend, it is necessary to have the rpm-sequoia library installed. https://gitlab.com/sequoia-pgp/rpm-sequoia Fixes rpm-software-management#1978.
@nwalfield I suggest fixing these tests in the internal parser, provided that tests are added that make sure the Sequoia backend won’t actually verify a signature using the bad subkey. My PR #1993 should do most of it. |
INSTALL would be the place to add such things.
That's a good point. I made bad subkey bindings fail just because it seemed the simplest right thing to do for that particular case at least: since --import equals trust in rpm, it seems questionable to import known invalid subkeys. |
Thanks for pointing that out. I've fixed that in the latest revision of this MR.
How do you want to proceed here? |
This change adds support for using Sequoia as an alternative to the internal OpenPGP backend. To use this backend, it is necessary to have the rpm-sequoia library installed. https://gitlab.com/sequoia-pgp/rpm-sequoia Fixes rpm-software-management#1978.
I created tests to check that Sequoia accepts a signature from a valid subkey, rejects a signature from an expired subkeys, and rejects a signature from a revoked subkey. Sequoia passes the tests. Unfortunately, as mentioned, the internal parser is unable to handle these certificates. How do you recommend that I proceed? |
@pmatilai: Despite @DemiMarie's efforts to improve the internal OpenPGP implementation, its semantics remain quite different from the Sequoia backend's semantics. How do you want to proceed here? I'm increasingly convinced that it doesn't make sense to invest any additional time in the internal OpenPGP implementation. The certificate handling needs to be completely reworked, which is effectively a rewrite. At that point, it is probably better to start over. So, we could just leave the internal OpenPGP implementation as is and declare it legacy. That raises the question of how to handle tests. For instance, the TOCTOU bug that we discussed means that the internal implementation returns NOKEY when verifying a signature made by a bad key, but the Sequoia backend returns FAIL. And, the internal implementation doesn't handle expired keys, but Sequoia does. Etc. Should we just allow the tests to fail with the internal implementation? Should we have two variants of the test depending on the selected implementation? |
@nwalfield @pmatilai Personally, I think that the internal implementation must meet the following requirement: It must be safe to run Beyond that, simple improvements to the internal implementation are fine, but complex ones, less so. More generally, there needs to be a discussion in the OpenPGP community about lightweight implementations. RPM is not the only place that has a stub implementation of OpenPGP: GRUB has one as well, and some distribution Linux kernels have had them in the past. Linux may even have one again in the future. |
Yes we need to be able to handle different results from tests, based on which PGP parser is used. Expecting failure or just skipping are both totally legit things to do there. @DemiMarie - your "must list" again goes into the subkey territory, which I think is just unsustainable because of the complexities involved. We need to axe that stuff from the internal parser, and the sooner we do that the better. |
Oh and just to confirm...
This is what I'm after, improving it and reviewing those improvements is a bottomless time-sink that I'm very determined to put a stop to. |
This change adds support for using Sequoia as an alternative to the internal OpenPGP backend. To use this backend, it is necessary to have the rpm-sequoia library installed. https://gitlab.com/sequoia-pgp/rpm-sequoia Fixes rpm-software-management#1978.
I modified the tests to not run when rpm is compiled with the internal PGP implementation using |
This change adds support for using Sequoia as an alternative to the internal OpenPGP backend. To use this backend, it is necessary to have the rpm-sequoia library installed. https://gitlab.com/sequoia-pgp/rpm-sequoia Fixes rpm-software-management#1978.
This change adds support for using Sequoia as an alternative to the internal OpenPGP backend. To use this backend, it is necessary to have the rpm-sequoia library installed. https://gitlab.com/sequoia-pgp/rpm-sequoia Fixes rpm-software-management#1978.
This change adds support for using Sequoia as an alternative to the internal OpenPGP backend. To use this backend, it is necessary to have the rpm-sequoia library installed. https://gitlab.com/sequoia-pgp/rpm-sequoia Fixes rpm-software-management#1978. (Backport db36ea8.)
This change adds support for using Sequoia as an alternative to the internal OpenPGP backend. To use this backend, it is necessary to have the rpm-sequoia library installed. https://gitlab.com/sequoia-pgp/rpm-sequoia Fixes #1978. (Backport db36ea8.)
Based on #1935, I've created an experimental OpenPGP backend based on Sequoia.
Sequoia is released under the terms of the LGPL2+. Sequoia is in Fedora. Sequoia's default cryptographic backend is Nettle, which is one of the cryptographic libraries that RHEL supports.
For this port, I took the following approach: reimplement the existing OpenPGP interface in terms of Sequoia using a Rust shim. To make life easier, I made pgpDigParams opaque to rpm. To accomplish this, I had to add a few accessor functions.
The port takes advantage of Sequoia's machinery to canonicalize OpenPGP certificates. This is extremely complicated and accounts for nearly a fifth of Sequoia's code. The port is also careful to check the validity of signatures.
Currently, the port is using Sequoia's null policy, which is not advisable. But, Sequoia's Standard Policy is stricter than what rpm currently appears to implement. If you decide that Sequoia is a reasonable way to go, we should discuss the right approach.
The code passes all tests except test 266. Test 266 checks that a certificate with a subkey that has no binding signature is rejected. (In particular, rpmchecksig.c:doImport calls rpmts.c:rpmtsImportPubkey, which calls rpmkeyring.c:rpmPubkeyNew, which calls rpmpgp.c:pgpPrtParams, which fails if a subkey is missing a binding signature.) This behavior of rejecting a certificate if there is an invalid component does not match the behavior of other OpenPGP implementations that I'm familiar with. Instead, invalid components are normally just ignored. This enables a degree of future compatibility.
I've written up build instructions to try the port.
After spending some time looking at the code, I've come to the conclusion that the current API is not ideal. One thing that I don't like is that the pgpDigGetParams can encapsulate either a certificate, a subkey, or a signature. I'd advise to switch to a new API, however, this might not be so easy given that the OpenPGP functionality is part of librpm's public API. An alternative approach is to add a second API, use it, recommend its use, and deprecate the existing API.
I look forward to your feedback.
The text was updated successfully, but these errors were encountered: