Proposal: add --allow-import=<allow_list> for checking dynamic imports/workers

[bartlomieju]

Context: Currently loading dynamic imports or worker code requires --allow-read and/or --allow-net flags. Appropriate permissions are checked for entry point as well as all dependent modules. This leads to a situation where even quite simple scenarios require to grant a lot of permissions.

Example 1:

// main.ts

console.log("hello");
const a = await import("./dyn_import.ts");
console.log(a);

// dyn_import.ts
import "./subdir/a.ts";
import "./subdir/b.ts";
import { join } from "https://deno.land/[email protected]/path/mod.ts";

export ...

I above example one needs to run with following flags:

deno run --allow-read=dyn_import.ts,subdir/ --allow-net=deno.land main.ts

Similarly for workers, example 2:

// main.ts

console.log("hello");
const worker = new Worker(new URL("./worker.ts", import.meta.url), { type: "module" });
worker.postMessage("foo")

// worker.ts
import "./subdir/a.ts";
import "./subdir/b.ts";
import { join } from "https://deno.land/[email protected]/path/mod.ts";

export ...

I above example one needs to run with following flags:

deno run --allow-read=worker.ts,subdir/ --allow-net=deno.land main.ts

In other words, doing dynamic imports or using workers requires to open up the sandbox by providing read and/or net permissions to load ES modules that will be executed. Of course one could revoke those permissions after the load is done, but with top-level-await this becomes quite tricky.

My proposal:
Introduce new flag --allow-import=<allow_list> and change semantics of permission check for loading modules. Instead of verifying permissions for each static import we should only check permission of entry point. User could specify --allow-import to allow all dynamic imports and workers. So invocations for above examples would look as follows:

Example 1:

deno run --allow-import=dyn_import.ts main.ts

Example 2:

deno run --allow-import=worker.ts main.ts

Notice that no runtime permissions were given (no --allow-read, nor --allow-net).

Going forward with this proposal would be a huge breaking change in semantics and require profound refactors. IMO we could start by introducing --allow-import and its semantics in next minor release, while keeping old semantics working (and showing warnings about it being deprecated). Then after one or two minor releases completely remove old semantics. Alternatively it could be 2.0 material.

This issue is directly related to #8109 and #8215

CC @kitsonk @ry @lucacasonato @piscisaureus

[bartlomieju]

Also CC @nayeemrmn @teleclimber

[kitsonk]

First, I think it would be far more common for people to use --allow-import. Also, really 🚲 🏠 , but --allow-import could be somewhat confusing if it applied to workers as well as dynamic import. Does it mean you have to have it to import anything? Does it not apply to workers? I guess we could ensure that error messages mention it properly, like:

error: A worker tried to load a script without permissions.  Re-run with --allow-import to provide access.
  at file:///some/script/a.js:1:8

Because I can't really think of a better name. I do believe it makes a lot more sense then --allow-read and --allow-net for these type of scripts, and my main argument is that it only opens up the door for loading scripts, not allowing full access for code to read or access the net, which is what you currently have to do if you want to use dynamic import or workers.

[bartlomieju]

Because I can't really think of a better name.

Neither do I, --allow-import was my first thought and I can't come up with anything better that applies to both import() and Worker() API, but of course I'm open to other suggestions.

I guess we could ensure that error messages mention it properly, like:

error: A worker tried to load a script without permissions.  Re-run with --allow-import to provide access.
  at file:///some/script/a.js:1:8

Sounds good to me

[teleclimber]

The proposal sounds good to me. It would cleanly solve the issue I had in #8109.

My only concern is what this does to the UX for Deno users. It's already a lot to have to specify all the permissions, now they also have to specifically allow the script to run its workers and dynamic imports. This requires the user to know what these scripts are and carefully type them in at the command line every time they want to use it.

[bartlomieju]

My only concern is what this does to the UX for Deno users. It's already a lot to have to specify all the permissions, now they also have to specifically allow the script to run its workers and dynamic imports. This requires the user to know what these scripts are and carefully type them in at the command line every time they want to use it.

@teleclimber thanks for raising this, I'm aware of the problem and have recently started discussions on Discord around this problem.

There are numerous proposals to solve problem of describing the workflow:
- #3675
- #3179
- #5897

[kitsonk]

This requires the user to know what these scripts are and carefully type them in at the command line every time they want to use it.

In my mind --allow-import by itself should allow workers and dynamic import to import any script, irrespective of what it is named. But can take an optional argument which allows the invoker to be more specific, just like --allow-net and --allow-read do. Currently, if someone wants to be more specific, they have to include the script and all of its dependencies, so this actually increases the UX in my opinion, to make it easier to be more secure.

[nayeemrmn]

My only concern is what this does to the UX for Deno users. It's already a lot to have to specify all the permissions, now they also have to specifically allow the script to run its workers and dynamic imports.

The fact that it's already a lot is exactly why this shouldn't be a factor. It's already a reality that shortcuts are essential. Suddenly worrying about length of commands at this point will just lead to design inconsistencies, unless the solution is general-purpose with respect to flags as a whole.

[teleclimber]

@teleclimber thanks for raising this, I'm aware of the problem and have recently started discussions on Discord around this problem.

Ok, glad to see that there are proposals to address this.

Suddenly worrying about length of commands at this point will just lead to design inconsistencies, unless the solution is general-purpose with respect to flags as a whole.

Agreed. Better to have a consistent set of flags and deal with the length of commands separately.

[benatkin]

I also would like it to be available for non-dynamic imports and for deno info --json script.ts

