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

OpenPGP Backend based on Sequoia #1978

Closed
nwalfield opened this issue Mar 28, 2022 · 46 comments
Closed

OpenPGP Backend based on Sequoia #1978

nwalfield opened this issue Mar 28, 2022 · 46 comments

Comments

@nwalfield
Copy link
Collaborator

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.

@pmatilai
Copy link
Member

I've come to the conclusion that the current API is not ideal.

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 error: unnecessary unsafe block and error: unused variable: v`` which look like a -Werror equivalent being enabled. Trying to build with RUSTFLAGS="-Awarnings" cargo build (which seems to be a way to disable that -Werror equivalent, but I'm totally clueless about Rust) trips up a compiler panic:

error: internal compiler error: compiler/rustc_codegen_llvm/src/context.rs:867:13: failed to get layout for [type error]: the type [type error] has an unknown layout
--> src/ffi.rs:126:22
|
126 | #[no_mangle] pub extern "C"
| ^
127 | | fn $f($($v: $t),*) -> $Crt {
| |
____________^
|
::: src/lib.rs:143:1
|
143 | / ffi!(fn pgpSignatureType(dig: *const PgpDigParams) -> c_int[-1] {
144 | | let dig = check_ptr!(dig);
145 | |
146 | | dig.signature()
... |
150 | | })
151 | | });
| |__- in this macro invocation
|
= note: this error: internal compiler error originates in the macro ffi (in Nightly builds, run with -Z macro-backtrace for more info)
thread 'rustc' panicked at 'Box', /builddir/build/BUILD/rustc-1.59.0-src/compiler/rustc_errors/src/lib.rs:1115:9
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

@pmatilai
Copy link
Member

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.

@nwalfield
Copy link
Collaborator Author

... trips up a compiler panic:

error: internal compiler error: compiler/rustc_codegen_llvm/src/context.rs:867:13: failed to get layout for [type error]: the type [type error] has an unknown layout

Thanks for reporting this. I created a smaller reproducer and opened an issue against rustc.

I was using 1.56.1 from rustup. If you are willing to install rustup on your system, that might be a way forward until this issue is resolved.

@pmatilai
Copy link
Member

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.

@nwalfield
Copy link
Collaborator Author

I was using 1.56.1 from rustup. If you are willing to install rustup on your system, that might be a way forward until this issue is resolved.

Further investigation suggests that 1.56.1 is the last release that doesn't exhibit this ICE (rust-lang/rust#95406 (comment))

@nwalfield
Copy link
Collaborator Author

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 laughing

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.

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.

@pmatilai
Copy link
Member

pmatilai commented Mar 29, 2022

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.

@nwalfield
Copy link
Collaborator Author

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.

@nwalfield
Copy link
Collaborator Author

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.

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.

@pmatilai
Copy link
Member

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.

@DemiMarie
Copy link
Contributor

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.

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.

@nwalfield
Copy link
Collaborator Author

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?

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:

.../sequoia/openpgp/src$ loc cert.rs cert
--------------------------------------------------------------------------------
 Language             Files        Lines        Blank      Comment         Code
--------------------------------------------------------------------------------
 Rust                    15        21543         1578        10378         9587

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.

The Sequoia backend would implement basically the entire OpenPGP standard and give helpful error messages, but would require more build-time and runtime dependencies.

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."

@pmatilai
Copy link
Member

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.

@DemiMarie
Copy link
Contributor

@nwalfield: to be clear: I am not the one who has concerns about Rust. Others do, however.

@nwalfield
Copy link
Collaborator Author

@DemiMarie: I have concerns about Rust 😄 (I also have concerns about C, C++, ...).

@cgwalters
Copy link
Contributor

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 😄

@nwalfield
Copy link
Collaborator Author

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.)

I guess really what we want is s/gpgme/gpgsequioame/ and hopefully the "me" part is true

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 sequoia-openpgp are not hard to use. Our experience porting applications from gpgme's high-level API to sequoia-openpgp's low/mid-level API is that using Sequoia actually results in less code. One of the reasons for this is that gpgme is quite opinionated (which is in the nature of a high-level API!) and applications typically have one or two things they want to do that gpgme doesn't quite support. This impedance mismatch requires a fair amount of code to workaround. For instance, most operations on a certificate in gpg require the program to import the certificate. But that is not always desirable.

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.

nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 4, 2022
nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 4, 2022
@nwalfield
Copy link
Collaborator Author

The following still needs to be done:

  • configure.ac hard wires the used of Sequoia. This needs to be fixed. Currently, rpm has two backends: a libgcrypt backend and an openssl backend. I'd make the sequoia backend a third option.

There are four stubs:

  • pgpNewDig and pgpFreeDig are only called by rpmPubkeyDig, which is never called in the rpm source code, not even in the tests. It is, however, a public function.

  • pgpDigGetParams, like the previous two stubs, is called by rpmPubkeyDig, and is called by the other stub, pgpPrtPkts.

  • pgpPrtPkts is only called by rpmPubkeyDig and rpmKeyringLookup. They also do not appeared to be exercised by make check.

Modulo pgpPrtPkts's "printing" option these stubs should be easy to implement. Nevertheless, I'm worried about implementing them without a test case. As for "printing", it's not clear to me to what degree a new backend should imitate the current output. I'd appreciate clarification about what is exactly necessary here.

Note: rpm-sequoia tickles an internal compiler error in rustc. As such, this code can only be tested with 1.56, which should be used automatically if you are using rustup.

Modulo the above issues, I'd appreciate a review regarding my approach. Is it acceptable?

@DemiMarie
Copy link
Contributor

Note: rpm-sequoia tickles an internal compiler error in rustc. As such, this code can only be tested with 1.56, which should be used automatically if you are using rustup.

Would it be possible to open-code the macro output to avoid that error?

@DemiMarie
Copy link
Contributor

As for "printing", it's not clear to me to what degree a new backend should imitate the current output. I'd appreciate clarification about what is exactly necessary here.

The printing code is unreachable in practice. I suggest not implementing it.

@nwalfield
Copy link
Collaborator Author

Note: rpm-sequoia tickles an internal compiler error in rustc. As such, this code can only be tested with 1.56, which should be used automatically if you are using rustup.

Would it be possible to open-code the macro output to avoid that error?

Sure.

@nwalfield
Copy link
Collaborator Author

As for "printing", it's not clear to me to what degree a new backend should imitate the current output. I'd appreciate clarification about what is exactly necessary here.

The printing code is unreachable in practice. I suggest not implementing it.

Are you saying the output is just an internal debugging tool, and its format is not part of the API contract?

@DemiMarie
Copy link
Contributor

As for "printing", it's not clear to me to what degree a new backend should imitate the current output. I'd appreciate clarification about what is exactly necessary here.

The printing code is unreachable in practice. I suggest not implementing it.

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.

@pmatilai
Copy link
Member

pmatilai commented Apr 5, 2022

* `configure.ac` hard wires the used of Sequoia.  This needs to be fixed.  Currently, rpm has two backends: a libgcrypt backend and an openssl backend.  I'd make the sequoia backend a third option.

👍

There are four stubs:

* `pgpNewDig` and `pgpFreeDig` are only called by [`rpmPubkeyDig`](https://github.com/rpm-software-management/rpm/blob/b113a9d/rpmio/rpmkeyring.c#L229), which is never called in the rpm source code, not even in the tests.  It is, however, a public function.

* `pgpDigGetParams`, like the previous two stubs, is called by `rpmPubkeyDig`, and is called by the other stub, `pgpPrtPkts`.

* `pgpPrtPkts` is only called by `rpmPubkeyDig` and `rpmKeyringLookup`.  They also do not appeared to be exercised by `make check`.

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.

Modulo pgpPrtPkts's "printing" option these stubs should be easy to implement. Nevertheless, I'm worried about implementing them without a test case. As for "printing", it's not clear to me to what degree a new backend should imitate the current output. I'd appreciate clarification about what is exactly necessary here.

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.

Note: rpm-sequoia tickles an internal compiler error in rustc. As such, this code can only be tested with 1.56, which should be used automatically if you are using rustup.

Modulo the above issues, I'd appreciate a review regarding my approach. Is it acceptable?

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" 😅

@DemiMarie
Copy link
Contributor

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.

@nwalfield
Copy link
Collaborator Author

Note: rpm-sequoia tickles an internal compiler error in rustc. As such, this code can only be tested with 1.56, which should be used automatically if you are using rustup.

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.

nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 11, 2022
nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 13, 2022
nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 13, 2022
@nwalfield
Copy link
Collaborator Author

The patch is now tiny. It changes configure.ac to check for rpm-sequoia, which is also the default OpenPGP backend, and it sets a few compiler / linker flags.

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 rpm-sequoia's location?

The test suite fails in two ways for me:

267: rpmkeys --import rsa (rpmdb)                    FAILED (rpmsigdig.at:196)
269: rpmkeys --import invalid keys                   FAILED (rpmsigdig.at:304)

267 fails this way:

...
runroot rpmkeys --import /data/keys/rpm.org-rsa-2048-test.pub
runroot rpm -qi gpg-pubkey-1964c5fc-58e63918|grep -v Date|grep -v Version:
runroot rpm -q --provides gpg-pubkey-1964c5fc-58e63918

--- -	2022-04-13 07:47:25.043536656 +0000
+++ /home/us/neal/work/pep/rpm/b/tests/rpmtests.dir/at-groups/267/stdout	2022-04-13 07:47:25.030411393 +0000
@@ -44,4 +44,6 @@
 gpg(rpm.org RSA testkey <[email protected]>) = 4:4344591e1964c5fc-58e63918
 gpg(1964c5fc) = 4:4344591e1964c5fc-58e63918
 gpg(4344591e1964c5fc) = 4:4344591e1964c5fc-58e63918
+gpg(f00650f8) = 4:185e6146f00650f8-58e63918
+gpg(185e6146f00650f8) = 4:185e6146f00650f8-58e63918
 
267. rpmsigdig.at:194: 267. rpmkeys --import rsa (rpmdb) (rpmsigdig.at:194): FAILED (rpmsigdig.at:196)

The Sequoia backend returns the f00650f8 subkey, which the internal parser does not:

$ sq inspect rpm.org-rsa-2048-test.pub
rpm.org-rsa-2048-test.pub: OpenPGP Certificate.

    Fingerprint: 771B18D3D7BAA28734333C424344591E1964C5FC
Public-key algo: RSA (Encrypt or Sign)
Public-key size: 2048 bits
  Creation time: 2017-04-06 12:48:24 UTC
      Key flags: certification, signing

         Subkey: B31E5AA680AF713915901533185E6146F00650F8
Public-key algo: RSA (Encrypt or Sign)
Public-key size: 2048 bits
  Creation time: 2017-04-06 12:48:24 UTC
      Key flags: transport encryption, data-at-rest encryption

         UserID: rpm.org RSA testkey <[email protected]>

This is because the Sequoia backend returns all subkeys and checks for validity when checking a signature.

269 fails in a similar way:

runroot rpmkeys --import /data/keys/CVE-2021-3521-badbind.asc

--- -	2022-04-13 07:47:37.877968868 +0000
+++ /home/us/neal/work/pep/rpm/b/tests/rpmtests.dir/at-groups/269/stderr	2022-04-13 07:47:37.866432615 +0000
@@ -1,2 +1 @@
-error: /data/keys/CVE-2021-3521-badbind.asc: key 1 import failed.
 
../../tests/rpmsigdig.at:304: exit code was 0, expected 1
269. rpmsigdig.at:300: 269. rpmkeys --import invalid keys (rpmsigdig.at:300): FAILED (rpmsigdig.at:304)

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.

nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 13, 2022
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.
@DemiMarie
Copy link
Contributor

@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.

@pmatilai
Copy link
Member

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 rpm-sequoia's location?

INSTALL would be the place to add such things.

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.

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.

@nwalfield
Copy link
Collaborator Author

INSTALL would be the place to add such things.

Thanks for pointing that out. I've fixed that in the latest revision of this MR.

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.

How do you want to proceed here?

nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 13, 2022
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
Copy link
Collaborator Author

nwalfield commented Apr 13, 2022

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.

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?

@nwalfield
Copy link
Collaborator Author

@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?

@DemiMarie
Copy link
Contributor

@nwalfield @pmatilai Personally, I think that the internal implementation must meet the following requirement: It must be safe to run gpg2 --export --export-options=export-minimal --armor --output=something.asc --batch --no-tty -- "$TRUSTED_FINGERPRINT" && rpmkeys --import -- something.asc, and RPM must be able to verify signatures made by the non-revoked, signing-capable subkeys in that key. Furthermore, RPM must reject signatures made by keys that are revoked or not capable of signing. Obviously, any means of producing a canonicalized certificate can be used in place of the first command. I believe that this PR + #2026 meets this requirement, as does #2027.

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.

@pmatilai
Copy link
Member

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.

@pmatilai
Copy link
Member

Oh and just to confirm...

So, we could just leave the internal OpenPGP implementation as is and declare it legacy.

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.

nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 29, 2022
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
Copy link
Collaborator Author

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.

I modified the tests to not run when rpm is compiled with the internal PGP implementation using AT_SKIP_IF, as is done elsewhere in the test suite.

@nwalfield
Copy link
Collaborator Author

@pmatilai I think all of the open issues are now resolved. I've opened a PR (#2043).

nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 29, 2022
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 added a commit to nwalfield/rpm that referenced this issue Apr 29, 2022
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 added a commit to nwalfield/rpm that referenced this issue Aug 15, 2022
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.)
pmatilai pushed a commit that referenced this issue Aug 17, 2022
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.)
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

No branches or pull requests

4 participants