permission: add initial environment permission

[daeyeon]

Add initial environment permission support. This restricts permission to access environment variables through process.env by using --allow-env flag.

usages

Proposed basic usages are as follows:

• --allow-env : All environment variables are allowed.
• --allow-env=HOST : HOST is allowed.
• --allow-env=HOST,PORT : HOST and PORT are allowed.
• --allow-env=DB_* : Env vars starting with DB_ are allowed.
• --allow-env=DB_*,-DB_PASSWORD : All env vars starting with DB_ except DB_PASSWORD are allowed.
• --allow-env=*,-DB_PASSWORD : All env vars except DB_PASSWORD are allowed.

process[env_private_symbol]

This is based on the idea of using a new privileged API for builtins to access the environment variables instead of process.env. It preserves current behaviors of process.env on the child processes and the worker threads by leveraging the existing native traps. This approach required manual changes to all internal uses of it.

WIP:

• Removing code parsing patterns related to wildcard and -.
• Adding flags to enable full access to envvars.
• Updating TCs.
• Updating documents.
• Seeking possible ways to safely enumeration/cloning of the env object.

Refs: nodejs/security-wg#993

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/modules
• @nodejs/net
• @nodejs/security-wg
• @nodejs/test_runner

[tniessen]

I don't think it's great that enumerating properties of process.env throws. (Then again, it's already bad for DX that asynchronous file system APIs can now also throw synchronous permission errors, in addition to existing asynchronous permission errors.)

On the other hand, hiding environment variables can be problematic, too, of course.

Deno chose a somewhat elegant approach by not making Deno.env an object with enumerable environment variables. Deno.env.toObject() requires a special "all" permission.

[RafaelGSS]

As mentioned by @tniessen, since we don't have a standard way to have access to env properties besides enumerated properties. I think this is the best we can do.

Regarding synchronous throws in asynchronous API. We can adjust it, but, permission is not the only throw-sync check that runs in async APIs (CHECK_NULL).

Amazing work! I'll review it with attention later. Why is it a draft? what's missing?

[ljharb]

you may want to use ObjectHasOwn for this, since potentially anyone could add NODE_UNIQUE_ID to the prototype chain.

Suggested change

	const childOrPrimary = 'NODE_UNIQUE_ID' in process[env_private_symbol] ? 'child' : 'primary';
	const childOrPrimary = ObjectHasOwn(process[env_private_symbol], 'NODE_UNIQUE_ID') ? 'child' : 'primary';

[Qard]

I feel like this might need a bit more thinking of how to handle enumeration/cloning of the env object safely. There's lots of cases of the process.env object getting cloned without any particular awareness of the contents to expect that it could suddenly fail. Perhaps it's better in enumeration to return undefined rather than throwing on blocked keys? I wonder if we could have a way to get a list of all present keys, even blocked ones, but not have access to map the key to the value?

[RafaelGSS]

Sorry delay, I didn't have time to jump into this discussion.

Deno was designed to facilitate a somewhat useful permission system from day one. Node.js wasn't. I don't think adding questionable features in an attempt to catch up with Deno should be a goal of the Node.js project.

I 100% agree. Honestly, after thinking for some time, I don't see a security reason to block the process.env.* access. It doesn't seem like a feature someone would use either in production or in development and Node.js doesn't have a history of exploits using environment variables. If we proceed with this restriction (--allow-env), it will mean there are hundreds of uncovered cases we'll need to deal with, and since this is a security feature, all those cases will be considered a vulnerability.

[Qard]

Well, the env could contain sensitive data such as AWS credentials so I do think restricting access could make sense. Though as has already been expressed, there are tools to do that externally.

I think this may be a case of a solution looking for a problem though. I would suggest we hold off for now and reconsider later if users ask for it or a clear need comes up. New features can be added any time, but removing them is incredibly difficult.

[daeyeon]

I'd like to respectfully step back from my original thought and express my agreement with everyone's opinion. I think it's appropriate to hold this request. Thank you for taking the time to think about this together. 🙏