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

Proposal: add permissions to config file #12763

Open
kitsonk opened this issue Nov 14, 2021 · 48 comments
Open

Proposal: add permissions to config file #12763

kitsonk opened this issue Nov 14, 2021 · 48 comments
Assignees
Labels
feat new feature (which has been agreed to/accepted) suggestion suggestions for new features (yet to be agreed)
Milestone

Comments

@kitsonk
Copy link
Contributor

kitsonk commented Nov 14, 2021

Context

Currently Deno supports a deno.jsonc configuration file which allow users to provide a configuration file that can provide TypeScript compiler options, lint options and format options.

It does not currently support other information that can only be expressed on the command line, while this proposal is what

Semantics

  • When a configuration file is applied and the "permissions" section is parsed and the permissions are applied from the "permissions" section. Any other flags on the command line are ignored.
  • If there are flags on the command line and a config file is being applied, and the config file contains "permissions" a warning should be issued to stderr that permissions from the config file are being applied.
  • Remote configuration files are supported, but a summary of the permissions is prompted before hand and requires user configuration to continue.
  • Keys names match the flags on the command line, and values accept a true or false. If the key supports a value, these can be either a string delimited the same way on the command line, or an array of strings.
  • In situations where a base path is needed for relative paths (for --allow-read and --allow-write) the config file is used as a base, versus the cwd.

Examples

An example of a configuration file using permissions:

{
  "permissions": {
    "allow-all": true,
    "allow-env": true,
    "allow-hrtime": false,
    "allow-read": [ ".", "/tmp" ],
    "allow-write": ".",
    "allow-net": "deno.land,nest.land"
  }
}

Considerations / Open Questions

  • Having explicit a section of "permissions" makes it easier to understand explicitly that these effect the runtime permissions. It also allows the definition of the semantics of "permissions" to evolve independently of the rest of the configuration file, as well as opens an easy opportunity to be able to set permissions on other things in the future, like tasks/scripts independent of the top level permissions.
  • Having an explicit allow in the keys provides a mechanism to introduce block in the future (for example "block-net": "deno.land") giving more granular permissions.
  • If a remote configuration file is used, and there is no TTY or --no-prompt/--quit is supplied on the configuration is set, should the process just terminate or just allow the program to run with the supplied permissions?
  • For remote config files, using the remote URL as a relative base for --allow-read --allow-write is not possible. Does this mean that relative paths just error, or that it defaults to the cwd in those situations?

cc/ @bartlomieju @ry

@crowlKats
Copy link
Member

crowlKats commented Nov 14, 2021

In my opinion allow-all should just be

{
 "permissions": true,
}
* Having an explicit `allow` in the keys provides a mechanism to introduce `block` in the future (for example `"block-net": "deno.land"`) giving more granular permissions.

not a fan of this. it would mean you'd be able to do something with the config file that is impossible to do with just flags.

Also I don't think it makes sense to support comma separated items in a string, only arrays should be usable (for the perms that take multiple values)

@bartlomieju
Copy link
Member

I'm in favor of the proposal in general (especially combined with tasks/scripts), but I'm not in favor of top-level permissions key. IMO there's not much benefit of having a single top-level definition if most projects contain several entrypoint, each of them requiring different permissions. With this proposal it becomes non-obvious how permissions would be applied to different entry-points.

As for the signature of permissions object I believe we should follow definition used in TestDefinition and WorkerOptions as closely as possible:

permissions?: "inherit" | "none" | {
/** Specifies if the `net` permission should be requested or revoked.
* If set to `"inherit"`, the current `env` permission will be inherited.
* If set to `true`, the global `net` permission will be requested.
* If set to `false`, the global `net` permission will be revoked.
*
* Defaults to "inherit".
*/
env?: "inherit" | boolean | string[];
/** Specifies if the `hrtime` permission should be requested or revoked.
* If set to `"inherit"`, the current `hrtime` permission will be inherited.
* If set to `true`, the global `hrtime` permission will be requested.
* If set to `false`, the global `hrtime` permission will be revoked.
*
* Defaults to "inherit".
*/
hrtime?: "inherit" | boolean;
/** Specifies if the `net` permission should be requested or revoked.
* if set to `"inherit"`, the current `net` permission will be inherited.
* if set to `true`, the global `net` permission will be requested.
* if set to `false`, the global `net` permission will be revoked.
* if set to `string[]`, the `net` permission will be requested with the
* specified host strings with the format `"<host>[:<port>]`.
*
* Defaults to "inherit".
*
* Examples:
*
* ```ts
* import { assertEquals } from "https://deno.land/std/testing/asserts.ts";
*
* Deno.test({
* name: "inherit",
* permissions: {
* net: "inherit",
* },
* async fn() {
* const status = await Deno.permissions.query({ name: "net" })
* assertEquals(status.state, "granted");
* },
* });
* ```
*
* ```ts
* import { assertEquals } from "https://deno.land/std/testing/asserts.ts";
*
* Deno.test({
* name: "true",
* permissions: {
* net: true,
* },
* async fn() {
* const status = await Deno.permissions.query({ name: "net" });
* assertEquals(status.state, "granted");
* },
* });
* ```
*
* ```ts
* import { assertEquals } from "https://deno.land/std/testing/asserts.ts";
*
* Deno.test({
* name: "false",
* permissions: {
* net: false,
* },
* async fn() {
* const status = await Deno.permissions.query({ name: "net" });
* assertEquals(status.state, "denied");
* },
* });
* ```
*
* ```ts
* import { assertEquals } from "https://deno.land/std/testing/asserts.ts";
*
* Deno.test({
* name: "localhost:8080",
* permissions: {
* net: ["localhost:8080"],
* },
* async fn() {
* const status = await Deno.permissions.query({ name: "net", host: "localhost:8080" });
* assertEquals(status.state, "granted");
* },
* });
* ```
*/
net?: "inherit" | boolean | string[];
/** Specifies if the `ffi` permission should be requested or revoked.
* If set to `"inherit"`, the current `ffi` permission will be inherited.
* If set to `true`, the global `ffi` permission will be requested.
* If set to `false`, the global `ffi` permission will be revoked.
*
* Defaults to "inherit".
*/
ffi?: "inherit" | boolean | Array<string | URL>;
/** Specifies if the `read` permission should be requested or revoked.
* If set to `"inherit"`, the current `read` permission will be inherited.
* If set to `true`, the global `read` permission will be requested.
* If set to `false`, the global `read` permission will be revoked.
* If set to `Array<string | URL>`, the `read` permission will be requested with the
* specified file paths.
*
* Defaults to "inherit".
*/
read?: "inherit" | boolean | Array<string | URL>;
/** Specifies if the `run` permission should be requested or revoked.
* If set to `"inherit"`, the current `run` permission will be inherited.
* If set to `true`, the global `run` permission will be requested.
* If set to `false`, the global `run` permission will be revoked.
*
* Defaults to "inherit".
*/
run?: "inherit" | boolean | Array<string | URL>;
/** Specifies if the `write` permission should be requested or revoked.
* If set to `"inherit"`, the current `write` permission will be inherited.
* If set to `true`, the global `write` permission will be requested.
* If set to `false`, the global `write` permission will be revoked.
* If set to `Array<string | URL>`, the `write` permission will be requested with the
* specified file paths.
*
* Defaults to "inherit".
*/
write?: "inherit" | boolean | Array<string | URL>;
};

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 15, 2021

IMO there's not much benefit of having a single top-level definition if most projects contain several entrypoint, each of them requiring different permissions.

But there are currently no semantics in the config file for entry points, so it becomes a chicken and egg.

And specifically the example of the "test" permissions seems to apply a top level "permissions" key that would be the default set of permissions to be applied. Given the current lack of a way to express entry points in the config file, it only seems logical to describe the default set of permissions, and then allow addition/different sets of permissions to be described on individual entry points when they become available.

My key point was that we shouldn't flatten permissions to be different top level keys.

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 15, 2021

not a fan of this. it would mean you'd be able to do something with the config file that is impossible to do with just flags.

That is already the case with TypeScript compiler options.

@kitsonk
Copy link
Contributor Author

kitsonk commented Nov 15, 2021

As for the signature of permissions object I believe we should follow definition used in TestDefinition and WorkerOptions as closely as possible:

I am agreeable with that, and it makes sense in the context of "top level" permissions and allowing overriding for different tasks/entry points.

@bartlomieju
Copy link
Member

