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

Restrict the amount of work being done in a privileged container in the reversed vpn client. #6352

Conversation

ScheererJ
Copy link
Contributor

@ScheererJ ScheererJ commented Jul 18, 2022

How to categorize this PR?

/area networking
/kind cleanup

What this PR does / why we need it:
Restrict the amount of work being done in a privileged container in the reversed vpn client.

It is not necessary for the vpn-seed-server to run as privileged container. The tun
device required for vpn operation can be mounted into the container namespace. The
remaining work requiring privileged work can be moved into an init container.
In case the container should run as non-root user in the future, it might help to
create a separate tun device in a privileged init container as described in
https://community.openvpn.net/openvpn/wiki/UnprivilegedUser#TUNTAPDevice .

Which issue(s) this PR fixes:
Partially addresses gardener-attic/vpn#41.

Special notes for your reviewer:

Release note:

The vpn-shoot/vpn-shoot container no longer runs in privileged mode (when ReversedVPN feature gate is enabled). As it still needs to still modify some kernel settings, this part is moved to init container that still has to run in privileged but the risk to cluster security is minimal because of the ephemeral nature of init containers.

@gardener-prow gardener-prow bot added the area/networking Networking related label Jul 18, 2022
@gardener-prow gardener-prow bot requested review from ary1992 and voelzmo July 18, 2022 16:00
@gardener-prow gardener-prow bot added kind/cleanup Something that is not needed anymore and can be cleaned up cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jul 18, 2022
@ScheererJ
Copy link
Contributor Author

/cc @DockToFuture @marwinski @ialidzhikov
/hold until #6349 is merged

@gardener-prow gardener-prow bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 18, 2022
@ialidzhikov ialidzhikov self-assigned this Jul 18, 2022
Copy link
Member

@DockToFuture DockToFuture left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 18, 2022
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

@marwinski can you also review this PR as it is related to #6346?

@ScheererJ ScheererJ force-pushed the reversed-vpn/restricted-privilege-for-client branch from 476950a to fa092d7 Compare July 19, 2022 07:39
@gardener-prow gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2022
@ScheererJ ScheererJ force-pushed the reversed-vpn/restricted-privilege-for-client branch from fa092d7 to 284a715 Compare July 19, 2022 07:59
@DockToFuture
Copy link
Member

/lgtm

@gardener-prow gardener-prow bot added the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2022
@ialidzhikov
Copy link
Member

@ScheererJ can you rebase after #6349 is merged?

@ScheererJ
Copy link
Contributor Author

@ScheererJ can you rebase after #6349 is merged?

@ialidzhikov I sure can even though I do not quite understand why it would be required (as the changes are in unrelated files).

@ialidzhikov
Copy link
Member

@ialidzhikov I sure can even though I do not quite understand why it would be required (as the changes are in unrelated files).

How I am supposed to test locally this PR as it relies on new env vars like DO_NOT_CONFIGURE_KERNEL_SETTINGS and EXIT_AFTER_CONFIGURING_KERNEL_SETTINGS that are introduced with the version bump in #6349? Shouldn't the e2e test about shoot creation fail for the same reason?

@ScheererJ
Copy link
Contributor Author

@ialidzhikov The end-to-end test will likely fail as the init container will not stop with the old image version. You can test it locally be referencing the new vpn2 release instead of the existing one.

@ScheererJ ScheererJ force-pushed the reversed-vpn/restricted-privilege-for-client branch from 284a715 to a5c46a6 Compare July 19, 2022 08:31
@gardener-prow gardener-prow bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 19, 2022
@ScheererJ
Copy link
Contributor Author

@ialidzhikov Rebased as requested.

@ScheererJ ScheererJ force-pushed the reversed-vpn/restricted-privilege-for-client branch from a5c46a6 to 89a6891 Compare July 19, 2022 11:14
@ScheererJ
Copy link
Contributor Author

/retest

Copy link
Contributor

@marwinski marwinski left a comment

Choose a reason for hiding this comment

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

I wasn't able to test this from from the /dev/net/tun point of view this will work. The way the init container is implemented looks a bit like a hack due to the environment variable control. Maybe a separate script would have been more appropriate but this is minor.

@gardener-prow
Copy link
Contributor

gardener-prow bot commented Jul 19, 2022

@marwinski: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

I wasn't able to test this from from the /dev/net/tun point of view this will work. The way the init container is implemented looks a bit like a hack due to the environment variable control. Maybe a separate script would have been more appropriate but this is minor.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@ScheererJ
Copy link
Contributor Author

/retest

@ScheererJ ScheererJ force-pushed the reversed-vpn/restricted-privilege-for-client branch from 89a6891 to be7d7ce Compare July 20, 2022 08:49
…he reversed vpn client.

It is not necessary for the vpn-seed-server to run as privileged container. The tun
device required for vpn operation can be mounted into the container namespace. The
remaining work requiring privileged work can be moved into an init container.
In case the container should run as non-root user in the future, it might help to
create a separate tun device in a privileged init container as described in
https://community.openvpn.net/openvpn/wiki/UnprivilegedUser#TUNTAPDevice .
@ScheererJ ScheererJ force-pushed the reversed-vpn/restricted-privilege-for-client branch from be7d7ce to 6d083de Compare July 20, 2022 09:32
Copy link
Member

@ialidzhikov ialidzhikov left a comment

Choose a reason for hiding this comment

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

/unhold
/lgtm
/approve

@gardener-prow gardener-prow bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 20, 2022
@gardener-prow
Copy link
Contributor

gardener-prow bot commented Jul 20, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: DockToFuture, ialidzhikov, marwinski

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@gardener-prow gardener-prow bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 20, 2022
@gardener-prow gardener-prow bot merged commit 3b823f2 into gardener:master Jul 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/networking Networking related cla: yes Indicates the PR's author has signed the cla-assistant.io CLA. kind/cleanup Something that is not needed anymore and can be cleaned up lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants