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

Introducing Audit Hooks to Node.js #626

Closed
vdeturckheim opened this issue Jan 24, 2020 · 16 comments
Closed

Introducing Audit Hooks to Node.js #626

vdeturckheim opened this issue Jan 24, 2020 · 16 comments
Labels

Comments

@vdeturckheim
Copy link
Member

Last year, Python introduced audit hooks to the language.

In short, this is an API allowing users to:

  • subscribe to certain actions (for instance, file system access)
  • prevent certain calls (for instance, preventing accessing a file)
  • add userland hooks (we could have some in DB libraries for instance)

Another bonus is that we could add a hook to eval and finally provide a good way to monitor its usage.

I remember that Microsoft team was intereted in having such an API (I understand they drove the effort in Python).

For reference, the Python spec is here https://www.python.org/dev/peps/pep-0578/

Is there any strong opinions on that topic?

@sam-github
Copy link
Contributor

I'm generally in favour, there isn't any particular blockers to doing this, other than that someone has to do the work. :-)

@cjihrig
Copy link
Contributor

cjihrig commented Jan 24, 2020

I'm also generally in favor but we'd have to consider potential performance overhead, particularly when the hooks are not in use.

@vdeturckheim
Copy link
Member Author

@sam-github I'll probably explore it to implement the eval hook at least and check about the syscall ones after we stabilize Async Storage APIs

@cjihrig yep, performance would be the main point here! I am not even sure of the right depth (having this implemented in JS vs in native land) yet.

@MarcinHoppe
Copy link
Contributor

I would very much like to see it happen 👍.

@mhdawson
Copy link
Member

If we can ensure the performance impact is acceptable this seems interesting to me as well.

@bmeck
Copy link
Member

bmeck commented Jan 28, 2020

This seems reasonable and aligns with the vision I have had. That said, I do think any such API needs to be much more robust than what we often do in Core these days with mutable internals.

@vdeturckheim
Copy link
Member Author

@bmeck in term of performance, mutable internals seems to be the simplest way. Do you forsee another strategy that could work here?

@bmeck
Copy link
Member

bmeck commented Jan 29, 2020

@vdeturckheim I guess I should clarify my concerns. My concern is mutable globals/builtins affecting audit hooks. E.G. Someone replacing globalThis.Promise and/or something like fs.readFile to intercept, prevent, or otherwise manipulate the audit hook and prevent its intended purpose when such a hook uses those APIs (such as reading a config file off disk and/or using Promise.all, etc.).

If the mutation is able to remove the hook without directly manipulating the actual hook code, that is problematic I would think. The only way that seems safe is if you audit all your hook code, allow it to run prior to any untrusted code, and have a way to ensure they have access to the primordials they need (I wouldn't want to encourage process.binding as a way to create a robust fs.readFile).

I'm not really worried about performance but about the reliability of the feature itself. If it isn't reliable, we probably shouldn't encourage it.

@vdeturckheim
Copy link
Member Author

@bmeck makes sense!

I was thinking about hooking internals that are not exposed to end user. It's an early idea, but for example, muting binding.open here rather than fs.readFile.

@bmeck
Copy link
Member

bmeck commented Jan 29, 2020

@vdeturckheim that seems reasonable, but we would want to make sure we document all the bindings we expose and I'd still be concerned with global mutations. I know we have primordials now but that seems extremely inconvenient/error prone to have developers code against, I know we are working on moving ESM Loaders to a separate thread for isolation. IDK how others feel if enabling this feature had isolation even if just in a separate Isolate since that would affect performance and we could not pass non-primitives across isolates without cloning.

@vdeturckheim
Copy link
Member Author

vdeturckheim commented Jan 29, 2020

@bmeck I understand. I guess there is only one way to know for sure!

I will try to PoC hooking eval and fs in the next months and this should give us visibility over API/strategy.

@DanielRuf
Copy link
Contributor

I'm also in favor of this. Sounds like that what the people of NodeSource provide with NSolid

https://nodesource.com/products/nsolid

It might make sense to allow users to control this through the Policy API, similar to Deno and other solutions on npm.

https://nodejs.org/api/policy.html

@vdeturckheim
Copy link
Member Author

@DanielRuf Thanks for the refs. I digged into NSolid's doc but could not find details about this. Do you know where I could find it?

@DanielRuf
Copy link
Contributor

I think there was more information in the past.

Only https://docs.nodesource.com/nsolid/3.8/docs is still available.
I tried to find the old docs in the wayback machine but it seems the relevant pages are not available there and returned a 302 when they were crawled.

@sam-github
Copy link
Contributor

@vdeturckheim I think this should be discussed in Node.js core, and ping the sec folks here to ask them if they have comments.

Of was there something specific to add in the agenda?

@github-actions
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants