OpenPGP Backend based on Sequoia

[nwalfield]

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]

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

That's putting it very politely I think :laughing: 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: unnecessaryunsafeblock and error: unused variable:v` which look like a -Werror equivalent being enabled. Trying to build withRUSTFLAGS="-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]

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]

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

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.

[nwalfield]

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 (https://github.com/rust-lang/rust/issues/95406#issuecomment-1081572190)

[nwalfield]

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]

Yup. Stuff happens :smile:

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]

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]

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]

Actually, I have no clue what a good API for this would look like :smile: 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]

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]

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]

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 :smile: 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]

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

[nwalfield]

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

[cgwalters]

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 :smile:

[nwalfield]

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]

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]

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]

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]

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]

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]

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]

* `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.

:+1:

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" :sweat_smile:

[DemiMarie]

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]

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.

[pmatilai]

Confirmed building and working on F35 stock rustc now, thanks!!

Performance with the debug-build is pretty terrible :laughing: but seems acceptable with a release build (and of course this is early days anyway)

[pmatilai]

Oh BTW, spotted this in the debug messages:

rpmDigestFinal: -> error: Failure: ctx must not be NULL

In rpm, "destructor" APIs always accept NULL as a perfectly valid no-op, so these must not be considered errors or even things to warn about.

[nwalfield]

Confirmed building and working on F35 stock rustc now, thanks!!

Performance with the debug-build is pretty terrible laughing but seems acceptable with a release build (and of course this is early days anyway)

The release builds should be pretty fast. Debug builds are indeed horribly slow :/.

[nwalfield]

Oh BTW, spotted this in the debug messages:

rpmDigestFinal: -> error: Failure: ctx must not be NULL

In rpm, "destructor" APIs always accept NULL as a perfectly valid no-op, so these must not be considered errors or even things to warn about.

Good catch. I think that was the only one. Fixed: https://gitlab.com/sequoia-pgp/rpm-sequoia/-/commit/d4ea64219a32aa53f26acb93c5c82bd8c241aa34

[pmatilai]

...except that seems I was mistaken about rpmDigestFinal(), both the existing backends return -1 on NULL ctx, it's the callers of that particular API that generally just ignore the return code entirely because it's actually expected that it gets called with NULL in various situations. So I guess in the right thing to do is to just have it silently return -1 afterall, like the other crypto variants do. Sorry about the mixup.

[nwalfield]

Reverted, thanks. https://gitlab.com/sequoia-pgp/rpm-sequoia/-/commit/e1245dff6da9c696803ed995dba6ff164c2b1ef7

[pmatilai]

We'll be releasing 4.18 alpha shortly, and after that is branched I'll start by purging all these old obsolete APIs.

4.18 is branched (although not released) now, so here we go: #1992

[nwalfield]

Autoconf

In the latest version of the MR, I've fixed the autoconf stuff. I've also removed the deprecated symbols from rpm-sequoia.

rpmpgp.c

Currently, rpm-sequoia does not completely replace rpmpgp.c. In particular, pgpValString and pgpIdentItem and pgpReadPkts are still implemented in C.

pgpValString and pgpIdentItem are implemented in C, because they are just pretty printers, which use enums and strings defined by rpm, and converting those to Rust would be a pain without any appreciable gain in safety. pgpIdentItem also uses internationalization and I think it is better to keep all of rpm's internationalization support in rpm itself, if possible.

pgpReadPkts is implemented in C, because it is just a thin wrapper around pgpParsePkts, which first uses an internal rpm function (rpmioSlurp).

Do you agree with these choices?

Since we want to keep the current pgp logic for systems and configurations where Sequoia is not an option, how should I remove the pgp code when the Sequoia backend is selected. I see two options: have a rpmpgp.c file for shared code and then split the gcrypt/openssl code into rpmpgp-gcrypt-openssl.c and add a rpmpgp-sequoia.c file for Sequoia-specific functionality. Alternatively we could use #ifdefs. I prefer the former. What do you think about this idea? What about the names?

digest.h

While reviewing my MR, I realized that I removed the pgpDigAlg definition and all of the function declarations in digest.h. This doesn't seem to cause any test failures. Should these symbols be removed? If I understood correctly, they are internal symbols so removing them won't require an so name bump. If you agree, then I make a separate MR for that bit.

[nwalfield]

Regarding digest.h, I wrote:

While reviewing my MR, I realized that I removed the pgpDigAlg definition and all of the function declarations in digest.h. This doesn't seem to cause any test failures. Should these symbols be removed? If I understood correctly, they are internal symbols so removing them won't require an so name bump. If you agree, then I make a separate MR for that bit.

This isn't correct. The internal PGP implementation with either libgcrypt or openssl uses these definitions. Since we want to keep the internal PGP implementation, they can't be removed.

pgpPubkeyNew, pgpSignatureNew, and pgpDigAlgFree are only used by rpmpgp.c, and pgpMpiLen is used by digest_libgcrypt.c and digest_openssl.c. pgpDigAlg_s is used by rpmpgp.c, digest_libgcrypt.c, and digest_openssl.c. So, these symbols appear to be private to the PGP backend.

pgpPubkeyNew, pgpSignatureNew are backend specific and are defined in both digest_libgcrypt.c and digest_openssl.c. pgpDigAlgFree is common to both backends and is defined is rpmpgp.c. pgpMpiLen and pgpDigAlg_s are common to both backends and are defined in digest.h.

The Sequoia backend doesn't need the pgpDigAlg_s struct or these four functions.

I've moved these declarations into digest_libgcrypt.h and digest_openssl.h and changed digest.h to use the right header based on the selected backend. (digest_openssl.h currently just #includes digest_libgcrypt.h to avoid duplicating code).

I also split rpmpgp.c into rpmpgp.h and rpmpgp_internal.c. rpmpgp.c now just contains the code that is common to all three variants. This patch does not depend on the Sequoia backend.

[nwalfield]

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 <rsa@rpm.org>) = 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 <rsa@rpm.org>

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.

[DemiMarie]

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

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]

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]

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]

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

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

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]

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]

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]

@pmatilai I think all of the open issues are now resolved. I've opened a PR (https://github.com/rpm-software-management/rpm/pull/2043).