Isolate secrets from others extensions #13

brichet · 2025-04-23T13:55:51Z

This PR aims to isolate the secrets from others extensions.

It includes two main ideas to prevent other extensions from recovering or defining secrets that don't belong to them.

Do not expose the SecretsConnector.
Before this PR, the secrets connector was exposed by a token, that was consumed by the SecretsManager. This token could therefore be consumed by any other extension, which could fetch the secrets of a any namespace.
With this PR, the secrets connector must be set to the secrets manager by the plugin itself. If several plugins try to set a connector, the manager is locked.
Allow only one extension per namespace (credit to @afshin for the idea and its implementation).
When a plugin want to use a namespace with the secrets manager, it has to 'sign in' and get a unique token. This token is required anytime the plugin needs to interact with the SecretsConnector (get, set, list or remove secrets).
The secrets manager should be the only one able to access the Map objects that keep the relationship between the token and the namespace. To do so, it uses a private namespace that stores:
- the namespace/token relationship
- the locked status
- the connector to use

Should fix #12

How to use it

const PLUGIN_ID = 'my-extension:my-plugin';
const testing: JupyterFrontEndPlugin<void> = SecretsManager.sign(
  PLUGIN_ID,
  token => ({
    id: PLUGIN_ID,
    requires: [ISecretsManager],
    autoStart: true,
    activate: (app: JupyterFrontEnd, manager: ISecretsManager): void => {
      // Set the secret
      manager.set(token, PLUGIN_ID, 'secretID', {
        namespace: PLUGIN_ID,
        id: 'secretID',
        value: 'secret-value'
      });

      // Get the secret
      const secret = await manager.get(token, PLUGIN_ID, 'secretID');
    }
  })
);

If the token is used in an object, for privacy it should also use a Private namespace to be stored.

cc. @afshin @jtpio @trungleduc

…secrets namespace

src/manager.ts

afshin · 2025-04-23T15:18:57Z

src/manager.ts

+      throw new Error(
+        `The secrets namespace ${namespace} is not available with the provided token`
+      );


Suggested change

throw new Error(

`The secrets namespace ${namespace} is not available with the provided token`

);

Private.lock(

`The secrets namespace ${namespace} is not available with the provided token`

);

Don't know if we want to lock the manager on any failing attempt. It should not occurs, though.

I don't feel strongly about this, but the only times I expect this to happen are in development so I erred on the side of security. I'm happy to defer to your judgement.

You could use a strict flag that can be set when starting jupyterlab.

src/manager.ts

afshin · 2025-04-23T15:29:33Z

I am a little unsure about the Promise<any> return types of some of the connector methods. Should they be void or unknown instead? Or do you think they should remain any?

afshin · 2025-04-23T15:29:57Z

(That doesn't need to be resolved in this PR, btw.)

Co-authored-by: Afshin Taylor Darian <git@darian.link>

src/manager.ts

trungleduc · 2025-04-23T20:31:44Z

Thanks @brichet for working on it!
What is the relation between a namespace and the unique connector? If i have 2 extensions that sign with the SecretsManager with different PLUGIN_ID, can both call the fetch method of the connector?

src/manager.ts

brichet · 2025-04-23T21:35:48Z

What is the relation between a namespace and the unique connector? If i have 2 extensions that sign with the SecretsManager with different PLUGIN_ID, can both call the fetch method of the connector?

The connector doesn't know the namespaces, it only associates an id to a secret.
The id is a string namespace:secretID, this is why the connector must be accessible only from the manager. Otherwise it would be easy to fetch the secrets.

So yes, the unique connector should handle several namespaces. And actually the current design doesn't allow registering several connectors.

trungleduc · 2025-04-23T22:00:27Z

So, how can I prevent an extension from fetching my AI provider secret key? I want only jupyterlite-ai can fetch this secret

trungleduc · 2025-04-23T22:05:48Z

Ah I understand now. I can have logic inside my connector.fetch to check the namespace of the secret ID and allow only the namespace claimed by jupyterlite-ai to do the real fetch.

brichet · 2025-04-24T07:48:50Z

So, how can I prevent an extension from fetching my AI provider secret key? I want only jupyterlite-ai can fetch this secret

An extension can only use the manager to fetch a secret, it should not be able to use the connector directly. And the manager only allow signed in extension to fetch its namespace...

brichet · 2025-04-24T11:22:29Z

src/manager.ts

  /**
   * Actually fetch the secret from the connector.
   */
  private async _get(id: string): Promise<ISecret | undefined> {


I'm also worried about this kind of method, that doesn't check the token.
This method is still available in javascript, even if private in typescript context.

you can move it into the Private namespace?

👍 I can make them static and move them to the Private namespace.

…ret ID ('namespace:id')

src/manager.ts

afshin · 2025-04-24T13:58:58Z

I would suggest making a:

get ready(): Promise<void> {
  return this._ready.promise;
}

And then in all the async methods, get, list, attach, etc., have await this.ready; near the top.

afshin · 2025-04-24T13:59:34Z

Also unless it's important not to, since attach and detachAll are async, perhaps detach should be as well?

brichet · 2025-04-24T14:08:46Z

I would suggest making a:
get ready(): Promise<void> {
  return this._ready.promise;
}
And then in all the async methods, get, list, attach, etc., have await this.ready; near the top.

It's already the case. Actually there are 2 PromiseDelegate that can be awaited:

_ready which is resolved when the connector is set. This promise is awaited for any action related to the connector.
_storing which is set during data storage on the connector, and resolved when it's over. This one is awaited when getting the secrets.

afshin · 2025-04-24T14:11:08Z

Right, I see those promise delegates, I mean to not await the .promise attribute of a PromiseDelegate and just to have a public or protected promise called simply ready.

trungleduc · 2025-04-24T19:26:47Z

src/manager.ts

-    const attachedId = Private.buildSecretId(namespace, id);
+    Private.checkNamespace(token, namespace);
+    const attachedId = Private.buildConnectorId(namespace, id);
    const attachedInput = this._attachedInputs.get(attachedId);


_attachedInputs should be in the private namespace, because i could do this in my extension

secretManager['_attachedInputs'].forEach(it => { it.addeventlistener(...) })

and get the value of the input when the secret value is filled.

Yes, I realized that as well, but your extension can also do

document.querySelector('[data-namespace]')

And I don't think that we can do anything about that.

You don't have to save namespace and secret id to the input, just reverse the _attachedInputs map is enough to find these value from input element

Yes, I agree that it would still be better to not expose it, but the extension can still do

document.querySelectorAll('input[type=password]')

yeah, at this point we can't do anything

brichet · 2025-04-25T09:27:32Z

Thanks @afshin and @trungleduc for your helpful comments and suggestions.

brichet added 2 commits April 23, 2025 14:16

Requires plugin to 'sign' to the secrets manager to be able to use a …

823fe44

…secrets namespace

Connector must be set to the manager, instead of providing a token

887fdd8

brichet added the enhancement New feature or request label Apr 23, 2025

brichet added 2 commits April 23, 2025 16:48

Add dependency to @jupyterlab/coreutils

3787fad

Export the SecretsManager

6cced74

afshin reviewed Apr 23, 2025

View reviewed changes

Apply suggestions from code review

51cd331

Co-authored-by: Afshin Taylor Darian <git@darian.link>

afshin reviewed Apr 23, 2025

View reviewed changes

src/manager.ts Outdated Show resolved Hide resolved

trungleduc reviewed Apr 23, 2025

View reviewed changes

src/manager.ts Show resolved Hide resolved

brichet commented Apr 24, 2025

View reviewed changes

brichet added 2 commits April 24, 2025 14:40

Wait for the connector to be provided before any action

f55089e

Avoid confusion between namespace and id when retrieving the full sec…

e3c3a3e

…ret ID ('namespace:id')

afshin reviewed Apr 24, 2025

View reviewed changes

src/manager.ts Show resolved Hide resolved

brichet added 2 commits April 24, 2025 15:53

Move the connector getter/setter in private namespace

5c5a2b5

Update comments

309dd1b

brichet added 2 commits April 24, 2025 16:16

Set detach method as async

6e4a826

Better handling of the promise delegate

142a5f2

brichet marked this pull request as ready for review April 24, 2025 14:23

trungleduc reviewed Apr 24, 2025

View reviewed changes

trungleduc approved these changes Apr 25, 2025

View reviewed changes

Make the inputs less discovarable

d9fe0e7

brichet merged commit 96bb498 into jupyterlab-contrib:main Apr 25, 2025
6 checks passed

brichet deleted the isolate_secrets_from_others_extensions branch April 25, 2025 10:07

brichet mentioned this pull request Apr 25, 2025

Update secrets manager to >=0.3.0 jupyterlite/ai#63

Merged

Isolate secrets from others extensions #13

Isolate secrets from others extensions #13

Uh oh!

Conversation

brichet commented Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use it

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

afshin commented Apr 23, 2025

Uh oh!

afshin commented Apr 23, 2025

Uh oh!

Uh oh!

trungleduc commented Apr 23, 2025

Uh oh!

Uh oh!

brichet commented Apr 23, 2025

Uh oh!

trungleduc commented Apr 23, 2025

Uh oh!

trungleduc commented Apr 23, 2025

Uh oh!

brichet commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

afshin commented Apr 24, 2025

Uh oh!

afshin commented Apr 24, 2025

Uh oh!

brichet commented Apr 24, 2025

Uh oh!

afshin commented Apr 24, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brichet Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brichet Apr 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brichet commented Apr 25, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brichet commented Apr 23, 2025 •

edited

Loading

brichet Apr 25, 2025 •

edited

Loading

brichet Apr 25, 2025 •

edited

Loading