-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
Comments
In my opinion {
"permissions": true,
}
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) |
I'm in favor of the proposal in general (especially combined with tasks/scripts), but I'm not in favor of top-level As for the signature of Lines 144 to 275 in dd91ece
|
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 My key point was that we shouldn't flatten permissions to be different top level keys. |
That is already the case with TypeScript compiler options. |
I am agreeable with that, and it makes sense in the context of "top level" permissions and allowing overriding for different tasks/entry points. |
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 👍 |
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. |
This could be tackled by allowing composition of config file, an |
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 |
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
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). |
EDIT: 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. |
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. |
Related: #13452 will become even more important if this feature is implemented |
This comment was marked as duplicate.
This comment was marked as duplicate.
I'm in favor too. Added to 1.22 milestone. |
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. |
@bartlomieju You linked to this issue 🪞. Did you mean #14520? |
Yes |
Yes, you can specify the permissions via tasks definition. The core team's hesitation is coming from the fact that if you specify permissions in the config file, adding the "write" permission allows to then update the config file by rogue dependency that would be able to escalate permissions further. We're currently focusing our efforts on other parts of the runtime, but we might revisit this issue in Q1 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. |
Here is another proposal #17177 It has in my opinion some advantages:
|
Maybe the maintainers can define a starting point for the threat model? I feel we're going back-and-forth in a piecemeal fashion. I'd love to have full lay of the land to be able to propose something that's able to be well received. |
My 2 cents:
|
My main problem with json config is that you cant express very useful permissions, like --allow-read=cache-dir() or even --allow-cache-dir=cache-dir().join(myapp) in a cross platform way In my opinion these happens all the time, and neither the current system or this proposal improve on thar, which is why ppl just give up and use --allow-read=all |
@sigmaSd I'm not sure that I understand this exactly, but it seems like a suggestion of some kind of dynamic value resolution. The ability to audit fully-resolved, static values prior to runtime is foundational to the security of permission configuration. |
That's a valid point It's a dynamic system, it can be improved by putting the permission file in a immutable registery like deno.land but it still is dynamic |
@lilnasy it's not as much of a concern because it doesn't apply to every |
How about another flag to use the permissions from the config file? Something like |
@DerZade yeah, that’s been discussed internally for certain commands like For |
I agree that |
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"
}
} |
Shebang is aleeady a nice place for permissions, it just doesn't work on Windows: denoland/deno_task_shell#23. |
I think I'm running into this problem. I want
|
Deno can't know which permissions you want to grant to executable code (whether it's code that's executed by 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 However, if you want to grant all permissions (disable the security sandbox) for all test modules whose filenames pattern match by default when running Example:
// 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 --version
deno 1.45.5 (release, aarch64-apple-darwin)
v8 12.7.224.13
typescript 5.5.2 |
Yes, perhaps I was not clear. I want to specify in my config file what permissions |
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).
Can you instruct users to type |
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 |
As |
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
"permissions"
section is parsed and the permissions are applied from the"permissions"
section. Any other flags on the command line are ignored."permissions"
a warning should be issued to stderr that permissions from the config file are being applied.true
orfalse
. 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.--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:
Considerations / Open Questions
"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.allow
in the keys provides a mechanism to introduceblock
in the future (for example"block-net": "deno.land"
) giving more granular permissions.--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?--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
The text was updated successfully, but these errors were encountered: