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

Migrate JShell Bot #91

Closed
Zabuzard opened this issue Sep 11, 2021 · 15 comments · Fixed by #870 or #998
Closed

Migrate JShell Bot #91

Zabuzard opened this issue Sep 11, 2021 · 15 comments · Fixed by #870 or #998
Assignees
Labels
new command Add a new command or group of commands to the bot priority: normal valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board.

Comments

@Zabuzard
Copy link
Member

Zabuzard commented Sep 11, 2021

We may want to migrate the JShell bot. It is actually another repo in this organization, see Together-Java/JShellBot.

For reference, this is how it works as of today:

!jshell <message> by Jshell bot for JShell support (executing Java code)

This bot has tight security restrictions as otherwise its easy for an user to escape the Java sandbox and access the actual system the bot is running on, which would be fatal. Also, the command should not be able to break the rest of the bot (its other commands).

@Zabuzard Zabuzard added enhancement New feature or request new command Add a new command or group of commands to the bot labels Sep 11, 2021
@Zabuzard Zabuzard added this to the Migrate existing commands milestone Sep 11, 2021
@Zabuzard Zabuzard changed the title [FEATURE] [FEATURE] Migrate JShell Bot Sep 11, 2021
@Zabuzard
Copy link
Member Author

Naturally blocked by #59

@Zabuzard Zabuzard added blocked This issue is currently blocked by another issue (see comments) and removed enhancement New feature or request labels Sep 11, 2021
@illuminator3 illuminator3 added priority: normal and removed blocked This issue is currently blocked by another issue (see comments) labels Sep 12, 2021
@illuminator3 illuminator3 changed the title [FEATURE] Migrate JShell Bot Migrate JShell Bot Sep 12, 2021
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Dec 17, 2021
@Zabuzard
Copy link
Member Author

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

Still relevant and one of the main targets that should be done in the first Migration milestone.

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Jan 17, 2022
@Zabuzard Zabuzard removed the stale label Jan 17, 2022
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label, comment or add the valid label or this will be closed in 5 days.

@github-actions github-actions bot added the stale label Feb 17, 2022
@Zabuzard Zabuzard added valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board. and removed stale labels Feb 17, 2022
@marko-radosavljevic marko-radosavljevic pinned this issue Feb 23, 2022
@illuminator3 illuminator3 self-assigned this Jun 27, 2022
@illuminator3
Copy link
Contributor

how do I implement this? last time I made a message command I was yelled at

@Zabuzard
Copy link
Member Author

My 2 cents:

Vast majority of jshell usages in our server are short and not multi-line. So I would say we should have a slash command as well.
For the multi-line usages, go with the message-id pattern u introduced with /tag for now.

And once ready for usage: context-commands maybe (right click > execute in jshell).

@illuminator3
Copy link
Contributor

not making this until we have context menus

@illuminator3 illuminator3 added the blocked This issue is currently blocked by another issue (see comments) label Jun 28, 2022
@illuminator3
Copy link
Contributor

blocked by #382

@marko-radosavljevic
Copy link
Contributor

I'm mostly concerned with the security aspect of it. It's critical to have jshell instance properly sandboxed, which is not trivial, especially if we want to keep all the functionality intact. And to be honest, because of the huge impact it can create, I'm not even sure if I would be comfortable reviewing it.

So I would recommend first addressing security aspect, after we agree on the strategy we can move to the implementation details.

Paging @I-Al-Istannen, since he co-authored current jshell bot. Istannen already shared some issues, worries and design ideas on the server that are valuable addition to this issue:

@marko-radosavljevic
Copy link
Contributor

marko-radosavljevic commented Jun 28, 2022

For a sandbox, we can go either containers or microVMs. MicroVMs have some security benefits which would make them preferable for a sandboxed environment, but I'm not familiar with that tech (for e.g. firecracker/kata). Big Virtelization on container sandboxes - "docker as a sandbox for untrusted code".

Since we already use docker in our stack, do we agree that we probably want:

  1. a docker container, so we have an isolated environment, by default we get

    • namespaces:
      • mount namespaces - container has its own filesystem and is unable to access host filesystem
      • network namespaces - container has its own network stack
      • pid namespaces - container has its own isolated pid space, so it can't see host processes just itself and it's children processes
      • user namespaces - container has its own isolated uids and gids. This feature allows for the root user in a container to be mapped to a non uid-0 user outside the container, which can help to mitigate the risks of container breakout. This facility is available but not enabled by default.
    • cgroups - limits resource usage (for e.g. cpu and memory), if process is compromised it can't starve all the resources and thus crash the host.
    • limited capabilities
    • seccomp - restrict which syscalls process can make, decreases the attack surface on the kernel
    • selinux/apparmor - linux security modules that can be used for fine-grained mandatory access control. Rules where subjects (processes or users) are allowed or denied to access objects (files, directories, sockets, etc.)
  2. harden the docker container, to create a sandbox for untrusted code

    • preferably, we completely disable the network stack
    • preferably, we drop all capabilities
    • preferably, read-only filesystem
    • possibly, we configure seccomp policies and selinux/apparmor profiles
  3. each user gets a new container

    • that way each user's execution is independant, and can't be affected by anyone else's
    • keeping execution state is much easier, so you can define a method and then call it with next command, and similar
  4. run jshell process as non-privildeged user inside the container

@illuminator3
Copy link
Contributor

I'm mostly concerned with the security aspect of it. It's critical to have jshell instance properly sandboxed, which is not trivial, especially if we want to keep all the functionality intact. And to be honest, because of the huge impact it can create, I'm not even sure if I would be comfortable reviewing it.

So I would recommend first addressing security aspect, after we agree on the strategy we can move to the implementation details.

Paging @I-Al-Istannen, since he co-authored current jshell bot. Istannen already shared some issues, worries and design ideas on the server that are valuable addition to this issue:

can't access two out of those links

@Zabuzard Zabuzard unpinned this issue Aug 13, 2022
@Zabuzard Zabuzard pinned this issue Aug 28, 2022
@illuminator3
Copy link
Contributor

someone remind me to implement this during the autumn break in germany, nrw

btw the same thing has been done before but for C++ https://github.com/Eelis/geordi

@illuminator3
Copy link
Contributor

no one reminded me :angwy:
will pick up the work now that we have context commands

@interacsion interacsion unpinned this issue Oct 16, 2022
@interacsion interacsion pinned this issue Oct 16, 2022
@SquidXTV SquidXTV unpinned this issue Apr 20, 2023
@marko-radosavljevic marko-radosavljevic pinned this issue May 13, 2023
@Alathreon Alathreon removed the blocked This issue is currently blocked by another issue (see comments) label Aug 4, 2023
@Alathreon Alathreon self-assigned this Aug 4, 2023
@Alathreon Alathreon linked a pull request Aug 5, 2023 that will close this issue
@marko-radosavljevic marko-radosavljevic unpinned this issue Sep 25, 2023
@Alathreon
Copy link
Contributor

This first completion wasn't good enough, and so JShell was never realed enabled in the bot.

@Alathreon Alathreon reopened this Nov 12, 2023
@Alathreon Alathreon linked a pull request Dec 23, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new command Add a new command or group of commands to the bot priority: normal valid This issue/PR is validated and ready to be picked. This auto adds items to TJ project board.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants