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

Support ferretdb to replace mongodb #142

Open
pravi opened this issue May 15, 2024 · 21 comments
Open

Support ferretdb to replace mongodb #142

pravi opened this issue May 15, 2024 · 21 comments

Comments

@pravi
Copy link
Contributor

pravi commented May 15, 2024

FerretDB aims to replace MongoDB since the latter is no longer Free Software.

At present it fails with MongoServerError: Index option "expireAfterSeconds" is not implemented yet

But it could be worked around until this is implemented, FerretDB/FerretDB#2415 (comment) "include a check for the property in question in your actual query, so results that are too old just don't come back, and do your own purges of stuff older than that periodically."

It would be nice to be able to setup mailvelope keyserver with fully Free Software dependencies.

@pravi
Copy link
Contributor Author

pravi commented May 16, 2024

I was able to get the keyserver to start with this change,

diff --git a/src/modules/public-key.js b/src/modules/public-key.js
index f6210ba..dbb6d47 100644
--- a/src/modules/public-key.js
+++ b/src/modules/public-key.js
@@ -54,7 +54,7 @@ class PublicKey {
 
   async init() {
     // create time to live (TTL) index to purge unverified keys
-    await this._mongo.createIndexes([{key: {verifyUntil: 1}, expireAfterSeconds: 1}], DB_TYPE);
+    await this._mongo.createIndexes([{key: {verifyUntil: 1}}], DB_TYPE);
   }
 
   /**

I could upload keys and receive verification emails, but after verifying emails, search gives an error "unknown operator: email" and trying to remove a key results in "User ID not found" when clicking on the verification link received via email. Not sure if this is related to the above change or something missing in ferretdb.

@pravi
Copy link
Contributor Author

pravi commented May 16, 2024

npm test shows 6 failing tests


 1) Mongo Integration Tests
       "before all" hook in "Mongo Integration Tests":
     MongoParseError: authMechanism one of MONGODB-AWS,MONGODB-CR,DEFAULT,GSSAPI,PLAIN,SCR
AM-SHA-1,SCRAM-SHA-256,MONGODB-X509,MONGODB-OIDC, got PLAIN-int
      at transform (node_modules/mongodb/lib/connection_string.js:486:23)
      at setOption (node_modules/mongodb/lib/connection_string.js:458:36)
      at parseOptions (node_modules/mongodb/lib/connection_string.js:283:13)
      at new MongoClient (node_modules/mongodb/lib/mongo_client.js:51:63)
      at Mongo.init (src/modules/mongo.js:25:20)
      at Context.<anonymous> (test/integration/mongo-test.js:19:17)
      at process.processImmediate (node:internal/timers:476:21)

  2) Mongo Integration Tests
       "after all" hook in "Mongo Integration Tests":
     TypeError: Cannot read properties of undefined (reading 'collection')
      at Mongo.clear (src/modules/mongo.js:135:26)
      at Context.<anonymous> (test/integration/mongo-test.js:28:17)
      at process.processImmediate (node:internal/timers:476:21)

  3) Public Key Integration Tests
       "before all" hook in "Public Key Integration Tests":
     MongoParseError: authMechanism one of MONGODB-AWS,MONGODB-CR,DEFAULT,GSSAPI,PLAIN,SCRAM-SHA-1,SCRAM-SHA-256,MONGODB-X509,MONGODB-OIDC, got PLAIN-int
      at transform (node_modules/mongodb/lib/connection_string.js:486:23)
      at setOption (node_modules/mongodb/lib/connection_string.js:458:36)
      at parseOptions (node_modules/mongodb/lib/connection_string.js:283:13)
      at new MongoClient (node_modules/mongodb/lib/mongo_client.js:51:63)
      at Mongo.init (src/modules/mongo.js:25:20)
      at Context.<anonymous> (test/integration/public-key-test.js:42:17)

  4) Public Key Integration Tests
       "after all" hook in "Public Key Integration Tests":
     TypeError: Cannot read properties of undefined (reading 'collection')
      at Mongo.clear (src/modules/mongo.js:135:26)
      at Context.<anonymous> (test/integration/public-key-test.js:82:17)
      at process.processImmediate (node:internal/timers:476:21)

  5) Key Server Integration Tests
       "before all" hook in "Key Server Integration Tests":
     MongoParseError: authMechanism one of MONGODB-AWS,MONGODB-CR,DEFAULT,GSSAPI,PLAIN,SCRAM-SHA-1,SCRAM-SHA-256,MONGODB-X509,MONGODB-OIDC, got PLAIN-int
      at transform (node_modules/mongodb/lib/connection_string.js:486:23)
      at setOption (node_modules/mongodb/lib/connection_string.js:458:36)
      at parseOptions (node_modules/mongodb/lib/connection_string.js:283:13)
      at new MongoClient (node_modules/mongodb/lib/mongo_client.js:51:63)
      at Mongo.init (src/modules/mongo.js:25:20)
      at Context.<anonymous> (test/integration/server-test.js:31:17)

  6) Key Server Integration Tests
       "after all" hook in "Key Server Integration Tests":
     TypeError: Cannot read properties of undefined (reading 'collection')
      at Mongo.clear (src/modules/mongo.js:135:26)
      at Context.<anonymous> (test/integration/server-test.js:54:17)
      at process.processImmediate (node:internal/timers:476:21)

@toberndo
Copy link
Member

But it could be worked around until this is implemented, FerretDB/FerretDB#2415 (comment) "include a check for the property in question in your actual query, so results that are too old just don't come back, and do your own purges of stuff older than that periodically."

Here you can revert bf2b856 to bring back unverified key purges without requiring TTL.

@toberndo
Copy link
Member

I could upload keys and receive verification emails, but after verifying emails, search gives an error "unknown operator: email" and trying to remove a key results in "User ID not found" when clicking on the verification link received via email. Not sure if this is related to the above change or something missing in ferretdb.

Does not sound like these errors are related to the change.

@pravi
Copy link
Contributor Author

pravi commented May 16, 2024

Does not sound like these errors are related to the change.
There are no errors in the console even with LOG_LEVEL=debug passed to npm start. Any other way to get more verbose logs?

@pravi
Copy link
Contributor Author

pravi commented May 16, 2024

Here you can revert bf2b856 to bring back unverified key purges without requiring TTL.

git revert does not work for this commit, so I checked out its parent commit. But same error for searching. I created the recommended indexes too. Any ideas for further debugging?

@pravi
Copy link
Contributor Author

pravi commented May 16, 2024

When I run db.publickey.find() in mogosh, I see all uploaded keys have verified: false, so it seems the problem is in key verification.


_id: ObjectId('6645f71ce1fc4753fd1365f4'),
    keyId: '8f53e0193b294b75',
    fingerprint: 'd30863e26020e543f4719a838f53e0193b294b75',
    userIds: [
      {
        name: 'Praveen Arimbrathodiyil',
        email: 'praveen@onenetbeyond.org',
        verified: false,
        publicKeyArmored: '-----BEGIN PGP PUBLIC KEY BLOCK-----\n' +

Though clicking on the email link says "Email address praveen@onenetbeyond.org successfully verified"

image

@pravi
Copy link
Contributor Author

pravi commented May 16, 2024

We should probably double check verified field is actually set to true before showing the verification successful message. Could this be a problem with ferretdb missing some features or behaving differently?

@toberndo
Copy link
Member

The verified field is set here: https://github.com/mailvelope/keyserver/blob/master/src/modules/public-key.js#L212
Maybe ferretdb does not work well with the userIds.$.verified semantic?

@pravi
Copy link
Contributor Author

pravi commented May 17, 2024

The verified field is set here: https://github.com/mailvelope/keyserver/blob/master/src/modules/public-key.js#L212 Maybe ferretdb does not work well with the userIds.$.verified semantic?

Searching for .$. gave this https://github.com/FerretDB/FerretDB/blob/def90a13a31e901361d04038792452f14e78a534/internal/handler/common/projection.go#L106 so it looks like positional projection can only be used at the end. Is there another way to express the same?

FerretDB/FerretDB#835

@pravi
Copy link
Contributor Author

pravi commented May 17, 2024

@asdofindia
Copy link

The update query matches nothing with ferretdb

ferretdb> db.publickey.updateOne({keyId: 'e19afa78dce1e3dc','userIds.nonce': '583499bb46c7f6177ec6ecf09274640b'}, {"$set": {verifyUntil: null}})
{
  acknowledged: true,
  insertedId: null,
  matchedCount: 0,
  modifiedCount: 0,
  upsertedCount: 0
}
ferretdb> db.publickey.updateOne({keyId: 'e19afa78dce1e3dc'}, {"$set": {verifyUntil: null}})
{
  acknowledged: true,
  insertedId: null,
  matchedCount: 1,
  modifiedCount: 1,
  upsertedCount: 0
}

This is likely because ferretdb does not support nested arrays.

And userIds is a nested array (an array of embedded documents).

So the query there which includes userIds.nonce is failing to match anything. (Please note that find, and findOne does seem to work with nested array with ferretdb, but only update fails).

This diff might help to get past this:

❯ git diff src/modules/public-key.js
diff --git a/src/modules/public-key.js b/src/modules/public-key.js
index f6210ba..8cc1810 100644
--- a/src/modules/public-key.js
+++ b/src/modules/public-key.js
@@ -209,12 +209,22 @@ class PublicKey {
       publicKeyArmored = await this._pgp.updateKey(key.publicKeyArmored, publicKeyArmored);
     }
     // flag the user ID as verified
-    await this._mongo.update(query, {
+    const updatedUserIds = key.userIds.map(uid => {
+      if (uid.nonce === nonce) {
+        return {
+          verified: true,
+          nonce: null,
+          publicKeyArmored: null,
+          name: uid.name,
+          email: uid.email
+        };
+      }
+      return uid;
+    });
+    await this._mongo.update({'_id': key['_id']}, {
       publicKeyArmored,
-      'userIds.$.verified': true,
-      'userIds.$.nonce': null,
-      'userIds.$.publicKeyArmored': null,
-      verifyUntil: null
+      verifyUntil: null,
+      userIds: updatedUserIds
     }, DB_TYPE);
     return {email};
   }

But that's again going to be heartbreak as when you search you'll get a new error: "unknown operator: email" which arises from

$elemMatch: {
'email': util.normalizeEmail(uid.email),

The $elemMatch seems to have different behaviour in ferret supporting only comparison operators like "$eq".

You could perhaps work around that too, but at this point, it might be easier to just write a module to make keyserver store data directly in postgres instead of this mongo -> ferret -> postgres incompatibilities

@pravi
Copy link
Contributor Author

pravi commented May 18, 2024

@asdofindia thanks, with this, search by keyid works. For example, https://keys.puri.sm/pks/lookup?op=get&search=0xD30863E26020E543F4719A838F53E0193B294B75

@pravi
Copy link
Contributor Author

pravi commented May 20, 2024

@toberndo do you think switching directly to PostgreSQL is possible?

@pravi
Copy link
Contributor Author

pravi commented May 24, 2024

With the following change, search by email is working.

--- a/src/modules/public-key.js
+++ b/src/modules/public-key.js
@@ -268,11 +268,10 @@ class PublicKey {
     // query by user id
     if (userIds) {
       queries = queries.concat(userIds.map(uid => ({
-        userIds: {
-          $elemMatch: {
-            'email': util.normalizeEmail(uid.email),
-            'verified': true
-          }
+        'userIds.email': {
+            $eq: util.normalizeEmail(uid.email)},
+        'userIds.verified': {
+            $eq: true
         }
       })));
     }

@pravi
Copy link
Contributor Author

pravi commented May 24, 2024

Now removing a key is failing with 'User ID not found' when clicking on the link received by email.

We need to replace 'userIds.$.nonce' like before in

https://github.com/mailvelope/keyserver/blob/master/src/modules/public-key.js#L342

@pravi
Copy link
Contributor Author

pravi commented May 24, 2024

With this change, I'm able to remove by email as well,

--- a/src/modules/public-key.js
+++ b/src/modules/public-key.js
@@ -345,7 +345,19 @@ class PublicKey {
     // flag only the provided user id
     if (email) {
       const nonce = util.random();
-      await this._mongo.update(query, {'userIds.$.nonce': nonce}, DB_TYPE);
+      const flaggedUserIds = key.userIds.map(uid => {
+        if (uid.email === email) {
+          return {
+            nonce: nonce,
+            verified: uid.verified,
+            publicKeyArmored: uid.publicKeyArmored,
+            name: uid.name,
+            email: uid.email
+          }
+        }
+        return uid;
+      });

@pravi
Copy link
Contributor Author

pravi commented May 24, 2024

I have pushed these changes here https://source.puri.sm/Purism/keyserver/-/commits/ferretdb-compat/?ref_type=HEADS

I'd be happy if this is maintained as separate branch in this repo itself.

@asdofindia
Copy link

@pravi happy to hear you got it working. I've done only an eyeball review. In the remove part, as per the code the key can be removed by keyId as well. I'm not sure if your patch supports that. Other changes in https://source.puri.sm/Purism/keyserver/-/compare/master...ferretdb-compat?from_project_id=3240&straight=false looks like they will work (although I'm not sure whether that breaks compatibility with any mongo version)

@pravi
Copy link
Contributor Author

pravi commented May 24, 2024

@asdofindia I saw the remove by keyid part in the code as well, but there is no such option in the web ui. So I thought I could skip that for now.

@toberndo
Copy link
Member

toberndo commented Jun 1, 2024

I have pushed these changes here https://source.puri.sm/Purism/keyserver/-/commits/ferretdb-compat/?ref_type=HEADS

I'd be happy if this is maintained as separate branch in this repo itself.

I pushed the changes to branch https://github.com/mailvelope/keyserver/tree/ferretdb-compat

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

3 participants