Implement openpgp.cert.d based keystore #3437

ffesti · 2024-11-08T13:07:30Z

Refactor code from the fs backend into shared helper functions

This does implement the layout on the file system and the write lock of the openpgp.cert.d proposal according to
https://www.ietf.org/archive/id/draft-nwjw-openpgp-cert-d-00.html but not the Trust root, Petname mapping or Trusted introducers.

This still is a mess of C and C++ style strings that we want to clean up later by adding C++ string based path handling and may be using the filesystem C++ library.

Resolves: #3341

lib/keystore.cc

pmatilai · 2024-11-08T14:13:23Z

lib/keystore.cc

+
+/*****************************************************************************/
+
+static rpmRC acquire_write_lock(rpmtxn txn)


The txn handle is the lock, that's the whole point of those things. There's no other locking needed.

I know. But the standard requires a lockfile on disk as part of the openpgp.cert.d format. If we want other tools to be able to read it we should try to adhere to that.

I don't see concurrent access with other tools being something we need to support. Poking at rpm directories with external tools is more like a developer convenience thing rather than something we'd be suggesting to users. But, if you want to support locking then the actual lock seems to be missing, https://www.ietf.org/archive/id/draft-nwjw-openpgp-cert-d-00.html#platform-specifics says it should use flock().

...ie it's not the existence of 'writelock' file that is the lock, but a flock() on that file. Mere file existence would be broken lock mechanism because if the process dies in the middle it de-facto leaves a stale lock behind, whereas locks on the file are cleaned up by the OS.

urgs should have read the last sentence, too...
If I read that correctly you are not actually required to delete writelock the file again. Just to be fine if it doesn't exist. @nwalfield any opinion on that?

Anyway implemented the flock and adjusted the test case.

If I read that correctly you are not actually required to delete writelock the file again. Just to be fine if it doesn't exist.

Yup, that's how such lockfiles commonly work, including our own transaction lock. Only the term "lockfile" is misleading because it makes you think the file itself is the lock, when it's only a placeholder for it.

pmatilai · 2024-11-11T10:40:17Z

lib/rpmts.cc