IMO there's not much benefit of having a single top-level definition if most projects contain several entrypoint, each of them requiring different permissions.

But there are currently no semantics in the config file for entry points, so it becomes a chicken and egg.

And specifically the example of the "test" permissions seems to apply a top level "permissions" key that would be the default set of permissions to be applied. Given the current lack of a way to express entry points in the config file, it only seems logical to describe the default set of permissions, and then allow addition/different sets of permissions to be described on individual entry points when they become available.

My key point was that we shouldn't flatten permissions to be different top level keys.

Okay, this is a valid point. You're also right about chicken and egg problem. I guess this is a good way to start iterating on these features. Let's do it 👍

@crowlKats
Copy link
Member

not a fan of this. it would mean you'd be able to do something with the config file that is impossible to do with just flags.

That is already the case with TypeScript compiler options.

yes, but those are "external", as in, they arent something made by/for deno exclusively. the only other option for thsoe would implement all options as flags, which would be unrealistic.
Also this would mean we have some permission related features as flags, but other features only in config file, making things just more confusing imo by having things "all over the place" (i am aware it isnt as bad as i just made it sound, but my point stands).

@Soremwar
Copy link
Contributor

I'm in favor of the proposal in general (especially combined with tasks/scripts), but I'm not in favor of top-level permissions key. IMO there's not much benefit of having a single top-level definition if most projects contain several entrypoint, each of them requiring different permissions. With this proposal it becomes non-obvious how permissions would be applied to different entry-points.

This could be tackled by allowing composition of config file, an extends of sorts

@crowlKats
Copy link
Member

crowlKats commented Nov 15, 2021

{
  "permissions": {
    "allow-all": true,
    "allow-env": true,
    "allow-hrtime": false,
    "allow-read": [ ".", "/tmp" ],
    "allow-write": ".",
    "allow-net": "deno.land,nest.land"
  }
}

Thought about the blocklist thing again: how about instead

{
  "permissions": {
    "env": {
      "block": ["PATH"],
      "allow": ["HOME"]
    },
    "hrtime": false,
    "read": [".", "/tmp"],
    "write": ["."],
    "net": ["deno.land", "nest.land"],
    "prompt": true
  }
}

so to specify the blocklist, its an object instead of a proper value for the perm directly. this would all us to add blocklist support at some later point as it would be an extension from what it would be without. also this would allow for allowing specifying --prompt behaviour on a permission level instead of just a global level

@lilnasy
Copy link

lilnasy commented Nov 18, 2021

I really like this proposal. It encourages fine-grained control over what directories can be read and written to, for example. At the moment, it's inconvenient to get this precise with command line options.

@jsejcksn
Copy link
Contributor

jsejcksn commented Dec 9, 2021

I really like this proposal. It encourages fine-grained control over what directories can be read and written to, for example. At the moment, it's inconvenient to get this precise with command line options.

@crowlKats When you said

the only other option for thsoe would implement all options as flags, which would be unrealistic

by "unrealistic" did you mean "inconvenient"? My impression is that most software which executes with more than a few flags either builds them using a script or hardcodes them into a script.

Adding additional CLI flags from options actually sounds useful (as no extra filesystem i/o in the form of reading/writing a config would need to happen in dynamic execution).

@remyrylan
Copy link

remyrylan commented Dec 9, 2021

EDIT:
I was initially in favor of this proposal as-is, but now after thinking it over the only thing I desire is a flag to opt-out of the functionality to load permissions from the config. I just think about a scenario of updating to a new version of some popular third-party module that's been hacked and having said module write to deno.json to provide new permissions for itself that could be utilized the next time the script is ran.

For the strictest security, I would want to be able to explicitly opt-out of reading permissions from the config and instead require all the permissions be provided via flags... that's what I would always want to run in production.

@stale
Copy link

stale bot commented Feb 9, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Feb 9, 2022
@stale stale bot removed the stale label Feb 9, 2022
@jsejcksn
Copy link
Contributor

jsejcksn commented Feb 9, 2022

For the strictest security, I would want to be able to explicitly opt-out of reading permissions from the config and instead require all the permissions be provided via flags... that's what I would always want to run in production

Related: #13452 will become even more important if this feature is implemented

@sno2

This comment was marked as duplicate.

@ry ry added this to the 1.22 milestone May 5, 2022
@ry
Copy link
Member

ry commented May 5, 2022

I'm in favor too. Added to 1.22 milestone.

@bartlomieju
Copy link
Member

I'm working on this in #12763.

For the first pass I think we should cut the scope a little bit, namely by not supporting permissions in remote configuration file. Instead we should print a warning, pointing to an issue about it. The reason is that it will cause a bifurcation in behavior depending if the config file is local or remote, in the former case we'll be resolving allowlists relative to config file, while in the latter we'd have to fallback to CWD. I think this is gonna be a surprising behavior.

@jsejcksn
Copy link
Contributor

jsejcksn commented May 9, 2022

I'm working on this in #12763.

@bartlomieju You linked to this issue 🪞. Did you mean #14520?

@bartlomieju
Copy link
Member

Yes

@dsherret
Copy link
Member

dsherret commented Feb 6, 2023

Isn't that just as much of a concern right now with the task runner? Task aliases are also vulnerable to being edited by a rogue dependency.

@lilnasy it's not as much of a concern because it doesn't apply to every deno run/test/etc invocation and with deno task you probably have some idea about what task in what config file you are running. With deno run if you don't have a config file and the script has write permissions, then the malicious script could write one in an ancestor directory which would then be auto-discovered on the next run. With the config file being modified for deno task, it is still something that people should be aware about (along with scripts that modify code that someone will probably execute with elevated permissions), but again, not as much of a concern.

@DerZade
Copy link

DerZade commented Mar 2, 2023

How about another flag to use the permissions from the config file? Something like --allow-from-config or --permissions-from-config 🤔 Then you would have to explicitly specify that you want to use the permissions from the config.
Admittedly that doesn't fix the problem all together, but imo brings it down the a similar threat level we currently have with deno task

@dsherret
Copy link
Member

dsherret commented Mar 2, 2023

@DerZade yeah, that’s been discussed internally for certain commands like deno test and bench. Basically if you run without the flag it would prompt and show you the permissions to confirm, or you could provide a flag to use the permissions from the config file and override the prompt. This would help prevent people from accidentally running their tests without permissions granted and have it fail halfway through.

For deno run, I’m not sure how useful it is to have permissions in the config file because someone could define a deno task. Anyway, this is just all my personal opinion.

@DerZade
Copy link

DerZade commented Mar 2, 2023

I agree that deno task helped with this a lot. My personal setup usually includes three tasks, which all execute deno run. One with just the permissions, another one with the permissions and --watch and a third one with the permissions and --inspect-brk.
Now I have basically the same task with just minor differences and each of them includes the same (very long) permissions options.
So every time I need change permissions, I have to do it three times. That is still bothering me a bit 😅 and why I personally would welcome defining the permissions in the Deno config (or even a external config).

@oscarotero
Copy link
Contributor

Maybe the permissions in the config file could be assigned only to tasks (instead of globally) because there can be different modules that you want to run with different permissions.

