feat: add keystore package #2756

TBonnin · 2024-09-23T14:58:30Z

Describe your changes

A new keystore package containing logic to create, store, retrieve and delete private keys.
I decided to make it an independant package instead of adding it to server so it can be self-contained and reusable. It means the table migrations are different (which creates new migrations table) but also means the package can be used independently.
The db client must be provided though so it can use the same as the one used by the service importing keystore.
In this PR I added keystore to server. In next PR I will add the endpoint used in the new auth flow to create and check session token

Issue ticket number and link

https://linear.app/nango/issue/NAN-1758/implement-keystore-package

Checklist before requesting a review (skip if just adding/editing APIs & templates)

I added tests, otherwise the reason is:
I added observability, otherwise the reason is:
I added analytics, otherwise the reason is:

linear · 2024-09-23T14:58:32Z

NAN-1758 Implement keystore package

TBonnin · 2024-09-23T14:59:49Z

packages/keystore/lib/db/migrations/20240920184658_initial_keystore_model.ts

+            CREATE TYPE entity_types AS ENUM (
+                'session',
+                'connection',
+                'environment'


we might eventually want to use this package to handle environment private keys.

TBonnin · 2024-09-23T15:00:29Z

packages/keystore/lib/models/privatekeys.ts

+    { displayName, entityType, entityId, ttlInMs }: Pick<PrivateKey, 'displayName' | 'entityType' | 'entityId'> & { ttlInMs?: number }
+): Promise<Result<string, PrivateKeyError>> {
+    const now = new Date();
+    const prefix = entityType.slice(0, 4);


adding a prefix to make it easier to debug

TBonnin · 2024-09-23T15:00:46Z

packages/keystore/lib/models/privatekeys.ts

+): Promise<Result<string, PrivateKeyError>> {
+    const now = new Date();
+    const prefix = entityType.slice(0, 4);
+    const random = crypto.randomBytes(32).toString('hex');


I picked a size of 32 bytes but I am happy to reconsider

bodinsamuel

Looks good, a few open questions and suggestions

bodinsamuel · 2024-09-23T15:36:14Z

packages/keystore/lib/db/migrations/20240920184658_initial_keystore_model.ts

+        await trx.raw(`
+            CREATE TABLE IF NOT EXISTS ${PRIVATE_KEYS_TABLE} (
+                id SERIAL PRIMARY KEY,
+                display_name VARCHAR(255) NOT NULL,


missing env_id and/or account_id to be able to quickly select / delete all of them. Especially if we have a put environment keys inside

packages/keystore/lib/types.ts

packages/keystore/lib/models/privatekeys.ts

packages/keystore/lib/db/migrations/20240920184658_initial_keystore_model.ts

packages/keystore/lib/models/privatekeys.ts

packages/keystore/lib/db/migrations/20240920184658_initial_keystore_model.ts

packages/keystore/lib/models/privatekeys.ts

bodinsamuel · 2024-09-23T16:13:28Z

packages/keystore/lib/db/migrate.ts

@@ -0,0 +1,32 @@
+import path from 'node:path';


I understand why you created a dedicated file, however, it's going to create two lock tables for each package that uses this pattern. I don't have a strong opinion because it's definitely handy to split everything, just not sure it's worth it imo

It is true the package could assume the tables exist and have a single place that ensure the database is in the correct state. The cost is two extra lock tables, the advantage if we ever use the package in another service is that we don't have to worry about order of deployment/migration since it is embedded in keystore package.
It also make testing easier, the package can just run the keystore related migrations without having to setup the entire db.

btw we are also creating extra migrations tables for the scheduler.

What if the migrations tables were in their own migrations schema?

Yeah, the main difference is that it's not in the same schema. Let's discuss tomorrow ☺️

TBonnin · 2024-09-23T19:02:21Z

@bodinsamuel thank you very much for the review. I think I have addressed all your comments.
I'll ping you to discuss further about the migration tables

packages/types/lib/keystore/index.ts

keystore package contains logic to create, store, retrieve and delete private keys

## Describe your changes A new keystore package containing logic to create, store, retrieve and delete private keys. I decided to make it an independant package instead of adding it to `server` so it can be self-contained and reusable. It means the table migrations are different (which creates new migrations table) but also means the package can be used independently. The db client must be provided though so it can use the same as the one used by the service importing `keystore`. In this PR I added keystore to `server`. In next PR I will add the endpoint used in the new auth flow to create and check session token ## Issue ticket number and link https://linear.app/nango/issue/NAN-1758/implement-keystore-package ## Checklist before requesting a review (skip if just adding/editing APIs & templates) - [x] I added tests, otherwise the reason is: - [x] I added observability, otherwise the reason is: - [ ] I added analytics, otherwise the reason is:

TBonnin requested review from bodinsamuel and khaliqgant September 23, 2024 14:58

TBonnin commented Sep 23, 2024

View reviewed changes

TBonnin force-pushed the tbonnin/nan-1758/keystore branch from b428a28 to 2463280 Compare September 23, 2024 15:27

bodinsamuel reviewed Sep 23, 2024

View reviewed changes

TBonnin force-pushed the tbonnin/nan-1758/keystore branch from 969ecca to c27c146 Compare September 23, 2024 18:58

TBonnin requested a review from bodinsamuel September 23, 2024 19:01

TBonnin force-pushed the tbonnin/nan-1758/keystore branch from c27c146 to 659eca6 Compare September 23, 2024 19:25

bodinsamuel approved these changes Sep 24, 2024

View reviewed changes

packages/types/lib/keystore/index.ts Outdated Show resolved Hide resolved

TBonnin force-pushed the tbonnin/nan-1758/keystore branch 2 times, most recently from acc49ea to 67f8eb9 Compare September 24, 2024 14:19

TBonnin added 3 commits September 24, 2024 10:34

feat: add keystore package

1daeec2

keystore package contains logic to create, store, retrieve and delete private keys

fixup! feat: add keystore package

82bd8ff

fixup! feat: add keystore package

3b7ea4e

TBonnin force-pushed the tbonnin/nan-1758/keystore branch from 67f8eb9 to 3b7ea4e Compare September 24, 2024 14:34

TBonnin enabled auto-merge (squash) September 24, 2024 14:34

TBonnin merged commit 89e68f0 into master Sep 24, 2024
22 checks passed

TBonnin deleted the tbonnin/nan-1758/keystore branch September 24, 2024 14:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add keystore package #2756

feat: add keystore package #2756

TBonnin commented Sep 23, 2024

linear bot commented Sep 23, 2024

TBonnin Sep 23, 2024

TBonnin Sep 23, 2024

TBonnin Sep 23, 2024

bodinsamuel left a comment

bodinsamuel Sep 23, 2024

bodinsamuel Sep 23, 2024

TBonnin Sep 23, 2024 •

edited

Loading

bodinsamuel Sep 24, 2024

TBonnin commented Sep 23, 2024

feat: add keystore package #2756

feat: add keystore package #2756

Conversation

TBonnin commented Sep 23, 2024

Describe your changes

Issue ticket number and link

Checklist before requesting a review (skip if just adding/editing APIs & templates)

linear bot commented Sep 23, 2024

TBonnin Sep 23, 2024

Choose a reason for hiding this comment

TBonnin Sep 23, 2024

Choose a reason for hiding this comment

TBonnin Sep 23, 2024

Choose a reason for hiding this comment

bodinsamuel left a comment

Choose a reason for hiding this comment

bodinsamuel Sep 23, 2024

Choose a reason for hiding this comment

bodinsamuel Sep 23, 2024

Choose a reason for hiding this comment

TBonnin Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

bodinsamuel Sep 24, 2024

Choose a reason for hiding this comment

TBonnin commented Sep 23, 2024

TBonnin Sep 23, 2024 •

edited

Loading