@@ -275,6 +275,8 @@ static keystore *getKeystore(rpmts ts)
 	    ts->keystore = new keystore_fs();
 	} else if (rstreq(krtype, "rpmdb")) {
 	    ts->keystore = new keystore_rpmdb();
+	} else if (rstreq(krtype, "openpgp")) {


I've a feeling we want to leave this name for a proper implementation from Sequoia side, this being a simple standalone version that does not deal with the trust stuff. But lets bikeshed that in the ticket instead.

yeah, I am not too sold on the name either.

This will be useful for the next commit. This still is a mess of C and C++ style strings that we want to clean up later by adding C++ string based path handling and may be using the filesystem C++ library. Related: rpm-software-management#3341

pmatilai · 2024-11-11T11:25:19Z

Another random thought on standard compliance: since this does not process trust-root, it should probably emit a warning if that file exists (eg from doing stuff with an external tool) to make it clear it's not handled.

Oh, and on a related note: since the interoperability with other tools is a goal, what better test than actually do so, ie check that sq can read what we produced.

pmatilai · 2024-11-11T11:29:02Z

tests/rpmsigdig.at

+RPMTEST_CLEANUP
+# ------------------------------
+# Test rpmkeys write errors
+AT_SETUP([[rpmkeys -K no space left on stdout]])


This looks like a copy-paste leftover, we already have such a test.

dmnks

Overall LGTM, some comments in line.

lib/keystore.cc

dmnks · 2024-11-11T12:19:21Z

lib/keystore.cc

+	return RPMRC_FAIL;
+
+    string fp = rpmPubkeyFingerprintAsHex(key);
+    string dir = fp.substr(0, 2);


This routine of obtaining the storage location (where the first two hex chars are the directory name) is a specific thing in this keystore implementation and it's currently duplicated in two places so maybe deserves a separate private method.

That said, we can always do that later, when there's the third use, so feel free to ignore.

I have considered that, but it doesn't really make things that much better. Guess it really is missing the third place.

Works for me.

dmnks · 2024-11-11T12:26:54Z

lib/rpmts.cc

@@ -275,6 +275,8 @@ static keystore *getKeystore(rpmts ts)
 	    ts->keystore = new keystore_fs();
 	} else if (rstreq(krtype, "rpmdb")) {
 	    ts->keystore = new keystore_rpmdb();
+	} else if (rstreq(krtype, "openpgp")) {
+	    ts->keystore = new keystore_openpgp_cert_d();


The class name seems a bit verbose to me, how about just keystore_openpgp? I suppose this would be the defacto standard openpgp keystore implementation so it doesn't have to be more specific than that.

Nevermind the above; I've realized that openpgp is way too generic, this whole signature business is about OpenPGP (at least keystore_rpmdb is, too) so just keep the name as is, it's good.

dmnks · 2024-11-11T12:32:22Z

lib/keystore.cc

+    string fp = rpmPubkeyFingerprintAsHex(key);
+    string dir = fp.substr(0, 2);
+    string filename = fp.substr(2);
+    char * filepath = rpmGetPath(rpmtxnRootDir(txn), "%{_keyringpath}/", dir.c_str(), "/", filename.c_str(), NULL);


I see a mixed use of rpmGetPath() and rpmGenPath() in the keystore.cc module. Is there a reason for preferring one over the other in those places?

Well, rpmGenPath was originally used. But as it only supports 3 pieces I used rpmGetPath here.

Ack, thanks.

dmnks · 2024-11-11T13:11:24Z

Insertion doesn't seem to follow the spec:

It says the key being inserted should be merged with an existing key with that fingerprint, if found. Currently, it's just inserted, never looked up beforehand.
It should write to a temporary file and rename(2) it. The current keystore interface has the replace argument that defaults to 1 in the openpgp implementation, however I wonder if there's a point in allowing that to be 0 in the first place, given the spec.

dmnks · 2024-11-11T13:33:49Z

Insertion doesn't seem to follow the spec:

* It says the key being inserted should be merged with an existing key with that fingerprint, if found. Currently, it's just inserted, never looked up beforehand.

That said, it's fine if this is not the goal of this (initial) implementation, in that case having a short note in the commit message would suffice.

ffesti · 2024-11-11T16:29:49Z

As people don't believe me otherwise: rpmtxnImportPubkey does indeed merge keys...

This is a low level storage interface. A lot of functionality about handling keys is in other parts of the code. E.g. the keyring and the rpmts and rpmtxn key handling functions.

dmnks · 2024-11-11T16:32:56Z

As people don't belive me otherwise: rpmtxnImportPubkey does indeed merge keys...

Oh, so it's handled at the level above the keystore itself. Checking that didn't occur to me.

I did notice your reply to Neal above regarding "merging is handled" but didn't connect the dots, so thanks for the context!

This does implement the layout on the file system and the write lock of the openpgp.cert.d proposal according to https://www.ietf.org/archive/id/draft-nwjw-openpgp-cert-d-00.html but not the Trust root, Petname mapping or Trusted introducers. Resolves: rpm-software-management#3341

lib/keystore.cc

dmnks · 2024-11-12T09:47:01Z

The latest version you've pushed looks fine. The only remaining questions are:

Do we care that replace can be set to 0 and thus not use rename(2) for insertion? The latter is mandated by the spec, ~~although it seems redundant, with the writelock in place.~~ and seems to be key for ensuring atomic read access (the writelock is only for writing)
How do we externally name the store

dmnks · 2024-11-12T09:53:22Z

Replying to the first question myself:

rpmtxnImportPubkey() is where replace is decided, based on whether the key can be found. That kinda alleviates the need for an atomic rename since only new keys would be inserted with it disabled.

That still doesn't ensure true atomicity but this is the best we can do without refactoring rpmtxnImportPubkey() or other parts. So I guess it's fine.

ffesti · 2024-11-12T10:21:49Z

Well, it kinda does. By not replacing you make sure you fail if a key was added in the mean time instead of just overwriting it without merging.

Also as far as rpm is concerned we do have the transaction log (which is represented by the txn parameter passed down) whihc protects parallel access from other rpm instances.

ffesti · 2024-11-12T10:23:56Z

I would suggest to not get hung up on the names especially as long as they are only internal. We might even still change the name in the configuration as soon as we know what back ends we'll end up with and what exactly they do.

dmnks · 2024-11-12T11:04:02Z

Ack, let's merge this then.

ffesti requested a review from a team as a code owner November 8, 2024 13:07

ffesti requested review from dmnks and removed request for a team November 8, 2024 13:07

nwalfield reviewed Nov 8, 2024

View reviewed changes

lib/keystore.cc Show resolved Hide resolved

pmatilai reviewed Nov 8, 2024

View reviewed changes

pmatilai reviewed Nov 11, 2024

View reviewed changes

pmatilai mentioned this pull request Nov 11, 2024

Implement a new openpgp.cert.d based keystore #3341

Closed

ffesti force-pushed the openpgp.cert.d branch from 6b56079 to 11bf961 Compare November 11, 2024 11:18

pmatilai reviewed Nov 11, 2024

View reviewed changes

ffesti force-pushed the openpgp.cert.d branch from 11bf961 to b8599f9 Compare November 11, 2024 11:37

dmnks reviewed Nov 11, 2024

View reviewed changes

ffesti force-pushed the openpgp.cert.d branch from b8599f9 to 427e5f9 Compare November 11, 2024 17:23

dmnks reviewed Nov 12, 2024

View reviewed changes

lib/keystore.cc Show resolved Hide resolved

dmnks merged commit 6e19c16 into rpm-software-management:master Nov 12, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement openpgp.cert.d based keystore #3437

Implement openpgp.cert.d based keystore #3437

ffesti commented Nov 8, 2024

pmatilai Nov 8, 2024

ffesti Nov 8, 2024

pmatilai Nov 11, 2024

pmatilai Nov 11, 2024

ffesti Nov 11, 2024

pmatilai Nov 12, 2024 •

edited

Loading

pmatilai Nov 11, 2024

ffesti Nov 11, 2024

pmatilai commented Nov 11, 2024 •

edited

Loading

pmatilai Nov 11, 2024

ffesti Nov 11, 2024

dmnks left a comment

dmnks Nov 11, 2024

ffesti Nov 11, 2024

dmnks Nov 12, 2024

dmnks Nov 11, 2024 •

edited

Loading

dmnks Nov 11, 2024

dmnks Nov 11, 2024 •

edited

Loading

ffesti Nov 11, 2024

dmnks Nov 12, 2024

dmnks commented Nov 11, 2024 •

edited

Loading

dmnks commented Nov 11, 2024

ffesti commented Nov 11, 2024 •

edited

Loading

dmnks commented Nov 11, 2024 •

edited

Loading

dmnks commented Nov 12, 2024 •

edited

Loading

dmnks commented Nov 12, 2024

ffesti commented Nov 12, 2024

ffesti commented Nov 12, 2024

dmnks commented Nov 12, 2024


		/*****************************************************************************/

		static rpmRC acquire_write_lock(rpmtxn txn)

Implement openpgp.cert.d based keystore #3437

Implement openpgp.cert.d based keystore #3437

Conversation

ffesti commented Nov 8, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai Nov 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmatilai commented Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmnks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmnks Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmnks Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmnks commented Nov 11, 2024 • edited Loading

dmnks commented Nov 11, 2024

ffesti commented Nov 11, 2024 • edited Loading

dmnks commented Nov 11, 2024 • edited Loading

dmnks commented Nov 12, 2024 • edited Loading

dmnks commented Nov 12, 2024

ffesti commented Nov 12, 2024

ffesti commented Nov 12, 2024

dmnks commented Nov 12, 2024

pmatilai Nov 12, 2024 •

edited

Loading

pmatilai commented Nov 11, 2024 •

edited

Loading

dmnks Nov 11, 2024 •

edited

Loading

dmnks Nov 11, 2024 •

edited

Loading

dmnks commented Nov 11, 2024 •

edited

Loading

ffesti commented Nov 11, 2024 •

edited

Loading

dmnks commented Nov 11, 2024 •

edited

Loading

dmnks commented Nov 12, 2024 •

edited

Loading