I remember an idea I suggested some time ago to include descriptions in the tasks (#14949). Although it was rejected, it had the ability to configure a task using an object with different properties, not only a plain string. This would allow to configure the permissions in a more ergonomic way:

{
  "tasks": {
    "serve": {
      "run": "deno run ./server.ts",
      "allow": {
        "net": "localhost:3000",
        "read": ["./img", "./styles"],
      },
    },
  }
}

It would be equivalent to:

{
  "tasks": {
    "serve": "deno run --allow-net=localhost:3000 --allow-read=./img,./styles ./server.ts"
  }
}

@wojpawlik
Copy link

Shebang is aleeady a nice place for permissions, it just doesn't work on Windows: denoland/deno_task_shell#23.

@domenic
Copy link

domenic commented Aug 3, 2024

I think I'm running into this problem. I want deno test to come with some permissions. But as far as I can tell, I have to either:

  • Specify the permissions on the command line every time; or
  • Give up on using deno test and instead define a custom task and remember to use deno task test.

@jsejcksn
Copy link
Contributor

jsejcksn commented Aug 3, 2024

I want deno test to come with some permissions. But as far as I can tell, I have to either:

  • Specify the permissions on the command line every time; or
  • Give up on using deno test and instead define a custom task and remember to use deno task test.

Deno can't know which permissions you want to grant to executable code (whether it's code that's executed by deno run, deno test, etc.) without your specifying them… somewhere.

This is my view of the current state of things: if you plan to locally execute code (more than once using the same options) by interactively typing a command into your terminal, and you don't want to type very much each time to execute that code (who does? 😅), then specifying a task using the task runner is the simplest way to granularly open up the security sandbox — then, like you said, it's as simple as deno task test to run your tests — or deno task start to start your program — with only the minimally necessary permissions defined in those tasks.

However, if you want to grant all permissions (disable the security sandbox) for all test modules whose filenames pattern match by default when running deno test (and all of those modules' dependencies)… then you can always use deno test -A.

Example:

fs_read_test.ts:

// Requires permission --allow-read

import { assertStrictEquals } from "jsr:@std/assert@^1.0.2";

Deno.test("can read the local filesystem", async () => {
  const fileInfo = await Deno.lstat(new URL(import.meta.url));
  assertStrictEquals(fileInfo.size, 298); // This test module is 298 bytes
});

Running without any granted permissions:

% deno test
running 1 test from ./fs_read_test.ts
can read the local filesystem ... FAILED (0ms)

 ERRORS 

can read the local filesystem => ./fs_read_test.ts:5:6
error: PermissionDenied: Requires read access to "/Users/deno/fs_read_test.ts", run again with the --allow-read flag
  const fileInfo = await Deno.lstat(new URL(import.meta.url));
                              ^
    at Object.lstat (ext:deno_fs/30_fs.js:410:21)
    at file:///Users/deno/fs_read_test.ts:6:31

 FAILURES 

can read the local filesystem => ./fs_read_test.ts:5:6

FAILED | 0 passed | 1 failed (1ms)

error: Test failed

Running with all permissions:

% deno test -A
running 1 test from ./fs_read_test.ts
can read the local filesystem ... ok (0ms)

ok | 1 passed | 0 failed (1ms)
% deno --version
deno 1.45.5 (release, aarch64-apple-darwin)
v8 12.7.224.13
typescript 5.5.2

@domenic
Copy link

domenic commented Aug 3, 2024

Yes, perhaps I was not clear. I want to specify in my config file what permissions deno test should run in, so that people coming to my project can use the standard deno test command. Right now I have to instead include special instructions in my readme for a nonstandard testing command.

@jsejcksn
Copy link
Contributor

jsejcksn commented Aug 3, 2024

I want to specify in my config file what permissions deno test should run in, so that people coming to my project can use the standard deno test command.

This would violate the security concept of sandbox-by-default and explicit granting of permissions: it would be implicit granting of permissions (because configuration files are implicitly resolved).

Right now I have to instead include special instructions in my readme for a nonstandard testing command.

Can you instruct users to type deno task test instead of deno test?

@domenic
Copy link

domenic commented Aug 5, 2024

Can you instruct users to type deno task test instead of deno test?

Yes, I can. And then this makes Deno an inferior ecosystem to npm, where to learn how to run a project's tests, I don't need to look it up in the project's readme. I just always use npm test.

@dandv
Copy link

dandv commented Sep 21, 2024

As deno run interactively asks for the most granular permissions the code requires, and the user answers, it would be really handy to have an option to save these permissions somehow, in a configuration file, as a tasks entry, or even as a CLI command.

@bartlomieju bartlomieju added this to the 2.1.0 milestone Sep 22, 2024
@omarcresp
Copy link

following @oscarotero idea, here is a real app example of why having it declare as an object improves the readability of allowing permissions in the deno config
2024-10-18T06-20-25_code

@brianpeiris
Copy link

brianpeiris commented Oct 19, 2024

Please consider my proposal #26372 as well. This proposal seems fine, but in my issue I suggest a simpler solution, but one that also supports multiple "profiles" that can be applied appropriately in different contexts. If my proposal is less appealing, I'd suggest adding the idea of multiple "profiles" to this proposal, so that you aren't globally applying the same set of permissions to all contexts. For example, my proposal would allow you to use one profile for deno run and a different profile for deno serve.

@mo
Copy link

mo commented Nov 20, 2024

I hit this as well today for my first deno project. I like having a default profile in deno.json plus the option to specify "-P custom_profile" for projects where it's actually necessary to have two profiles.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat new feature (which has been agreed to/accepted) suggestion suggestions for new features (yet to be agreed)
Projects
None yet