I wrote some of my thoughts here: #11052

[hmdhk]

I'm also interested in applying allow-import to "static" imports. I can think of a few scenarios in which this can useful.

1. Prevent leaking the IP address of the client/server that is running an untrusted code
2. Enforcing imports only from company domain/internal network to avoid running code that hasn't been reviewed by that company.
3. In a multi-tenant situation, allowing an instance to only import from a certain directory makes it more convenient to manage access control.

Of course, all of the above could be addressed at other levels, e.g. at the process level but the permission model of Deno already makes a lot of things easier so I think this is the right direction.

Furthermore, IMO, imports should be treated as an indirect access to file system/network, so in that sense, they should also have the same permissions as existing allow-net and allow-read flags.

[hmdhk]

To make the UX a bit better, we could also have reasonable defaults for allow-import, e.g. only allow import from deno.land and also only the subdirectory of the main module (instead of all or nothing defaults).

[benatkin]

The ability to load JSON makes Deno's allow-read permission less usable in securing the local filesystem. There are private keys in JSON - for instance, google service accounts. If you can get someone to run a file, even with no permissions, and you know the path, you can get the contents of a file.

Come to think of it, it also works over the network. What if you have an HTTP server for JWK serving on a port, with Basic Auth in a reverse proxy server, and are running a script intended to be sandboxed on the same server? JWK uses JSON.

To use Deno for sandboxing, you now have to do deno info --json and check for json import assertions and make sure it doesn't change between when you run deno info and deno run - or do deno cache and search all the code for import assertions.

~/scratch
❯ ls ~/Downloads/service-account.json
/Users/bat/Downloads/service-account.json

~/scratch
❯ cat mod.ts
import foo from "../Downloads/service-account.json" assert { type: "json" };

console.log(JSON.stringify(foo, null, 2));

~/scratch
❯ deno run mod.ts
{
  "type": "service_account",
  "project_id": "my-project",
  "private_key_id": "xxxxxxxxxxxxxxxxxxxxxxxxxx",
  "private_key": "-----BEGIN PRIVATE KEY-----\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n",
  "client_email": "[email protected]",
  "client_id": "xxxxxxxxxxxxxxx",
  "auth_uri": "https://accounts.google.com/o/oauth2/auth",
  "token_uri": "https://oauth2.googleapis.com/token",
  "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
  "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/my-service%40my-project.iam.gserviceaccount.com"
}

Now that I think about it, I wish importing JSON was blocked by default. sigh

I know secrets can be stored in .ts and .js files but it's not common and it certainly is common to store them in json files.

[benatkin]

Another complication for trying to allowlist imports: it appears JSON modules aren't cached and --cached-only isn't respected:

~/scratch
❯ ls
mod.ts

~/scratch
❯ mkdir denodir

~/scratch
❯ export DENO_DIR=./denodir

~/scratch
❯ cat mod.ts
import foo from "../Downloads/service-account.json" assert { type: "json" };

console.log(JSON.stringify(foo, null, 2));

~/scratch
❯ deno cache mod.ts
Check file:///Users/bat/scratch/mod.ts

~/scratch
❯ tree denodir
denodir
└── gen
    └── file
        └── Users
            └── bat
                └── scratch
                    ├── mod.ts.buildinfo
                    ├── mod.ts.js
                    └── mod.ts.meta

5 directories, 3 files

~/scratch
❯ deno run mod.ts
{
  "type": "service_account",
  "project_id": "my-project",
  "private_key_id": "xxxxxxxxxxxxxxxxxxxxxxxxxx",
  "private_key": "-----BEGIN PRIVATE KEY-----\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\nxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx\n",
  "client_email": "[email protected]",
  "client_id": "xxxxxxxxxxxxxxx",
  "auth_uri": "https://accounts.google.com/o/oauth2/auth",
  "token_uri": "https://oauth2.googleapis.com/token",
  "auth_provider_x509_cert_url": "https://www.googleapis.com/oauth2/v1/certs",
  "client_x509_cert_url": "https://www.googleapis.com/robot/v1/metadata/x509/my-service%40my-project.iam.gserviceaccount.com"
}

~/scratch
❯

There is one way that might work, but perhaps shouldn't be depended upon, to check all the imports, including json:

~/scratch
❯ deno cache -L debug mod.ts
DEBUG RS - deno::file_fetcher:648 - FileFetcher::fetch() - specifier: file:///Users/bat/scratch/mod.ts
DEBUG RS - deno_runtime::permissions:51 - ⚠️️  Granted read access to "/Users/bat/scratch/mod.ts"
DEBUG RS - deno::file_fetcher:648 - FileFetcher::fetch() - specifier: file:///Users/bat/Downloads/service-account.json
DEBUG RS - deno_runtime::permissions:51 - ⚠️️  Granted read access to "/Users/bat/Downloads/service-account.json"
DEBUG RS - deno::proc_state:415 - Compilation statistics:


~/scratch
❯

[benatkin]

The lockfile unintentionally disallowed importing a Blob that wasn't defined in the lockadded Blob imports to the lockfile, and that was just fixed:

#16558

The fix makes it clear that --v8-flags=--disallow-code-generation-from-strings no longer isn't sufficient to prevent something like eval from being used.

So this is another use case for this feature – making it so eval and similar things can be disabled, in conjunction with --v8-flags=--disallow-code-generation-from-strings.

[lowlighter]

Since #25469 is getting implemented, any chance it could also help towards #25360 too ?

[benatkin]

It doesn't look like #25469 allows blocking file imports or restricting them to a directory.