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

PGP key identifiers use binding signature's creation time, not certificate creation time #2004

Open
nwalfield opened this issue Apr 11, 2022 · 29 comments
Labels
crypto Signatures, keys, hashes and their verification

Comments

@nwalfield
Copy link
Contributor

In #1993, @DemiMarie added a test for revoked subkeys. I'm using that to test the Sequoia backend and I've found some interesting behavior, which I don't completely understand. In short, when I run the test, I see:

runroot rpmkeys --import /data/keys/rpm.org-ed25519-subkey-test.pub
runroot rpm -qi gpg-pubkey-a0e0c1c7-623f6177|grep -v Date|grep -v Version:
runroot rpm -q --provides gpg-pubkey
...
-gpg(test rpm.org ed25519 key with valid signing and encryption subkeys and a revoked signing subkey <[email protected]>) = 4:9dd6f039a0e0c1c7-623f6177
-gpg(a0e0c1c7) = 4:9dd6f039a0e0c1c7-623f6177 <---------------------------------
-gpg(9dd6f039a0e0c1c7) = 4:9dd6f039a0e0c1c7-623f6177
+package gpg-pubkey-a0e0c1c7-623f6177 is not installed
+gpg(test rpm.org ed25519 key with valid signing and encryption subkeys and a revoked signing subkey <[email protected]>) = 4:9dd6f039a0e0c1c7-623f5e80
+gpg(a0e0c1c7) = 4:9dd6f039a0e0c1c7-623f5e80 <---------------------------------
+gpg(9dd6f039a0e0c1c7) = 4:9dd6f039a0e0c1c7-623f5e80
+gpg(63e4b138) = 4:511ed0d463e4b138-623f5f69
+gpg(511ed0d463e4b138) = 4:511ed0d463e4b138-623f5f69
 gpg(f5757247) = 4:a9ecf25cf5757247-623f5e9e
 gpg(a9ecf25cf5757247) = 4:a9ecf25cf5757247-623f5e9e
+gpg(7bab107a) = 4:dbf8808f7bab107a-623f5eb6
+gpg(dbf8808f7bab107a) = 4:dbf8808f7bab107a-623f5eb6
+gpg(59015be2) = 4:f6eabb0459015be2-623f5e80
+gpg(f6eabb0459015be2) = 4:f6eabb0459015be2-623f5e80

The first thing I see is that there is a slight mismatch between the semantics that I've implemented in the Sequoia backend: the Sequoia backend returns all subkeys, even if they are revoked (it checks their validity when verifying a signature). That's easy enough to fix.

The second thing is that the "creation time" for the primary key appears to refer to the binding signature's creation time and not the primary key's creation time. Compare: 623f6177 => Sat Mar 26 18:54:47 UTC 2022, and 623f5e80 => Sat Mar 26 18:42:08 UTC 2022:

Public-Key Packet, old CTB, 51 bytes
    Version: 4
    Creation time: 2022-03-26 18:42:08 UTC
...
$ sq packet dump rpm.org-ed25519-subkey-test.pub
User ID Packet, old CTB, 121 bytes
    Value: test rpm.org ed25519 key with valid signing and encryption subkeys and a revoked signing subkey <[email protected]>
  
Signature Packet, old CTB, 154 bytes
    Version: 4
    ...
    Hashed area:
      Issuer Fingerprint: 7B32CBC30D3423162732DD859DD6F039A0E0C1C7
      Signature creation time: 2022-03-26 18:54:47 UTC
...

It's quite unusual to consider the binding signature as part of the certificate's identifier, and it means that the certificate's identifier (gpg-pubkey-a0e0c1c7-623f6177) is unstable: if the certificate is ever updated, then a new binding signature will be created and rpm will give it a new name (I guess).

So, my question: am I understanding the behavior correctly? Is this the desired behavior? If not, can we change the behavior without breaking something?

