Load permission settings from config files

This issue is to discuss implementation of the point referenced in the title of #898.

My first concern would be on which flag to use to pass the file. The options that I can think of are:

Use the --experimental-policy flag. My main concern is this would imply the need to also set policy.
Add an optional file to the --experimental-permission flag (I'm still unsure that's even possible).
Creating a new flag (e.g. --permission-file).

Another doubt I have is what would be the expected behaviour on precedence when permissions are set on both flags (e.g. --allow-fs-read) and file. Options:

Fail when both set.
Fail when both set for the same setting (e.g. not fail if permission to read set on file and permission to write set on flag but fail if read set on both) This doesn't make much sense IMO.
Use the union of both.
Use the intersection of both.
Ignore file.
Ignore flag.

I would go with:
1.2. Add an optional file to the --experimental-permission flag (If possible) as we wouldn't need a new flag.
2.1. Fail when both set. I'd assume that to be a mistake, and throw an error explaining the problem. Also this might prevent some sort of attacks using the unconfigured one.

Any ideas and suggestions are welcomed.

@tniessen

As discussed in today's call. I have two questions:

Do we need this feature? With the addition of .env (src: add built-in .env file support node#48890) + NODE_OPTIONS eventually all the --allow-* flags can be passed by NODE_OPTIONS environment variable in .env file.
If we decide that we want to support it, we should be careful with the proposed design. While the addition of a new flag --permission-file seems ok, we're introducing a new experimental CLI flag which, IMO, is bad for the developer experience. On the other hand, adding an optional file to the --experimental-permission flag seems far from ideal since we'll need to:
- Check if a prior --experimental-policy is passed with another file name
- Check somehow if --experimental-permission is passed with no file. For instance, if we change the experimental_permission to std::string, it will be empty when no file is passed. How would we identify that --experimental-permission is true?
- I'm pretty sure there are other definitions to make

We've also found this Deno discussion: denoland/deno#12763. We must discuss both points before a PR.

In any case, I'm pinging @nodejs/tsc, specially @tniessen and @bmeck to hear your thoughts.

I would encourage people to avoid building config files for specific features; see nodejs/node#48852 and nodejs/node#43973 as attempts to create config files for the test runner and module loaders, respectively. I think users deserve to be able to configure Node in its entirety through a single config file. nodejs/node#48890 was a starting point; we can add support for configuration via a JSON (or other format) file if desired.

@GeoffreyBooth

The idea is to use an existing config file @GeoffreyBooth (--experimental-policy=policy). But, as you said, looks like the env feature might solve it all.

Before --experimental-policy goes stable we should probably figure out how to create a single unified config file for all of Node, and then what’s currently in policy.json could live under a policy key or something.

@RafaelGSS

@RafaelGSS there are a few major concerns I have with environment variables:

it is exposed on process.env for people to muck around with or around to avoid detection
environment variables do have limits! ARG_MAX is something I've seen in the wild
locality, editing .env for permissions but a file for policies would be strange
i'm not sure exactly about the size expected for all possible permissions but inlining them might lead to incredibly long lines in your .env file which is possible but not something easy to write tooling against
tooling to automate generating/analyzing would like some different kind of format that is more capable for potential forwards compatibility.
consider moving from per process to per worker scenarios, allowing different files seems reasonable but .env is a shared implicit location generally; splitting the env seems possible but odd to split not on environment exactly but on
accidental override via shell ENV; there are existing NODE_OPTIONS usages in the wild and could be problematic due to the collision destroying this value or inability to merge due to lack of something akin to "cascade" we had to add in policies

I'm pretty -1 on using ENV for this since it just isn't designed for complex usages or large values. I agree for very simple not fully specified things it would be fine but would cause a preference on large scale permissions grants due to problems at large scale constraints.

I would note I am not proposing that we ban these flags from .env etc. just that it seems a poor best practice at a glance.

The concern I raised in this issue was that is not clear, at the first moment, what issue a separated config file for the permission model will solve. I mean, currently, it's annoying to allow a bunch of paths through the CLI. Imagine: --allow-fs-read=/path1/ --allow-fs-read=/pat2 and so on. While the wildcard would solve 70% of this issue, it migth be annoying for some use cases.

However, due to the fact, you can pass the same flags through the NODE_OPTIONS env var, that could be a solution (or at very least, workaround) for these cases.

As a separate discussion, but that overlaps with it. Have we ever considered a unified nodejs configuration file? Not by env vars, but a single file where I can define multiple rules of nodejs mechanism, such as policies.

@RafaelGSS

@RafaelGSS simply put if you have finer grained / verbose levels of settings you end up with problems due to how shells work. On my machine:

% getconf ARG_MAX
1048576

Is the maxium space I can use up as seen with:

% node -p "child_process.spawnSync('echo', [' '.repeat($(getconf ARG_MAX))]).error.message"
spawnSync echo E2BIG

This includes normal environment variables and argv. It isn't too hard to hit this limit with any complex system.

@anonrig

Have we ever considered a unified nodejs configuration file? Not by env vars, but a single file where I can define multiple rules of nodejs mechanism, such as policies.

Yes, see https://github.com/orgs/nodejs/discussions/44975 and the issues it cites. Basically we thought that supporting .env files would be a quick win and useful in its own right, separate from that feature’s ability to configure Node. But I think everyone agrees that something like --config node.config.json would be nicer still, and complementary. The challenges there are figuring out how to parse JSON at that early point of startup, before V8 has loaded (which we thought would be really hard, but @anonrig seems to think might be doable without too much trouble thanks to simdjson) and the design challenge of finding a JSON schema that everyone can agree on. Maybe the latter isn’t insurmountable, if we use the flags as our starting point, and just go with camelcased versions of all of them, something like that. So yeah I think it’s very doable, at least now that nodejs/node#48890 has landed and paved the way.

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

Navigation Menu

Load permission settings from config files #1074

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Load permission settings from config files #1074

Description

Activity

RafaelGSS commented on Aug 17, 2023

GeoffreyBooth commented on Aug 17, 2023

RafaelGSS commented on Aug 18, 2023

GeoffreyBooth commented on Aug 18, 2023

bmeck commented on Aug 18, 2023

bmeck commented on Aug 18, 2023

RafaelGSS commented on Aug 18, 2023

bmeck commented on Aug 18, 2023

GeoffreyBooth commented on Aug 18, 2023

github-actions commented on Nov 30, 2023

github-actions commented on Feb 29, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions