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

late code injection #37440

Closed
gireeshpunathil opened this issue Feb 19, 2021 · 15 comments
Closed

late code injection #37440

gireeshpunathil opened this issue Feb 19, 2021 · 15 comments
Labels
feature request Issues that request new features to be added to Node.js. stale

Comments

@gireeshpunathil
Copy link
Member

Is your feature request related to a problem? Please describe.

Currently, a node process cannot receive and execute custom code after it has started. For monitoring tools, a common use case is to be able to attach to an arbitrary process, inject a (tracing) code, instruct the runtime to execute that, and optionally detach from the process.

Describe the solution you'd like

  • define a mechanism to interrupt the runtime (signals in platforms where supported)
  • specify a Javascript interface for the code injection:
    • the signal is delegated to a Javascript handler function
    • a module with a predefined name (such as agent) is loaded
    • a function with a predefined name (such as execute) is executed
    • a user can define this module and place it in the search path, prior to signalling
    • upon completion, the module is unloaded
  • define security restrictions (only requests from matching UID / GID allowed)

Describe alternatives you've considered

  • modify the application and integrate with the monitoring tools
  • define a specific function in the application that receives code at runtime
@vdeturckheim
Copy link
Member

@gireeshpunathil, I have been working on a somehow linked use case for that through using the debugger: https://blog.sqreen.com/remote-debugging-nodejs-runtime-code-injection/ would that match your user story ?
This method require local access which actually provides pretty a good security scope.

@RaisinTen RaisinTen added the feature request Issues that request new features to be added to Node.js. label Feb 19, 2021
@gireeshpunathil
Copy link
Member Author

thanks @vdeturckheim , this is a great article!

technically this matches my use case in that:

  • it allows the process to start without any special configuration
  • it allows the process be interrupted
  • it allows the process to run arbitrary code

here are potential divergence:

  • my deployment is container based, so no chrome dev tools
  • my target environment is production, so inspector and usage of chrome-remote-interface may not be great
  • we don't want the inspector or any open port to linger, after the injected work is complete.

RaisinTen added a commit to RaisinTen/node that referenced this issue Feb 24, 2021
@RaisinTen
Copy link
Contributor

PR: #37503

@addaleax
Copy link
Member

my target environment is production, so inspector and usage of chrome-remote-interface may not be great

Can you explain why that would not be good?

we don't want the inspector or any open port to linger, after the injected work is complete.

Doesn't https://nodejs.org/api/inspector.html#inspector_inspector_close solve this for you?

@devsnek
Copy link
Member

devsnek commented Feb 24, 2021

I can agree that using the inspector is not always ideal. Certain commands may randomly deoptimize code, for example.

I also agree that NODE_OPTIONS=--require seems like an appropriate solution to this.

@gireeshpunathil
Copy link
Member Author

Can you explain why that would not be good?

@addaleax - it is not about functional correctness, or my conviction of its greatness. Containerized production deployments run with a lot of audit / restrictions / compliance, and I don't think ability to opening a debug port will pass such an audit.

Doesn't https://nodejs.org/api/inspector.html#inspector_inspector_close solve this for you?

ok, I wasn't sure how can I get a reference to the running instance of the inspector, from my handler code.

@addaleax
Copy link
Member

Can you explain why that would not be good?

@addaleax - it is not about functional correctness, or my conviction of its greatness. Containerized production deployments run with a lot of audit / restrictions / compliance, and I don't think ability to opening a debug port will pass such an audit.

But executing arbitrary code at runtime will pass such an audit...? That sounds very odd to me.

Ultimately, if you're trying to do that, then you'll need to communicate with that process, and there's a limited number of ways to do so. If raising a signal doesn't work because passing parameters that way is hard and you don't want to pre-set paramters in advance (which is fair), then you will need another communication channel, and most likely that's going to look like a debug port or a unix domain socket or something similar, I think. It might make sense to be more specific about the exact requirements you have here.

@gireeshpunathil
Copy link
Member Author

@addaleax - the inspector is experimental, and according to our own documentation, use of such feature is not recommended in production environments. @vdeturckheim's blog also specifically warns against using this method in production.

Exact requirement:

  • run an arbitrary code in a running node process
  • the target process needs zero preparation (by the user)
  • use case: monitoring tools, that can attach to and monitor any (legacy) apps
  • how the communication occurs and how the code is passed is left to implementation

Reference: Java has a feature called late attach. With only the knowledge of the target process's PID, a user is able to attach to a JVM and inject a monitoring code.

https://docs.oracle.com/en/java/javase/15/docs/api/jdk.attach/com/sun/tools/attach/VirtualMachine.html

VirtualMachine vm = VirtualMachine.attach("2177");

They use a second Java process as an attacher and complex hand-shaking process before the code is attached I guess, but I don't know the internals fully to comment further.

@devsnek
Copy link
Member

devsnek commented Feb 25, 2021

aside from googling "java late attach" showing a bunch of articles recommending that you disable it for security reasons, doesn't that go against your own point about about auditing? it might help if you were more concrete in your requirements.

@gireeshpunathil
Copy link
Member Author

@devsnek - the provider does not warn against any security issues.

In terms of concreteness, didn't my previous comment cover that? I specified the requirement, the use case, and the choice that can be made in the implementation. What else would you like to know?

here is a user scenario:

  • I have a node.js application deployed (say in container)
  • It can be running as a single or a thousand replicas.
  • I have no ability to change its code or config.
  • I have a monitoring module. I need to monitor those deployments.
  • I am able to install the module, alert the target process to launch the module & run some code.

@devsnek
Copy link
Member

devsnek commented Feb 25, 2021

For example, what would or would not pass the audits you say need to be passed? And assuming that between now and when you want to use this feature, there will have to be another deploy, at least to update node, why can't you use that opportunity to deploy your own system as was suggested in the other thread?

@gireeshpunathil
Copy link
Member Author

For example, what would or would not pass the audits you say need to be passed?

standard practices: usage of stable features.

why can't you use that opportunity to deploy your own system as was suggested in the other thread?

this is not just about fixing any currently running deployments. In future, if a deployment needs to be monitored on demand for a fixed period of time (which is otherwise not configured to be monitored) and detached from the target, it does not make sense to wait for the next available maintenance window?

@addaleax
Copy link
Member

@addaleax - the inspector is experimental, and according to our own documentation, use of such feature is not recommended in production environments.


For example, what would or would not pass the audits you say need to be passed?

standard practices: usage of stable features.

Then, from a practical point of view, the best way forward might be if you opened a PR to mark the inspector module as stable. That seems long overdue anyway, it’s been 1½ years since the last API change.

@vdeturckheim's blog also specifically warns against using this method in production.

I would absolutely also warn against late code injection in production in general. But if you really want to do it, then I think “the inspector is not built with production environments in mind” is not a strong enough reason not to use it and to introduce a new, feature-equivalent API that does a subset of what you can already do through the inspector API.

(Also, I’m not sure about the V8 internals here, but I would not expect V8 to de-optimize anything if you’re only running new code that was not previously present in the process.)

The inspector is also the only thing that would currently let you inject new code into arbitrary Worker threads, as far as I know – since those are similar to full Node.js processes, that seems like a concern here as well.

gireeshpunathil added a commit to gireeshpunathil/node that referenced this issue Mar 14, 2021
The module has been there for a while,
evidence of usage in the field exists,
and we have good test coverage for it.
Last API change has been 18 months ago.

Refs: nodejs#37440 (comment)
jasnell pushed a commit that referenced this issue Jul 19, 2021
The module has been there for a while,
evidence of usage in the field exists,
and we have good test coverage for it.
Last API change has been 18 months ago.

Refs: #37440 (comment)

PR-URL: #37748
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Jul 20, 2021
The module has been there for a while,
evidence of usage in the field exists,
and we have good test coverage for it.
Last API change has been 18 months ago.

Refs: #37440 (comment)

PR-URL: #37748
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
BethGriggs pushed a commit that referenced this issue Jul 29, 2021
The module has been there for a while,
evidence of usage in the field exists,
and we have good test coverage for it.
Last API change has been 18 months ago.

Refs: #37440 (comment)

PR-URL: #37748
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue Sep 4, 2021
The module has been there for a while,
evidence of usage in the field exists,
and we have good test coverage for it.
Last API change has been 18 months ago.

Refs: #37440 (comment)

PR-URL: #37748
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Jiawen Geng <technicalcute@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@github-actions
Copy link
Contributor

There has been no activity on this feature request for 5 months and it is unlikely to be implemented. It will be closed 6 months after the last non-automated comment.

For more information on how the project manages feature requests, please consult the feature request management document.

@github-actions github-actions bot added the stale label Mar 22, 2022
@gireeshpunathil
Copy link
Member Author

closing, as its stated purpose is partially met through inspector.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Issues that request new features to be added to Node.js. stale
Projects
Development

Successfully merging a pull request may close this issue.

5 participants