nwalfield pushed a commit to nwalfield/rpm that referenced this issue Apr 11, 2022
@DemiMarie
Copy link
Contributor

The first thing I see is that there is a slight mismatch between the semantics that I've implemented in the Sequoia backend: the Sequoia backend returns all subkeys, even if they are revoked (it checks their validity when verifying a signature). That's easy enough to fix.

I’d prefer Sequoia’s semantics here. #1993 also returns all subkeys, though it refuses to verify a signature made by an invalid subkey (revoked or cannot sign). I suggest returning RPMRC_NOTTRUSTED here, so that RPM doesn’t get confused when verifying header signatures from the RPMDB. The goal of #1993 is that gpg2 --export --export-options=export-minimal --armor "--output=trusted.asc" -- "$TRUSTED_FINGERPRINT" && rpmkeys --import - < trusted.asc should be safe, and it comes quite close to achieving that goal.

@pmatilai
Copy link
Member

The first thing I see is that there is a slight mismatch between the semantics that I've implemented in the Sequoia backend: the Sequoia backend returns all subkeys, even if they are revoked (it checks their validity when verifying a signature). That's easy enough to fix.

Rpm does not check for revocation currently .. ehm, except that it does since commit a9cca03 which sneaked a check for revocation into what was supposed to be about signing capable subkeys. Yes there is a comment in the code and I even saw that but as all too often happens, the actual meaning got lost in the noise of dealing with cosmetics.

The second thing is that the "creation time" for the primary key appears to refer to the binding signature's creation time and not the primary key's creation time. [...] It's quite unusual to consider the binding signature as part of the certificate's identifier [...]

I think the only safe assumption is to assume it a bug in rpm's parser. Shock horror 😆

@nwalfield
Copy link
Contributor Author

I think the only safe assumption is to assume it a bug in rpm's parser. Shock horror 😆

No worries.

I have few goals:

  • The semantics of the OpenPGP functionality should be sane, and understood (i.e., documented)
  • All OpenPGP backends should implement the same semantics.
  • I want to avoid investing too much time in developing new features for the internal implementation.

Clearly there is some tension here. If we change the semantics, then we need to change the internal implementation, or we need to accept that the semantics depend on the backend that is in use.

How should we proceed with this? Should I try to fix the internal OpenPGP implementation? Should we ignore the divergence?

@pmatilai
Copy link
Member

👍 on the goals.

Otherwise I'd just note the bug and chug on, but as the create time ends up in the gpg-pubkey header release I guess we need to do something about it. Lets see if it's easy to fix in the internal parser, and if not then we can consider avoiding the release string in (some) of the tests and mark the current behavior as expected failure (in others) if necessary.

@DemiMarie
Copy link
Contributor

Lets see if it's easy to fix in the internal parser

It’s very easy indeed.

@pmatilai
Copy link
Member

It’s very easy indeed.

Sounds like we have a volunteer, then 😁

@DemiMarie
Copy link
Contributor

It’s very easy indeed.

Sounds like we have a volunteer, then 😁

We do 😆

DemiMarie added a commit to DemiMarie/rpm that referenced this issue Apr 12, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes rpm-software-management#2004.
nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 12, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See rpm-software-management#2004.
@nwalfield
Copy link
Contributor Author

I added a test case on top of @DemiMarie patch (thanks for that!).

nwalfield added a commit to nwalfield/rpm that referenced this issue Apr 12, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See rpm-software-management#2004.
@DemiMarie
Copy link
Contributor

I added a test case on top of @DemiMarie patch (thanks for that!).

You’re welcome!

@DemiMarie
Copy link
Contributor

@nwalfield @pmatilai Is the purpose of the existing behavior to make updating a key look like upgrading a package? The identifier in question is the package release, and I wonder if the idea is that a newer key is essentially a newer release of the older key.

pmatilai pushed a commit that referenced this issue Apr 13, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See #2004.
@nwalfield
Copy link
Contributor Author

@nwalfield @pmatilai Is the purpose of the existing behavior to make updating a key look like upgrading a package?

Good question. I don't know.

pmatilai pushed a commit that referenced this issue Apr 13, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes #2004.

(backported from commit 2b48aa7)
pmatilai pushed a commit that referenced this issue Apr 13, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See #2004.

(cherry picked from commit ee2f59c)
@pmatilai
Copy link
Member

I don't know either, gpg-pubkey headers were added well before my time.

That said I think it looks intentionally that way, with timestamp on the release, only never actually implemented.

@DemiMarie
Copy link
Contributor

I don't know either, gpg-pubkey headers were added well before my time.

That said I think it looks intentionally that way, with timestamp on the release, only never actually implemented.

What do you mean?

@pmatilai
Copy link
Member

I mean the "upgrade" was never really implemented.

dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 7, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes rpm-software-management#2004.

Backported from commit 2b48aa7
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 7, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See rpm-software-management#2004.

(cherry picked from commit ee2f59c)
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 7, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes rpm-software-management#2004.

Backported from commit 2b48aa7
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 7, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See rpm-software-management#2004.

(cherry picked from commit ee2f59c)
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 7, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes rpm-software-management#2004.

Backported from commit 2b48aa7
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 7, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See rpm-software-management#2004.

(cherry picked from commit ee2f59c)
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 8, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes rpm-software-management#2004.

Backported from commit 2b48aa7
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 8, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See rpm-software-management#2004.

(cherry picked from commit ee2f59c)
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 30, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes rpm-software-management#2004.

Backported from commit 2b48aa7
dmnks pushed a commit to dmnks/rpm that referenced this issue Jun 30, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See rpm-software-management#2004.

(cherry picked from commit ee2f59c)
dmnks pushed a commit that referenced this issue Jul 1, 2022
The public key parser needs to set PGPDIG_SAVED_TIME, so that future
iterations in pgpDigParams() do not clobber the key’s creation time.

Fixes #2004.

Backported from commit 2b48aa7
dmnks pushed a commit that referenced this issue Jul 1, 2022
When getting a certificate's creation time, assert that the
certificate's creation time (the Primary Key's creation time field) is
used, not the active binding signature's creation time.

See #2004.

(cherry picked from commit ee2f59c)
@mlschroe
Copy link
Contributor

mlschroe commented Apr 3, 2024

This somehow slipped my radar. The "time" used in rpm is not supposed to be the key creation time, but the last time the key was changed. I don't think you should break this.

@mlschroe mlschroe reopened this Apr 3, 2024
@mlschroe
Copy link
Contributor

mlschroe commented Apr 3, 2024

I.e. pgpDigParamsCreationTime() is somewhat misnamed, it does not the key creation time.

@mlschroe
Copy link
Contributor

mlschroe commented Apr 3, 2024

It needs to get a new release when the key us updated, otherwise the rpm --import will just do nothing.

@mlschroe
Copy link
Contributor

mlschroe commented Apr 3, 2024

OTOH rpm only looks at the keyid to check if the key is already present since some time...

@mlschroe
Copy link
Contributor

mlschroe commented Apr 3, 2024

...since the keyring changes done in 2008. I'm so out of touch with rpm...

@nwalfield
Copy link
Contributor Author

It needs to get a new release when the key us updated, otherwise the rpm --import will just do nothing.

That is at best a heuristic. Even if you take the creation time of the most recent component or signature, you aren't guaranteed to have the latest version of the certificate. A certificate is composed of multiple packets, and some can be stripped. keys.openpgp.org, for instance, does this: it only returns user ID packets that it has verified.

@mlschroe
Copy link
Contributor

mlschroe commented Apr 5, 2024

Yes. The old code was very stupid in that regard, it just took the time from the first signature. It didn't even check if the signature really was a self-signature.

My proposal is to add a "pgpDigParamsModificationTime()" that returns the maximum of all self-signature creation time (they can all be verified). That's pretty much what the old code should have done since the beginning.

We could use that time to simulate a "release date" for the key

@nwalfield
Copy link
Contributor Author

My proposal is to add a "pgpDigParamsModificationTime()" that returns the maximum of all self-signature creation time (they can all be verified). That's pretty much what the old code should have done since the beginning.

That doesn't work. It is at best a heuristic. Even if you take the creation time of the most recent component or signature, you aren't guaranteed to have the latest version of the certificate. A certificate is composed of multiple packets, and some can be stripped. keys.openpgp.org, for instance, does this: it only returns user ID packets that it has verified.

@mlschroe
Copy link
Contributor

mlschroe commented Apr 5, 2024

I know that. It does not need to be 100% correct (it obviously can't). The use case is to have a different release when the expire time of a key is extended. For example, SUSE has those keys:

build-rsa-307e3d54-44201d5d.asc
build-rsa-307e3d54-4be01a65.asc
build-rsa-307e3d54-53287cdc.asc
build-rsa-307e3d54-5aaa90a5.asc
build-rsa-307e3d54-61dc47d0.asc

@mlschroe
Copy link
Contributor

mlschroe commented Apr 5, 2024

And you should certainly not ask a keyserver for keys you want to import into the rpm database.

@nwalfield
Copy link
Contributor Author

I know that. It does not need to be 100% correct (it obviously can't). The use case is to have a different release when the expire time of a key is extended.

In your model, do you think the user should authenticate the whole certificate? That seems implausible in practice. In OpenPGP, the usual way to authenticate a certificate is to authenticate the fingerprint, and then use the binding signatures to authenticate the various components.

Also, I think your proposed heuristic introduces an attack vector. AIUI, an attacker would be able to provide a user with a subset of the certificate, and rpm won't import a more complete version as long as there are no newer components or signatures, which could result in a DoS.

And you should certainly not ask a keyserver for keys you want to import into the rpm database.

You certainly should. It is problematic to not check for updates on public directories. For instance, I want to promptly know about revocations.

@mlschroe
Copy link
Contributor

mlschroe commented Apr 5, 2024

I don't get that. Currently rpm will not import anything at all if the keyid is already known. I'm not even talking about what rpm --import does (it should probably merge pgp packets), I'm just talking about how the pseudo-package rpm generates is named.

@nwalfield
Copy link
Contributor Author

I was responding to your stated convictions, e.g., "you should certainly not ask a keyserver for keys." My impression is that you haven't thought this stuff through, so I was trying to point out some flaws in your thinking.

@mlschroe
Copy link
Contributor

mlschroe commented Apr 8, 2024

You can't trust keys.openpgp.org to only return key material for the query, so you need to check the returned data to make sure it doesn't contain an extra pubkey.
It would be safe if rpmkeys had a --freshen option that makes sure no new pubkeys are imported.

@nwalfield
Copy link
Contributor Author

You can't trust keys.openpgp.org to only return key material for the query, so you need to check the returned data to make sure it doesn't contain an extra pubkey.

Yes. Your trust root is the fingerprint (which is what is normally authenticated--if anything is authenticated at all), and then you chain forward via the binding signatures and drop or ignore any components that you can't authenticate.

It would be safe if rpmkeys had a --freshen option that makes sure no new pubkeys are imported.

Yes that would indeed be an improvement.

@dmnks dmnks added this to RPM Apr 10, 2024
@github-project-automation github-project-automation bot moved this to Backlog in RPM Apr 10, 2024
@dmnks dmnks moved this from Backlog to Todo in RPM Apr 10, 2024
@dmnks dmnks added the crypto Signatures, keys, hashes and their verification label Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crypto Signatures, keys, hashes and their verification
Projects
Status: Todo
Development

Successfully merging a pull request may close this issue.

5 participants