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

Enable console over ssh #56

Merged
merged 3 commits into from
Jan 6, 2024
Merged

Enable console over ssh #56

merged 3 commits into from
Jan 6, 2024

Conversation

Kaiede
Copy link
Contributor

@Kaiede Kaiede commented Jan 5, 2024

Purpose

For Bedrock, we don't have access to anything like rcon which would enable the ability to issue save-related commands for the purposes of making backups easier.

On Java, rcon only allows us to issue commands and doesn't let a container like "Bedrockifier" (it supports both Java and Bedrock) listen to the console output to trigger things when an event fires.

In both cases, in order to create a backup container that can fire backups and listen to events like user login/logout, it needs access to console itself of the container. Using docker attach from the backup container is the natural approach here, but in many cases, a user might not have the tech savvy required to configure docker safely for this, or might be using a different container engine like Kubernetes. Configuring docker so that docker attach is successful accounts for almost all support issues raised on the Bedrockifier backup container.

So the next best approach is to offer some sort of secure access to the console directly.

Approach

This adds support for SSH to mc-server-runner. It uses gliderlabs' package to function, and it has been tested against official SSH clients. It uses the RCON password as the account password, and allows for any username. The username and remote address are logged, as well as any authentication failures for auditing and debugging purposes. Services should use a fixed username to make identifying their service easier in logs. For example, 'bedrockifier' for the Bedrockifier backup container.

In terms of how this would be used: In a bedrock/java container that uses mc-server-runner, the command needs to be passed -remote-console to enable the server. How this is done from the docker-compose YAML is TBD for those projects (env variable such as ENABLE_SSH?). Then the user would need to add expose: to their docker-compose to expose the port to other containers, much like with rcon. This ensures the defaults are as secure as possible.

It generates a host key that shares the lifespan of the container on the host. In docker-compose cases, this generally should be adequate as any known_hosts on client containers should have similar lifespans.

Example Log Output

// Successful Session
2024-01-05T00:08:28.974Z	INFO	mc-server-runner	build/remote_shell_service.go:72	Remote console session accepted (bedrockifier/172.17.0.1:56424) isTTY: true
< ... Log Content From Minecraft ... >
2024-01-05T00:08:31.809Z	INFO	mc-server-runner	build/remote_shell_service.go:112	Remote console session disconnected (bedrockifier/172.17.0.1:56424)

// Rejected Password
2024-01-05T00:08:35.703Z	WARN	mc-server-runner	build/remote_shell_service.go:63	Remote console session rejected (bedrockifier/172.17.0.1:56426)

Future Thoughts

The host key is probably the weakest part of this design. An idea would be to put it in the data folder with the rest of the Minecraft content, but it also reduces the security of the key as it exposes it to anyone with read access to the docker volume holding the Minecraft content.

remote_shell_service.go Outdated Show resolved Hide resolved
remote_shell_service.go Outdated Show resolved Hide resolved
Copy link
Owner

@itzg itzg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Just a few minor comments.

remote_shell_service.go Outdated Show resolved Hide resolved
remote_shell_service.go Show resolved Hide resolved
remote_shell_service.go Show resolved Hide resolved
remote_shell_service.go Show resolved Hide resolved
remote_shell_service.go Outdated Show resolved Hide resolved
remote_shell_service.go Outdated Show resolved Hide resolved
remote_shell_service.go Outdated Show resolved Hide resolved
remote_shell_service.go Outdated Show resolved Hide resolved
@Kaiede Kaiede marked this pull request as ready for review January 6, 2024 00:59
@Kaiede
Copy link
Contributor Author

Kaiede commented Jan 6, 2024

I've gone through your feedback, and also merged the lines and clientExit channels into just an input channel that closes when the client triggers exit or there's some other error with the terminal. I double checked everything and did some testing with multiple ssh clients connected at the same time and things looked good. The attached docker terminal, and every ssh session was able to all see the same output (stdout and stderr), and they were all able to issue commands properly.

@itzg
Copy link
Owner

itzg commented Jan 6, 2024

I'll get it merged and released for inclusion in the bedrock image.

@itzg itzg merged commit 1336cb2 into itzg:master Jan 6, 2024
1 check passed
@itzg
Copy link
Owner

itzg commented Jan 6, 2024

@Kaiede
Copy link
Contributor Author

Kaiede commented Jan 6, 2024

I'll get it merged and released for inclusion in the bedrock image.

Any chance Java can pick this up as well? :)

@itzg
Copy link
Owner

itzg commented Jan 6, 2024

I was getting wrappers mixed up. The Java edition images is what uses mc-server-runner, so that's what I actually meant 😉.

Bedrock image will need a different strategy since it uses just entrypoint-demoter and then directly exec's the bedrock server executable.

@Kaiede
Copy link
Contributor Author

Kaiede commented Jan 6, 2024

I’ve started playing with both projects seeing if I can put the PRs together and do some testing.

in principle it should still be possible to bring in mc-server-runner as the wrapper in the bedrock-entry script. Just need to build my changes and do some testing to see if it regresses anything.

Comment on lines -69 to -71
// directly assign stdout/err to pass through terminal, if applicable
cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just realized this was quite important for server types/mods that detect TTY/console in order to render logs with color codes. Will need to make this behavior conditional on the use of remote console.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adjusted defaults in #57

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

Successfully merging this pull request may close these issues.

2 participants