-
Notifications
You must be signed in to change notification settings - Fork 321
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
use rootless containers where possible #493
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,11 @@ | ||
## UNRELEASED | ||
|
||
FEATURES: | ||
* Connect: Add a security context to the init copy container and the envoy sidecar and ensure they do not run as root. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)] | ||
|
||
BUG FIXES: | ||
* Connect: Use `RunAsNonRoot: false` for connect-init's container when tproxy is enabled. [[GH-493](https://github.com/hashicorp/consul-k8s/pull/493)] | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should also call out that we will error out if you have the same user as envoy as a breaking change. Unless we want to error out only when tproxy is enabled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah good point. But now I have a FEATURE + BREAKING CHANGE + BUG FIX entry in the changelog against the same PR, can we consolidate this somehow? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Haha, yes that's true. Maybe change the non-bug fix to a breaking change and add a note about what's breaking. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @ishustava how does this look? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perfect! |
||
## 0.26.0-beta1 (April 16, 2021) | ||
|
||
BREAKING CHANGES: | ||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -13,6 +13,7 @@ const ( | |||||
InjectInitCopyContainerName = "copy-consul-bin" | ||||||
InjectInitContainerName = "consul-connect-inject-init" | ||||||
rootUserAndGroupID = 0 | ||||||
envoyUserAndGroupID = 5995 | ||||||
netAdminCapability = "NET_ADMIN" | ||||||
) | ||||||
|
||||||
|
@@ -37,6 +38,8 @@ type initContainerCommandData struct { | |||||
PrometheusScrapePath string | ||||||
// PrometheusBackendPort configures where the listener on Envoy will point to. | ||||||
PrometheusBackendPort string | ||||||
// EnvoyUID is the UID that `consul connect redirect-traffic` will use when tproxy is enabled. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I found this comment confusing because it says it's envoy's UID but then it says it's for the command Upon further reading I see that this is what gets passed into that command's There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
For folks that don't know what UID stands for. |
||||||
EnvoyUID string | ||||||
|
||||||
// EnableTransparentProxy configures this init container to run in transparent proxy mode, | ||||||
// i.e. run consul connect redirect-traffic command and add the required privileges to the | ||||||
|
@@ -60,6 +63,12 @@ func (h *Handler) containerInitCopyContainer() corev1.Container { | |||||
}, | ||||||
}, | ||||||
Command: []string{"/bin/sh", "-ec", cmd}, | ||||||
SecurityContext: &corev1.SecurityContext{ | ||||||
RunAsUser: pointerToInt64(envoyUserAndGroupID), | ||||||
RunAsGroup: pointerToInt64(envoyUserAndGroupID), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why does the copy container need to run as the envoy user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without setting a UID it will run as root because that is the default of the consul container. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ahh interesting. So we can pick any random uid here as long at it's not 0 and we're good? Can you add a comment explaining that here 🙏 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yup, exactly! And I just picked the same UID as envoy so I didn't have to make another constant. I'll definitely add a comment to clarify thanks for pointing this out!!
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe to avoid confusion, we should use a different user? In the RFC, we proposed |
||||||
RunAsNonRoot: pointerToBool(true), | ||||||
ReadOnlyRootFilesystem: pointerToBool(true), | ||||||
}, | ||||||
} | ||||||
} | ||||||
|
||||||
|
@@ -78,6 +87,7 @@ func (h *Handler) containerInit(pod corev1.Pod, k8sNamespace string) (corev1.Con | |||||
NamespaceMirroringEnabled: h.EnableK8SNSMirroring, | ||||||
ConsulCACert: h.ConsulCACert, | ||||||
EnableTransparentProxy: tproxyEnabled, | ||||||
EnvoyUID: strconv.Itoa(envoyUserAndGroupID), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can pass it in as an |
||||||
} | ||||||
|
||||||
if data.AuthMethod != "" { | ||||||
|
@@ -170,6 +180,8 @@ func (h *Handler) containerInit(pod corev1.Pod, k8sNamespace string) (corev1.Con | |||||
container.SecurityContext = &corev1.SecurityContext{ | ||||||
RunAsUser: pointerToInt64(rootUserAndGroupID), | ||||||
RunAsGroup: pointerToInt64(rootUserAndGroupID), | ||||||
// RunAsNonRoot overrides any setting in the Pod so that we can still run as root here as required. | ||||||
RunAsNonRoot: pointerToBool(false), | ||||||
kschoche marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
Capabilities: &corev1.Capabilities{ | ||||||
Add: []corev1.Capability{netAdminCapability}, | ||||||
}, | ||||||
|
@@ -194,6 +206,11 @@ func pointerToInt64(i int64) *int64 { | |||||
return &i | ||||||
} | ||||||
|
||||||
// pointerToBool takes a bool and returns a pointer to it. | ||||||
func pointerToBool(b bool) *bool { | ||||||
return &b | ||||||
} | ||||||
|
||||||
// initContainerCommandTpl is the template for the command executed by | ||||||
// the init container. | ||||||
const initContainerCommandTpl = ` | ||||||
|
@@ -254,6 +271,6 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |||||
-namespace="{{ .ConsulNamespace }}" \ | ||||||
{{- end }} | ||||||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||||||
-proxy-uid=0 | ||||||
-proxy-uid={{ .EnvoyUID }} | ||||||
{{- end }} | ||||||
` |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -192,10 +192,11 @@ func TestHandlerContainerInit_transparentProxy(t *testing.T) { | |
Capabilities: &corev1.Capabilities{ | ||
Add: []corev1.Capability{netAdminCapability}, | ||
}, | ||
RunAsNonRoot: pointerToBool(false), | ||
} | ||
expectedCmd := `/consul/connect-inject/consul connect redirect-traffic \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0` | ||
-proxy-uid=5995` | ||
container, err := h.containerInit(*pod, k8sNamespace) | ||
require.NoError(t, err) | ||
actualCmd := strings.Join(container.Command, " ") | ||
|
@@ -384,7 +385,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
/consul/connect-inject/consul connect redirect-traffic \ | ||
-namespace="default" \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0`, | ||
-proxy-uid=5995`, | ||
"", | ||
}, | ||
|
||
|
@@ -415,7 +416,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
/consul/connect-inject/consul connect redirect-traffic \ | ||
-namespace="non-default" \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0`, | ||
-proxy-uid=5995`, | ||
"", | ||
}, | ||
|
||
|
@@ -453,7 +454,7 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
/consul/connect-inject/consul connect redirect-traffic \ | ||
-namespace="k8snamespace" \ | ||
-proxy-id="$(cat /consul/connect-inject/proxyid)" \ | ||
-proxy-uid=0`, | ||
-proxy-uid=5995`, | ||
"", | ||
}, | ||
} | ||
|
@@ -467,9 +468,9 @@ consul-k8s connect-init -pod-name=${POD_NAME} -pod-namespace=${POD_NAMESPACE} \ | |
container, err := h.containerInit(*tt.Pod(minimal()), k8sNamespace) | ||
require.NoError(err) | ||
actual := strings.Join(container.Command, " ") | ||
require.Equal(actual, tt.Cmd) | ||
require.Equal(tt.Cmd, actual) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🍺 |
||
if tt.CmdNot != "" { | ||
require.NotContains(actual, tt.CmdNot) | ||
require.NotContains(tt.CmdNot, actual) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like none of the test cases set There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nice catch! |
||
} | ||
}) | ||
} | ||
|
@@ -598,16 +599,18 @@ func TestHandlerContainerInit_Resources(t *testing.T) { | |
}, container.Resources) | ||
} | ||
|
||
// Test that the init copy container has the correct command. | ||
// Test that the init copy container has the correct command and SecurityContext. | ||
func TestHandlerContainerInitCopyContainer(t *testing.T) { | ||
require := require.New(t) | ||
h := Handler{} | ||
container := h.containerInitCopyContainer() | ||
expectedSecurityContext := &corev1.SecurityContext{ | ||
RunAsUser: pointerToInt64(envoyUserAndGroupID), | ||
RunAsGroup: pointerToInt64(envoyUserAndGroupID), | ||
RunAsNonRoot: pointerToBool(true), | ||
ReadOnlyRootFilesystem: pointerToBool(true), | ||
} | ||
require.Equal(container.SecurityContext, expectedSecurityContext) | ||
actual := strings.Join(container.Command, " ") | ||
require.Contains(actual, `cp /bin/consul /consul/connect-inject/consul`) | ||
} | ||
|
||
// pointerToBool takes a boolean and returns a pointer to it. | ||
func pointerToBool(b bool) *bool { | ||
return &b | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -10,7 +10,7 @@ import ( | |||||
"k8s.io/apimachinery/pkg/api/resource" | ||||||
) | ||||||
|
||||||
func (h *Handler) envoySidecar(pod corev1.Pod, k8sNamespace string) (corev1.Container, error) { | ||||||
kschoche marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
func (h *Handler) envoySidecar(pod corev1.Pod) (corev1.Container, error) { | ||||||
resources, err := h.envoySidecarResources(pod) | ||||||
if err != nil { | ||||||
return corev1.Container{}, err | ||||||
|
@@ -21,6 +21,21 @@ func (h *Handler) envoySidecar(pod corev1.Pod, k8sNamespace string) (corev1.Cont | |||||
return corev1.Container{}, err | ||||||
} | ||||||
|
||||||
if pod.Spec.SecurityContext != nil { | ||||||
// User container and Envoy container cannot have the same UID. | ||||||
if pod.Spec.SecurityContext.RunAsUser != nil && *pod.Spec.SecurityContext.RunAsUser == envoyUserAndGroupID { | ||||||
kschoche marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
return corev1.Container{}, fmt.Errorf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID) | ||||||
} | ||||||
} | ||||||
// Ensure that none of the user's containers have the same UID as Envoy. At this point in injection the handler | ||||||
// has only injected init containers so all containers defined in pod.Spec.Containers are from the user. | ||||||
for _, c := range pod.Spec.Containers { | ||||||
// User container and Envoy container cannot have the same UID. | ||||||
if c.SecurityContext != nil && c.SecurityContext.RunAsUser != nil && *c.SecurityContext.RunAsUser == envoyUserAndGroupID { | ||||||
return corev1.Container{}, fmt.Errorf("user containers cannot have the same uid as envoy: %v", envoyUserAndGroupID) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
This error will be shown during |
||||||
} | ||||||
} | ||||||
|
||||||
container := corev1.Container{ | ||||||
Name: "envoy-sidecar", | ||||||
Image: h.ImageEnvoy, | ||||||
|
@@ -40,6 +55,12 @@ func (h *Handler) envoySidecar(pod corev1.Pod, k8sNamespace string) (corev1.Cont | |||||
}, | ||||||
}, | ||||||
Command: cmd, | ||||||
SecurityContext: &corev1.SecurityContext{ | ||||||
RunAsUser: pointerToInt64(envoyUserAndGroupID), | ||||||
RunAsGroup: pointerToInt64(envoyUserAndGroupID), | ||||||
RunAsNonRoot: pointerToBool(true), | ||||||
ReadOnlyRootFilesystem: pointerToBool(true), | ||||||
}, | ||||||
} | ||||||
return container, nil | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,7 @@ | ||
package connectinject | ||
|
||
import ( | ||
"fmt" | ||
"testing" | ||
|
||
"github.com/stretchr/testify/require" | ||
|
@@ -27,7 +28,7 @@ func TestHandlerEnvoySidecar(t *testing.T) { | |
}, | ||
}, | ||
} | ||
container, err := h.envoySidecar(pod, k8sNamespace) | ||
container, err := h.envoySidecar(pod) | ||
require.NoError(err) | ||
require.Equal(container.Command, []string{ | ||
"envoy", | ||
|
@@ -42,6 +43,51 @@ func TestHandlerEnvoySidecar(t *testing.T) { | |
}) | ||
} | ||
|
||
// Test that if the user specifies a pod security context with the same uid as `envoyUserAndGroupID` that we return | ||
// an error to the handler. | ||
func TestHandlerEnvoySidecar_FailsWithDuplicatePodSecurityContextUID(t *testing.T) { | ||
require := require.New(t) | ||
h := Handler{} | ||
pod := corev1.Pod{ | ||
Spec: corev1.PodSpec{ | ||
Containers: []corev1.Container{ | ||
{ | ||
Name: "web", | ||
}, | ||
}, | ||
SecurityContext: &corev1.PodSecurityContext{ | ||
RunAsUser: pointerToInt64(envoyUserAndGroupID), | ||
}, | ||
}, | ||
} | ||
_, err := h.envoySidecar(pod) | ||
require.Error(err, fmt.Sprintf("pod security context cannot have the same uid as envoy: %v", envoyUserAndGroupID)) | ||
} | ||
|
||
// Test that if the user specifies a container with security context with the same uid as `envoyUserAndGroupID` | ||
// that we return an error to the handler. | ||
func TestHandlerEnvoySidecar_FailsWithDuplicateContainerSecurityContextUID(t *testing.T) { | ||
require := require.New(t) | ||
h := Handler{} | ||
pod := corev1.Pod{ | ||
Spec: corev1.PodSpec{ | ||
Containers: []corev1.Container{ | ||
{ | ||
Name: "web", | ||
}, | ||
{ | ||
Name: "app", | ||
SecurityContext: &corev1.SecurityContext{ | ||
RunAsUser: pointerToInt64(envoyUserAndGroupID), | ||
}, | ||
}, | ||
}, | ||
}, | ||
} | ||
_, err := h.envoySidecar(pod) | ||
require.Error(err, fmt.Sprintf("user containers cannot have the same uid as envoy: %v", envoyUserAndGroupID)) | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 good test coverage. |
||
// Test that we can pass extra args to envoy via the extraEnvoyArgs flag | ||
// or via pod annotations. When arguments are passed in both ways, the | ||
// arguments set via pod annotations are used. | ||
|
@@ -126,7 +172,7 @@ func TestHandlerEnvoySidecar_EnvoyExtraArgs(t *testing.T) { | |
EnvoyExtraArgs: tc.envoyExtraArgs, | ||
} | ||
|
||
c, err := h.envoySidecar(*tc.pod, k8sNamespace) | ||
c, err := h.envoySidecar(*tc.pod) | ||
require.NoError(t, err) | ||
require.Equal(t, tc.expectedContainerCommand, c.Command) | ||
}) | ||
|
@@ -300,7 +346,7 @@ func TestHandlerEnvoySidecar_Resources(t *testing.T) { | |
}, | ||
}, | ||
} | ||
container, err := c.handler.envoySidecar(pod, k8sNamespace) | ||
container, err := c.handler.envoySidecar(pod) | ||
if c.expErr != "" { | ||
require.NotNil(err) | ||
require.Contains(err.Error(), c.expErr) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -191,8 +191,7 @@ func (h *Handler) Handle(_ context.Context, req admission.Request) admission.Res | |
container.Env = append(container.Env, containerEnvVars...) | ||
} | ||
|
||
// TODO: rename both of these initcontainers appropriately | ||
// Add the consul-init container | ||
// Add the init container which copies the Consul binary to /consul/connect-inject/. | ||
initCopyContainer := h.containerInitCopyContainer() | ||
pod.Spec.InitContainers = append(pod.Spec.InitContainers, initCopyContainer) | ||
|
||
|
@@ -205,8 +204,8 @@ func (h *Handler) Handle(_ context.Context, req admission.Request) admission.Res | |
} | ||
pod.Spec.InitContainers = append(pod.Spec.InitContainers, initContainer) | ||
|
||
// Add the Envoy and Consul sidecars. | ||
envoySidecar, err := h.envoySidecar(pod, req.Namespace) | ||
// Add the Envoy sidecar. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙏 |
||
envoySidecar, err := h.envoySidecar(pod) | ||
if err != nil { | ||
h.Log.Error(err, "error configuring injection sidecar container", "request name", req.Name) | ||
return admission.Errored(http.StatusInternalServerError, fmt.Errorf("error configuring injection sidecar container: %s", err)) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.