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

Add a wrapper for Vm.assemble to create Sshable for the service vms #752

Merged
merged 1 commit into from Nov 3, 2023

Conversation

enescakir
Copy link
Member

We manage customer virtual machines, as well as the virtual machines utilized by our other services. The latter are managed by the control plane, which connects these virtual machines via SSH. Or services like GitHub runners, PostgreSQL, MinIO, and E2E tests all rely on these virtual machines. Each service generates an Sshable, assembles the virtual machine, and then updates the host of the Sshable once the machine is ready. To streamline this process, I've consolidated the generation and testing from 4 different locations into one.

Vm::Nexus.assemble_with_sshable serves as a wrapper for Vm::Nexus.assemble, sharing the same signature with one key difference: it receives a unix_user as the first argument instead of a public_key. Consequently, it generates an Sshable for the virtual machine

@enescakir enescakir self-assigned this Oct 19, 2023
@enescakir enescakir requested review from furkansahin, pykello, fdr and byucesoy and removed request for furkansahin October 19, 2023 13:25
@enescakir enescakir temporarily deployed to E2E-CI October 19, 2023 13:31 — with GitHub Actions Inactive
Copy link
Collaborator

@fdr fdr left a comment

Choose a reason for hiding this comment

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

Nice size reduction

@enescakir
Copy link
Member Author

I'm curious about the ideas of @byucesoy, @furkansahin, and @pykello, as they have experience writing services using our VMs under the hood.

Copy link
Member

@byucesoy byucesoy left a comment

Choose a reason for hiding this comment

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

I think this looks good overall. I'm wondering if we use this opportunity to standardize sshable user names for internal vms.

@enescakir
Copy link
Member Author

I think this looks good overall. I'm wondering if we use this opportunity to standardize sshable user names for internal vms.

Current usernames:

minio_server_nexus ➔ minio-user
postgres_nexus     ➔ ubi
test_vm_group      ➔ ubi
github_runner      ➔ runner

I don't know minio_server_nexus (hello @furkansahin). However, github_runner should use runner to maintain consistency with the default GitHub runners. Buildjet has this difference https://buildjet.com/for-github-actions/docs/runners/hardware#known-differences-between-build-jet-and-git-hub-actions

Copy link
Contributor

@pykello pykello left a comment

Choose a reason for hiding this comment

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

awesome!

@furkansahin
Copy link
Contributor

I don't know minio_server_nexus (hello @furkansahin). However, github_runner should use runner to maintain consistency with the default GitHub runners

I would like to keep minio-user as well since it's the default service user for minio.

@byucesoy
Copy link
Member

byucesoy commented Oct 25, 2023

I don't know minio_server_nexus (hello @furkansahin). However, github_runner should use runner to maintain consistency with the default GitHub runners

I would like to keep minio-user as well since it's the default service user for minio.

I think exactly for that reasons, we should not use runner, minio-user or any other service user as our SSH user. It is serious security loop hole. Giving examples from postgres;

  • Default service users would have capabilities that we don't need (and thus should not gain (least privilege principle)) in the scope of user we use for SSH (For example postgres OS user can connect to any database, my SSH user should not).
  • My SSH user has capabilities that the service user don't need (and thus should not gain (least privilege principle)) in the OS, such as being able to modify rhizome files. Customers can easily access to OS using the service user;
    • Users of postgres can use COPY FROM command to access OS layer.
    • Users of github runners can run arbitrary commands as this is its primary use-case. It is OK to have runner user to have consistency with Github Actions. What I'm saying is that it should be different than our SSH user.
    • I don't know if minio provides interface like postgres' COPY FROM command, but even if it doesn't, there is always possibility of remote code execution/privilege escalation vulnerabilities, and they are not rare [1], [2].

@enescakir
Copy link
Member Author

Your point is valid. We should consider changing our SSH user for them as well. Should we implement this change in a separate PR or include it in the current one?

@byucesoy
Copy link
Member

Initially I thought we can do it in this PR, but now I'm not sure if changing the user would break the services and bloat this PR with lots of changes to unbreak them. Feel free to do whatever makes sense to you.

Base automatically changed from vm-assoc-destroy to main October 31, 2023 07:39
@enescakir enescakir force-pushed the assemble-ssh branch 2 times, most recently from 73bbc52 to 407685a Compare October 31, 2023 15:10
@enescakir enescakir temporarily deployed to E2E-CI October 31, 2023 15:10 — with GitHub Actions Inactive
@enescakir
Copy link
Member Author

Initially I thought we can do it in this PR, but now I'm not sure if changing the user would break the services and bloat this PR with lots of changes to unbreak them. Feel free to do whatever makes sense to you.

GitHub runners need more changes to switch the user. I will do it in another PR.

@enescakir
Copy link
Member Author

enescakir commented Oct 31, 2023

We need to update the Sshable of the virtual machines already in the pool, as they have passed the wait_sshable step that where we update the host.

Vm.exclude(pool_id: nil).all.each do |vm|
    next unless vm.sshable
    vm.sshable.update(host: vm.ephemeral_net4)
end;

We manage customer virtual machines, as well as the virtual machines
utilized by our other services. The latter are managed by the control
plane, which connects these virtual machines via SSH. Or services like
GitHub runners, PostgreSQL, MinIO, and E2E tests all rely on these
virtual machines. Each service generates an Sshable, assembles the
virtual machine, and then updates the host of the Sshable once the
machine is ready. To streamline this process, I've consolidated the
generation and testing from 4 different locations into one.

`Vm::Nexus.assemble_with_sshable` serves as a wrapper for
`Vm::Nexus.assemble`, sharing the same signature with one key
difference: it receives a `unix_user` as the first argument instead of a
`public_key`.  Consequently, it generates an Sshable for the virtual
machine
@enescakir enescakir merged commit 712ec35 into main Nov 3, 2023
5 checks passed
@enescakir enescakir deleted the assemble-ssh branch November 3, 2023 11:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants